From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57000) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZDckQ-0000Gp-LD for qemu-devel@nongnu.org; Fri, 10 Jul 2015 14:10:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZDckL-0004vZ-Lz for qemu-devel@nongnu.org; Fri, 10 Jul 2015 14:10:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34854) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZDckL-0004vB-Ed for qemu-devel@nongnu.org; Fri, 10 Jul 2015 14:10:01 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 5EBB7165BC5 for ; Fri, 10 Jul 2015 18:10:00 +0000 (UTC) Date: Fri, 10 Jul 2015 20:09:54 +0200 From: Thomas Huth Message-ID: <20150710200954.7349f5a3@thh440s> In-Reply-To: <87y4ixbiuo.fsf@blackfin.pond.sub.org> References: <1435161381-31521-1-git-send-email-thuth@redhat.com> <1435161381-31521-2-git-send-email-thuth@redhat.com> <87y4ixbiuo.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] net/dump: Add support for receive_iov function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi On Fri, 03 Jul 2015 13:06:55 +0200 Markus Armbruster wrote: > Thomas Huth 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 > > --- > > 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