[Qemu-devel] [PATCH 0/5] Network traffic dumping for -netdev, second try

[Qemu-devel] [PATCH 0/5] Network traffic dumping for -netdev, second try

[Thomas Huth]

The "-net dump" option only works with the "-net" option. So far, it
is not possible to dump network traffic with the "-netdev" option yet.
This patch series now fixes this ugliness by enabling dumping for the
"-netdev" option, too.

Unlike with my first attempt a couple of weeks ago ("net: Enable vlans
and dump for -netdev, too"), this patch series now does not "abuse"
the disliked internal vlan hub infrastructure anymore but introduces
a new, clean "dumpfile=xxx" option for the -netdev parameters instead.

It's likely too late for version 2.4 for this patch series, but it
would be great to get some review feedback for this anyway.

Thomas Huth (5):
  net/dump: Add support for receive_iov function
  net/dump: Move DumpState into NetClientState
  net/dump: Rework net-dump init functions
  net/dump: Add dump option for netdev devices
  qemu options: Add information about dumpfile to help text

 include/net/net.h |  8 +++++
 net/clients.h     |  5 ++++
 net/dump.c        | 89 ++++++++++++++++++++++++++++++++++++-------------------
 net/net.c         | 48 ++++++++++++++++++++++++++++++
 qapi-schema.json  | 12 ++++++--
 qemu-options.hx   | 26 +++++++++++-----
 6 files changed, 147 insertions(+), 41 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/5] net/dump: Add support for receive_iov function

[Thomas Huth]

Adding a proper receive_iov function to the net dump module. This
will make it easier to support the dump feature for the -netdev
option in later patches.
Also make the receive functions available to the other parts of the
source code so we can later use them for dumping from net.c, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 net/clients.h |  3 +++
 net/dump.c    | 48 ++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/net/clients.h b/net/clients.h
index d47530e..5092f3d 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -29,6 +29,9 @@
 
 int net_init_dump(const NetClientOptions *opts, const char *name,
                   NetClientState *peer, Error **errp);
+ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size);
+ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
+                             int cnt);
 
 #ifdef CONFIG_SLIRP
 int net_init_slirp(const NetClientOptions *opts, const char *name,
diff --git a/net/dump.c b/net/dump.c
index 02c8064..383718a 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -57,27 +57,49 @@ struct pcap_sf_pkthdr {
     uint32_t len;
 };
 
-static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
+ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
+                             int cnt)
 {
     DumpState *s = DO_UPCAST(DumpState, nc, nc);
     struct pcap_sf_pkthdr hdr;
     int64_t ts;
-    int caplen;
+    int caplen, i;
+    size_t size = 0;
+    struct iovec dumpiov[cnt + 1];
 
     /* Early return in case of previous error. */
     if (s->fd < 0) {
         return size;
     }
 
+    dumpiov[0].iov_base = &hdr;
+    dumpiov[0].iov_len = sizeof(hdr);
+    caplen = s->pcap_caplen;
+
+    /* Copy iov, limit maximum size to caplen, and count total input size */
+    for (i = 0; i < cnt; i++) {
+        dumpiov[i + 1].iov_base = iov[i].iov_base;
+        if (size + iov[i].iov_len <= caplen) {
+            dumpiov[i + 1].iov_len = iov[i].iov_len;
+        } else if (size < caplen) {
+            dumpiov[i + 1].iov_len = caplen - size;
+        } else {
+            dumpiov[i + 1].iov_len = 0;
+        }
+        size += iov[i].iov_len;
+    }
+
     ts = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 1000000, get_ticks_per_sec());
-    caplen = size > s->pcap_caplen ? s->pcap_caplen : size;
+    if (size < caplen) {
+        caplen = size;
+    }
 
     hdr.ts.tv_sec = ts / 1000000 + s->start_ts;
     hdr.ts.tv_usec = ts % 1000000;
     hdr.caplen = caplen;
     hdr.len = size;
