All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
@ 2015-09-09 14:38 Markus Armbruster
  2015-09-09 14:46 ` Andreas Färber
  0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2015-09-09 14:38 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-devel

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.

I messed around in GDB a bit, and found that the second qom-list's final
object_resolve_path_component() returns NULL like this:

    Breakpoint 5, object_resolve_path_component (parent=0x55555659f0f0, 
        part=0x5555564b0660 "fdt") at /home/armbru/work/qemu/qom/object.c:1492
    1492	    if (prop == NULL) {
    (gdb) p prop
    $27 = (ObjectProperty *) 0x55555659faf0
    (gdb) n
    1496	    if (prop->resolve) {
    (gdb) 
    1501	}
    (gdb) p prop->resolve
    $28 = (ObjectPropertyResolve *) 0x0

The "fdt" property exists, but its ->resolve() callback is null.

object_resolve_path_component()'s function comment:

 * Returns: The final component in the object's canonical path.  The canonical
 * path is the path within the composition tree starting from the root.

Doesn't sound like NULL means failure.

Its caller object_resolve_abs_path() then also returns NULL.  No
function comment.  Likewise, its caller object_resolve_path_type()
returns NULL.  Its function comment:

 * Returns: The matched object or NULL on path lookup failure.

Here, NULL definitely means failure, and a specific one: path lookup
failure.

Its caller qmp_qom_list() then fails, setting a "Device
'/machine/unattached/device[5]/dr-connector[255]/fdt' not found" error.

Does this work as intended?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
  2015-09-09 14:38 [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt Markus Armbruster
@ 2015-09-09 14:46 ` Andreas Färber
  2015-09-09 15:22   ` Markus Armbruster
  0 siblings, 1 reply; 8+ messages in thread
From: Andreas Färber @ 2015-09-09 14:46 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel

Hi Markus,

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...

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
  2015-09-09 14:46 ` Andreas Färber
@ 2015-09-09 15:22   ` Markus Armbruster
  2015-09-09 16:04     ` Andreas Färber
  0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2015-09-09 15:22 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-devel

Andreas Färber <afaerber@suse.de> writes:

> Hi Markus,
>
> 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": "/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": "/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.

Moving on to my next confusion: qom-get.

    QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[255]", "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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
  2015-09-09 15:22   ` Markus Armbruster
@ 2015-09-09 16:04     ` Andreas Färber
  2015-09-09 16:16       ` Paolo Bonzini
  2015-09-10  8:55       ` Markus Armbruster
  0 siblings, 2 replies; 8+ messages in thread
From: Andreas Färber @ 2015-09-09 16:04 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Luiz Capitulino

Am 09.09.2015 um 17:22 schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> 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": "/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": "/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 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.

> 
> Moving on to my next confusion: qom-get.
> 
>     QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[255]", "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/

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.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
  2015-09-09 16:04     ` Andreas Färber
@ 2015-09-09 16:16       ` Paolo Bonzini
  2015-09-09 16:30         ` Andreas Färber
  2015-09-13  2:04         ` Eric Blake
  2015-09-10  8:55       ` Markus Armbruster
  1 sibling, 2 replies; 8+ messages in thread
From: Paolo Bonzini @ 2015-09-09 16:16 UTC (permalink / raw)
  To: Andreas Färber, Markus Armbruster; +Cc: qemu-devel, Luiz Capitulino



On 09/09/2015 18:04, Andreas Färber wrote:
>> > 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

This is more like

$ ls vl.c/*
ls: cannot access vl.c/*: Not a directory

So it's ENOTDIR.  Not really DeviceNotFound, but perhaps close enough.

FWIW, "struct" isn't a well-defined QAPI name.  The type probably should
be changed to "any" (right?).

> > Moving on to my next confusion: qom-get.
> > 
> >     QMP> { "execute": "qom-get", "arguments": { "path": "/machine/unattached/device[5]/dr-connector[255]", "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.
> > 
> > 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.

JSON null support in QObject is new, it should be the result of
object_property_get_qobject() or even qmp_output_get_qobject().  Needs
auditing the code.

Paolo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
  2015-09-09 16:16       ` Paolo Bonzini
@ 2015-09-09 16:30         ` Andreas Färber
  2015-09-13  2:04         ` Eric Blake
  1 sibling, 0 replies; 8+ messages in thread
From: Andreas Färber @ 2015-09-09 16:30 UTC (permalink / raw)
  To: Paolo Bonzini, Markus Armbruster; +Cc: qemu-devel, Luiz Capitulino

Am 09.09.2015 um 18:16 schrieb Paolo Bonzini:
> On 09/09/2015 18:04, Andreas Färber wrote:
>>>> 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
> 
> This is more like
> 
> $ ls vl.c/*
> ls: cannot access vl.c/*: Not a directory
> 
> So it's ENOTDIR.  Not really DeviceNotFound, but perhaps close enough.
> 
> FWIW, "struct" isn't a well-defined QAPI name.  The type probably should
> be changed to "any" (right?).

If you guys want to change either, just discuss among yourselves and
post a patch. I couldn't care less...

fdt does not resolve as an object, so any error message optimizations
would just add unnecessary complexity - unless I'm missing something.
(parsing the path, checking whether the parent object does resolve,
checking whether a property of the same name exists on that object)
Due to the QMP vs. HMP mess, two callsites may be affected.

The choice of type is done at the callsite, not in QOM code, so I'm even
more the wrong one to complain to. -> ppc maintainers?

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
  2015-09-09 16:04     ` Andreas Färber
  2015-09-09 16:16       ` Paolo Bonzini
@ 2015-09-10  8:55       ` Markus Armbruster
  1 sibling, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2015-09-10  8:55 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-devel, Luiz Capitulino

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

Andreas Färber <afaerber@suse.de> writes:

> Am 09.09.2015 um 17:22 schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> 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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt
  2015-09-09 16:16       ` Paolo Bonzini
  2015-09-09 16:30         ` Andreas Färber
@ 2015-09-13  2:04         ` Eric Blake
  1 sibling, 0 replies; 8+ messages in thread
From: Eric Blake @ 2015-09-13  2:04 UTC (permalink / raw)
  To: Paolo Bonzini, Andreas Färber, Markus Armbruster
  Cc: qemu-devel, Luiz Capitulino

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

On 09/09/2015 10:16 AM, Paolo Bonzini wrote:

>>> Is returning NULL without setting an error okay?
>>>
>>> Should it return qnull() instead?  Then the QMP return value would be
>>> JSON null.
> 
> JSON null support in QObject is new, it should be the result of
> object_property_get_qobject() or even qmp_output_get_qobject().  Needs
> auditing the code.

Yes, returning QNull rather than NULL seems like the right approach.

-- 
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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-09-13  2:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-09 14:38 [Qemu-devel] Confused by QOM: /machine/unattached/device[5]/dr-connector[255]/fdt Markus Armbruster
2015-09-09 14:46 ` Andreas Färber
2015-09-09 15:22   ` Markus Armbruster
2015-09-09 16:04     ` Andreas Färber
2015-09-09 16:16       ` Paolo Bonzini
2015-09-09 16:30         ` Andreas Färber
2015-09-13  2:04         ` Eric Blake
2015-09-10  8:55       ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.