Re: [PATCH v2 2/2] net: Track netdevs in NetClientState rather than QemuOpt

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> As mentioned in the previous patch, our use of QemuOpt group "netdev"
> has two purposes: collect the CLI arguments, and serve as a witness
> for monitor hotplug actions.  As the latter didn't use anything but an
> id, it felt rather unclean to have to touch QemuOpts at all when going
> through QMP, so let's instead track things with a bool field in
> NetClientState.
>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  include/net/net.h |  1 +
>  monitor/misc.c    |  4 +---
>  net/net.c         | 37 +++++++++++--------------------------
>  3 files changed, 13 insertions(+), 29 deletions(-)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index 96e6eae8176e..094e966af9ec 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -98,6 +98,7 @@ struct NetClientState {
>      unsigned rxfilter_notify_enabled:1;
>      int vring_enable;
>      int vnet_hdr_len;
> +    bool is_netdev;
>      QTAILQ_HEAD(, NetFilterState) filters;
>  };
>
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 41a86e7012a1..6c45fa490ff5 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -2035,13 +2035,11 @@ void netdev_del_completion(ReadLineState *rs, int 
> nb_args, const char *str)
>      count = qemu_find_net_clients_except(NULL, ncs, NET_CLIENT_DRIVER_NIC,
>                                           MAX_QUEUE_NUM);
>      for (i = 0; i < MIN(count, MAX_QUEUE_NUM); i++) {
> -        QemuOpts *opts;
>          const char *name = ncs[i]->name;
>          if (strncmp(str, name, len)) {
>              continue;
>          }
> -        opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), name);
> -        if (opts) {
> +        if (ncs[i]->is_netdev) {
>              readline_add_completion(rs, name);
>          }
>      }
> diff --git a/net/net.c b/net/net.c
> index a2065aabede2..38778e831da2 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1060,6 +1060,15 @@ static int net_client_init1(const void *object, bool 
> is_netdev, Error **errp)
>          }
>          return -1;
>      }
> +
> +    if (is_netdev) {
> +        NetClientState *nc;
> +
> +        nc = qemu_find_netdev(netdev->id);
> +        assert(nc);
> +        nc->is_netdev = true;
> +    }
> +
>      return 0;
>  }
>

Would be simpler if net_client_init_fun[]() returned the NetClientState.
But that's clearly more than we can get done today.

> @@ -1172,34 +1181,12 @@ void netdev_add(QemuOpts *opts, Error **errp)
>
>  void qmp_netdev_add(Netdev *netdev, Error **errp)
>  {
> -    Error *local_err = NULL;
> -    QemuOptsList *opts_list;
> -    QemuOpts *opts;
> -
> -    opts_list = qemu_find_opts_err("netdev", &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    opts = qemu_opts_create(opts_list, netdev->id, 1, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    net_client_init1(netdev, true, &local_err);
> -    if (local_err) {
> -        qemu_opts_del(opts);
> -        goto out;
> -    }
> -
> -out:
> -    error_propagate(errp, local_err);
> +    net_client_init1(netdev, true, errp);
>  }

Nice!

>
>  void qmp_netdev_del(const char *id, Error **errp)
>  {
>      NetClientState *nc;
> -    QemuOpts *opts;
>
>      nc = qemu_find_netdev(id);
>      if (!nc) {
> @@ -1208,14 +1195,12 @@ void qmp_netdev_del(const char *id, Error **errp)
>          return;
>      }
>
> -    opts = qemu_opts_find(qemu_find_opts_err("netdev", NULL), id);
> -    if (!opts) {
> +    if (!nc->is_netdev) {
>          error_setg(errp, "Device '%s' is not a netdev", id);
>          return;
>      }
>
>      qemu_del_net_client(nc);
> -    qemu_opts_del(opts);
>  }
>
>  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)

Reviewed-by: Markus Armbruster <address@hidden>