| Message ID | 1435161381-31521-2-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
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,
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
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...
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
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,
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(-)