-    if (write(s->fd, &hdr, sizeof(hdr)) != sizeof(hdr) ||
-        write(s->fd, buf, caplen) != caplen) {
+
+    if (writev(s->fd, dumpiov, cnt + 1) != sizeof(hdr) + caplen) {
         qemu_log("-net dump write error - stop dump\n");
         close(s->fd);
         s->fd = -1;
@@ -86,7 +108,16 @@ static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     return size;
 }
 
-static void dump_cleanup(NetClientState *nc)
+ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
+{
+    struct iovec iov = {
+        .iov_base = (void *)buf,
+        .iov_len = size
+    };
+    return net_dump_receive_iov(nc, &iov, 1);
+}
+
+static void net_dump_cleanup(NetClientState *nc)
 {
     DumpState *s = DO_UPCAST(DumpState, nc, nc);
 
@@ -96,8 +127,9 @@ static void dump_cleanup(NetClientState *nc)
 static NetClientInfo net_dump_info = {
     .type = NET_CLIENT_OPTIONS_KIND_DUMP,
     .size = sizeof(DumpState),
-    .receive = dump_receive,
-    .cleanup = dump_cleanup,
+    .receive = net_dump_receive,
+    .receive_iov = net_dump_receive_iov,
+    .cleanup = net_dump_cleanup,
 };
 
 static int net_dump_init(NetClientState *peer, const char *device,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/5] net/dump: Add support for receive_iov function

[Jason Wang]

On 06/24/2015 11:56 PM, Thomas Huth wrote:
> Adding a proper receive_iov function to the net dump module. This
> will make it easier to support the dump feature for the -netdev
> option in later patches.
> Also make the receive functions available to the other parts of the
> source code so we can later use them for dumping from net.c, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  net/clients.h |  3 +++
>  net/dump.c    | 48 ++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/net/clients.h b/net/clients.h
> index d47530e..5092f3d 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -29,6 +29,9 @@
>  
>  int net_init_dump(const NetClientOptions *opts, const char *name,
>                    NetClientState *peer, Error **errp);
> +ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size);
> +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> +                             int cnt);
>  
>  #ifdef CONFIG_SLIRP
>  int net_init_slirp(const NetClientOptions *opts, const char *name,
> diff --git a/net/dump.c b/net/dump.c
> index 02c8064..383718a 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -57,27 +57,49 @@ struct pcap_sf_pkthdr {
>      uint32_t len;
>  };
>  
> -static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> +                             int cnt)
>  {
>      DumpState *s = DO_UPCAST(DumpState, nc, nc);
>      struct pcap_sf_pkthdr hdr;
>      int64_t ts;
> -    int caplen;
> +    int caplen, i;
> +    size_t size = 0;
> +    struct iovec dumpiov[cnt + 1];
>  
>      /* Early return in case of previous error. */
>      if (s->fd < 0) {
>          return size;
>      }
>  
> +    dumpiov[0].iov_base = &hdr;
> +    dumpiov[0].iov_len = sizeof(hdr);
> +    caplen = s->pcap_caplen;
> +
> +    /* Copy iov, limit maximum size to caplen, and count total input size */
> +    for (i = 0; i < cnt; i++) {
> +        dumpiov[i + 1].iov_base = iov[i].iov_base;
> +        if (size + iov[i].iov_len <= caplen) {
> +            dumpiov[i + 1].iov_len = iov[i].iov_len;
> +        } else if (size < caplen) {
> +            dumpiov[i + 1].iov_len = caplen - size;
> +        } else {
> +            dumpiov[i + 1].iov_len = 0;
> +        }
> +        size += iov[i].iov_len;
> +    }

I believe we could use iov_copy() here.

> +
>      ts = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 1000000, get_ticks_per_sec());
> -    caplen = size > s->pcap_caplen ? s->pcap_caplen : size;
> +    if (size < caplen) {
> +        caplen = size;
> +    }
>  
>      hdr.ts.tv_sec = ts / 1000000 + s->start_ts;
>      hdr.ts.tv_usec = ts % 1000000;
>      hdr.caplen = caplen;
>      hdr.len = size;
> -    if (write(s->fd, &hdr, sizeof(hdr)) != sizeof(hdr) ||
> -        write(s->fd, buf, caplen) != caplen) {
> +
> +    if (writev(s->fd, dumpiov, cnt + 1) != sizeof(hdr) + caplen) {
>          qemu_log("-net dump write error - stop dump\n");
>          close(s->fd);
>          s->fd = -1;
> @@ -86,7 +108,16 @@ static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      return size;
>  }
>  
> -static void dump_cleanup(NetClientState *nc)
> +ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> +{
> +    struct iovec iov = {
> +        .iov_base = (void *)buf,
> +        .iov_len = size
> +    };
> +    return net_dump_receive_iov(nc, &iov, 1);
> +}
> +
> +static void net_dump_cleanup(NetClientState *nc)
>  {
>      DumpState *s = DO_UPCAST(DumpState, nc, nc);
>  
> @@ -96,8 +127,9 @@ static void dump_cleanup(NetClientState *nc)
>  static NetClientInfo net_dump_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_DUMP,
>      .size = sizeof(DumpState),
> -    .receive = dump_receive,
> -    .cleanup = dump_cleanup,
> +    .receive = net_dump_receive,
> +    .receive_iov = net_dump_receive_iov,
> +    .cleanup = net_dump_cleanup,
>  };
>  
>  static int net_dump_init(NetClientState *peer, const char *device,

Re: [Qemu-devel] [PATCH 1/5] net/dump: Add support for receive_iov function

[Thomas Huth]

On Fri, 26 Jun 2015 14:38:42 +0800
Jason Wang <jasowang@redhat.com> wrote:

> 
> 
> On 06/24/2015 11:56 PM, Thomas Huth wrote:
> > Adding a proper receive_iov function to the net dump module. This
> > will make it easier to support the dump feature for the -netdev
> > option in later patches.
> > Also make the receive functions available to the other parts of the
> > source code so we can later use them for dumping from net.c, too.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  net/clients.h |  3 +++
> >  net/dump.c    | 48 ++++++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/clients.h b/net/clients.h
> > index d47530e..5092f3d 100644
> > --- a/net/clients.h
> > +++ b/net/clients.h
> > @@ -29,6 +29,9 @@
> >  
> >  int net_init_dump(const NetClientOptions *opts, const char *name,
> >                    NetClientState *peer, Error **errp);
> > +ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size);
> > +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> > +                             int cnt);
> >  
> >  #ifdef CONFIG_SLIRP
> >  int net_init_slirp(const NetClientOptions *opts, const char *name,
> > diff --git a/net/dump.c b/net/dump.c
> > index 02c8064..383718a 100644
> > --- a/net/dump.c
> > +++ b/net/dump.c
> > @@ -57,27 +57,49 @@ struct pcap_sf_pkthdr {
> >      uint32_t len;
> >  };
> >  
> > -static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> > +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> > +                             int cnt)
> >  {
> >      DumpState *s = DO_UPCAST(DumpState, nc, nc);
> >      struct pcap_sf_pkthdr hdr;
> >      int64_t ts;
> > -    int caplen;
> > +    int caplen, i;
> > +    size_t size = 0;
> > +    struct iovec dumpiov[cnt + 1];
> >  
> >      /* Early return in case of previous error. */
> >      if (s->fd < 0) {
> >          return size;
> >      }
> >  
> > +    dumpiov[0].iov_base = &hdr;
> > +    dumpiov[0].iov_len = sizeof(hdr);
> > +    caplen = s->pcap_caplen;
> > +
> > +    /* Copy iov, limit maximum size to caplen, and count total input size */
> > +    for (i = 0; i < cnt; i++) {
> > +        dumpiov[i + 1].iov_base = iov[i].iov_base;
> > +        if (size + iov[i].iov_len <= caplen) {
> > +            dumpiov[i + 1].iov_len = iov[i].iov_len;
> > +        } else if (size < caplen) {
> > +            dumpiov[i + 1].iov_len = caplen - size;
> > +        } else {
> > +            dumpiov[i + 1].iov_len = 0;
> > +        }
> > +        size += iov[i].iov_len;
> > +    }
> 
> I believe we could use iov_copy() here.

Looks like that could do the job, too, thanks for the hint! However, I
also need the total size of bytes in the source iov, so I'd then need to
call iov_size() here, too - but I guess it's ok to loop through the iov
twice since dumping is not really time critical.

 Thomas

Re: [Qemu-devel] [PATCH 1/5] net/dump: Add support for receive_iov function

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> Adding a proper receive_iov function to the net dump module. This
> will make it easier to support the dump feature for the -netdev
> option in later patches.
> Also make the receive functions available to the other parts of the
> source code so we can later use them for dumping from net.c, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  net/clients.h |  3 +++
>  net/dump.c    | 48 ++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/net/clients.h b/net/clients.h
> index d47530e..5092f3d 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -29,6 +29,9 @@
>  
>  int net_init_dump(const NetClientOptions *opts, const char *name,
>                    NetClientState *peer, Error **errp);
> +ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size);
> +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> +                             int cnt);
>  
>  #ifdef CONFIG_SLIRP
>  int net_init_slirp(const NetClientOptions *opts, const char *name,
> diff --git a/net/dump.c b/net/dump.c
> index 02c8064..383718a 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -57,27 +57,49 @@ struct pcap_sf_pkthdr {
>      uint32_t len;
>  };
>  
> -static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> +                             int cnt)
>  {
>      DumpState *s = DO_UPCAST(DumpState, nc, nc);
>      struct pcap_sf_pkthdr hdr;
>      int64_t ts;
> -    int caplen;
> +    int caplen, i;
> +    size_t size = 0;
> +    struct iovec dumpiov[cnt + 1];

Variable length array.  Ignorant question: okay to use VLAs in QEMU?

>  
>      /* Early return in case of previous error. */
>      if (s->fd < 0) {
>          return size;

Before your patch: return the size argument.

Afterwards: return zero.  Sure that's what you want?

>      }
>  
> +    dumpiov[0].iov_base = &hdr;
> +    dumpiov[0].iov_len = sizeof(hdr);
> +    caplen = s->pcap_caplen;
> +
> +    /* Copy iov, limit maximum size to caplen, and count total input size */
> +    for (i = 0; i < cnt; i++) {
> +        dumpiov[i + 1].iov_base = iov[i].iov_base;
> +        if (size + iov[i].iov_len <= caplen) {
> +            dumpiov[i + 1].iov_len = iov[i].iov_len;
> +        } else if (size < caplen) {
> +            dumpiov[i + 1].iov_len = caplen - size;
> +        } else {
> +            dumpiov[i + 1].iov_len = 0;

When you hit caplen before the last iovec, this produces trailing
iovec[] with zero iov_len instead of shortening the array.  Okay.

> +        }
> +        size += iov[i].iov_len;
> +    }
> +
>      ts = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 1000000, get_ticks_per_sec());
> -    caplen = size > s->pcap_caplen ? s->pcap_caplen : size;
> +    if (size < caplen) {
> +        caplen = size;
> +    }
>  
>      hdr.ts.tv_sec = ts / 1000000 + s->start_ts;
>      hdr.ts.tv_usec = ts % 1000000;
>      hdr.caplen = caplen;
>      hdr.len = size;
> -    if (write(s->fd, &hdr, sizeof(hdr)) != sizeof(hdr) ||
> -        write(s->fd, buf, caplen) != caplen) {
> +
> +    if (writev(s->fd, dumpiov, cnt + 1) != sizeof(hdr) + caplen) {

Bonus: saves a system call :)

>          qemu_log("-net dump write error - stop dump\n");
>          close(s->fd);
>          s->fd = -1;
> @@ -86,7 +108,16 @@ static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      return size;
>  }
>  
> -static void dump_cleanup(NetClientState *nc)
> +ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> +{
> +    struct iovec iov = {
> +        .iov_base = (void *)buf,
> +        .iov_len = size
> +    };
> +    return net_dump_receive_iov(nc, &iov, 1);
> +}
> +
> +static void net_dump_cleanup(NetClientState *nc)
>  {
>      DumpState *s = DO_UPCAST(DumpState, nc, nc);
>  
> @@ -96,8 +127,9 @@ static void dump_cleanup(NetClientState *nc)
>  static NetClientInfo net_dump_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_DUMP,
>      .size = sizeof(DumpState),
> -    .receive = dump_receive,
> -    .cleanup = dump_cleanup,
> +    .receive = net_dump_receive,
> +    .receive_iov = net_dump_receive_iov,
> +    .cleanup = net_dump_cleanup,
>  };
>  
>  static int net_dump_init(NetClientState *peer, const char *device,

Any particular reason to rename dump_cleanup()?  Not that I mind...

Re: [Qemu-devel] [PATCH 1/5] net/dump: Add support for receive_iov function

[Thomas Huth]

On Fri, 03 Jul 2015 13:06:55 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Thomas Huth <thuth@redhat.com> writes:
> 
> > Adding a proper receive_iov function to the net dump module. This
> > will make it easier to support the dump feature for the -netdev
> > option in later patches.
> > Also make the receive functions available to the other parts of the
> > source code so we can later use them for dumping from net.c, too.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  net/clients.h |  3 +++
> >  net/dump.c    | 48 ++++++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/clients.h b/net/clients.h
> > index d47530e..5092f3d 100644
> > --- a/net/clients.h
> > +++ b/net/clients.h
> > @@ -29,6 +29,9 @@
> >  
> >  int net_init_dump(const NetClientOptions *opts, const char *name,
> >                    NetClientState *peer, Error **errp);
> > +ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size);
> > +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> > +                             int cnt);
> >  
> >  #ifdef CONFIG_SLIRP
> >  int net_init_slirp(const NetClientOptions *opts, const char *name,
> > diff --git a/net/dump.c b/net/dump.c
> > index 02c8064..383718a 100644
> > --- a/net/dump.c
> > +++ b/net/dump.c
> > @@ -57,27 +57,49 @@ struct pcap_sf_pkthdr {
> >      uint32_t len;
> >  };
> >  
> > -static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> > +ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
> > +                             int cnt)
> >  {
> >      DumpState *s = DO_UPCAST(DumpState, nc, nc);
> >      struct pcap_sf_pkthdr hdr;
> >      int64_t ts;
> > -    int caplen;
> > +    int caplen, i;
> > +    size_t size = 0;
> > +    struct iovec dumpiov[cnt + 1];
> 
> Variable length array.  Ignorant question: okay to use VLAs in QEMU?

I guess so - at least there are already some other spots that use VLAs,
e.g. in tap_receive_iov()

> >  
> >      /* Early return in case of previous error. */
> >      if (s->fd < 0) {
> >          return size;
> 
> Before your patch: return the size argument.
> 
> Afterwards: return zero.  Sure that's what you want?

That was of course a stupid bug. Thanks for pointing it out, it will be
fixed in v2!

> >      }
> >  
> > +    dumpiov[0].iov_base = &hdr;
> > +    dumpiov[0].iov_len = sizeof(hdr);
> > +    caplen = s->pcap_caplen;
> > +
> > +    /* Copy iov, limit maximum size to caplen, and count total input size */
> > +    for (i = 0; i < cnt; i++) {
> > +        dumpiov[i + 1].iov_base = iov[i].iov_base;
> > +        if (size + iov[i].iov_len <= caplen) {
> > +            dumpiov[i + 1].iov_len = iov[i].iov_len;
> > +        } else if (size < caplen) {
> > +            dumpiov[i + 1].iov_len = caplen - size;
> > +        } else {
> > +            dumpiov[i + 1].iov_len = 0;
> 
> When you hit caplen before the last iovec, this produces trailing
> iovec[] with zero iov_len instead of shortening the array.  Okay.

I'll use iov_copy in the next version, as suggested by Jason - and that
returns the shortened count if necessary.

> > +        }
> > +        size += iov[i].iov_len;
> > +    }
> > +
> >      ts = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 1000000, get_ticks_per_sec());
> > -    caplen = size > s->pcap_caplen ? s->pcap_caplen : size;
> > +    if (size < caplen) {
> > +        caplen = size;
> > +    }
> >  
> >      hdr.ts.tv_sec = ts / 1000000 + s->start_ts;
> >      hdr.ts.tv_usec = ts % 1000000;
> >      hdr.caplen = caplen;
> >      hdr.len = size;
> > -    if (write(s->fd, &hdr, sizeof(hdr)) != sizeof(hdr) ||
> > -        write(s->fd, buf, caplen) != caplen) {
> > +
> > +    if (writev(s->fd, dumpiov, cnt + 1) != sizeof(hdr) + caplen) {
> 
> Bonus: saves a system call :)
> 
> >          qemu_log("-net dump write error - stop dump\n");
> >          close(s->fd);
> >          s->fd = -1;
> > @@ -86,7 +108,16 @@ static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> >      return size;
> >  }
> >  
> > -static void dump_cleanup(NetClientState *nc)
> > +ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> > +{
> > +    struct iovec iov = {
> > +        .iov_base = (void *)buf,
> > +        .iov_len = size
> > +    };
> > +    return net_dump_receive_iov(nc, &iov, 1);
> > +}
> > +
> > +static void net_dump_cleanup(NetClientState *nc)
> >  {
> >      DumpState *s = DO_UPCAST(DumpState, nc, nc);
> >  
> > @@ -96,8 +127,9 @@ static void dump_cleanup(NetClientState *nc)
> >  static NetClientInfo net_dump_info = {
> >      .type = NET_CLIENT_OPTIONS_KIND_DUMP,
> >      .size = sizeof(DumpState),
> > -    .receive = dump_receive,
> > -    .cleanup = dump_cleanup,
> > +    .receive = net_dump_receive,
> > +    .receive_iov = net_dump_receive_iov,
> > +    .cleanup = net_dump_cleanup,
> >  };
> >  
> >  static int net_dump_init(NetClientState *peer, const char *device,
> 
> Any particular reason to rename dump_cleanup()?  Not that I mind...

It's more consistent ... and in v2 I'll also need this function
exported (to make sure that the fd gets closed for -netdev, too), so I
then should rename it anyway.

 Thomas

[Qemu-devel] [PATCH 2/5] net/dump: Move DumpState into NetClientState

[Thomas Huth]

We want to provide (almost) every netdev device with the ability
to dump network traffic, so let's embed the DumpState into the
NetClientState structure.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/net/net.h |  7 +++++++
 net/dump.c        | 17 +++++------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 6a6cbef..b265047 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -58,6 +58,12 @@ typedef void (SetVnetHdrLen)(NetClientState *, int);
 typedef int (SetVnetLE)(NetClientState *, bool);
 typedef int (SetVnetBE)(NetClientState *, bool);
 
+typedef struct NetDumpState {
+    int64_t start_ts;
+    int fd;
+    int pcap_caplen;
+} NetDumpState;
+
 typedef struct NetClientInfo {
     NetClientOptionsKind type;
     size_t size;
@@ -92,6 +98,7 @@ struct NetClientState {
     NetClientDestructor *destructor;
     unsigned int queue_index;
     unsigned rxfilter_notify_enabled:1;
+    NetDumpState nds;
 };
 
 typedef struct NICState {
diff --git a/net/dump.c b/net/dump.c
index 383718a..b7a8b30 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -29,13 +29,6 @@
 #include "qemu/timer.h"
 #include "hub.h"
 
-typedef struct DumpState {
-    NetClientState nc;
-    int64_t start_ts;
-    int fd;
-    int pcap_caplen;
-} DumpState;
-
 #define PCAP_MAGIC 0xa1b2c3d4
 
 struct pcap_file_hdr {
@@ -60,7 +53,7 @@ struct pcap_sf_pkthdr {
 ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
                              int cnt)
 {
-    DumpState *s = DO_UPCAST(DumpState, nc, nc);
+    NetDumpState *s = &nc->nds;
     struct pcap_sf_pkthdr hdr;
     int64_t ts;
     int caplen, i;
@@ -119,14 +112,14 @@ ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 
 static void net_dump_cleanup(NetClientState *nc)
 {
-    DumpState *s = DO_UPCAST(DumpState, nc, nc);
+    NetDumpState *s = &nc->nds;
 
     close(s->fd);
 }
 
 static NetClientInfo net_dump_info = {
     .type = NET_CLIENT_OPTIONS_KIND_DUMP,
-    .size = sizeof(DumpState),
+    .size = sizeof(NetClientState),
     .receive = net_dump_receive,
     .receive_iov = net_dump_receive_iov,
     .cleanup = net_dump_cleanup,
@@ -138,7 +131,7 @@ static int net_dump_init(NetClientState *peer, const char *device,
 {
     struct pcap_file_hdr hdr;
     NetClientState *nc;
-    DumpState *s;
+    NetDumpState *s;
     struct tm tm;
     int fd;
 
@@ -167,7 +160,7 @@ static int net_dump_init(NetClientState *peer, const char *device,
     snprintf(nc->info_str, sizeof(nc->info_str),
              "dump to %s (len=%d)", filename, len);
 
-    s = DO_UPCAST(DumpState, nc, nc);
+    s = &nc->nds;
 
     s->fd = fd;
     s->pcap_caplen = len;
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/5] net/dump: Rework net-dump init functions

[Thomas Huth]

Moved the creation of the dump client from net_dump_init() into
net_init_dump(), renamed net_dump_init() to net_dump_state_init()
and make it available for other parts of the source code, too,
so we can use it for the -netdev option in a later patch.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 net/clients.h |  2 ++
 net/dump.c    | 24 +++++++++++++-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/net/clients.h b/net/clients.h
index 5092f3d..fc76503 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -29,6 +29,8 @@
 
 int net_init_dump(const NetClientOptions *opts, const char *name,
                   NetClientState *peer, Error **errp);
+int net_dump_state_init(NetClientState *nc, const char *filename, int len,
+                        Error **errp);
 ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size);
 ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
                              int cnt);
diff --git a/net/dump.c b/net/dump.c
index b7a8b30..48a8087 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -125,12 +125,10 @@ static NetClientInfo net_dump_info = {
     .cleanup = net_dump_cleanup,
 };
 
-static int net_dump_init(NetClientState *peer, const char *device,
-                         const char *name, const char *filename, int len,
-                         Error **errp)
+int net_dump_state_init(NetClientState *nc, const char *filename, int len,
+                        Error **errp)
 {
     struct pcap_file_hdr hdr;
-    NetClientState *nc;
     NetDumpState *s;
     struct tm tm;
     int fd;
@@ -155,11 +153,6 @@ static int net_dump_init(NetClientState *peer, const char *device,
         return -1;
     }
 
-    nc = qemu_new_net_client(&net_dump_info, peer, device, name);
-
-    snprintf(nc->info_str, sizeof(nc->info_str),
-             "dump to %s (len=%d)", filename, len);
-
     s = &nc->nds;
 
     s->fd = fd;
@@ -174,10 +167,11 @@ static int net_dump_init(NetClientState *peer, const char *device,
 int net_init_dump(const NetClientOptions *opts, const char *name,
                   NetClientState *peer, Error **errp)
 {
-    int len;
+    int len, rc;
     const char *file;
     char def_file[128];
     const NetdevDumpOptions *dump;
+    NetClientState *nc;
 
     assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
     dump = opts->dump;
@@ -207,5 +201,13 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
         len = 65536;
     }
 
-    return net_dump_init(peer, "dump", name, file, len, errp);
+    nc = qemu_new_net_client(&net_dump_info, peer, "dump", name);
+    snprintf(nc->info_str, sizeof(nc->info_str),
+             "dump to %s (len=%d)", file, len);
+
+    rc = net_dump_state_init(nc, file, len, errp);
+    if (rc) {
+        qemu_del_net_client(nc);
+    }
+    return rc;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/5] net/dump: Rework net-dump init functions

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> Moved the creation of the dump client from net_dump_init() into
> net_init_dump(), renamed net_dump_init() to net_dump_state_init()
> and make it available for other parts of the source code, too,
> so we can use it for the -netdev option in a later patch.

The usual commit message style uses present tense: Move the creation...

>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  net/clients.h |  2 ++
>  net/dump.c    | 24 +++++++++++++-----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/net/clients.h b/net/clients.h
> index 5092f3d..fc76503 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -29,6 +29,8 @@
>  
>  int net_init_dump(const NetClientOptions *opts, const char *name,
>                    NetClientState *peer, Error **errp);
> +int net_dump_state_init(NetClientState *nc, const char *filename, int len,
> +                        Error **errp);
>  ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t size);
>  ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
>                               int cnt);
> diff --git a/net/dump.c b/net/dump.c
> index b7a8b30..48a8087 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -125,12 +125,10 @@ static NetClientInfo net_dump_info = {
>      .cleanup = net_dump_cleanup,
>  };
>  
> -static int net_dump_init(NetClientState *peer, const char *device,
> -                         const char *name, const char *filename, int len,
> -                         Error **errp)
> +int net_dump_state_init(NetClientState *nc, const char *filename, int len,
> +                        Error **errp)
>  {
>      struct pcap_file_hdr hdr;
> -    NetClientState *nc;
>      NetDumpState *s;
>      struct tm tm;
>      int fd;
> @@ -155,11 +153,6 @@ static int net_dump_init(NetClientState *peer, const char *device,
>          return -1;
>      }
>  
> -    nc = qemu_new_net_client(&net_dump_info, peer, device, name);
> -
> -    snprintf(nc->info_str, sizeof(nc->info_str),
> -             "dump to %s (len=%d)", filename, len);
> -
>      s = &nc->nds;
>  
>      s->fd = fd;
> @@ -174,10 +167,11 @@ static int net_dump_init(NetClientState *peer, const char *device,
>  int net_init_dump(const NetClientOptions *opts, const char *name,
>                    NetClientState *peer, Error **errp)
>  {
> -    int len;
> +    int len, rc;
>      const char *file;
>      char def_file[128];
>      const NetdevDumpOptions *dump;
> +    NetClientState *nc;
>  
>      assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
>      dump = opts->dump;
> @@ -207,5 +201,13 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
>          len = 65536;
>      }
>  
> -    return net_dump_init(peer, "dump", name, file, len, errp);
> +    nc = qemu_new_net_client(&net_dump_info, peer, "dump", name);
> +    snprintf(nc->info_str, sizeof(nc->info_str),
> +             "dump to %s (len=%d)", file, len);
> +
> +    rc = net_dump_state_init(nc, file, len, errp);
> +    if (rc) {
> +        qemu_del_net_client(nc);

My first impulse was "Is this a bug fix?"  But then I realized that old
code allocated nc after its last error return, whule the new code does
it before some.  Good.

> +    }
> +    return rc;
>  }

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

[Thomas Huth]

So far it is not possible to dump network traffic with the "-netdev"
option yet - this is only possible with the "-net" option and an
internal "hub".
This patch now fixes this ugliness by adding a proper, generic
dumpfile parameter to the "-netdev" option.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/net/net.h |  1 +
 net/net.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json  | 12 ++++++++++--
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index b265047..62abc98 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -98,6 +98,7 @@ struct NetClientState {
     NetClientDestructor *destructor;
     unsigned int queue_index;
     unsigned rxfilter_notify_enabled:1;
+    unsigned netdev_dump_enabled:1;
     NetDumpState nds;
 };
 
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);
+    }
+
     if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
         ret = nc->info->receive_raw(nc, data, size);
     } else {
@@ -684,6 +690,12 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
         return 0;
     }
 
+    if (nc->netdev_dump_enabled) {
+        net_dump_receive_iov(nc, iov, iovcnt);
+    } else if (sender->netdev_dump_enabled) {
+        net_dump_receive_iov(sender, iov, iovcnt);
+    }
+
     if (nc->info->receive_iov) {
         ret = nc->info->receive_iov(nc, iov, iovcnt);
     } else {
@@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
     return idx;
 }
 
+static int netdev_init_dumpfile(const Netdev *netdev, const char *name,
+                                Error **errp)
+{
+    NetClientState *nc;
+    int dumplen = 65536;
+    int rc;
+
+    if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP
+        && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) {
+        error_setg(errp, "dumping does not work with vhost enabled");
+        return -1;
+    }
+
+    if (netdev->has_dumplen) {
+        dumplen = netdev->dumplen;
+    }
+
+    nc = qemu_find_netdev(name);
+    if (!nc) {
+        error_setg(errp, "failed to lookup netdev for dump");
+        return -1;
+    }
+
+    rc = net_dump_state_init(nc, netdev->dumpfile, dumplen, errp);
+    if (rc == 0) {
+        nc->netdev_dump_enabled = true;
+    }
+
+    return rc;
+}
 
 static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
     const NetClientOptions *opts,
@@ -982,6 +1024,12 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
             }
             return -1;
         }
+
+        if (is_netdev && u.netdev->has_dumpfile) {
+            if (netdev_init_dumpfile(u.netdev, name, errp)) {
+                return -1;
+            }
+        }
     }
     return 0;
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 106008c..79c3ed7 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2528,14 +2528,22 @@
 #
 # @id: identifier for monitor commands.
 #
