Re: [Qemu-devel] [PATCH 4/5] net/dump: Add dump option for netdev devices

[Markus Armbruster]

Thomas Huth <address@hidden> 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 <address@hidden>
> ---
>  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