[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 16/17] qapi: Tweak QmpInputVisitor to optiona
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v9 16/17] qapi: Tweak QmpInputVisitor to optionally do string conversion |
Date: |
Thu, 14 Jul 2016 08:04:29 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 07/14/2016 07:03 AM, Daniel P. Berrange wrote:
>>> I'm not really a huge fan of the approach there. IMHO the
>>> input visitor should strictly operate in "all native types"
>>> or "all string types" mode. The way you've done it here
>>> means that users can actually mix & match strings vs native
>>> types for each individual parameter :-(
>>>
>>> IMHO, I'd suggest you try to parse netdev_add args with
>>> it in "all native types" mode, if that fails, then re-parse
>>> in "all string types" mode.
>>
>> Sadly, this idea won't work. Without this series, the existing QMP
>> command 'netdev_add' takes mixed integers and strings in a single call,
>> by virtue of converting QObject into QemuOpts and back into QObject.
>
> Ok, so lemme check I understand. The original QObject has properties
> which are integers, but with existing code QMP allows them to be passed
> as strings or ints, converts them to QemuOpts which makes them all
> strings and converts them back to QObject leaving them all as strings.
Yes, the existing code (using 'gen':false) just passes an entire QObject
to hand-written code; the hand-written code converted everything to
QemuOpts (which flattens both integers and strings into options), then
expanded QemuOpts back into QObject (strings-only).
>
> You've now got rid of the QObject -> QemuOpts conversion, so we're
> not string-ifying everything, and so have to cope with arbitrary
> mix directly.
Yes.
>
>> Forcing the parser to allow only integers or only strings would reject
>> current uses of netdev_add, and we don't yet have a good idea whether
>> any existing clients were depending on that behavior.
>>
>> Also, see patch 17/17 - the easiest way to implement a relaxed QMP parse
>> is by setting a single flag which controls which visitor constructor is
>> used. Your idea of parsing once with integer-only, then falling back to
>> parse a second time with string-only, would complicate the generator to
>> have to create two different visitors, rather than passing a single
>> boolean parameter through to the single visitor constructor.
>
> So I don't think a simple boolean flag is desirable, as we have 3 distinct
> scenarios:
>
> 1. Strongly typed only
> 2. String conversion only
> 3. Strongly typed, with string conversion fallback
>
> My current patch provides 1 + 2, your alternative patch provides 1 + 3.
> If we use option 3, in cases which only actually need option 2, then we
> are potentially introducing more scenarios where arbitrarily mixed
> types are accepted. IOW, I think we must implement 1, 2 and 3 and avoid
> using 3 except where absolutely needed.
3 is a superset of 2, and your command-line conversion is the only case
where we can achieve 2. That is, code dealing with QMP can only choose
between 1 and 3, based on whether the QAPI .json file used
'autocast':true for back-compat reasons (the only candidates are
'netdev_add' and 'device_add'). And code dealing with command line
parsing can only choose 2 (QemuOpts is string-only), but parsing
string-only via 2 is no different than the result achieved from parsing
strongly-typed with string fallback via 3.
I still don't buy the fact that we need a string-only parser at the
moment, but it would not be hard to change the 'bool autocast' into a
tri-state enum, and then make the implementation specifically honor 1,
2, or 3 based on the enum value.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v9 11/17] block: Simplify drive-mirror, (continued)
- [Qemu-devel] [PATCH v9 11/17] block: Simplify drive-mirror, Eric Blake, 2016/07/13
- [Qemu-devel] [PATCH v9 15/17] option: make parse_option_bool/number non-static, Eric Blake, 2016/07/13
- [Qemu-devel] [PATCH v9 10/17] block: Simplify block_set_io_throttle, Eric Blake, 2016/07/13
- [Qemu-devel] [PATCH v9 14/17] net: Complete qapi-fication of netdev_add, Eric Blake, 2016/07/13
- [Qemu-devel] [PATCH v9 16/17] qapi: Tweak QmpInputVisitor to optionally do string conversion, Eric Blake, 2016/07/13
[Qemu-devel] [PATCH v9 17/17] qapi: Restore autocast behavior in 'netdev_add', Eric Blake, 2016/07/13
[Qemu-devel] [PATCH v9 12/17] qapi: Change Netdev into a flat union, Eric Blake, 2016/07/13
Re: [Qemu-devel] [PATCH for-2.7 v9 00/17] qapi netdev_add introspection (post-introspection cleanups subset F), Markus Armbruster, 2016/07/14