| Message ID | 1435161381-31521-5-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/24/2015 11:56 PM, Thomas Huth wrote: > So far it is not possible to dump network traffic with the "-netdev" > option yet - this is only possible with the "-net" option and an > internal "hub". > This patch now fixes this ugliness by adding a proper, generic > dumpfile parameter to the "-netdev" option. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > include/net/net.h | 1 + > net/net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > qapi-schema.json | 12 ++++++++++-- > 3 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/include/net/net.h b/include/net/net.h > index b265047..62abc98 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -98,6 +98,7 @@ struct NetClientState { > NetClientDestructor *destructor; > unsigned int queue_index; > unsigned rxfilter_notify_enabled:1; > + unsigned netdev_dump_enabled:1; > NetDumpState nds; > }; > > diff --git a/net/net.c b/net/net.c > index cc36c7b..8871b77 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > return 0; > } > > + if (nc->netdev_dump_enabled) { > + net_dump_receive(nc, data, size); > + } else if (sender->netdev_dump_enabled) { > + net_dump_receive(sender, data, size); > + } > + > if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > ret = nc->info->receive_raw(nc, data, size); > } else { > @@ -684,6 +690,12 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > return 0; > } > > + if (nc->netdev_dump_enabled) { > + net_dump_receive_iov(nc, iov, iovcnt); > + } else if (sender->netdev_dump_enabled) { > + net_dump_receive_iov(sender, iov, iovcnt); > + } > + > if (nc->info->receive_iov) { > ret = nc->info->receive_iov(nc, iov, iovcnt); > } else { > @@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name, > return idx; > } > > +static int netdev_init_dumpfile(const Netdev *netdev, const char *name, > + Error **errp) > +{ > + NetClientState *nc; > + int dumplen = 65536; > + int rc; > + > + if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP > + && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) { > + error_setg(errp, "dumping does not work with vhost enabled"); > + return -1; And vhost user is also not suitable here. Looks we can check with get_vhost_net() here. > + } > + > + if (netdev->has_dumplen) { > + dumplen = netdev->dumplen; > + } > + > + nc = qemu_find_netdev(name); > + if (!nc) { > + error_setg(errp, "failed to lookup netdev for dump"); > + return -1; > + } Need consider the case of multiqueue. In this case, more than one NetClientState was used and needs more thought. Maybe we can start from only support single queue. > + > + rc = net_dump_state_init(nc, netdev->dumpfile, dumplen, errp); > + if (rc == 0) { > + nc->netdev_dump_enabled = true; > + } > + > + return rc; > +} > > static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])( > const NetClientOptions *opts, > @@ -982,6 +1024,12 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) > } > return -1; > } > + > + if (is_netdev && u.netdev->has_dumpfile) { > + if (netdev_init_dumpfile(u.netdev, name, errp)) { > + return -1; > + } > + } Since dump is not a NetClient, we need close the fd when netdev is deleted. > } > return 0; > } > diff --git a/qapi-schema.json b/qapi-schema.json > index 106008c..79c3ed7 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2528,14 +2528,22 @@ > # > # @id: identifier for monitor commands. > # > +# @dumpfile: #optional name of a file where network traffic should be logged > +# (since: 2.4) > +# > +# @dumplen: #optional maximum length of the network packets in the dump > +# (since: 2.4) > +# > # @opts: device type specific properties > # > # Since 1.2 > ## > { 'struct': 'Netdev', > 'data': { > - 'id': 'str', > - 'opts': 'NetClientOptions' } } > + 'id': 'str', > + '*dumpfile': 'str', > + '*dumplen': 'int32', > + 'opts': 'NetClientOptions' } } > > ## > # @InetSocketAddress
On Wed, Jun 24, 2015 at 05:56:20PM +0200, Thomas Huth wrote: > diff --git a/net/net.c b/net/net.c > index cc36c7b..8871b77 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > return 0; > } > > + if (nc->netdev_dump_enabled) { > + net_dump_receive(nc, data, size); > + } else if (sender->netdev_dump_enabled) { > + net_dump_receive(sender, data, size); > + } Why "else if"? If two interfaces have packet capture enabled then both should get the full traffic log: if (nc->netdev_dump_enabled) { net_dump_receive(nc, data, size); } if (sender->netdev_dump_enabled) { net_dump_receive(sender, data, size); } Perhaps dumping should happen after ->receive() has returned size. If ->receive() returns -1 the packet is discarded, and if it returns 0 the packet is queued (not delivered yet). If you dump unconditionally before ->receive() you will see queued packets dumped multiple times (each time the queue gets flushed).
On Fri, 26 Jun 2015 10:44:59 +0100 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Jun 24, 2015 at 05:56:20PM +0200, Thomas Huth wrote: > > diff --git a/net/net.c b/net/net.c > > index cc36c7b..8871b77 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > > return 0; > > } > > > > + if (nc->netdev_dump_enabled) { > > + net_dump_receive(nc, data, size); > > + } else if (sender->netdev_dump_enabled) { > > + net_dump_receive(sender, data, size); > > + } > > Why "else if"? If two interfaces have packet capture enabled then both > should get the full traffic log: > > if (nc->netdev_dump_enabled) { > net_dump_receive(nc, data, size); > } > if (sender->netdev_dump_enabled) { > net_dump_receive(sender, data, size); > } I think I assumed that only the interfaces that are created with "-netdev" can get a dump option. So it's either the receiver or the sender that dumps. If both interfaces would have the dump flag set, that would mean that two interfaces created with "-netdev" are talking to each other - and this can not happen, can it? > Perhaps dumping should happen after ->receive() has returned size. If > ->receive() returns -1 the packet is discarded, and if it returns 0 the > packet is queued (not delivered yet). > > If you dump unconditionally before ->receive() you will see queued > packets dumped multiple times (each time the queue gets flushed). Good point, thanks, I'll move the code accordingly. Thomas
On Fri, 26 Jun 2015 10:44:59 +0100 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Jun 24, 2015 at 05:56:20PM +0200, Thomas Huth wrote: > > diff --git a/net/net.c b/net/net.c > > index cc36c7b..8871b77 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > > return 0; > > } > > > > + if (nc->netdev_dump_enabled) { > > + net_dump_receive(nc, data, size); > > + } else if (sender->netdev_dump_enabled) { > > + net_dump_receive(sender, data, size); > > + } ... > Perhaps dumping should happen after ->receive() has returned size. If > ->receive() returns -1 the packet is discarded, and if it returns 0 the > packet is queued (not delivered yet). > > If you dump unconditionally before ->receive() you will see queued > packets dumped multiple times (each time the queue gets flushed). I've now tried this, but then I suddenly get the packets in the wrong order in the dump file (when using slirp networking), e.g.: 1 0.000000 10.0.2.2 -> 255.255.255.255 DHCP 590 DHCP Offer 2 0.000012 0.0.0.0 -> 255.255.255.255 DHCP 342 DHCP Discover instead of: 1 0.000000 0.0.0.0 -> 255.255.255.255 DHCP 342 DHCP Discover 2 0.000035 10.0.2.2 -> 255.255.255.255 DHCP 590 DHCP Offer Looks like with slirp, the answer is already sent before the initial receive() function returns? Any idea how to avoid that issue? If not, I think I'll simply keep the dump hooks before calling the receive functions. Thomas
On Mon, Jun 29, 2015 at 11:57:15AM +0200, Thomas Huth wrote: > On Fri, 26 Jun 2015 10:44:59 +0100 > Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > On Wed, Jun 24, 2015 at 05:56:20PM +0200, Thomas Huth wrote: > > > diff --git a/net/net.c b/net/net.c > > > index cc36c7b..8871b77 100644 > > > --- a/net/net.c > > > +++ b/net/net.c > > > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > > > return 0; > > > } > > > > > > + if (nc->netdev_dump_enabled) { > > > + net_dump_receive(nc, data, size); > > > + } else if (sender->netdev_dump_enabled) { > > > + net_dump_receive(sender, data, size); > > > + } > > > > Why "else if"? If two interfaces have packet capture enabled then both > > should get the full traffic log: > > > > if (nc->netdev_dump_enabled) { > > net_dump_receive(nc, data, size); > > } > > if (sender->netdev_dump_enabled) { > > net_dump_receive(sender, data, size); > > } > > I think I assumed that only the interfaces that are created with > "-netdev" can get a dump option. So it's either the receiver or the > sender that dumps. If both interfaces would have the dump flag set, > that would mean that two interfaces created with "-netdev" are talking > to each other - and this can not happen, can it? qemu_deliver_packet() doesn't know about -netdev vs -device. It just knows about NetClientState and peers. Avoiding the 2nd memory load probably isn't worth baking the assumption into this code.
On Tue, Jun 30, 2015 at 12:37:46PM +0200, Thomas Huth wrote: > On Fri, 26 Jun 2015 10:44:59 +0100 > Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > On Wed, Jun 24, 2015 at 05:56:20PM +0200, Thomas Huth wrote: > > > diff --git a/net/net.c b/net/net.c > > > index cc36c7b..8871b77 100644 > > > --- a/net/net.c > > > +++ b/net/net.c > > > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > > > return 0; > > > } > > > > > > + if (nc->netdev_dump_enabled) { > > > + net_dump_receive(nc, data, size); > > > + } else if (sender->netdev_dump_enabled) { > > > + net_dump_receive(sender, data, size); > > > + } > ... > > Perhaps dumping should happen after ->receive() has returned size. If > > ->receive() returns -1 the packet is discarded, and if it returns 0 the > > packet is queued (not delivered yet). > > > > If you dump unconditionally before ->receive() you will see queued > > packets dumped multiple times (each time the queue gets flushed). > > I've now tried this, but then I suddenly get the packets in the wrong > order in the dump file (when using slirp networking), e.g.: > > 1 0.000000 10.0.2.2 -> 255.255.255.255 DHCP 590 DHCP Offer > 2 0.000012 0.0.0.0 -> 255.255.255.255 DHCP 342 DHCP Discover > > instead of: > > 1 0.000000 0.0.0.0 -> 255.255.255.255 DHCP 342 DHCP Discover > 2 0.000035 10.0.2.2 -> 255.255.255.255 DHCP 590 DHCP Offer > > Looks like with slirp, the answer is already sent before the initial > receive() function returns? > > Any idea how to avoid that issue? If not, I think I'll simply keep the > dump hooks before calling the receive functions. I see. So either way the packet capture will be misleading. Please leave add a comment to the code mentioning that either queued packets are duplicated or slirp traffic is out-of-order. That way we won't forget why the code is the way it is. Stefan
Thomas Huth <thuth@redhat.com> writes: > So far it is not possible to dump network traffic with the "-netdev" > option yet - this is only possible with the "-net" option and an > internal "hub". > This patch now fixes this ugliness by adding a proper, generic > dumpfile parameter to the "-netdev" option. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > include/net/net.h | 1 + > net/net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > qapi-schema.json | 12 ++++++++++-- > 3 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/include/net/net.h b/include/net/net.h > index b265047..62abc98 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -98,6 +98,7 @@ struct NetClientState { > NetClientDestructor *destructor; > unsigned int queue_index; > unsigned rxfilter_notify_enabled:1; > + unsigned netdev_dump_enabled:1; > NetDumpState nds; > }; > > diff --git a/net/net.c b/net/net.c > index cc36c7b..8871b77 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > return 0; > } > > + if (nc->netdev_dump_enabled) { > + net_dump_receive(nc, data, size); > + } else if (sender->netdev_dump_enabled) { > + net_dump_receive(sender, data, size); > + } > + > if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > ret = nc->info->receive_raw(nc, data, size); > } else { > @@ -684,6 +690,12 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > return 0; > } > > + if (nc->netdev_dump_enabled) { > + net_dump_receive_iov(nc, iov, iovcnt); > + } else if (sender->netdev_dump_enabled) { > + net_dump_receive_iov(sender, iov, iovcnt); > + } > + > if (nc->info->receive_iov) { > ret = nc->info->receive_iov(nc, iov, iovcnt); > } else { > @@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name, > return idx; > } > > +static int netdev_init_dumpfile(const Netdev *netdev, const char *name, > + Error **errp) > +{ > + NetClientState *nc; > + int dumplen = 65536; > + int rc; > + > + if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP > + && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) { > + error_setg(errp, "dumping does not work with vhost enabled"); > + return -1; > + } > + > + if (netdev->has_dumplen) { > + dumplen = netdev->dumplen; I keep reading "dumpling" for some reason... %-) > + } > + > + nc = qemu_find_netdev(name); > + if (!nc) { > + error_setg(errp, "failed to lookup netdev for dump"); > + return -1; Can this happen? Hmm, see below. > + } > + > + rc = net_dump_state_init(nc, netdev->dumpfile, dumplen, errp); > + if (rc == 0) { > + nc->netdev_dump_enabled = true; > + } > + > + return rc; > +} > > static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])( > const NetClientOptions *opts, > @@ -982,6 +1024,12 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) if (net_client_init_fun[opts->kind](opts, name, peer, errp) < 0) { /* FIXME drop when all init functions store an Error */ if (errp && !*errp) { error_setg(errp, QERR_DEVICE_INIT_FAILED, NetClientOptionsKind_lookup[opts->kind]); > } > return -1; > } > + > + if (is_netdev && u.netdev->has_dumpfile) { > + if (netdev_init_dumpfile(u.netdev, name, errp)) { > + return -1; > + } > + } > } > return 0; > } net_client_init_fun() creates a netdev with this name. Since it doesn't return it, we have to look it up with qemu_find_netdev(). So the answer to "can this happen?" appears to be no. assert(!rc)? > diff --git a/qapi-schema.json b/qapi-schema.json > index 106008c..79c3ed7 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2528,14 +2528,22 @@ > # > # @id: identifier for monitor commands. > # > +# @dumpfile: #optional name of a file where network traffic should be logged > +# (since: 2.4) > +# > +# @dumplen: #optional maximum length of the network packets in the dump > +# (since: 2.4) > +# > # @opts: device type specific properties > # > # Since 1.2 > ## > { 'struct': 'Netdev', > 'data': { > - 'id': 'str', > - 'opts': 'NetClientOptions' } } > + 'id': 'str', > + '*dumpfile': 'str', > + '*dumplen': 'int32', > + 'opts': 'NetClientOptions' } } > > ## > # @InetSocketAddress
On Fri, 03 Jul 2015 13:28:45 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Thomas Huth <thuth@redhat.com> writes: > > > So far it is not possible to dump network traffic with the "-netdev" > > option yet - this is only possible with the "-net" option and an > > internal "hub". > > This patch now fixes this ugliness by adding a proper, generic > > dumpfile parameter to the "-netdev" option. > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > include/net/net.h | 1 + > > net/net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > qapi-schema.json | 12 ++++++++++-- > > 3 files changed, 59 insertions(+), 2 deletions(-) > > > > diff --git a/include/net/net.h b/include/net/net.h > > index b265047..62abc98 100644 > > --- a/include/net/net.h > > +++ b/include/net/net.h ... > > @@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name, > > return idx; > > } > > > > +static int netdev_init_dumpfile(const Netdev *netdev, const char *name, > > + Error **errp) > > +{ > > + NetClientState *nc; > > + int dumplen = 65536; > > + int rc; > > + > > + if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP > > + && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) { > > + error_setg(errp, "dumping does not work with vhost enabled"); > > + return -1; > > + } > > + > > + if (netdev->has_dumplen) { > > + dumplen = netdev->dumplen; > > I keep reading "dumpling" for some reason... %-) Were you hungry while reading the patch? :-) > > + } > > + > > + nc = qemu_find_netdev(name); > > + if (!nc) { > > + error_setg(errp, "failed to lookup netdev for dump"); > > + return -1; > > Can this happen? > > Hmm, see below. ... > net_client_init_fun() creates a netdev with this name. Since it doesn't > return it, we have to look it up with qemu_find_netdev(). So the answer > to "can this happen?" appears to be no. assert(!rc)? I agree, an assert() should be enough here! Thomas
diff --git a/include/net/net.h b/include/net/net.h index b265047..62abc98 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -98,6 +98,7 @@ struct NetClientState { NetClientDestructor *destructor; unsigned int queue_index; unsigned rxfilter_notify_enabled:1; + unsigned netdev_dump_enabled:1; NetDumpState nds; }; diff --git a/net/net.c b/net/net.c index cc36c7b..8871b77 100644 --- a/net/net.c +++ b/net/net.c @@ -568,6 +568,12 @@ ssize_t qemu_deliver_packet(NetClientState *sender, return 0; } + if (nc->netdev_dump_enabled) { + net_dump_receive(nc, data, size); + } else if (sender->netdev_dump_enabled) { + net_dump_receive(sender, data, size); + } + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { ret = nc->info->receive_raw(nc, data, size); } else { @@ -684,6 +690,12 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, return 0; } + if (nc->netdev_dump_enabled) { + net_dump_receive_iov(nc, iov, iovcnt); + } else if (sender->netdev_dump_enabled) { + net_dump_receive_iov(sender, iov, iovcnt); + } + if (nc->info->receive_iov) { ret = nc->info->receive_iov(nc, iov, iovcnt); } else { @@ -877,6 +889,36 @@ static int net_init_nic(const NetClientOptions *opts, const char *name, return idx; } +static int netdev_init_dumpfile(const Netdev *netdev, const char *name, + Error **errp) +{ + NetClientState *nc; + int dumplen = 65536; + int rc; + + if (netdev->opts->kind == NET_CLIENT_OPTIONS_KIND_TAP + && netdev->opts->tap->has_vhost && netdev->opts->tap->vhost) { + error_setg(errp, "dumping does not work with vhost enabled"); + return -1; + } + + if (netdev->has_dumplen) { + dumplen = netdev->dumplen; + } + + nc = qemu_find_netdev(name); + if (!nc) { + error_setg(errp, "failed to lookup netdev for dump"); + return -1; + } + + rc = net_dump_state_init(nc, netdev->dumpfile, dumplen, errp); + if (rc == 0) { + nc->netdev_dump_enabled = true; + } + + return rc; +} static int (* const net_client_init_fun[NET_CLIENT_OPTIONS_KIND_MAX])( const NetClientOptions *opts, @@ -982,6 +1024,12 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) } return -1; } + + if (is_netdev && u.netdev->has_dumpfile) { + if (netdev_init_dumpfile(u.netdev, name, errp)) { + return -1; + } + } } return 0; } diff --git a/qapi-schema.json b/qapi-schema.json index 106008c..79c3ed7 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2528,14 +2528,22 @@ # # @id: identifier for monitor commands. # +# @dumpfile: #optional name of a file where network traffic should be logged +# (since: 2.4) +# +# @dumplen: #optional maximum length of the network packets in the dump +# (since: 2.4) +# # @opts: device type specific properties # # Since 1.2 ## { 'struct': 'Netdev', 'data': { - 'id': 'str', - 'opts': 'NetClientOptions' } } + 'id': 'str', + '*dumpfile': 'str', + '*dumplen': 'int32', + 'opts': 'NetClientOptions' } } ## # @InetSocketAddress
So far it is not possible to dump network traffic with the "-netdev" option yet - this is only possible with the "-net" option and an internal "hub". This patch now fixes this ugliness by adding a proper, generic dumpfile parameter to the "-netdev" option. Signed-off-by: Thomas Huth <thuth@redhat.com> --- include/net/net.h | 1 + net/net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 12 ++++++++++-- 3 files changed, 59 insertions(+), 2 deletions(-)