Re: [Qemu-devel] [PULL 6/7] net: complete all queued packets on VM stop

[Jason Wang]

On 09/04/2014 11:50 PM, Stefan Hajnoczi wrote:
> From: "Michael S. Tsirkin" <address@hidden>
>
> This completes all packets, ensuring that callbacks
> will not run when VM is stopped.
>
> Cc: address@hidden
> Cc: Jason Wang <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  net/net.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index 962c05f..7acc162 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -48,6 +48,7 @@
>  # define CONFIG_NET_BRIDGE
>  #endif
>  
> +static VMChangeStateEntry *net_change_state_entry;
>  static QTAILQ_HEAD(, NetClientState) net_clients;
>  
>  const char *host_net_devices[] = {
> @@ -511,7 +512,8 @@ void qemu_purge_queued_packets(NetClientState *nc)
>      qemu_net_queue_purge(nc->peer->incoming_queue, nc);
>  }
>  
> -void qemu_flush_queued_packets(NetClientState *nc)
> +static
> +void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge)
>  {
>      nc->receive_disabled = 0;
>  
> @@ -525,9 +527,17 @@ void qemu_flush_queued_packets(NetClientState *nc)
>           * the file descriptor (for tap, for example).
>           */
>          qemu_notify_event();
> +    } else if (purge) {
> +        /* Unable to empty the queue, purge remaining packets */
> +        qemu_net_queue_purge(nc->incoming_queue, nc);
>      }
>  }
>  
> +void qemu_flush_queued_packets(NetClientState *nc)
> +{
> +    qemu_flush_or_purge_queued_packets(nc, false);
> +}
> +
>  static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>                                                   unsigned flags,
>                                                   const uint8_t *buf, int 
> size,
> @@ -1175,6 +1185,22 @@ void qmp_set_link(const char *name, bool up, Error 
> **errp)
>      }
>  }
>  
> +static void net_vm_change_state_handler(void *opaque, int running,
> +                                        RunState state)
> +{
> +    /* Complete all queued packets, to guarantee we don't modify
> +     * state later when VM is not running.
> +     */
> +    if (!running) {
> +        NetClientState *nc;
> +        NetClientState *tmp;
> +
> +        QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) {
> +            qemu_flush_or_purge_queued_packets(nc, true);
> +        }
> +    }
> +}

Something like net_drain_all_queue() in do_vm_stop() looks simpler. And
doing this is tricky if it depends on other handlers to be called first.

E.g virtio-net vm change state handler will set vm_running to false. And
net_vm_change_state_handler() will be called after this. This means
virtio_net_flush_tx() will still hit the assert since it will be called
by packet cb (virtio_net_tx_complete()).
> +
>  void net_cleanup(void)
>  {
>      NetClientState *nc;
> @@ -1190,6 +1216,8 @@ void net_cleanup(void)
>              qemu_del_net_client(nc);
>          }
>      }
> +
> +    qemu_del_vm_change_state_handler(net_change_state_entry);
>  }
>  
>  void net_check_clients(void)
> @@ -1275,6 +1303,9 @@ int net_init_clients(void)
>  #endif
>      }
>  
> +    net_change_state_entry =
> +        qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL);
> +
>      QTAILQ_INIT(&net_clients);
>  
>      if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 
> 1) == -1)