Re: [Qemu-devel] [PATCH v8 14/16] net: Complete qapi-fication of netdev_add

[Markus Armbruster]

Eric Blake <address@hidden> 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, and exposes
> those types through introspection, and without breaking command
> line parsing.
>
> Inline the function netdev_add() into its lone remaining caller.
>
> There are a few places where the QMP 'netdev_add' command is now
> more strict: anywhere that the QAPI lists an integer member, we
> now require a strict JSON integer (previously, we allowed both
> integers and strings, because the conversion from QMP to QemuOpts
> back to QObject collapsed them into integers).  For example,
> pre-patch, both of these examples succeed, but post-patch, the
> second example fails:
>
> {'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"}}

Needs to be covered in release notes.

> If that turns out to be problematic, we could resolve the problem
> by either adding a loose parsing mode to qmp-input-visitor (teaching
> it to accept a string encoding of an integer in place of an actual
> integer), or by using QAPI alternates.  But that work should be
> in followup patches, if at all.

The sloppy typing is an accidental misfeature that is at odds with QMP's
design.  As far as I can tell, it's not documented anywhere.  But lots
of things aren't documented anywhere; the questions is whether they're
used.

If we find we must keep netdev_add misfeature-compatible, I want it
deprecated in favor of a new command that adheres to the QMP design.

Do we have to keep it misfeature-compatible proactively?  How
comfortable are we with "wait see whether anything breaks" here?

The scenarios I want to avoid are "OMG, hold the release while we
improvise a misfeature-compatibility hack", and "OMG, we need a stable
release for misfeature compatibility a.s.a.p."

I'm okay with breaking misfeature compatibility in this patch, and
unbreak it separately if necessary.

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

This use of QemuOpts as a network backend directory has always been
somewhat questionable.  More so now the QemuOpts is empty.  But it's not
this patch's business.

> Signed-off-by: Eric Blake <address@hidden>