[PATCH v2 0/2] netdev qapification

[PATCH v2 0/2] netdev qapification

[Eric Blake]

In v2:
- patch 1 contents unchanged, but commit message improved [Markus]
- patch 2 is new

Eric Blake (2):
  net: Complete qapi-fication of netdev_add
  net: Track netdevs in NetClientState rather than QemuOpt

 include/net/net.h |  2 +-
 monitor/misc.c    |  6 +-----
 net/net.c         | 39 ++++++++++++---------------------------
 qapi/net.json     | 14 +-------------
 4 files changed, 15 insertions(+), 46 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] net: Complete qapi-fication of netdev_add

[Eric Blake]

We've had all the required pieces for doing a type-safe representation
of netdev_add as a flat union for quite some time now (since
0e55c381f6 in v2.7.0, released in 2016), but did not make the final
switch to using it because of concern about whether a command-line
regression in accepting "1" in place of 1 for integer arguments would
be problematic.  Back then, we did not have the deprecation cycle to
allow us to make progress.  But now that we have waited so long, other
problems have crept in: for example, our desire to add
qemu-storage-daemon is hampered by the inability to express net
objects, and we are unable to introspect what we actually accept.
Additionally, our round-trip through QemuOpts silently eats any
argument that expands to an array, rendering dnssearch, hostfwd, and
guestfwd useless through QMP:

{"execute": "netdev_add", "arguments": { "id": "netdev0",
  "type": "user", "dnssearch": [
    { "str": "8.8.8.8" }, { "str": "8.8.4.4" }
  ]}}

So without further ado, let's turn on proper QAPI.  netdev_add() was a
trivial wrapper around net_client_init(), which did a few steps prior
to calling net_client_init1(); with this patch, we now skip directly
to net_client_init1().  In addition to fixing array parameters, the
following additional differences occur:

-  {"execute": "netdev_add", "arguments": {"type": "help"}}
no longer attempts to print help to stdout and exit.  Bug fix, broken
in 547203ead4 'net: List available netdevs with "-netdev help"',
v2.12.0.

-  {"execute": "netdev_add", "arguments': {... "ip6-net": "..." }}
no longer attempts to desugar the undocumented ip6-net magic string
into the proper "ipv6-prefix" and "ipv6-prefixlen".  Undocumented
misfeature, introduced in commit 7aac531ef2 "qapi-schema, qemu-options
& slirp: Adding Qemu options for IPv6 addresses", v2.6.0.

-  {'execute':'netdev_add',
     'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
   {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'hubid', expected: integer"}}
Used to succeed: since our command line treats everything as strings,
our not-so-round-trip conversion from QAPI -> QemuOpts -> QAPI lost
the original typing and turned everything into a string; now that we
skip the QemuOpts, the JSON input has to match the exact QAPI type.
But this stricter QMP is desirable, and introspection is sufficient
for any affected applications to make sure they use it correctly.

In qmp_netdev_add(), we still have to create a QemuOpts object so that
qmp_netdev_del() will be able to remove a hotplugged network device;
but the opts->head remains empty since we now manage all parsing
through the QAPI object rather than QemuOpts; a separate patch will
address the abuse of QemuOpts as a witness for whether a
NetClientState is a netdev.  In the meantime, our argument that we are
okay requires auditing all uses of option group "netdev":

- qemu_netdev_opts: option group definition, empty .desc[]
- CLI (CLI netdev parsing ends before monitors start, so while
  monitors can mess with CLI netdevs, CLI cannot mess with
  monitor netdevs):
  - main() case QEMU_OPTION_netdev: store CLI definition
  - main() case QEMU_OPTION_readconfig, case QEMU_OPTION_writeconfig:
  similar, dealing only with CLI
  - net_init_clients(): Pass CLI to net_client_init()
- Monitor:
  - hmp_netdev_add(): straightforward parse into net_client_init()
  - qmp_netdev_add(): subject of this patch, used to add full
  object to option group, now just adds bare-bones id
  - qmp_netdev_del(), netdev_del_completion(): check the option group
  solely for id, as a 'is this a netdev' predicate

Reported-by: Alex Kirillov <lekiravi@yandex-team.ru>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/net/net.h |  1 -
 monitor/misc.c    |  2 --
 net/net.c         |  6 +++---
 qapi/net.json     | 14 +-------------
 4 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index e175ba9677dc..96e6eae8176e 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -203,7 +203,6 @@ void net_cleanup(void);
 void hmp_host_net_add(Monitor *mon, const QDict *qdict);
 void hmp_host_net_remove(Monitor *mon, const QDict *qdict);
 void netdev_add(QemuOpts *opts, Error **errp);
-void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp);

 int net_hub_id_for_client(NetClientState *nc, int *id);
 NetClientState *net_hub_port_find(int hub_id);
diff --git a/monitor/misc.c b/monitor/misc.c
index c3bc34c099dd..41a86e7012a1 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -247,8 +247,6 @@ static void monitor_init_qmp_commands(void)
                          qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
     qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
                          QCO_NO_OPTIONS);
-    qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
-                         QCO_NO_OPTIONS);
     qmp_register_command(&qmp_commands, "object-add", qmp_object_add,
                          QCO_NO_OPTIONS);

