[v7,13/15] net: Complete qapi-fication of netdev_add

We finally have all the required pieces for doing a type-safe
representation of netdev_add as a flat union, where the
discriminator 'type' now selects which additional members may
appear in the "arguments" JSON object sent over QMP, while
making no changes to the set of previously-valid QMP commands
that would work, and without breaking command line parsing.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: no change
v6: don't lose qemu_opts handling
---
 qapi-schema.json  | 15 +++------------
 include/net/net.h |  1 -
 net/net.c         |  6 +++---
 qmp-commands.hx   |  2 +-
 4 files changed, 7 insertions(+), 17 deletions(-)

Eric Blake <eblake@redhat.com> writes:

> We finally have all the required pieces for doing a type-safe
> representation of netdev_add as a flat union, where the
> discriminator 'type' now selects which additional members may
> appear in the "arguments" JSON object sent over QMP, while
> making no changes to the set of previously-valid QMP commands
> that would work, and without breaking command line parsing.

Isn't it amazing that you pulled this off without a compatibility break?

>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: no change
> v6: don't lose qemu_opts handling
> ---
>  qapi-schema.json  | 15 +++------------
>  include/net/net.h |  1 -
>  net/net.c         |  6 +++---
>  qmp-commands.hx   |  2 +-
>  4 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 4fee44b..fee4d07 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2301,26 +2301,17 @@
>  #
>  # Add a network backend.
>  #
> -# @type: the type of network backend.  Current valid values are 'user', 'tap',
> -#        'vde', 'socket', 'dump' and 'bridge'
> +# @type: the type of network backend; determines which additional arguments
> +#        are valid. See Netdev documentation for more details.
>  #
>  # @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
>  #          If @type is not a valid network backend, DeviceNotFound
>  ##
> -{ 'command': 'netdev_add',
> -  'data': {'type': 'str', 'id': 'str'},
> -  'gen': false }                # so we can get the additional arguments
> +{ 'command': 'netdev_add', 'data': 'Netdev', 'box': true }
>
>  ##
>  # @netdev_del:
> diff --git a/include/net/net.h b/include/net/net.h
> index 74b32e0..8f9be98 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -193,7 +193,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);

Only a dead QemuOpts is a good QemuOpts.

>
>  int net_hub_id_for_client(NetClientState *nc, int *id);
>  NetClientState *net_hub_port_find(int hub_id);
> diff --git a/net/net.c b/net/net.c
> index e159506..b93ff00 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1188,7 +1188,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;
> @@ -1199,12 +1199,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);

We keep creating a QemuOpts for the network backend, but its opts->head
remains empty.  We need it for qmp_netdev_del().  Possibly more, but I
doubt it.

I think this is worth a mention in the commit message.  

>      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;

Suggest to inline netdev_add() into its only remaining caller
hmp_netdev_add().

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 94847e5..3804030 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -949,7 +949,7 @@ EQMP
>      {
>          .name       = "netdev_add",
>          .args_type  = "netdev:O",
> -        .mhandler.cmd_new = qmp_netdev_add,
> +        .mhandler.cmd_new = qmp_marshal_netdev_add,
>      },
>
>  SQMP

On 06/16/2016 07:40 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> We finally have all the required pieces for doing a type-safe
>> representation of netdev_add as a flat union, where the
>> discriminator 'type' now selects which additional members may
>> appear in the "arguments" JSON object sent over QMP, while
>> making no changes to the set of previously-valid QMP commands
>> that would work, and without breaking command line parsing.
> 
> Isn't it amazing that you pulled this off without a compatibility break?

No command line compatibility break, but in testing, I _did_ notice a
potential QMP break [it's hard to argue whether it is a break, given
that it was previously undocumented - I don't know if any QMP clients
were actually relying on loose behavior]:

Pre-patch:
    {'execute':'netdev_add',
      'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}}
    {"return": {}}
    {'execute':'netdev_add',
      'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
    {"return": {}}

Post-patch:
    {'execute':'netdev_add',
      'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}}
    {"return": {}}
    {'execute':'netdev_add',
      'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
    {"error": {"class": "GenericError", "desc": "Invalid parameter type
for 'hubid', expected: integer"}}

I'm half-tempted to claim that we should update the QMP spec to say that
our parser is ALWAYS loose (anywhere a built-in scalar type is listed in
introspection, whether bool or integer, the parser will always accept an
equivalent string on input - but output will always be the named type),
and then relax qmp-input-visitor accordingly.  In fact, danpb has
already proposed patches that allow "parse-string-as-int" as intentional
behavior, although under the guise of a new visitor rather than tweaking
qmp-input-visitor - so it just becomes a question of do we do it in
limited situations, or always.  "Be liberal in what you accept" comes to
mind.

And as a followon thought: if we DO update the QMP spec to state that we
always accept a string in place of an integer, then we also have the
luxury of stating that accepting a string "inf" for a QAPI 'number' is
valid (even though strict JSON will not let us pass a bare-word inf) -
and that hits back on my proposal of whether we want to accept bare-word
inf on input as an extension, and whether outputting a string "inf" when
we specified a QAPI type of 'number' would be acceptable (since we would
be canonicalizing input "2" into output 2, going the other direction and
canonicalizing input inf into output "inf" is a bit easier to justify).

But given that it is soft freeze time, I guess we need to be
conservative at what changes we want to support at this phase of
development.

Eric Blake <eblake@redhat.com> writes:

> On 06/16/2016 07:40 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> We finally have all the required pieces for doing a type-safe
>>> representation of netdev_add as a flat union, where the
>>> discriminator 'type' now selects which additional members may
>>> appear in the "arguments" JSON object sent over QMP, while
>>> making no changes to the set of previously-valid QMP commands
>>> that would work, and without breaking command line parsing.
>> 
>> Isn't it amazing that you pulled this off without a compatibility break?
>
> No command line compatibility break, but in testing, I _did_ notice a
> potential QMP break [it's hard to argue whether it is a break, given
> that it was previously undocumented - I don't know if any QMP clients
> were actually relying on loose behavior]:
>
> Pre-patch:
>     {'execute':'netdev_add',
>       'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}}
>     {"return": {}}
>     {'execute':'netdev_add',
>       'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
>     {"return": {}}
>
> Post-patch:
>     {'execute':'netdev_add',
>       'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}}
>     {"return": {}}
>     {'execute':'netdev_add',
>       'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}}
>     {"error": {"class": "GenericError", "desc": "Invalid parameter type
> for 'hubid', expected: integer"}}

You're right.  My brain keeps suppressing this ugly case.

> I'm half-tempted to claim that we should update the QMP spec to say that
> our parser is ALWAYS loose (anywhere a built-in scalar type is listed in
> introspection, whether bool or integer, the parser will always accept an
> equivalent string on input - but output will always be the named type),
> and then relax qmp-input-visitor accordingly.  In fact, danpb has
> already proposed patches that allow "parse-string-as-int" as intentional
> behavior, although under the guise of a new visitor rather than tweaking
> qmp-input-visitor - so it just becomes a question of do we do it in
> limited situations, or always.  "Be liberal in what you accept" comes to
> mind.

I'm afraid we may indeed need a lose mode for backward compatibility of
QMP commands that used to take a detour through the not-really-typed
parts of the command line code (netdev_add now, device_add later).

Nevertheless, I'm reluctant to adopt it wholesale.  With a few decades
of being "liberal in what you accept" under our belts, it doesn't sound
like good advice anymore.  All too often we've ended up with differently
liberal implementations (both on the accepting and the sending side) to
the point where nobody really knows what crap you have to accept for
interoperability.

Instead, I'd prefer a way for a QMP client to opt into a type-correct
version of the command.  Easiest way to do it is to add a new one, and
deprecate the old one.

> And as a followon thought: if we DO update the QMP spec to state that we
> always accept a string in place of an integer, then we also have the
> luxury of stating that accepting a string "inf" for a QAPI 'number' is
> valid (even though strict JSON will not let us pass a bare-word inf) -
> and that hits back on my proposal of whether we want to accept bare-word
> inf on input as an extension, and whether outputting a string "inf" when
> we specified a QAPI type of 'number' would be acceptable (since we would
> be canonicalizing input "2" into output 2, going the other direction and
> canonicalizing input inf into output "inf" is a bit easier to justify).

I've come to the conclusion that the "can't express non-finite numbers
in QMP" problem is basically irrelevant now.  Solving it wouldn't really
accomplish much, as we'd nevertheless want to avoid non-finite numbers
for compatibility with older clients.  There are very few places that
can conceivably emit them anyway.

Besides, if you don't like JSON as a transport encoding, then switching
to one you like better makes a whole lot more sense to me than using it
in unnatural ways like "if the string contents looks like a number (my
definition of 'number', not JSON's), then it can also be a number, and
whether it is depends on the context."

> But given that it is soft freeze time, I guess we need to be
> conservative at what changes we want to support at this phase of
> development.

Yup.