+# @dumpfile: #optional name of a file where network traffic should be logged
+#            (since: 2.4)
+#
+# @dumplen: #optional maximum length of the network packets in the dump
+#           (since: 2.4)
+#
 # @opts: device type specific properties
 #
 # Since 1.2
 ##
 { 'struct': 'Netdev',
   'data': {
-    'id':   'str',
-    'opts': 'NetClientOptions' } }
+    'id':        'str',
+    '*dumpfile': 'str',
+    '*dumplen':  'int32',
+    'opts':      'NetClientOptions' } }
 
 ##
 # @InetSocketAddress
-- 
1.8.3.1

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

[Jason Wang]

On 06/24/2015 11:56 PM, Thomas Huth wrote:
> So far it is not possible to dump network traffic with the "-netdev"
> option yet - this is only possible with the "-net" option and an
> internal "hub".
> This patch now fixes this ugliness by adding a proper, generic
> dumpfile parameter to the "-netdev" option.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/net/net.h |  1 +
>  net/net.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json  | 12 ++++++++++--
>  3 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index b265047..62abc98 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -98,6 +98,7 @@ struct NetClientState {
>      NetClientDestructor *destructor;
>      unsigned int queue_index;
>      unsigned rxfilter_notify_enabled:1;
> +    unsigned netdev_dump_enabled:1;
>      NetDumpState nds;
>  };
>  
> 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);
> +    }
> +
>      if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
>          ret = nc->info->receive_raw(nc, data, size);
>      } else {
> @@ -684,6 +690,12 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
>          return 0;
>      }
>  
> +    if (nc->netdev_dump_enabled) {
> +        net_dump_receive_iov(nc, iov, iovcnt);
> +    } else if (sender->netdev_dump_enabled) {
> +        net_dump_receive_iov(sender, iov, iovcnt);
> +    }
> +
>      if (nc->info->receive_iov) {
>          ret = nc->info->receive_iov(nc, iov, iovcnt);
>      } else {
> @@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
>      return idx;
>  }
>  
> +static int netdev_init_dumpfile(const Netdev *netdev, const char *name,
> +                                Error **errp)
> +{
> +    NetClientState *nc;
> +    int dumplen = 65536;
> +    int rc;
> +
> +    if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP
> +        && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) {
> +        error_setg(errp, "dumping does not work with vhost enabled");
> +        return -1;

And vhost user is also not suitable here. Looks we can check with
get_vhost_net() here.
> +    }
> +
> +    if (netdev->has_dumplen) {
> +        dumplen = netdev->dumplen;
> +    }
> +
> +    nc = qemu_find_netdev(name);
> +    if (!nc) {
> +        error_setg(errp, "failed to lookup netdev for dump");
> +        return -1;
> +    }

