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

[Markus Armbruster]

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?

>  
>      /* 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...