* [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes @ 2015-12-03 16:37 Markus Armbruster 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly Markus Armbruster ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Markus Armbruster @ 2015-12-03 16:37 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david I hope we can get these reviewed and applied in time. Markus Armbruster (3): spapr_drc: Handle visitor errors properly spapr_drc: Change value of property "fdt" from null back to {} spapr_drc: Make device "spapr-dr-connector" unavailable with -device hw/ppc/spapr_drc.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) -- 2.4.3 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly 2015-12-03 16:37 [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster @ 2015-12-03 16:37 ` Markus Armbruster 2015-12-03 16:45 ` Eric Blake 2015-12-03 23:45 ` David Gibson 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} Markus Armbruster ` (2 subsequent siblings) 3 siblings, 2 replies; 20+ messages in thread From: Markus Armbruster @ 2015-12-03 16:37 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david Since prop_get_fdt() is only used with QmpOutputVisitor, errors shouldn't actually happen, so this is only a latent bug. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/ppc/spapr_drc.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index f34bc04..4e7a1d3 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -254,6 +254,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj); + Error *err = NULL; int fdt_offset_next, fdt_offset, fdt_depth; void *fdt; @@ -276,24 +277,43 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, case FDT_BEGIN_NODE: fdt_depth++; name = fdt_get_name(fdt, fdt_offset, &name_len); - visit_start_struct(v, NULL, NULL, name, 0, NULL); + visit_start_struct(v, NULL, NULL, name, 0, &err); + if (err) { + error_propagate(errp, err); + return; + } break; case FDT_END_NODE: /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */ g_assert(fdt_depth > 0); - visit_end_struct(v, NULL); + visit_end_struct(v, &err); + if (err) { + error_propagate(errp, err); + return; + } fdt_depth--; break; case FDT_PROP: { int i; prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len); name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); - visit_start_list(v, name, NULL); + visit_start_list(v, name, &err); + if (err) { + error_propagate(errp, err); + return; + } for (i = 0; i < prop_len; i++) { - visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL); - + visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, &err); + if (err) { + error_propagate(errp, err); + return; + } + } + visit_end_list(v, &err); + if (err) { + error_propagate(errp, err); + return; } - visit_end_list(v, NULL); break; } default: -- 2.4.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly Markus Armbruster @ 2015-12-03 16:45 ` Eric Blake 2015-12-03 23:45 ` David Gibson 1 sibling, 0 replies; 20+ messages in thread From: Eric Blake @ 2015-12-03 16:45 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david [-- Attachment #1: Type: text/plain, Size: 848 bytes --] On 12/03/2015 09:37 AM, Markus Armbruster wrote: > Since prop_get_fdt() is only used with QmpOutputVisitor, errors > shouldn't actually happen, so this is only a latent bug. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/ppc/spapr_drc.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) I posted a similar patch, but using &error_abort rather than propagating, here: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05788.html But I like yours better. > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index f34bc04..4e7a1d3 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c Reviewed-by: Eric Blake <eblake@redhat.com> -- 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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly Markus Armbruster 2015-12-03 16:45 ` Eric Blake @ 2015-12-03 23:45 ` David Gibson 1 sibling, 0 replies; 20+ messages in thread From: David Gibson @ 2015-12-03 23:45 UTC (permalink / raw) To: Markus Armbruster; +Cc: peter.maydell, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 3043 bytes --] On Thu, Dec 03, 2015 at 05:37:38PM +0100, Markus Armbruster wrote: > Since prop_get_fdt() is only used with QmpOutputVisitor, errors > shouldn't actually happen, so this is only a latent bug. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_drc.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index f34bc04..4e7a1d3 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -254,6 +254,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > { > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj); > + Error *err = NULL; > int fdt_offset_next, fdt_offset, fdt_depth; > void *fdt; > > @@ -276,24 +277,43 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, > case FDT_BEGIN_NODE: > fdt_depth++; > name = fdt_get_name(fdt, fdt_offset, &name_len); > - visit_start_struct(v, NULL, NULL, name, 0, NULL); > + visit_start_struct(v, NULL, NULL, name, 0, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > break; > case FDT_END_NODE: > /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */ > g_assert(fdt_depth > 0); > - visit_end_struct(v, NULL); > + visit_end_struct(v, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > fdt_depth--; > break; > case FDT_PROP: { > int i; > prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len); > name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); > - visit_start_list(v, name, NULL); > + visit_start_list(v, name, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > for (i = 0; i < prop_len; i++) { > - visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL); > - > + visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + } > + visit_end_list(v, &err); > + if (err) { > + error_propagate(errp, err); > + return; > } > - visit_end_list(v, NULL); > break; > } > default: -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 16:37 [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly Markus Armbruster @ 2015-12-03 16:37 ` Markus Armbruster 2015-12-03 16:54 ` Eric Blake 2015-12-03 23:54 ` David Gibson 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device Markus Armbruster 2015-12-03 18:15 ` [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster 3 siblings, 2 replies; 20+ messages in thread From: Markus Armbruster @ 2015-12-03 16:37 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't visit anything. object_property_get_qobject() happily object_property_get_qobject(). Amazingly, the latter survives the misuse. Turns out we've papered over it long before prop_get_fdt() existed, in commit 1d10b44. However, commit 6c2f9a1 changed how we paper over it, and as a side effect changed qom-get's value from {} to null. Change it right back by fixing the visitor misuse. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/ppc/spapr_drc.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 4e7a1d3..dad157f 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, void *fdt; if (!drc->fdt) { + visit_start_struct(v, NULL, NULL, name, 0, &err); + if (!err) { + visit_end_struct(v, &err); + } + error_propagate(errp, err); return; } -- 2.4.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} Markus Armbruster @ 2015-12-03 16:54 ` Eric Blake 2015-12-03 17:13 ` Markus Armbruster 2015-12-03 23:54 ` David Gibson 1 sibling, 1 reply; 20+ messages in thread From: Eric Blake @ 2015-12-03 16:54 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david [-- Attachment #1: Type: text/plain, Size: 1963 bytes --] On 12/03/2015 09:37 AM, Markus Armbruster wrote: > prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't > visit anything. object_property_get_qobject() happily > object_property_get_qobject(). Amazingly, the latter survives the Something got lost or otherwise corrupted in that sentence. Were you trying to say one function happily calls another? If so, which of the two "object_property_get_qobject()" strings should be changed, to what? > misuse. Turns out we've papered over it long before prop_get_fdt() > existed, in commit 1d10b44. > > However, commit 6c2f9a1 changed how we paper over it, and as a side > effect changed qom-get's value from {} to null. Change it right back > by fixing the visitor misuse. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/ppc/spapr_drc.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 4e7a1d3..dad157f 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, > void *fdt; > > if (!drc->fdt) { > + visit_start_struct(v, NULL, NULL, name, 0, &err); visit_start_struct(v, NULL, "fdt", name, 0, &err) would give a nicer error message, but only if there were an error message to give. As already mentioned on 1/3, we are only using this for output visitors which don't raise errors (in fact, &error_abort would do, rather than worrying about error propagation). But I'm fine with the patch as you proposed it, once the commit message is fixed. Reviewed-by: Eric Blake <eblake@redhat.com> > + if (!err) { > + visit_end_struct(v, &err); > + } > + error_propagate(errp, err); > return; > } > > -- 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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 16:54 ` Eric Blake @ 2015-12-03 17:13 ` Markus Armbruster 2015-12-03 17:30 ` Eric Blake 0 siblings, 1 reply; 20+ messages in thread From: Markus Armbruster @ 2015-12-03 17:13 UTC (permalink / raw) To: Eric Blake; +Cc: peter.maydell, david, qemu-ppc, qemu-devel, agraf Eric Blake <eblake@redhat.com> writes: > On 12/03/2015 09:37 AM, Markus Armbruster wrote: >> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't >> visit anything. object_property_get_qobject() happily >> object_property_get_qobject(). Amazingly, the latter survives the > > Something got lost or otherwise corrupted in that sentence. Were you > trying to say one function happily calls another? If so, which of the > two "object_property_get_qobject()" strings should be changed, to what? No idea what happened. Correction: insert "calls" after "happily": prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't visit anything. object_property_get_qobject() happily calls object_property_get_qobject(). Amazingly, the latter survives the misuse. Turns out we've papered over it long before prop_get_fdt() existed, in commit 1d10b44. >> misuse. Turns out we've papered over it long before prop_get_fdt() >> existed, in commit 1d10b44. >> >> However, commit 6c2f9a1 changed how we paper over it, and as a side >> effect changed qom-get's value from {} to null. Change it right back >> by fixing the visitor misuse. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/ppc/spapr_drc.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c >> index 4e7a1d3..dad157f 100644 >> --- a/hw/ppc/spapr_drc.c >> +++ b/hw/ppc/spapr_drc.c >> @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, >> void *fdt; >> >> if (!drc->fdt) { >> + visit_start_struct(v, NULL, NULL, name, 0, &err); > > visit_start_struct(v, NULL, "fdt", name, 0, &err) > > would give a nicer error message, but only if there were an error > message to give. As already mentioned on 1/3, we are only using this > for output visitors which don't raise errors (in fact, &error_abort > would do, rather than worrying about error propagation). > > But I'm fine with the patch as you proposed it, once the commit message > is fixed. Some callers pass a third argument, some don't. We'll have to audit all visitor users anyway. We can decide whether and what to do about this issue then. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks for your quick review! >> + if (!err) { >> + visit_end_struct(v, &err); >> + } >> + error_propagate(errp, err); >> return; >> } >> >> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 17:13 ` Markus Armbruster @ 2015-12-03 17:30 ` Eric Blake 2015-12-03 17:55 ` Markus Armbruster 0 siblings, 1 reply; 20+ messages in thread From: Eric Blake @ 2015-12-03 17:30 UTC (permalink / raw) To: Markus Armbruster; +Cc: peter.maydell, david, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 1186 bytes --] On 12/03/2015 10:13 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 12/03/2015 09:37 AM, Markus Armbruster wrote: >>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't >>> visit anything. object_property_get_qobject() happily >>> object_property_get_qobject(). Amazingly, the latter survives the >> >> Something got lost or otherwise corrupted in that sentence. Were you >> trying to say one function happily calls another? If so, which of the >> two "object_property_get_qobject()" strings should be changed, to what? > > No idea what happened. Correction: insert "calls" after "happily": > > prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't > visit anything. object_property_get_qobject() happily calls > object_property_get_qobject(). That still reads "A() happily calls A()" - are we talking about recursion here? I still wonder if you meant a second function name, and/or mention of the fact that we are calling a function with NULL rather than a QObject? -- 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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 17:30 ` Eric Blake @ 2015-12-03 17:55 ` Markus Armbruster 2015-12-04 13:22 ` Eric Blake 0 siblings, 1 reply; 20+ messages in thread From: Markus Armbruster @ 2015-12-03 17:55 UTC (permalink / raw) To: Eric Blake; +Cc: peter.maydell, agraf, qemu-ppc, qemu-devel, david Eric Blake <eblake@redhat.com> writes: > On 12/03/2015 10:13 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 12/03/2015 09:37 AM, Markus Armbruster wrote: >>>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't >>>> visit anything. object_property_get_qobject() happily >>>> object_property_get_qobject(). Amazingly, the latter survives the >>> >>> Something got lost or otherwise corrupted in that sentence. Were you >>> trying to say one function happily calls another? If so, which of the >>> two "object_property_get_qobject()" strings should be changed, to what? >> >> No idea what happened. Correction: insert "calls" after "happily": >> >> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't >> visit anything. object_property_get_qobject() happily calls >> object_property_get_qobject(). >> > That still reads "A() happily calls A()" - are we talking about > recursion here? I still wonder if you meant a second function name, > and/or mention of the fact that we are calling a function with NULL > rather than a QObject? I'm incapable of proof-reading anything I wrote myself %-} prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't visit anything. object_property_get_qobject() happily calls qmp_output_get_qobject() then. Amazingly, the latter survives the misuse. Turns out we've papered over it long before prop_get_fdt() existed, in commit 1d10b44. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 17:55 ` Markus Armbruster @ 2015-12-04 13:22 ` Eric Blake 0 siblings, 0 replies; 20+ messages in thread From: Eric Blake @ 2015-12-04 13:22 UTC (permalink / raw) To: Markus Armbruster; +Cc: peter.maydell, agraf, qemu-ppc, qemu-devel, david [-- Attachment #1: Type: text/plain, Size: 712 bytes --] On 12/03/2015 10:55 AM, Markus Armbruster wrote: > I'm incapable of proof-reading anything I wrote myself %-} I know the feeling :) > > prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't > visit anything. object_property_get_qobject() happily calls > qmp_output_get_qobject() then. Amazingly, the latter survives the > misuse. Turns out we've papered over it long before prop_get_fdt() > existed, in commit 1d10b44. Oh well, it got pushed to master with original confusing spelling after all (commit ab8bf1d7). Not the end of the world :) -- 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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} Markus Armbruster 2015-12-03 16:54 ` Eric Blake @ 2015-12-03 23:54 ` David Gibson 2015-12-04 2:41 ` Eric Blake 1 sibling, 1 reply; 20+ messages in thread From: David Gibson @ 2015-12-03 23:54 UTC (permalink / raw) To: Markus Armbruster; +Cc: peter.maydell, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 1911 bytes --] On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote: > prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't > visit anything. object_property_get_qobject() happily > object_property_get_qobject(). Amazingly, the latter survives the > misuse. Turns out we've papered over it long before prop_get_fdt() > existed, in commit 1d10b44. > > However, commit 6c2f9a1 changed how we paper over it, and as a side > effect changed qom-get's value from {} to null. Change it right back > by fixing the visitor misuse. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/ppc/spapr_drc.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 4e7a1d3..dad157f 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void *opaque, > void *fdt; > > if (!drc->fdt) { > + visit_start_struct(v, NULL, NULL, name, 0, &err); > + if (!err) { > + visit_end_struct(v, &err); > + } > + error_propagate(errp, err); > return; > } > I'm not entirely convinced by this. IIUC, this makes the output in the case of NULL (i.e. missing) fdt identical to the output in the case of an empty, valid fdt - in dtc syntax, this: / { }; Those are different cases from the point of view of the code which actually uses the fdt, and for purposes of debugging it, I suspect we want to expose that difference. I don't know what the QOMishly correct way of doing that is, though. Can we somehow make the "fdt" property disappear entirely if fdt is NULL? -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-03 23:54 ` David Gibson @ 2015-12-04 2:41 ` Eric Blake 2015-12-04 5:50 ` David Gibson 2015-12-04 8:15 ` Markus Armbruster 0 siblings, 2 replies; 20+ messages in thread From: Eric Blake @ 2015-12-04 2:41 UTC (permalink / raw) To: David Gibson, Markus Armbruster Cc: peter.maydell, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 2426 bytes --] On 12/03/2015 04:54 PM, David Gibson wrote: > On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote: >> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't >> visit anything. object_property_get_qobject() happily >> object_property_get_qobject(). Amazingly, the latter survives the >> misuse. Turns out we've papered over it long before prop_get_fdt() >> existed, in commit 1d10b44. >> >> However, commit 6c2f9a1 changed how we paper over it, and as a side >> effect changed qom-get's value from {} to null. Change it right back >> by fixing the visitor misuse. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/ppc/spapr_drc.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> > I'm not entirely convinced by this. IIUC, this makes the output in > the case of NULL (i.e. missing) fdt identical to the output in the > case of an empty, valid fdt - in dtc syntax, this: > / { > }; > > Those are different cases from the point of view of the code which > actually uses the fdt, and for purposes of debugging it, I suspect we > want to expose that difference. Expressing null may be the right thing, but it should be a conscious decision, and not a side-effect of an unrelated patch. This patch is just about avoiding a regression for 2.5, because outputting {} for both a missing fdt and an empty one was the behavior we had back in 2.4 (that is, we've already returned {} in at least one release, so it won't hurt to do it for one more). For 2.6 we can revisit things to actually express what is wanted. > > I don't know what the QOMishly correct way of doing that is, though. > Can we somehow make the "fdt" property disappear entirely if fdt is > NULL? In qapi terms, if a variable is marked optional and has_FOO is false, then the variable disappears completely. But I'm not sure if that maps over to qom. Maybe you do it by setting errp if drc->fdt is NULL, so that prop_get_fdt() only succeeds when there is something for it to return. Or maybe returning qnull() is right after all, but in that case, explicitly calling 'QObject *n = qnull(); visit_type_any(v, &n, NULL, &err) seems like a nicer way than relying on side effects of how the qmp output visitor behaves when nothing was visited. -- 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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-04 2:41 ` Eric Blake @ 2015-12-04 5:50 ` David Gibson 2015-12-04 8:15 ` Markus Armbruster 1 sibling, 0 replies; 20+ messages in thread From: David Gibson @ 2015-12-04 5:50 UTC (permalink / raw) To: Eric Blake; +Cc: peter.maydell, agraf, qemu-ppc, Markus Armbruster, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2752 bytes --] On Thu, Dec 03, 2015 at 07:41:59PM -0700, Eric Blake wrote: > On 12/03/2015 04:54 PM, David Gibson wrote: > > On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote: > >> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't > >> visit anything. object_property_get_qobject() happily > >> object_property_get_qobject(). Amazingly, the latter survives the > >> misuse. Turns out we've papered over it long before prop_get_fdt() > >> existed, in commit 1d10b44. > >> > >> However, commit 6c2f9a1 changed how we paper over it, and as a side > >> effect changed qom-get's value from {} to null. Change it right back > >> by fixing the visitor misuse. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- > >> hw/ppc/spapr_drc.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > > > I'm not entirely convinced by this. IIUC, this makes the output in > > the case of NULL (i.e. missing) fdt identical to the output in the > > case of an empty, valid fdt - in dtc syntax, this: > > / { > > }; > > > > Those are different cases from the point of view of the code which > > actually uses the fdt, and for purposes of debugging it, I suspect we > > want to expose that difference. > > Expressing null may be the right thing, but it should be a conscious > decision, and not a side-effect of an unrelated patch. This patch is > just about avoiding a regression for 2.5, because outputting {} for both > a missing fdt and an empty one was the behavior we had back in 2.4 (that > is, we've already returned {} in at least one release, so it won't hurt > to do it for one more). For 2.6 we can revisit things to actually > express what is wanted. > > > > > I don't know what the QOMishly correct way of doing that is, though. > > Can we somehow make the "fdt" property disappear entirely if fdt is > > NULL? > > In qapi terms, if a variable is marked optional and has_FOO is false, > then the variable disappears completely. But I'm not sure if that maps > over to qom. Maybe you do it by setting errp if drc->fdt is NULL, so > that prop_get_fdt() only succeeds when there is something for it to > return. Or maybe returning qnull() is right after all, but in that > case, explicitly calling 'QObject *n = qnull(); visit_type_any(v, &n, > NULL, &err) seems like a nicer way than relying on side effects of how > the qmp output visitor behaves when nothing was visited. Ok, thank makes sense. Acked-by: David Gibson <david@gibson.dropbear.id.au> -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} 2015-12-04 2:41 ` Eric Blake 2015-12-04 5:50 ` David Gibson @ 2015-12-04 8:15 ` Markus Armbruster 1 sibling, 0 replies; 20+ messages in thread From: Markus Armbruster @ 2015-12-04 8:15 UTC (permalink / raw) To: Eric Blake; +Cc: peter.maydell, agraf, qemu-ppc, qemu-devel, David Gibson Eric Blake <eblake@redhat.com> writes: > On 12/03/2015 04:54 PM, David Gibson wrote: >> On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote: >>> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't >>> visit anything. object_property_get_qobject() happily >>> object_property_get_qobject(). Amazingly, the latter survives the >>> misuse. Turns out we've papered over it long before prop_get_fdt() >>> existed, in commit 1d10b44. >>> >>> However, commit 6c2f9a1 changed how we paper over it, and as a side >>> effect changed qom-get's value from {} to null. Change it right back >>> by fixing the visitor misuse. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> hw/ppc/spapr_drc.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> > >> I'm not entirely convinced by this. IIUC, this makes the output in >> the case of NULL (i.e. missing) fdt identical to the output in the >> case of an empty, valid fdt - in dtc syntax, this: >> / { >> }; >> >> Those are different cases from the point of view of the code which >> actually uses the fdt, and for purposes of debugging it, I suspect we >> want to expose that difference. > > Expressing null may be the right thing, but it should be a conscious > decision, and not a side-effect of an unrelated patch. This patch is > just about avoiding a regression for 2.5, because outputting {} for both > a missing fdt and an empty one was the behavior we had back in 2.4 (that > is, we've already returned {} in at least one release, so it won't hurt > to do it for one more). For 2.6 we can revisit things to actually > express what is wanted. Yes. >> I don't know what the QOMishly correct way of doing that is, though. >> Can we somehow make the "fdt" property disappear entirely if fdt is >> NULL? > > In qapi terms, if a variable is marked optional and has_FOO is false, > then the variable disappears completely. If I understand QOM correctly, we should be able to add the property dynamically, so that it exists exactly when fdt is non-null. > But I'm not sure if that maps > over to qom. Maybe you do it by setting errp if drc->fdt is NULL, so > that prop_get_fdt() only succeeds when there is something for it to > return. Works, but is it really an erroneous state or operation? If not, a special value seems more appropriate than an error. > Or maybe returning qnull() is right after all, but in that > case, explicitly calling 'QObject *n = qnull(); visit_type_any(v, &n, > NULL, &err) seems like a nicer way than relying on side effects of how > the qmp output visitor behaves when nothing was visited. I think we should have visit_none(), and attempting to retrieve a visits value when you haven't visited anything should be an error. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device 2015-12-03 16:37 [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly Markus Armbruster 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} Markus Armbruster @ 2015-12-03 16:37 ` Markus Armbruster 2015-12-03 16:55 ` Eric Blake ` (2 more replies) 2015-12-03 18:15 ` [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster 3 siblings, 3 replies; 20+ messages in thread From: Markus Armbruster @ 2015-12-03 16:37 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david It should only be created via spapr_dr_connector_new(). Attempting to create it with -device crashes. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/ppc/spapr_drc.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index dad157f..8be62c3 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data) drck->attach = attach; drck->detach = detach; drck->release_pending = release_pending; + /* + * Reason: it crashes FIXME find and document the real reason + */ + dk->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo spapr_dr_connector_info = { -- 2.4.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device Markus Armbruster @ 2015-12-03 16:55 ` Eric Blake 2015-12-03 17:18 ` Thomas Huth 2015-12-03 23:55 ` David Gibson 2 siblings, 0 replies; 20+ messages in thread From: Eric Blake @ 2015-12-03 16:55 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david [-- Attachment #1: Type: text/plain, Size: 1135 bytes --] On 12/03/2015 09:37 AM, Markus Armbruster wrote: > It should only be created via spapr_dr_connector_new(). Attempting to > create it with -device crashes. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/ppc/spapr_drc.c | 4 ++++ > 1 file changed, 4 insertions(+) For 2.5, this is an appropriate last-minute crash-avoider. The FIXME reminds us to do better in 2.6. Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index dad157f..8be62c3 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data) > drck->attach = attach; > drck->detach = detach; > drck->release_pending = release_pending; > + /* > + * Reason: it crashes FIXME find and document the real reason > + */ > + dk->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo spapr_dr_connector_info = { > -- 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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device Markus Armbruster 2015-12-03 16:55 ` Eric Blake @ 2015-12-03 17:18 ` Thomas Huth 2015-12-03 23:55 ` David Gibson 2 siblings, 0 replies; 20+ messages in thread From: Thomas Huth @ 2015-12-03 17:18 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david On 03/12/15 17:37, Markus Armbruster wrote: > It should only be created via spapr_dr_connector_new(). Attempting to > create it with -device crashes. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/ppc/spapr_drc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index dad157f..8be62c3 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data) > drck->attach = attach; > drck->detach = detach; > drck->release_pending = release_pending; > + /* > + * Reason: it crashes FIXME find and document the real reason > + */ > + dk->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo spapr_dr_connector_info = { That fix should be fine for 2.5. Reviewed-by: Thomas Huth <thuth@redhat.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device Markus Armbruster 2015-12-03 16:55 ` Eric Blake 2015-12-03 17:18 ` Thomas Huth @ 2015-12-03 23:55 ` David Gibson 2 siblings, 0 replies; 20+ messages in thread From: David Gibson @ 2015-12-03 23:55 UTC (permalink / raw) To: Markus Armbruster; +Cc: peter.maydell, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 1137 bytes --] On Thu, Dec 03, 2015 at 05:37:40PM +0100, Markus Armbruster wrote: > It should only be created via spapr_dr_connector_new(). Attempting to > create it with -device crashes. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_drc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index dad157f..8be62c3 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -599,6 +599,10 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data) > drck->attach = attach; > drck->detach = detach; > drck->release_pending = release_pending; > + /* > + * Reason: it crashes FIXME find and document the real reason > + */ > + dk->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo spapr_dr_connector_info = { -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes 2015-12-03 16:37 [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster ` (2 preceding siblings ...) 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device Markus Armbruster @ 2015-12-03 18:15 ` Markus Armbruster 2015-12-03 23:57 ` David Gibson 3 siblings, 1 reply; 20+ messages in thread From: Markus Armbruster @ 2015-12-03 18:15 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, qemu-ppc, agraf, david Markus Armbruster <armbru@redhat.com> writes: > I hope we can get these reviewed and applied in time. Peter, can you apply directly with 2/3's commit message fixed? Let me know if you'd prefer a pull request. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes 2015-12-03 18:15 ` [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster @ 2015-12-03 23:57 ` David Gibson 0 siblings, 0 replies; 20+ messages in thread From: David Gibson @ 2015-12-03 23:57 UTC (permalink / raw) To: Markus Armbruster; +Cc: peter.maydell, qemu-ppc, qemu-devel, agraf [-- Attachment #1: Type: text/plain, Size: 621 bytes --] On Thu, Dec 03, 2015 at 07:15:15PM +0100, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > I hope we can get these reviewed and applied in time. > > Peter, can you apply directly with 2/3's commit message fixed? Let me > know if you'd prefer a pull request. I've applied 1 & 3 to my ppc-for-2.5 branch, if we want a pull req from there. 2 I'm holding off for a reply to my query. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2015-12-04 13:22 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-03 16:37 [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 1/3] spapr_drc: Handle visitor errors properly Markus Armbruster 2015-12-03 16:45 ` Eric Blake 2015-12-03 23:45 ` David Gibson 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {} Markus Armbruster 2015-12-03 16:54 ` Eric Blake 2015-12-03 17:13 ` Markus Armbruster 2015-12-03 17:30 ` Eric Blake 2015-12-03 17:55 ` Markus Armbruster 2015-12-04 13:22 ` Eric Blake 2015-12-03 23:54 ` David Gibson 2015-12-04 2:41 ` Eric Blake 2015-12-04 5:50 ` David Gibson 2015-12-04 8:15 ` Markus Armbruster 2015-12-03 16:37 ` [Qemu-devel] [PATCH for-2.5 3/3] spapr_drc: Make device "spapr-dr-connector" unavailable with -device Markus Armbruster 2015-12-03 16:55 ` Eric Blake 2015-12-03 17:18 ` Thomas Huth 2015-12-03 23:55 ` David Gibson 2015-12-03 18:15 ` [Qemu-devel] [PATCH for-2.5 0/3] spapr_drc: Last minute bug fixes Markus Armbruster 2015-12-03 23:57 ` David Gibson
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.