Need consider the case of multiqueue. In this case, more than one
NetClientState was used and needs more thought. Maybe we can start from
only support single queue.

> +
> +    rc = net_dump_state_init(nc, netdev->dumpfile, dumplen, errp);
> +    if (rc == 0) {
> +        nc->netdev_dump_enabled = true;
> +    }
> +
> +    return rc;
> +}
>  
>  static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
>      const NetClientOptions *opts,
> @@ -982,6 +1024,12 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
>              }
>              return -1;
>          }
> +
> +        if (is_netdev && u.netdev->has_dumpfile) {
> +            if (netdev_init_dumpfile(u.netdev, name, errp)) {
> +                return -1;
> +            }
> +        }

Since dump is not a NetClient, we need close the fd when netdev is deleted.

>      }
>      return 0;
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 106008c..79c3ed7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2528,14 +2528,22 @@
>  #
>  # @id: identifier for monitor commands.
>  #
> +# @dumpfile: #optional name of a file where network traffic should be logged
> +#            (since: 2.4)
> +#
> +# @dumplen: #optional maximum length of the network packets in the dump
> +#           (since: 2.4)
> +#
>  # @opts: device type specific properties
>  #
>  # Since 1.2
>  ##
>  { 'struct': 'Netdev',
>    'data': {
> -    'id':   'str',
> -    'opts': 'NetClientOptions' } }
> +    'id':        'str',
> +    '*dumpfile': 'str',
> +    '*dumplen':  'int32',
> +    'opts':      'NetClientOptions' } }
>  
>  ##
>  # @InetSocketAddress

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

