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 <address@hidden> wrote:

> Thomas Huth <address@hidden> 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 <address@hidden>
> > ---
> >  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