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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Tue, Jun 30, 2015 at 12:37:46PM +0200, Thomas Huth wrote:
> 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);
> > > +    }
> ...
> > 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).
> 
> I've now tried this, but then I suddenly get the packets in the wrong
> order in the dump file (when using slirp networking), e.g.:
> 
>   1   0.000000     10.0.2.2 -> 255.255.255.255 DHCP 590 DHCP Offer
>   2   0.000012      0.0.0.0 -> 255.255.255.255 DHCP 342 DHCP Discover
> 
> instead of:
> 
>   1   0.000000      0.0.0.0 -> 255.255.255.255 DHCP 342 DHCP Discover
>   2   0.000035     10.0.2.2 -> 255.255.255.255 DHCP 590 DHCP Offer
> 
> Looks like with slirp, the answer is already sent before the initial
> receive() function returns?
> 
> Any idea how to avoid that issue? If not, I think I'll simply keep the
> dump hooks before calling the receive functions.

I see.  So either way the packet capture will be misleading.

Please leave add a comment to the code mentioning that either queued
packets are duplicated or slirp traffic is out-of-order.  That way we
won't forget why the code is the way it is.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]