Re: [Qemu-devel] [PATCH] qmp: add support for mixed typed input visitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/14/2016 08:39 AM, Daniel P. Berrange wrote:
>> On Thu, Jul 14, 2016 at 08:23:18AM -0600, Eric Blake wrote:
>>> On 07/14/2016 08:16 AM, Daniel P. Berrange wrote:
>>>> Add a qmp_mixed_input_visitor_new() method which returns
>>>> a QMP input visitor that accepts either strings or the
>>>> native data types.
>
> Question: do we want to allow: "key":1 when the QAPI is written
> 'key':'str'?  Your current patches allow the converse (allowing
> "key":"1" when the QAPI is written 'key':'int').  To allow native types
> to be consumed in mixed-mode where string is expected would require yet
> another method for deciding how to handle non-strings in
> v->visitor.type_str.  Where it might be useful is in SocketAddress
> parsing, in particular where InetSocketAddress.port is currently 'str'
> but where it often takes an integer port number in addition to a string
> for a named port alias; callers currently have to pass a stringized
> integer, where mixed mode might make it easier to fudge things.

I think we shouldn't do that.  Let me explain why.

The QMP input visitor is designed for QMP input.  Works like this: the
JSON parser converts a JSON value to a QObject, and the QMP input
visitor converts the QObject to a QAPI object.  Observations:

* In JSON, types are obvious.  The JSON parser's conversion to QObject
  is mostly straightforward: JSON object becomes QDict, array becomes
  QList, string becomes QString, false and true become QBool, null
  becomes qnull().  The only complication is JSON number, which becomes
  either QInt or QFloat, depending on its value.

* QInt represents int64_t.  Integers outside its range become QFloat.
  In particular, INT64_MAX+1..UINT64_MAX become QFloat.

* The QMP input visitor checks the QObject's type matches the QAPI
  object's type.  For object and array, this is recursive.  To
  compensate for the split of JSON number into QInt and QFloat, it
  accepts both for QAPI type 'number'.

* Despite its name, the QMP input visitor doesn't visit QMP, it visits a
  QObject.  Makes it useful in other contexts, such as QemuOpts input.

QemuOpts input works like this: the QemuOpts parser converts a
key=value,... string to a QemuOpts, qemu_opts_to_qdict() converts to a
QObject, and the QMP input visitor converts to a QAPI object.
Observations:

* In the key=value,... string, types are ambiguous.  The QemuOpts parser
  disambiguates to bool, uint64_t, char * when given an option
  description (opts->list->desc[] not empty), else it treats all values
  as strings.  Even when it disambiguates, it retains the string value.

* QemuOpts that are ultimately fed to the QMP input visitor typically
  have no option description.  Even if they have one, the types are
  thrown away: qemu_opts_to_qdict() works with the strings.

* Since all scalars are strings, the QMP input visitor's type checking
  will fail when it runs into a scalar QAPI type other than str.  This
  is the problem Dan needs solved.

Let's compare the two pipelines JSON -> QObject -> QAPI object and
key=value,... -> QObject -> QAPI object.  Their first stages are
conceptually similar, and the remaining stages are identical.  The
difference of interest here is how they pick QObject types:

* The JSON pipeline picks in its first stage.

* The QemuOpts pipeline can't do that, but to be able to reuse the rest
  of the pipeline, it arbitrarily picks QString.  Good enough for its
  intial uses.  Not good for the uses we need to support now.

I believe we should change the QemuOpts pipeline to resolve types in its
last stage.  This requires a different input visitor: one that resolves
types rather than checking them.  I guess this is basically what Dan's
"[PATCH v7 3/7] qapi: add a QmpInputVisitor that does string conversion"
does.  It's even less a *QMP* input visitor than the original, but let's
not worry about that now.

Now it gets ugly.

A long time ago, under a lot of pressure to have QMP reach feature
parity with HMP, I shoehorned device_add into QMP.  QAPI didn't exist
back then, so the pipeline was just JSON -> QObject.  I added a ->
QemuOpts stage, done by qemu_opts_from_qdict(), so I could use the
existing qdev_device_add() unmodified.  Despite such shortcuts, it took
me ~50 patches (commit 0aef426..8bc27249).  I've regretted it ever
since.

This JSON -> QObject -> QemuOpts pipeline is problematic: its second
stage throws away all type information.  It preserves JSON string
values, but anything else gets converted to a string.  Which may, if
you're lucky, even resemble your JSON token string.  Examples:

    JSON                            QemuOpts value of key "foo"
     "foo": "abracadabra"           "abracadabra"
     "foo": -1                      "-1"
     "foo": 1.024e3                 "1024"
     "foo": 9223372036854775808     "9.2233720368547758e+18"
     "foo": 6.022140857e23          "6.0221408570000002e+23"
     "foo": false                   "off"
     "foo": null                    key does not exist *boggle*

