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

[Jason Wang <jasowang@redhat.com>]

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 <thibaut.collet@6wind.com>
> ---
>  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