[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH] qmp: add support for mixed typed i
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [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.
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-block] [Qemu-devel] [PATCH] qmp: add support for mixed typed input visitor,
Markus Armbruster <=