Re: [Qemu-devel] [PATCH 4/5] net/dump: Add dump option for netdev devices

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

On Fri, 26 Jun 2015 10:44:59 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Jun 24, 2015 at 05:56:20PM +0200, Thomas Huth wrote:
> > diff --git a/net/net.c b/net/net.c
> > index cc36c7b..8871b77 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender,
> >          return 0;
> >      }
> >  
> > +    if (nc->netdev_dump_enabled) {
> > +        net_dump_receive(nc, data, size);
> > +    } else if (sender->netdev_dump_enabled) {
> > +        net_dump_receive(sender, data, size);
> > +    }
> 
> Why "else if"?  If two interfaces have packet capture enabled then both
> should get the full traffic log:
> 
>   if (nc->netdev_dump_enabled) {
>       net_dump_receive(nc, data, size);
>   }
>   if (sender->netdev_dump_enabled) {
>       net_dump_receive(sender, data, size);
>   }

I think I assumed that only the interfaces that are created with
"-netdev" can get a dump option. So it's either the receiver or the
sender that dumps. If both interfaces would have the dump flag set,
that would mean that two interfaces created with "-netdev" are talking
to each other - and this can not happen, can it?

> Perhaps dumping should happen after ->receive() has returned size.  If
> ->receive() returns -1 the packet is discarded, and if it returns 0 the
> packet is queued (not delivered yet).
> 
> If you dump unconditionally before ->receive() you will see queued
> packets dumped multiple times (each time the queue gets flushed).

Good point, thanks, I'll move the code accordingly.

 Thomas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]