diff --git a/net/net.c b/net/net.c
index 9e93c3f8a1e2..a2065aabede2 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1170,7 +1170,7 @@ void netdev_add(QemuOpts *opts, Error **errp)
     net_client_init(opts, true, errp);
 }

-void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp)
+void qmp_netdev_add(Netdev *netdev, Error **errp)
 {
     Error *local_err = NULL;
     QemuOptsList *opts_list;
@@ -1181,12 +1181,12 @@ void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp)
         goto out;
     }

-    opts = qemu_opts_from_qdict(opts_list, qdict, &local_err);
+    opts = qemu_opts_create(opts_list, netdev->id, 1, &local_err);
     if (local_err) {
         goto out;
     }

-    netdev_add(opts, &local_err);
+    net_client_init1(netdev, true, &local_err);
     if (local_err) {
         qemu_opts_del(opts);
         goto out;
diff --git a/qapi/net.json b/qapi/net.json
index 1cb9a7d782b4..cebb1b52e3b1 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -39,18 +39,8 @@
 #
 # Add a network backend.
 #
-# @type: the type of network backend. Possible values are listed in
-#        NetClientDriver (excluding 'none' and 'nic')
-#
-# @id: the name of the new network backend
-#
 # Additional arguments depend on the type.
 #
-# TODO: This command effectively bypasses QAPI completely due to its
-#       "additional arguments" business.  It shouldn't have been added to
-#       the schema in this form.  It should be qapified properly, or
-#       replaced by a properly qapified command.
-#
 # Since: 0.14.0
 #
 # Returns: Nothing on success
@@ -64,9 +54,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'netdev_add',
-  'data': {'type': 'str', 'id': 'str'},
-  'gen': false }                # so we can get the additional arguments
+{ 'command': 'netdev_add', 'data': 'Netdev', 'boxed': true }

 ##
 # @netdev_del:
-- 
2.25.1