[Stefan Hajnoczi]

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

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);
  }

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).

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

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

[Thomas Huth]

[-- 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 --]

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

[Stefan Hajnoczi]

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

On Mon, Jun 29, 2015 at 11:57:15AM +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);
> > > +    }
> > 
> > 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?

qemu_deliver_packet() doesn't know about -netdev vs -device.  It just
knows about NetClientState and peers.

Avoiding the 2nd memory load probably isn't worth baking the assumption
into this code.

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

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

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1595 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);
> > +    }
...
> 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.

 Thomas

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

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

[Stefan Hajnoczi]

[-- 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 --]

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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> So far it is not possible to dump network traffic with the "-netdev"
> option yet - this is only possible with the "-net" option and an
> internal "hub".
> This patch now fixes this ugliness by adding a proper, generic
> dumpfile parameter to the "-netdev" option.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/net/net.h |  1 +
>  net/net.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json  | 12 ++++++++++--
>  3 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index b265047..62abc98 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -98,6 +98,7 @@ struct NetClientState {
>      NetClientDestructor *destructor;
>      unsigned int queue_index;
>      unsigned rxfilter_notify_enabled:1;
> +    unsigned netdev_dump_enabled:1;
>      NetDumpState nds;
>  };
>  
> 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);
> +    }
> +
>      if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) {
>          ret = nc->info->receive_raw(nc, data, size);
>      } else {
> @@ -684,6 +690,12 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
>          return 0;
>      }
>  
> +    if (nc->netdev_dump_enabled) {
> +        net_dump_receive_iov(nc, iov, iovcnt);
> +    } else if (sender->netdev_dump_enabled) {
> +        net_dump_receive_iov(sender, iov, iovcnt);
> +    }
> +
>      if (nc->info->receive_iov) {
>          ret = nc->info->receive_iov(nc, iov, iovcnt);
>      } else {
> @@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
>      return idx;
>  }
>  
> +static int netdev_init_dumpfile(const Netdev *netdev, const char *name,
> +                                Error **errp)
> +{
> +    NetClientState *nc;
> +    int dumplen = 65536;
> +    int rc;
> +
> +    if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP
> +        && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) {
> +        error_setg(errp, "dumping does not work with vhost enabled");
> +        return -1;
> +    }
> +
> +    if (netdev->has_dumplen) {
> +        dumplen = netdev->dumplen;

I keep reading "dumpling" for some reason... %-)

