Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt

[Markus Armbruster]

TL;DR: Andreas, there's one question specifically for you, search for
"QOM:".

Andreas Färber <address@hidden> writes:

> Am 09.09.2015 um 17:22 schrieb Markus Armbruster:
>> Andreas Färber <address@hidden> writes:
>>> Am 09.09.2015 um 16:38 schrieb Markus Armbruster:
>>>> I ran into this:
>>>>
>>>>     $ qemu-system-ppc64 -nodefaults -S -display none -monitor stdio
>>>> -machine pseries-2.4
>>>>     QEMU 2.4.50 monitor - type 'help' for more information
>>>>     (qemu) qom-list /machine/unattached/device[5]/dr-connector[255]
>>>>     fdt (struct)
>>>>     entity-sense (uint32)
>>>>     name (string)
>>>>     connector_type (uint32)
>>>>     index (uint32)
>>>>     id (uint32)
>>>>     allocation-state (uint32)
>>>>     indicator-state (uint32)
>>>>     isolation-state (uint32)
>>>>     parent_bus (link<bus>)
>>>>     hotplugged (bool)
>>>>     hotpluggable (bool)
>>>>     realized (bool)
>>>>     type (string)
>>>>     (qemu) qom-list /machine/unattached/device[5]/dr-connector[255]/fdt
>>>>     Device '/machine/unattached/device[5]/dr-connector[255]/fdt' not found
>>>>
>>>> According to the first qom-list, .../fdt exists.  According to the
>>>> second, it doesn't.
>>>
>>> Actually this is fully expected: qom-list operates on QOM objects. The
>>> fdt property returns a struct, which is considered a value QOM-wise, so
>>> to read it you need to use qom-get, not qom-list.
>>>
>>> Now, it may well be that visiting a struct does not work as expected, I
>>> remember we ran into issues there, that held up the qom-tree stuff...
>> 
>> Okay, switching to QMP, because there's no qom-get in HMP (is that
>> intentional?).
>> 
>>     QMP> { "execute": "qom-list", "arguments": { "path":
>>     QMP> "/machine/unattached/device[5]/dr-connector[255]" } }
>>     {"return": [{"name": "fdt", "type": "struct"}, {"name":
>> "entity-sense", "type": "uint32"}, {"name": "name", "type":
>> "string"}, {"name": "connector_type", "type": "uint32"}, {"name":
>> "index", "type": "uint32"}, {"name": "id", "type": "uint32"},
>> {"name": "allocation-state", "type": "uint32"}, {"name":
>> "indicator-state", "type": "uint32"}, {"name": "isolation-state",
>> "type": "uint32"}, {"name": "parent_bus", "type": "link<bus>"},
>> {"name": "hotplugged", "type": "bool"}, {"name": "hotpluggable",
>> "type": "bool"}, {"name": "realized", "type": "bool"}, {"name":
>> "type", "type": "string"}]}
>>     QMP> { "execute": "qom-list", "arguments": { "path":
>>     QMP> "/machine/unattached/device[5]/dr-connector[255]/fdt" } }
>>     {"error": {"class": "DeviceNotFound", "desc": "Device
>> '/machine/unattached/device[5]/dr-connector[255]/fdt' not found"}}
>> 
>> Should qom-list really fail DeviceNotFound?  I find it rather confusing.
>> For what it's worth, attempting to read a directory fails with EISDIR,
>> not ENOENT.
>
> Listing a non-existing directory on my system results in:
>
>   ls: cannot access foo: No such file or directory

As Paolo remarked, we're listing an existing non-directory here, which
fails ENOTDIR.  I used the wrong example (file-only op on directory
instead of directory-only op on file).

> As for the DeviceNotFound, I merely implemented the HMP glue, so you
> should rather direct such questions at Eric or Luiz (or Anthony).
> I believe that an ObjectNotFound is out of the question, as any new code
> would just use the Generic class.

Yes.

My (badly worded) question was about the misleading error *message*.  I
don't care for the error *class*.

>> 
>> Moving on to my next confusion: qom-get.
>> 
>>     QMP> { "execute": "qom-get", "arguments": { "path":
>>     QMP> "/machine/unattached/device[5]/dr-connector[255]",
>>     QMP> "property": "fdt" } }
>>     {"return": {}}
>> 
>> The return value {} is weird.  Let's see where it comes from.
>> 
>> qmp_qom_get() first calls object_resolve_path() on the path, then
>> object_property_get_qobject() on the property.  For our test case,
>> object_resolve_path() succeeds.  Now have a closer look at
>> object_property_get_qobject()'s behavior:
>> 
>>     QObject *object_property_get_qobject(Object *obj, const char *name,
>>                                          Error **errp)
>>     {
>>         QObject *ret = NULL;
>>         Error *local_err = NULL;
>>         QmpOutputVisitor *mo;
>> 
>>         mo = qmp_output_visitor_new();
>>         object_property_get(obj, qmp_output_get_visitor(mo), name, 
>> &local_err);
>> 
>> This call succeeds, and we enter the conditional.
>> 
>>         if (!local_err) {
>>             ret = qmp_output_get_qobject(mo);
>> 
>> This call returns NULL.
>> 
>>         }
>>         error_propagate(errp, local_err);
>> 
>> This sets *errp = NULL.
>> 
>>         qmp_output_visitor_cleanup(mo);
>>         return ret;
>>     }
>> 
>> Function returns NULL without setting an error.  Its function comment:
>> 
>> /*
>>  * object_property_get_qobject:
>>  * @obj: the object
>>  * @name: the name of the property
>>  * @errp: returns an error if this function fails
>>  *
>>  * Returns: the value of the property, converted to QObject, or NULL if
>>  * an error occurs.
>>  */
>> 
>> Is returning NULL without setting an error okay?
>> 
>> Should it return qnull() instead?  Then the QMP return value would be
>> JSON null.
>> 
>
> That smells like the StringOutputVisitor problem that was holding up HMP
> qom-get:
>
> http://patchwork.ozlabs.org/patch/449596/

Interesting.

There are two software layers to consider, though:

1. QOM: What's the contract for object_property_add()'s argument @get()?

   In particular, under what circumstances may it return without doing
   anything (prop_get_fdt() does for me), and what does that mean?

   This is where I could use guidance from core QOM maintainers.

2. Visitors: dealing with null

   Not maintained by you.  Of course, your advice is as welcome as
   anyone's.

   Besides the patch you quoted, there's also a suspicious FIXME in
   qmp_output_first().

> IIRC I needed to add a test case - not sure if I did, and busy now.
>
> Searching for that subject should find you the qom-get patch as well.

Thanks.