The QemuOpts string value is then converted by object_property_parse(),
which uses a string input visitor to do the work.  A second round of
parsing, with its very own syntactic sugar.  qemu_opts_from_qdict()
needs to match it.

Amazingly, this contraption mostly behaves as users can reasonably
expect as long as they use the correct JSON type (so it gets converted
to string and back for no change of value *fingers crossed*).  It should
also behave predictably if you use strings for everything (so it gets
parsed just once, by the the string input visitor).

Ways to write a QAPI bool false include false, "false", "off", "no".

Ways to write a QAPI number 42 include 42, "42", " 42" (but not "42 ", I
think), 0.042e3 (but not "0.042e3"), "42,42", "42-42".

QAPI lists are even more fun, as the string input visitor has its very
own integer list syntax.  For instance, JSON "1,2,0,2-4,20,5-9,1-8"
would be an acceptable int16List, same as [ 0, 1, 2, 3, 4, 5, 6, 7, 8,
9, 20 ].  I can't find anything list-valued in device_add or netdev_add
right now, so this is theoretical.

netdev_add got the same treatment as device_add (commit 928059a).
However, it feeds the QemuOpts to an options visitor instead of a string
input visitor.  Yet another parser, differing from the string input
visitor's in many amusing ways.  For starters, it lets you write a QAPI
bool false as "n", too.  The integer parsing differs as well, but I'll
be hanged if I know how.  Anyway, qemu_opts_from_qdict() better matches
this one, too.  I'll be hanged if I know whether it matches either.

Naturally, this is all undocumented.  I'd be willing to bet actual money
on us having broken backwards compatibility around here a bunch of times
already.

With QAPI came an additional -> QAPI object stage, but only for QAPIfied
QMP commands.  The QMP pipeline got a fork:

    JSON -> QObject -> QAPI object -> QAPIfied QMP command
               |
               +--------------------> pre-QAPI QMP command

The plan was to eliminate the pre-QAPI fork, but as usual with such
plans, it's taking us forever and a day.

Rotate 90° and add detail:

           JSON
             |
        JSON parser
             |
          QObject
             |
             +-----------------------+------------------------+
             |                       |                        |
       QMP input vis.       qemu_opts_from_qdict()            |
             |                       |                        |
             |                    QemuOpts                    |
             |                    /       \                   |
             |                   /         \                  |
             |           options vis.  string input vis.      |
             |                  |            |                |
        QAPI object       QAPI object   qdev/QOM object       |
             |                  |            |                |
    QAPIfied QMP command   netdev_add     device_add     other command

Eric's netdev-add QAPIfication eliminated the netdev_add fork.  Since
this also eliminates the nutty unparsing and parsing, it isn't
misfeature-compatible.

To make it misfeature-compatible, he proposes to add a QMP input visitor
option (to be used with netdev_add) to silently convert strings to any
scalar type.  Issues:

* We also have to similarly convert any scalar type to string, exactly
  like qemu_opts_from_qdict().

* The conversions from string to integers other than size use
  parse_option_number().  This isn't how the options visitor parses
  numbers.

* The conversion from string to bool uses parse_option_bool().  This
  isn't how the options visitor parses bools.

* The conversion from string to floating-point uses strtod().  The
  options visitor doesn't do that at all.

Fixing these issues would probably achieve a decent degree of
misfeature-compatibility.  I'm afraid the only practical way to ensure
100% misfeature-compatibility is to retain the misfeature: detour
through QemuOpts as before.

I doubt we can complete either solution before the hard freeze.

Note that device_add's misfeatures differ from netdev_add's.  Perhaps we
can unify them (i.e. add more misfeatures, with the excuse that one big
set of misfeatures is less bad than two somewhat smaller sets), and use
the same QMP input visitor misfeature-compatibility mode for both.
Else, we'll have to add a separate misfeature-compatibility mode for
device_add.

By now this starts to feel like a fool's errand to me.  Are we sure
there's anything out there that relies on the misfeatures?  Remember,
they're undocumented, and we've probably broken the more obscure ones
several times over without noticing.

Convince me that punting this to the next cycle is not necessary.

Back to the question I'm nominally replying to :)

Creating QMP visitor variants to fudge types so we don't have to model
the types correctly just because we're fudging types (and values!)
already for backward compatibility strikes me as a Bad Idea.  If we want
to accept numeric port numbers and string service names, let's say so in
the schema!  Define a port type, alternate of string and int16.