On Fri, 03 Jul 2015 13:06:55 +0200 Markus Armbruster <[email protected]> wrote:
> Thomas Huth <[email protected]> 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 <[email protected]> > > --- > > 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
