| Message ID | 1333451753-3550-8-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 03/04/2012 14:28, Jan Kiszka ha scritto: >> > if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) { >> > value = object_property_get_str(OBJECT(dev), legacy_name, &err); > This is not a criticism on this patch, but I'd like to underline that > the line above only works because legacy props also define print ^^^^ You mean always, I guess. > But this statement above is still inconsistent. We should either > assert(print && parse) or handle their non-existence properly. Yes, asserting that print/parse are always present in pairs (if at all) would be good. Paolo
On 04/03/2012 06:15 AM, Paolo Bonzini wrote: > Otherwise, non-string properties without a legacy counterpart are missed. > Also fix error propagation in object_property_print itself, otherwise > pointer properties are printed as "<null>". > > Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> Regards, Anthony Liguori > --- > hw/qdev-monitor.c | 2 +- > qom/object.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index 0acfc82..07ac525 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -492,7 +492,7 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props, > if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) { > value = object_property_get_str(OBJECT(dev), legacy_name,&err); > } else { > - value = object_property_get_str(OBJECT(dev), props->name,&err); > + value = object_property_print(OBJECT(dev), props->name,&err); > } > g_free(legacy_name); > > diff --git a/qom/object.c b/qom/object.c > index f3ffaa6..ff36946 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -850,7 +850,7 @@ char *object_property_print(Object *obj, const char *name, > char *string; > > mo = string_output_visitor_new(); > - object_property_get(obj, string_output_get_visitor(mo), name, NULL); > + object_property_get(obj, string_output_get_visitor(mo), name, errp); > string = string_output_get_string(mo); > string_output_visitor_cleanup(mo); > return string;
On 2012-04-03 18:06, Anthony Liguori wrote: > On 04/03/2012 06:15 AM, Paolo Bonzini wrote: >> Otherwise, non-string properties without a legacy counterpart are missed. >> Also fix error propagation in object_property_print itself, otherwise >> pointer properties are printed as "<null>". >> >> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> > > Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > The bug that this fixes is still present in current 1.1-rc. Any chance that the patch makes it into the release? Jan
Il 10/05/2012 22:58, Jan Kiszka ha scritto: >>> >> Otherwise, non-string properties without a legacy counterpart are missed. >>> >> Also fix error propagation in object_property_print itself, otherwise >>> >> pointer properties are printed as "<null>". >>> >> >>> >> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> >> > >> > Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> >> > > The bug that this fixes is still present in current 1.1-rc. Any chance > that the patch makes it into the release? Andreas, are you queueing this patch and 6/25 for 1.1-rc2? Paolo
Am 11.05.2012 13:28, schrieb Paolo Bonzini: > Il 10/05/2012 22:58, Jan Kiszka ha scritto: >>>>>> Otherwise, non-string properties without a legacy counterpart are missed. >>>>>> Also fix error propagation in object_property_print itself, otherwise >>>>>> pointer properties are printed as "<null>". >>>>>> >>>>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> >>>> >>>> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> >>>> >> The bug that this fixes is still present in current 1.1-rc. Any chance >> that the patch makes it into the release? > > Andreas, are you queueing this patch and 6/25 for 1.1-rc2? I need to review it first and I try to pick the latest one (which would presumable be in the QBus series), but yes, this and one other of yours (the one with the prop->set or so check) is on my radar for 1.1-rc2. I think I'll start a qom-1.1 branch to make this more transparent. Andreas
Am 03.04.2012 15:05, schrieb Paolo Bonzini: > Il 03/04/2012 14:28, Jan Kiszka ha scritto: >>>> if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) { >>>> value = object_property_get_str(OBJECT(dev), legacy_name, &err); >> This is not a criticism on this patch, but I'd like to underline that >> the line above only works because legacy props also define print > ^^^^ > > You mean always, I guess. > >> But this statement above is still inconsistent. We should either >> assert(print && parse) or handle their non-existence properly. > > Yes, asserting that print/parse are always present in pairs (if at all) > would be good. Paolo, has this issue been addressed somewhere? Andreas
> Am 03.04.2012 15:05, schrieb Paolo Bonzini: > > Il 03/04/2012 14:28, Jan Kiszka ha scritto: > >>>> if (object_property_get_type(OBJECT(dev), legacy_name, > >>>> NULL)) { > >>>> value = object_property_get_str(OBJECT(dev), > >>>> legacy_name, &err); > >> [...] We should either > >> assert(print && parse) or handle their non-existence properly. > > > > Yes, asserting that print/parse are always present in pairs (if at > > all) would be good. > > Paolo, has this issue been addressed somewhere? No, but it's a simple assertion. It doesn't look like 1.1 material. I should send and rebase the bus series today, I'll include it. Paolo
----- Messaggio originale ----- > Da: "Paolo Bonzini" <pbonzini@redhat.com> > A: "Andreas Färber" <afaerber@suse.de> > Cc: "Jan Kiszka" <jan.kiszka@siemens.com>, aliguori@us.ibm.com, qemu-devel@nongnu.org > Inviato: Mercoledì, 16 maggio 2012 9:40:12 > Oggetto: Re: [Qemu-devel] [PATCH 07/25] qdev: use object_property_print in info qtree > > > Am 03.04.2012 15:05, schrieb Paolo Bonzini: > > > Il 03/04/2012 14:28, Jan Kiszka ha scritto: > > >>>> if (object_property_get_type(OBJECT(dev), > > >>>> legacy_name, > > >>>> NULL)) { > > >>>> value = object_property_get_str(OBJECT(dev), > > >>>> legacy_name, &err); > > >> [...] We should either > > >> assert(print && parse) or handle their non-existence properly. > > > > > > Yes, asserting that print/parse are always present in pairs (if > > > at > > > all) would be good. > > > > Paolo, has this issue been addressed somewhere? > > No, but it's a simple assertion. It doesn't look like 1.1 material. > I should send and rebase the bus series today, I'll include it. Doh, it was handled by commit 68ee356941801d0a17fdc43b11ac3e6b72fcd597 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Thu Feb 2 10:17:19 2012 +0100 qdev: allow reusing get/set for legacy property print/parse do not have to be present in pairs anymore. Paolo
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index 0acfc82..07ac525 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -492,7 +492,7 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props, if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) { value = object_property_get_str(OBJECT(dev), legacy_name, &err); } else { - value = object_property_get_str(OBJECT(dev), props->name, &err); + value = object_property_print(OBJECT(dev), props->name, &err); } g_free(legacy_name); diff --git a/qom/object.c b/qom/object.c index f3ffaa6..ff36946 100644 --- a/qom/object.c +++ b/qom/object.c @@ -850,7 +850,7 @@ char *object_property_print(Object *obj, const char *name, char *string; mo = string_output_visitor_new(); - object_property_get(obj, string_output_get_visitor(mo), name, NULL); + object_property_get(obj, string_output_get_visitor(mo), name, errp); string = string_output_get_string(mo); string_output_visitor_cleanup(mo); return string;
Otherwise, non-string properties without a legacy counterpart are missed. Also fix error propagation in object_property_print itself, otherwise pointer properties are printed as "<null>". Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/qdev-monitor.c | 2 +- qom/object.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)