qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] qmp: add support for mixed typed input visitor
Date: Wed, 20 Jul 2016 11:02:12 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Copying Kevin and Max to alert them to possibly related issues due to
the block layer's use of QemuOpts and QDict instead of QAPI.

A bit of context: QMP netdev_add and device_add interpret their
arguments in highly unorthodox ways.  This is an artifact of their
(non-QAPI) implementation, discussed in detail below (search for "Now it
gets ugly").  It results in nasty backward compatibility problems for a
proper QAPI implementation.

The block layer also avoids QAPI in some parts.  We need to take care to
avoid similar ugliness and future backward compatibility problems.
Enjoy!

Markus Armbruster <address@hidden> writes:

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]