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 <arm...@redhat.com> writes: > Eric Blake <ebl...@redhat.com> 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.
