On Thu, Sep 04, 2014 at 06:15:12PM +0800, Jason Wang wrote:
> On 09/04/2014 04:39 PM, Michael S. Tsirkin wrote:
> > This completes all packets, ensuring that callbacks
> > will not run when VM is stopped.
> >
> > Cc: qemu-sta...@nongnu.org
> > Cc: Jason Wang <jasow...@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> > ---
> > net/net.c | 33 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 6d930ea..25fdb07 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -47,6 +47,7 @@
> > # define CONFIG_NET_BRIDGE
> > #endif
> >
> > +static VMChangeStateEntry *net_change_state_entry;
> > static QTAILQ_HEAD(, NetClientState) net_clients;
> >
> > const char *host_net_devices[] = {
> > @@ -504,7 +505,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;
> >
> > @@ -518,9 +520,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,
> > @@ -1168,6 +1178,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);
> > + }
> > + }
> > +}
> > +
> > void net_cleanup(void)
> > {
> > NetClientState *nc;
> > @@ -1183,6 +1209,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)
> > @@ -1268,6 +1296,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)
>
> A problem is the dependency between state change handlers (e.g. virtio).
> Current virtio vmstate change handler will be called before this
> handler. Which means vdev->vm_running was false when we purge the queue,
> this will trigger the assert of vdev->vm_running in virtio_net_flush_tx().
True but that's a virtio bug: it changes vm_running too early.
I will send a patch to fix that now.
Long term with the core changes, we mught be able to get rid of
vm_running field completely.