> +    }
> +
> +    nc = qemu_find_netdev(name);
> +    if (!nc) {
> +        error_setg(errp, "failed to lookup netdev for dump");
> +        return -1;

Can this happen?

Hmm, see below.

> +    }
> +
> +    rc = net_dump_state_init(nc, netdev->dumpfile, dumplen, errp);
> +    if (rc == 0) {
> +        nc->netdev_dump_enabled = true;
> +    }
> +
> +    return rc;
> +}
>  
>  static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])(
>      const NetClientOptions *opts,
> @@ -982,6 +1024,12 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)

       if (net_client_init_fun[opts->kind](opts, name, peer, errp) < 0) {
           /* FIXME drop when all init functions store an Error */
           if (errp && !*errp) {
               error_setg(errp, QERR_DEVICE_INIT_FAILED,
                          NetClientOptionsKind_lookup[opts->kind]);
>              }
>              return -1;
>          }
> +
> +        if (is_netdev && u.netdev->has_dumpfile) {
> +            if (netdev_init_dumpfile(u.netdev, name, errp)) {
> +                return -1;
> +            }
> +        }
>      }
>      return 0;
>  }

net_client_init_fun() creates a netdev with this name.  Since it doesn't
return it, we have to look it up with qemu_find_netdev().  So the answer
to "can this happen?" appears to be no.  assert(!rc)?

> diff --git a/qapi-schema.json b/qapi-schema.json
> index 106008c..79c3ed7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2528,14 +2528,22 @@
>  #
>  # @id: identifier for monitor commands.
>  #
> +# @dumpfile: #optional name of a file where network traffic should be logged
> +#            (since: 2.4)
> +#
> +# @dumplen: #optional maximum length of the network packets in the dump
> +#           (since: 2.4)
> +#
>  # @opts: device type specific properties
>  #
>  # Since 1.2
>  ##
>  { 'struct': 'Netdev',
>    'data': {
> -    'id':   'str',
> -    'opts': 'NetClientOptions' } }
> +    'id':        'str',
> +    '*dumpfile': 'str',
> +    '*dumplen':  'int32',
> +    'opts':      'NetClientOptions' } }
>  
>  ##
>  # @InetSocketAddress

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

[Thomas Huth]

On Fri, 03 Jul 2015 13:28:45 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Thomas Huth <thuth@redhat.com> writes:
> 
> > So far it is not possible to dump network traffic with the "-netdev"
> > option yet - this is only possible with the "-net" option and an
> > internal "hub".
> > This patch now fixes this ugliness by adding a proper, generic
> > dumpfile parameter to the "-netdev" option.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  include/net/net.h |  1 +
> >  net/net.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json  | 12 ++++++++++--
> >  3 files changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/net.h b/include/net/net.h
> > index b265047..62abc98 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
...
> > @@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
> >      return idx;
> >  }
> >  
> > +static int netdev_init_dumpfile(const Netdev *netdev, const char *name,
> > +                                Error **errp)
> > +{
> > +    NetClientState *nc;
> > +    int dumplen = 65536;
> > +    int rc;
> > +
> > +    if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP
> > +        && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) {
> > +        error_setg(errp, "dumping does not work with vhost enabled");
> > +        return -1;
> > +    }
> > +
> > +    if (netdev->has_dumplen) {
> > +        dumplen = netdev->dumplen;
> 
> I keep reading "dumpling" for some reason... %-)

Were you hungry while reading the patch? :-)

