Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Jason Wang]

On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote:
> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
>> also when the peer's link is down. But the check was not done properly, the
>> vhost were only started when:
>>
>> 1) peer's link is not down
>> 2) virtio-net has already been started.
>>
>> Since == have a higher precedence than &&, place a brace to make sure both 
>> the
>> conditions were met then does the check. This fixes the crash when doing a 
>> savem
>> after set the link off which let qemu crash and complains:
>>
>> virtio_net_save: Assertion `!n->vhost_started' failed.
>>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Signed-off-by: Jason Wang <address@hidden>
> Hmm okay, but now that I think about this,
> e.g. if link is up later, vhost will not be started.

If vm has been stopeed, and the link is up later, vhost won't be
started. this is expected.
If vm has been started, and the link is up later, since n->vhost_started
is false but both virtio_net_started() and !nc->peer->link_down is true,
so the vhost will be started.

Looks ok?

> So the correct thing is maybe to start vhost but use
> some backend that will drop all packets.
> And add a callback so we know peer state changed.
> Hmm do we need a kernel change for this?
>
>> ---
>>  hw/net/virtio-net.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 4d2cdd2..6222039 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, 
>> uint8_t status)
>>          return;
>>      }
>>  
>> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
>> -                              !nc->peer->link_down) {
>> +    if (!!n->vhost_started ==
>> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
>>          return;
>>      }
>>      if (!n->vhost_started) {
>> -- 
>> 1.7.1