* [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: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
* 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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).