> > +    }
> > +
> > +    nc = qemu_find_netdev(name);
> > +    if (!nc) {
> > +        error_setg(errp, "failed to lookup netdev for dump");
> > +        return -1;
> 
> Can this happen?
> 
> Hmm, see below.
...
> net_client_init_fun() creates a netdev with this name.  Since it doesn't
> return it, we have to look it up with qemu_find_netdev().  So the answer
> to "can this happen?" appears to be no.  assert(!rc)?

I agree, an assert() should be enough here!

 Thomas

[Qemu-devel] [PATCH 5/5] qemu options: Add information about dumpfile to help text

[Thomas Huth]

Describe the new dumpfile and dumplen options in the help text.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qemu-options.hx | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 987dbf0..138aa87 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1461,20 +1461,21 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
 #ifdef CONFIG_SLIRP
     "-netdev user,id=str[,net=addr[/mask]][,host=addr][,restrict=on|off]\n"
     "         [,hostname=host][,dhcpstart=addr][,dns=addr][,dnssearch=domain][,tftp=dir]\n"
-    "         [,bootfile=f][,hostfwd=rule][,guestfwd=rule]"
+    "         [,bootfile=f][,hostfwd=rule][,guestfwd=rule][,dumpfile=file[,dumplen=len]]\n"
 #ifndef _WIN32
-                                             "[,smb=dir[,smbserver=addr]]\n"
+    "         [,smb=dir[,smbserver=addr]]\n"
 #endif
     "                configure a user mode network backend with ID 'str',\n"
     "                its DHCP server and optional services\n"
 #endif
 #ifdef _WIN32
-    "-netdev tap,id=str,ifname=name\n"
+    "-netdev tap,id=str,ifname=name[,dumpfile=file[,dumplen=len]]\n"
     "                configure a host TAP network backend with ID 'str'\n"
 #else
     "-netdev tap,id=str[,fd=h][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile]\n"
     "         [,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off]\n"
     "         [,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n]\n"
+    "         [,dumpfile=file[,dumplen=len]]\n"
     "                configure a host TAP network backend with ID 'str'\n"
     "                use network scripts 'file' (default=" DEFAULT_NETWORK_SCRIPT ")\n"
     "                to configure it and 'dfile' (default=" DEFAULT_NETWORK_DOWN_SCRIPT ")\n"
@@ -1494,7 +1495,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
     "                use 'vhostfd=h' to connect to an already opened vhost net device\n"
     "                use 'vhostfds=x:y:...:z to connect to multiple already opened vhost net devices\n"
     "                use 'queues=n' to specify the number of queues to be created for multiqueue TAP\n"
-    "-netdev bridge,id=str[,br=bridge][,helper=helper]\n"
+    "-netdev bridge,id=str[,br=bridge][,helper=helper][,dumpfile=file[,dumplen=len]]\n"
     "                configure a host TAP network backend with ID 'str' that is\n"
     "                connected to a bridge (default=" DEFAULT_BRIDGE_INTERFACE ")\n"
     "                using the program 'helper (default=" DEFAULT_BRIDGE_HELPER ")\n"
@@ -1503,7 +1504,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
     "-netdev l2tpv3,id=str,src=srcaddr,dst=dstaddr[,srcport=srcport][,dstport=dstport]\n"
     "         [,rxsession=rxsession],txsession=txsession[,ipv6=on/off][,udp=on/off]\n"
     "         [,cookie64=on/off][,counter][,pincounter][,txcookie=txcookie]\n"
-    "         [,rxcookie=rxcookie][,offset=offset]\n"
+    "         [,rxcookie=rxcookie][,offset=offset][,dumpfile=file[,dumplen=len]]\n"
     "                configure a network backend with ID 'str' connected to\n"
     "                an Ethernet over L2TPv3 pseudowire.\n"
     "                Linux kernel 3.3+ as well as most routers can talk\n"
@@ -1527,30 +1528,34 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
     "                use 'offset=X' to add an extra offset between header and data\n"
 #endif
     "-netdev socket,id=str[,fd=h][,listen=[host]:port][,connect=host:port]\n"
+    "         [,dumpfile=file[,dumplen=len]]\n"
     "                configure a network backend to connect to another network\n"
     "                using a socket connection\n"
     "-netdev socket,id=str[,fd=h][,mcast=maddr:port[,localaddr=addr]]\n"
+    "         [,dumpfile=file[,dumplen=len]]\n"
     "                configure a network backend to connect to a multicast maddr and port\n"
     "                use 'localaddr=addr' to specify the host address to send packets from\n"
     "-netdev socket,id=str[,fd=h][,udp=host:port][,localaddr=host:port]\n"
+    "         [,dumpfile=file[,dumplen=len]]\n"
     "                configure a network backend to connect to another network\n"
     "                using an UDP tunnel\n"
 #ifdef CONFIG_VDE
     "-netdev vde,id=str[,sock=socketpath][,port=n][,group=groupname][,mode=octalmode]\n"
+    "         [,dumpfile=file[,dumplen=len]]\n"
     "                configure a network backend to connect to port 'n' of a vde switch\n"
     "                running on host and listening for incoming connections on 'socketpath'.\n"
     "                Use group 'groupname' and mode 'octalmode' to change default\n"
     "                ownership and permissions for communication port.\n"
 #endif
 #ifdef CONFIG_NETMAP
-    "-netdev netmap,id=str,ifname=name[,devname=nmname]\n"
+    "-netdev netmap,id=str,ifname=name[,devname=nmname][,dumpfile=file[,dumplen=len]]\n"
     "                attach to the existing netmap-enabled network interface 'name', or to a\n"
     "                VALE port (created on the fly) called 'name' ('nmname' is name of the \n"
     "                netmap device, defaults to '/dev/netmap')\n"
 #endif
     "-netdev vhost-user,id=str,chardev=dev[,vhostforce=on|off]\n"
     "                configure a vhost-user network, backed by a chardev 'dev'\n"
-    "-netdev hubport,id=str,hubid=n\n"
+    "-netdev hubport,id=str,hubid=n[,dumpfile=file[,dumplen=len]]\n"
     "                configure a hub port on QEMU VLAN 'n'\n", QEMU_ARCH_ALL)
 DEF("net", HAS_ARG, QEMU_OPTION_net,
     "-net nic[,vlan=n][,macaddr=mac][,model=type][,name=str][,addr=str][,vectors=v]\n"
@@ -1981,10 +1986,15 @@ qemu -m 512 -object memory-backend-file,id=mem,size=512M,mem-path=/hugetlbfs,sha
      -device virtio-net-pci,netdev=net0
 @end example
 
+@item -netdev ...,dumpfile=@var{file}[,dumplen=@var{len}]
 @item -net dump[,vlan=@var{n}][,file=@var{file}][,len=@var{len}]
-Dump network traffic on VLAN @var{n} to file @var{file} (@file{qemu-vlan0.pcap} by default).
+Dump network traffic to file @var{file} (@file{qemu-vlan0.pcap} by default).
 At most @var{len} bytes (64k by default) per packet are stored. The file format is
 libpcap, so it can be analyzed with tools such as tcpdump or Wireshark.
+When dumping is wanted for -netdev devices, simply add the 'dumpfile=file'
+option as additional parameter. When dumping is wanted for a legacy -net
+network, specify an additional "-net dump,..." to dump the traffic on
+VLAN @var{n} (0 by default).
 
 @item -net none
 Indicate that no network devices should be configured. It is used to
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/5] qemu options: Add information about dumpfile to help text

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> Describe the new dumpfile and dumplen options in the help text.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Could be squashed into PATCH 4.  Separate patch is of course okay, too.

