Re: [Qemu-devel] [PATCH v15 09/23] qom: Wrap prop visit in visit_start_struct

[Eric Blake]

On 04/28/2016 08:46 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> The qmp-input visitor was allowing callers to play rather fast
>> and loose: when visiting a QDict, you could grab members of the
>> root dictionary without first pushing into the dict; the final
>> such culprit was the QOM code for converting to and from object
>> properties.  But we are about to tighten the input visitor, at
>> which point user_creatable_add_type() as called with a QMP input
>> visitor via qmp_object_add() MUST follow the same paradigms as
>> everyone else, of pushing into the struct before grabbing its
>> keys.
>>

>>
>> $ ./x86_64-softmmu/qemu-system-x86_64 -nographic -nodefaults -qmp stdio 
>> -object secret,id=sec0,data=letmein,format=raw,foo=bar
>> qemu-system-x86_64: Property '.foo' not found
> 
> Let's update the error message now that the error message regression is
> fixed.  Can do on commit.

I see when you fixed the error message regression your commits omitted
data=letmein,format=raw; I included them here because they are marked
mandatory in qapi, to make sure that we didn't have to think about
whether excess arguments trump missing mandatory arguments in terms of
error reporting.  Up to you if you also feel comfortable shortening the
command line to match the one in your commit message that fixed the
regression.


>> +++ b/qom/object_interfaces.c
>> @@ -120,12 +120,20 @@ Object *user_creatable_add_type(const char *type, 
>> const char *id,
>>
>>      obj = object_new(type);
>>      if (qdict) {
>> +        visit_start_struct(v, NULL, NULL, 0, &local_err);
>> +        if (local_err) {
>> +            goto out;
>> +        }
>>          for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
>>              object_property_set(obj, v, e->key, &local_err);
>>              if (local_err) {
>> -                goto out;
>> +                break;
>>              }
>>          }
>> +        visit_end_struct(v, local_err ? NULL : &local_err);
>> +        if (local_err) {
>> +            goto out;
>> +        }
>>      }
>>
>>      object_property_add_child(object_get_objects_root(),
> 
> Hmm.  How should this behave if !qdict?

Pre-existing.

!qdict is not possible from qmp_object-add() (which throws
QERR_INVALID_PARAMETER_TYPE on a missing or wrong QObject type for 'props').

Also not possible from user_creatable_add().  We could turn it into
assert(qdict) and drop a level of indentation.

> 
> However, both callers seem to pass non-null qdict.  Should we sidestep
> the question by making that a precondition?

We came to the same conclusion, so yes :)

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

signature.asc
Description: OpenPGP digital signature