qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v14 15/19] qapi-commands: Wrap argument visit in


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v14 15/19] qapi-commands: Wrap argument visit in visit_start_struct
Date: Tue, 26 Apr 2016 06:56:23 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 04/15/2016 05:42 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> The qmp-input visitor was playing rather fast and loose: when
> 
> I guess (some of) its *users* are playing fast and loose, and the
> visitor itself lets them.  The patch's title suggests "some of its
> users" == qapi-commands.py.
> 
>> visiting a QDict, you could grab members of the root dictionary
>> without first pushing into the dict.  But we are about to tighten
>> the input visitor, at which point the generated marshal code
>> MUST follow the same paradigms as everyone else, of pushing into
>> the struct before grabbing its keys, because the value of 'name'
>> should be ignored on the top-level visit.
>>
>> Generated code grows as follows:
>>
>> |@@ -515,7 +695,15 @@ void qmp_marshal_blockdev_backup(QDict *
>> |     BlockdevBackup arg = {0};
>> |
>> |     v = qmp_input_get_visitor(qiv);
>> |+    visit_start_struct(v, NULL, NULL, 0, &err);
>> |+    if (err) {
>> |+        goto out;
>> |+    }
>> |     visit_type_BlockdevBackup_members(v, &arg, &err);
>> |+    if (!err) {
>> |+        visit_check_struct(v, &err);
>> |+    }
> 
> Does this find errors that weren't found before?

All that this could find is excess input, but we are already checking
for excess input prior to calling into the generated marshaling code, so
it doesn't find anything new.

>>
>> Note that this change could also make it possible for the
>> marshalling code to automatically detect excess input at the top
>> level, and not just in nested dictionaries.  However, that checking
>> is not currently useful (and we rely on the manual checking in
>> monitor.c:qmp_check_client_args() instead) as long as qmp-commands.hx
>> uses .args_type, and as long as we have 'name:O' as an arg-type that
>> explicitly allows unknown top-level keys because we haven't yet
>> converted 'device_add' and 'netdev_add' to introspectible use of
>> 'any'.
> 
> Hmm, that explains why finding these additional errors wouldn't be
> useful.  Good to know, but doesn't quite answer my question.

I guess what it really translates to is we are now doing redundant
checking, and I should do a followup patch to simplify monitor.c.

>> @@ -150,7 +158,9 @@ out:
>>      qmp_input_visitor_cleanup(qiv);
>>      qdv = qapi_dealloc_visitor_new();
>>      v = qapi_dealloc_get_visitor(qdv);
>> +    visit_start_struct(v, NULL, NULL, 0, NULL);
>>      visit_type_%(c_name)s_members(v, &arg, NULL);
>> +    visit_end_struct(v);
>>      qapi_dealloc_visitor_cleanup(qdv);
>>  ''',
>>                       c_name=arg_type.c_name())
> 
> No visit_check_struct() here.  I think its contract should explicitly
> state that you may omit it when you're not interested in errors.

Indeed, calling visit_check_struct(, NULL) can't report any errors, so
skipping it should be documented as safe.

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

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