Re: [Qemu-devel] [PATCH 0/5] Network traffic dumping for -netdev, second try

[Stefan Hajnoczi]

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

On Wed, Jun 24, 2015 at 05:56:16PM +0200, Thomas Huth wrote:
> The "-net dump" option only works with the "-net" option. So far, it
> is not possible to dump network traffic with the "-netdev" option yet.
> This patch series now fixes this ugliness by enabling dumping for the
> "-netdev" option, too.
> 
> Unlike with my first attempt a couple of weeks ago ("net: Enable vlans
> and dump for -netdev, too"), this patch series now does not "abuse"
> the disliked internal vlan hub infrastructure anymore but introduces
> a new, clean "dumpfile=xxx" option for the -netdev parameters instead.
> 
> It's likely too late for version 2.4 for this patch series, but it
> would be great to get some review feedback for this anyway.
> 
> Thomas Huth (5):
>   net/dump: Add support for receive_iov function
>   net/dump: Move DumpState into NetClientState
>   net/dump: Rework net-dump init functions
>   net/dump: Add dump option for netdev devices
>   qemu options: Add information about dumpfile to help text
> 
>  include/net/net.h |  8 +++++
>  net/clients.h     |  5 ++++
>  net/dump.c        | 89 ++++++++++++++++++++++++++++++++++++-------------------
>  net/net.c         | 48 ++++++++++++++++++++++++++++++
>  qapi-schema.json  | 12 ++++++--
>  qemu-options.hx   | 26 +++++++++++-----
>  6 files changed, 147 insertions(+), 41 deletions(-)

Overall this approach looks good, let's do it in QEMU 2.5.

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

Re: [Qemu-devel] [PATCH 0/5] Network traffic dumping for -netdev, second try

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Jun 24, 2015 at 05:56:16PM +0200, Thomas Huth wrote:
>> The "-net dump" option only works with the "-net" option. So far, it
>> is not possible to dump network traffic with the "-netdev" option yet.
>> This patch series now fixes this ugliness by enabling dumping for the
>> "-netdev" option, too.
>> 
>> Unlike with my first attempt a couple of weeks ago ("net: Enable vlans
>> and dump for -netdev, too"), this patch series now does not "abuse"
>> the disliked internal vlan hub infrastructure anymore but introduces
>> a new, clean "dumpfile=xxx" option for the -netdev parameters instead.
>> 
>> It's likely too late for version 2.4 for this patch series, but it
>> would be great to get some review feedback for this anyway.
>> 
>> Thomas Huth (5):
>>   net/dump: Add support for receive_iov function
>>   net/dump: Move DumpState into NetClientState
>>   net/dump: Rework net-dump init functions
>>   net/dump: Add dump option for netdev devices
>>   qemu options: Add information about dumpfile to help text
>> 
>>  include/net/net.h |  8 +++++
>>  net/clients.h     |  5 ++++
>>  net/dump.c        | 89 ++++++++++++++++++++++++++++++++++++-------------------
>>  net/net.c         | 48 ++++++++++++++++++++++++++++++
>>  qapi-schema.json  | 12 ++++++--
>>  qemu-options.hx   | 26 +++++++++++-----
>>  6 files changed, 147 insertions(+), 41 deletions(-)
>
> Overall this approach looks good, let's do it in QEMU 2.5.

Seconded.

[Qemu-devel] [PATCH 0/5] Network traffic dumping via netfilter

[Thomas Huth]

The "-net dump" option only works with the "-net" option. So far, it
is not possible to dump network traffic with the "-netdev" option yet.
This patch series now fixes this ugliness by enabling dumping for the
netdev devices via the new netfilter infrastructure from Yang Hongyang
(integration into that infrastructure was pretty easy, so thanks for
this great work!)
The dumping filter can be used like this for example:

 ppc64-softmmu/qemu-system-ppc64 -device virtio-net,netdev=mynet \
     -netdev user,id=mynet,tftp=/tmp/tftp,bootfile=zImage \
     -object filter-dump,id=f0,netdev=mynet,file=/tmp/dumpfile.dat

The patches have to be applied on top of the Jason's net branch
(since the netfilter patches have not been pulled into master yet).

Thomas Huth (5):
  net/dump: Add support for receive_iov function
  net/dump: Rework net-dump init functions
  net/dump: Separate the NetClientState from the DumpState
  net/dump: Provide the dumping facility as a net filter
  net/dump: Add documentation

 net/dump.c      | 228 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 qemu-options.hx |   8 ++
 vl.c            |   8 +-
 3 files changed, 212 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/5] net/dump: Add support for receive_iov function

[Thomas Huth]

Adding a proper receive_iov function to the net dump module.
This will make it easier to support the dump filter feature for
the -netdev option in later patches.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 net/dump.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/dump.c b/net/dump.c
index 02c8064..f8afb83 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -25,6 +25,7 @@
 #include "clients.h"
 #include "qemu-common.h"
 #include "qemu/error-report.h"
+#include "qemu/iov.h"
 #include "qemu/log.h"
 #include "qemu/timer.h"
 #include "hub.h"
@@ -57,12 +58,15 @@ struct pcap_sf_pkthdr {
     uint32_t len;
 };
 
-static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
+static ssize_t dump_receive_iov(NetClientState *nc, const struct iovec *iov,
+                                int cnt)
 {
     DumpState *s = DO_UPCAST(DumpState, nc, nc);
     struct pcap_sf_pkthdr hdr;
     int64_t ts;
     int caplen;
+    size_t size = iov_size(iov, cnt);
+    struct iovec dumpiov[cnt + 1];
 
     /* Early return in case of previous error. */
     if (s->fd < 0) {
@@ -76,8 +80,12 @@ static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     hdr.ts.tv_usec = ts % 1000000;
     hdr.caplen = caplen;
     hdr.len = size;
-    if (write(s->fd, &hdr, sizeof(hdr)) != sizeof(hdr) ||
-        write(s->fd, buf, caplen) != caplen) {
+
+    dumpiov[0].iov_base = &hdr;
+    dumpiov[0].iov_len = sizeof(hdr);
+    cnt = iov_copy(&dumpiov[1], cnt, iov, cnt, 0, caplen);
+
+    if (writev(s->fd, dumpiov, cnt + 1) != sizeof(hdr) + caplen) {
         qemu_log("-net dump write error - stop dump\n");
         close(s->fd);
         s->fd = -1;
@@ -86,6 +94,15 @@ static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     return size;
 }
 
+static ssize_t dump_receive(NetClientState *nc, const uint8_t *buf, size_t size)
+{
+    struct iovec iov = {
+        .iov_base = (void *)buf,
+        .iov_len = size
+    };
+    return dump_receive_iov(nc, &iov, 1);
+}
+
 static void dump_cleanup(NetClientState *nc)
 {
     DumpState *s = DO_UPCAST(DumpState, nc, nc);
@@ -97,6 +114,7 @@ static NetClientInfo net_dump_info = {
     .type = NET_CLIENT_OPTIONS_KIND_DUMP,
     .size = sizeof(DumpState),
     .receive = dump_receive,
+    .receive_iov = dump_receive_iov,
     .cleanup = dump_cleanup,
 };
 
-- 
1.8.3.1