Re: [Qemu-devel] [PATCH 1/1] net: fix queue's purge on VM stop

On Mon, Jun 1, 2015 at 11:17 AM, Jason Wang <address@hidden> wrote:

On 05/28/2015 04:03 PM, Thibaut Collet wrote:
> For netdev backend with no receive callback, packets must not be enqueued. When
> VM stops the enqueued packets are purged (see fixes ca77d85e1dbf). That causes a
> qemu segfault due to a call of a NULL pointer function.
>
> Function to create net client is modified to allow backend to specify the size
> of the queue.
> For netdev backend with no receive callback, the queue size is set to 0 to
> prevent enqueuing of packets.
> For other netdev backend, a default queue size is provided. This value (set to
> 10000) is the value previously set to any queue.
>
> Signed-off-by: Thibaut Collet <address@hidden>
> ---
> include/net/net.h | 3 ++-
> include/net/queue.h | 5 ++++-
> net/dump.c | 3 ++-
> net/hub.c | 3 ++-
> net/l2tpv3.c | 3 ++-
> net/net.c | 10 ++++++----
> net/netmap.c | 3 ++-
> net/queue.c | 4 ++--
> net/slirp.c | 3 ++-
> net/socket.c | 9 ++++++---
> net/tap-win32.c | 3 ++-
> net/tap.c | 3 ++-
> net/vde.c | 3 ++-
> net/vhost-user.c | 4 +++-
> 14 files changed, 39 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index e66ca03..cb3e2df 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -104,7 +104,8 @@ int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
> NetClientState *qemu_new_net_client(NetClientInfo *info,
> NetClientState *peer,
> const char *model,
> - const char *name);
> + const char *name,
> + uint32_t queue_size);
> NICState *qemu_new_nic(NetClientInfo *info,
> NICConf *conf,
> const char *model,
> diff --git a/include/net/queue.h b/include/net/queue.h
> index fc02b33..4659c5a 100644
> --- a/include/net/queue.h
> +++ b/include/net/queue.h
> @@ -34,7 +34,10 @@ typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret);
> #define QEMU_NET_PACKET_FLAG_NONE 0
> #define QEMU_NET_PACKET_FLAG_RAW (1<<0)
>
> -NetQueue *qemu_new_net_queue(void *opaque);
> +#define QEMU_DEFAULT_QUEUE_SIZE 10000
> +#define QEMU_EMPTY_QUEUE 0
> +
> +NetQueue *qemu_new_net_queue(void *opaque, uint32_t queue_size);
>
> void qemu_del_net_queue(NetQueue *queue);
>
> diff --git a/net/dump.c b/net/dump.c
> index 9d3a09e..299b09e 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -129,7 +129,8 @@ static int net_dump_init(NetClientState *peer, const char *device,
> return -1;
> }
>
> - nc = qemu_new_net_client(&net_dump_info, peer, device, name);
> + nc = qemu_new_net_client(&net_dump_info, peer, device, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> snprintf(nc->info_str, sizeof(nc->info_str),
> "dump to %s (len=%d)", filename, len);
> diff --git a/net/hub.c b/net/hub.c
> index 2b60ab9..a42cac3 100644
> --- a/net/hub.c
> +++ b/net/hub.c
> @@ -151,7 +151,8 @@ static NetHubPort *net_hub_port_new(NetHub *hub, const char *name)
> name = default_name;
> }
>
> - nc = qemu_new_net_client(&net_hub_port_info, NULL, "hub", name);
> + nc = qemu_new_net_client(&net_hub_port_info, NULL, "hub", name,
> + QEMU_DEFAULT_QUEUE_SIZE);
> port = DO_UPCAST(NetHubPort, nc, nc);
> port->id = id;
> port->hub = hub;
> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
> index 8c598b0..1d2e4e5 100644
> --- a/net/l2tpv3.c
> +++ b/net/l2tpv3.c
> @@ -548,7 +548,8 @@ int net_init_l2tpv3(const NetClientOptions *opts,
> struct addrinfo *result = NULL;
> char *srcport, *dstport;
>
> - nc = qemu_new_net_client(&net_l2tpv3_info, peer, "l2tpv3", name);
> + nc = qemu_new_net_client(&net_l2tpv3_info, peer, "l2tpv3", name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> s = DO_UPCAST(NetL2TPV3State, nc, nc);
>
> diff --git a/net/net.c b/net/net.c
> index 7427f6a..bcf69da 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -214,6 +214,7 @@ static void qemu_net_client_setup(NetClientState *nc,
> NetClientState *peer,
> const char *model,
> const char *name,
> + uint32_t queue_size,
> NetClientDestructor *destructor)
> {
> nc->info = info;
> @@ -231,21 +232,22 @@ static void qemu_net_client_setup(NetClientState *nc,
> }
> QTAILQ_INSERT_TAIL(&net_clients, nc, next);
>
> - nc->incoming_queue = qemu_new_net_queue(nc);
> + nc->incoming_queue = qemu_new_net_queue(nc, queue_size);
> nc->destructor = destructor;
> }
>
> NetClientState *qemu_new_net_client(NetClientInfo *info,
> NetClientState *peer,
> const char *model,
> - const char *name)
> + const char *name,
> + uint32_t queue_size)
> {
> NetClientState *nc;
>
> assert(info->size >= sizeof(NetClientState));
>
> nc = g_malloc0(info->size);
> - qemu_net_client_setup(nc, info, peer, model, name,
> + qemu_net_client_setup(nc, info, peer, model, name, queue_size,
> qemu_net_client_destructor);
>
> return nc;
> @@ -271,7 +273,7 @@ NICState *qemu_new_nic(NetClientInfo *info,
>
> for (i = 0; i < queues; i++) {
> qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, name,
> - NULL);
> + QEMU_DEFAULT_QUEUE_SIZE, NULL);
> nic->ncs[i].queue_index = i;
> }
>
> diff --git a/net/netmap.c b/net/netmap.c
> index 0c1772b..3ddfc8e 100644
> --- a/net/netmap.c
> +++ b/net/netmap.c
> @@ -461,7 +461,8 @@ int net_init_netmap(const NetClientOptions *opts,
> return -1;
> }
> /* Create the object. */
> - nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name);
> + nc = qemu_new_net_client(&net_netmap_info, peer, "netmap", name,
> + QEMU_DEFAULT_QUEUE_SIZE);
> s = DO_UPCAST(NetmapState, nc, nc);
> s->me = me;
> s->vnet_hdr_len = 0;
> diff --git a/net/queue.c b/net/queue.c
> index ebbe2bb..d6ddda5 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -58,14 +58,14 @@ struct NetQueue {
> unsigned delivering : 1;
> };
>
> -NetQueue *qemu_new_net_queue(void *opaque)
> +NetQueue *qemu_new_net_queue(void *opaque, uint32_t queue_size)
> {
> NetQueue *queue;
>
> queue = g_new0(NetQueue, 1);
>
> queue->opaque = opaque;
> - queue->nq_maxlen = 10000;
> + queue->nq_maxlen = queue_size;
> queue->nq_count = 0;
>
> QTAILQ_INIT(&queue->packets);
> diff --git a/net/slirp.c b/net/slirp.c
> index 9bbed74..c91e929 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -234,7 +234,8 @@ static int net_slirp_init(NetClientState *peer, const char *model,
> }
> #endif
>
> - nc = qemu_new_net_client(&net_slirp_info, peer, model, name);
> + nc = qemu_new_net_client(&net_slirp_info, peer, model, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> snprintf(nc->info_str, sizeof(nc->info_str),
> "net=%s,restrict=%s", inet_ntoa(net),
> diff --git a/net/socket.c b/net/socket.c
> index c30e03f..d5c718c 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -387,7 +387,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
> }
> }
>
> - nc = qemu_new_net_client(&net_dgram_socket_info, peer, model, name);
> + nc = qemu_new_net_client(&net_dgram_socket_info, peer, model, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> s = DO_UPCAST(NetSocketState, nc, nc);
>
> @@ -436,7 +437,8 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
> NetClientState *nc;
> NetSocketState *s;
>
> - nc = qemu_new_net_client(&net_socket_info, peer, model, name);
> + nc = qemu_new_net_client(&net_socket_info, peer, model, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> snprintf(nc->info_str, sizeof(nc->info_str), "socket: fd=%d", fd);
>
> @@ -543,7 +545,8 @@ static int net_socket_listen_init(NetClientState *peer,
> return -1;
> }
>
> - nc = qemu_new_net_client(&net_socket_info, peer, model, name);
> + nc = qemu_new_net_client(&net_socket_info, peer, model, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
> s = DO_UPCAST(NetSocketState, nc, nc);
> s->fd = -1;
> s->listen_fd = fd;
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 8aee611..dd6046a 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -737,7 +737,8 @@ static int tap_win32_init(NetClientState *peer, const char *model,
> return -1;
> }
>
> - nc = qemu_new_net_client(&net_tap_win32_info, peer, model, name);
> + nc = qemu_new_net_client(&net_tap_win32_info, peer, model, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> s = DO_UPCAST(TAPState, nc, nc);
>
> diff --git a/net/tap.c b/net/tap.c
> index 968df46..383d3ad 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -346,7 +346,8 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
> NetClientState *nc;
> TAPState *s;
>
> - nc = qemu_new_net_client(&net_tap_info, peer, model, name);
> + nc = qemu_new_net_client(&net_tap_info, peer, model, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> s = DO_UPCAST(TAPState, nc, nc);
>
> diff --git a/net/vde.c b/net/vde.c
> index 2a619fb..b6d28cf 100644
> --- a/net/vde.c
> +++ b/net/vde.c
> @@ -95,7 +95,8 @@ static int net_vde_init(NetClientState *peer, const char *model,
> return -1;
> }
>
> - nc = qemu_new_net_client(&net_vde_info, peer, model, name);
> + nc = qemu_new_net_client(&net_vde_info, peer, model, name,
> + QEMU_DEFAULT_QUEUE_SIZE);
>
> snprintf(nc->info_str, sizeof(nc->info_str), "sock=%s,fd=%d",
> sock, vde_datafd(vde));
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 1d86a2b..71f8758 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -137,7 +137,9 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
> NetClientState *nc;
> VhostUserState *s;
>
> - nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
> + /* We don't provide a valid queue: no receive callback */
> + nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name,
> + QEMU_EMPTY_QUEUE);
>
> snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user to %s",
> chr->label);

Two questions:

- vhost-user set received_disabled to true to prevent this from
happening. but looks like qemu_flush_or_purge_queue_packets() change it
to false unconditionally. And I don't quite understand this, probably we
can just remove this and the issue is fixed?
- If not, maybe you just need another flag like receive_disabled. This
seems can save a lot of codes.

Thanks

From:	Thibaut Collet
Subject:	Re: [Qemu-devel] [PATCH 1/1] net: fix queue's purge on VM stop
Date:	Mon, 1 Jun 2015 12:14:27 +0200