Re: [Qemu-devel] [PATCH v8 3/7] qapi: add a QmpInputVisitor that does string conversion

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3022 bytes --]

On 07/15/2016 09:36 AM, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
>> Currently the QmpInputVisitor assumes that all scalar
>> values are directly represented as their final types.
>> ie it assumes an 'int' is using QInt, and a 'bool' is
>> using QBool.
>>
>> This adds an alternative constructor for QmpInputVisitor
>> that will set it up such that it expects a QString for
>> all scalar types instead.
>>
>> This makes it possible to use QmpInputVisitor with a
>> QDict produced from QemuOpts, where everything is in
>> string format.
>>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---

>> +Visitor *qmp_string_input_visitor_new(QObject *obj, bool strict);
> 
> We have a QMP input visitor that isn't really about QMP, and a string
> visitor.  You're adding a QMP string input visitor that's even less
> about QMP (using it with QMP would be wrong, in fact), and that is
> related to the string visitor only insofar as both parse strings.
> Differently, of course; this is QEMU.
> 
> Can't think of a better name, and I'm running out of Friday.
> 

Sounds like we might want a prereq patch that just does a global rename
of s/qmp-\(input-visitor\|output-visitor\)/qobj-\1/ through all affected
files.  Since it really is a QObject visitor, the rename will make it
easier to give the new visitor a nicer name.

> Should we specify how strings are parsed?
> 
> Do you actually need both strict = true and strict = false here?

I'd be fine if we just enforced that the new QObject-string parser is
always strict.  Non-strict parsers should only be used where we are
worried about back-compat for hand-written code that knows how to deal
with unparsed keys (such as device_add, where we MUST accept arguments
not part of the QAPI schema, so long as we don't have a schema that
fully describes it).


>>  static void qmp_input_optional(Visitor *v, const char *name, bool *present)
>>  {
>>      QmpInputVisitor *qiv = to_qiv(v);
> 
> Separate callbacks result in cleaner code than what I reviewed last.
> Cleaner semantics, too: either all the scalars have sane types, or all
> are strings.

In other words, it forces us to strongly be tied to the input source,
where command line is strongly-typed to strings, and QMP is
strongly-typed to native types.  It doesn't solve the problem of
netdev_add previously being loose and taking both types, but as we've
mentioned in other threads, we may not care (it may be sufficient to
state that the undocumented looseness of netdev_add is not worth
fixing); and Dan did have the nice followup proposal of adding yet
another callback with peek semantics that can then delegate to the
correct strongly-typed callback, rather than my approach that munged it
all into a single callback.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]