Re: [PATCH v2 1/2] net: Complete qapi-fication of netdev_add

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> We've had all the required pieces for doing a type-safe representation
> of netdev_add as a flat union for quite some time now (since
> 0e55c381f6 in v2.7.0, released in 2016), but did not make the final
> switch to using it because of concern about whether a command-line
> regression in accepting "1" in place of 1 for integer arguments would
> be problematic.  Back then, we did not have the deprecation cycle to
> allow us to make progress.  But now that we have waited so long, other
> problems have crept in: for example, our desire to add
> qemu-storage-daemon is hampered by the inability to express net
> objects, and we are unable to introspect what we actually accept.
> Additionally, our round-trip through QemuOpts silently eats any
> argument that expands to an array, rendering dnssearch, hostfwd, and
> guestfwd useless through QMP:
>
> {"execute": "netdev_add", "arguments": { "id": "netdev0",
>   "type": "user", "dnssearch": [
>     { "str": "8.8.8.8" }, { "str": "8.8.4.4" }
>   ]}}
>
> So without further ado, let's turn on proper QAPI.  netdev_add() was a
> trivial wrapper around net_client_init(), which did a few steps prior
> to calling net_client_init1(); with this patch, we now skip directly
> to net_client_init1().  In addition to fixing array parameters, the
> following additional differences occur:
>
> -  {"execute": "netdev_add", "arguments": {"type": "help"}}
> no longer attempts to print help to stdout and exit.  Bug fix, broken
> in 547203ead4 'net: List available netdevs with "-netdev help"',
> v2.12.0.
>
> -  {"execute": "netdev_add", "arguments': {... "ip6-net": "..." }}
> no longer attempts to desugar the undocumented ip6-net magic string
> into the proper "ipv6-prefix" and "ipv6-prefixlen".  Undocumented
> misfeature, introduced in commit 7aac531ef2 "qapi-schema, qemu-options
> & slirp: Adding Qemu options for IPv6 addresses", v2.6.0.
>
> -  {'execute':'netdev_add',
>      'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
>    {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'hubid', expected: integer"}}
> Used to succeed: since our command line treats everything as strings,
> our not-so-round-trip conversion from QAPI -> QemuOpts -> QAPI lost
> the original typing and turned everything into a string; now that we
> skip the QemuOpts, the JSON input has to match the exact QAPI type.
> But this stricter QMP is desirable, and introspection is sufficient
> for any affected applications to make sure they use it correctly.
>
> In qmp_netdev_add(), we still have to create a QemuOpts object so that
> qmp_netdev_del() will be able to remove a hotplugged network device;
> but the opts->head remains empty since we now manage all parsing
> through the QAPI object rather than QemuOpts; a separate patch will
> address the abuse of QemuOpts as a witness for whether a
> NetClientState is a netdev.  In the meantime, our argument that we are
> okay requires auditing all uses of option group "netdev":
>
> - qemu_netdev_opts: option group definition, empty .desc[]
> - CLI (CLI netdev parsing ends before monitors start, so while
>   monitors can mess with CLI netdevs, CLI cannot mess with
>   monitor netdevs):
>   - main() case QEMU_OPTION_netdev: store CLI definition
>   - main() case QEMU_OPTION_readconfig, case QEMU_OPTION_writeconfig:
>   similar, dealing only with CLI
>   - net_init_clients(): Pass CLI to net_client_init()
> - Monitor:
>   - hmp_netdev_add(): straightforward parse into net_client_init()
>   - qmp_netdev_add(): subject of this patch, used to add full
>   object to option group, now just adds bare-bones id
>   - qmp_netdev_del(), netdev_del_completion(): check the option group
>   solely for id, as a 'is this a netdev' predicate
>
> Reported-by: Alex Kirillov <lekiravi@yandex-team.ru>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Thanks for the nice commit message.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 1/2] net: Complete qapi-fication of netdev_add

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> We've had all the required pieces for doing a type-safe representation
> of netdev_add as a flat union for quite some time now (since
> 0e55c381f6 in v2.7.0, released in 2016), but did not make the final
> switch to using it because of concern about whether a command-line
> regression in accepting "1" in place of 1 for integer arguments would
> be problematic.  Back then, we did not have the deprecation cycle to
> allow us to make progress.  But now that we have waited so long, other
> problems have crept in: for example, our desire to add
> qemu-storage-daemon is hampered by the inability to express net
> objects, and we are unable to introspect what we actually accept.
> Additionally, our round-trip through QemuOpts silently eats any
> argument that expands to an array, rendering dnssearch, hostfwd, and
> guestfwd useless through QMP:
>
> {"execute": "netdev_add", "arguments": { "id": "netdev0",
>   "type": "user", "dnssearch": [
>     { "str": "8.8.8.8" }, { "str": "8.8.4.4" }
>   ]}}
>
> So without further ado, let's turn on proper QAPI.  netdev_add() was a
> trivial wrapper around net_client_init(), which did a few steps prior
> to calling net_client_init1(); with this patch, we now skip directly
> to net_client_init1().  In addition to fixing array parameters, the
> following additional differences occur:
>
> -  {"execute": "netdev_add", "arguments": {"type": "help"}}
> no longer attempts to print help to stdout and exit.  Bug fix, broken
> in 547203ead4 'net: List available netdevs with "-netdev help"',
> v2.12.0.
>
> -  {"execute": "netdev_add", "arguments': {... "ip6-net": "..." }}
> no longer attempts to desugar the undocumented ip6-net magic string

s/ip6-net/ipv6-net/ two times.  Will fix in my tree.

> into the proper "ipv6-prefix" and "ipv6-prefixlen".  Undocumented
> misfeature, introduced in commit 7aac531ef2 "qapi-schema, qemu-options
> & slirp: Adding Qemu options for IPv6 addresses", v2.6.0.
[...]

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

[Eric Blake]

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 <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 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;
 }

@@ -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);
 }

 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)
-- 
2.25.1

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  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 <armbru@redhat.com>

Re: [PATCH v2 0/2] netdev qapification

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> In v2:
> - patch 1 contents unchanged, but commit message improved [Markus]
> - patch 2 is new

Queued, thanks!