| Message ID | 2f8f007ce2152ac3b65f0811199662799c509225.1397155389.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2014-04-10 at 14:47 -0400, Cole Robinson wrote: > Commit 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 changed the type of > 'opaque' for link properties, but missed updating this call site. > Reproducer: > > ./x86_64-softmmu/qemu-system-x86_64 -qmp unix:./qmp.sock,server & > ./scripts/qmp/qmp-shell ./qmp.sock > (QEMU) qom-list path=//machine/i440fx/pci.0/child[2] > > Reported-by: Marcin Gibuła <m.gibula@beyond.pl> > Signed-off-by: Cole Robinson <crobinso@redhat.com> > --- > qom/object.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/qom/object.c b/qom/object.c > index f4de619..9a730e7 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -1225,7 +1225,8 @@ Object *object_resolve_path_component(Object *parent, const gchar *part) > } > > if (object_property_is_link(prop)) { > - return *(Object **)prop->opaque; > + LinkProperty *lprop = prop->opaque; > + return *lprop->child; Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com> You may want another review from maintainers :), but I think the fix is fine. Thanks, Marcel > } else if (object_property_is_child(prop)) { > return prop->opaque; > } else {
On 10 April 2014 19:47, Cole Robinson <crobinso@redhat.com> wrote: > Commit 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 changed the type of > 'opaque' for link properties, but missed updating this call site. > Reproducer: > > ./x86_64-softmmu/qemu-system-x86_64 -qmp unix:./qmp.sock,server & > ./scripts/qmp/qmp-shell ./qmp.sock > (QEMU) qom-list path=//machine/i440fx/pci.0/child[2] > > Reported-by: Marcin Gibuła <m.gibula@beyond.pl> > Signed-off-by: Cole Robinson <crobinso@redhat.com> > --- > qom/object.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/qom/object.c b/qom/object.c > index f4de619..9a730e7 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -1225,7 +1225,8 @@ Object *object_resolve_path_component(Object *parent, const gchar *part) > } > > if (object_property_is_link(prop)) { > - return *(Object **)prop->opaque; > + LinkProperty *lprop = prop->opaque; > + return *lprop->child; > } else if (object_property_is_child(prop)) { > return prop->opaque; > } else { > -- > 1.9.0 Reviewed-by: Peter Maydell <peter.maydell@linaro.org> -- PMM
Am 10.04.2014 20:47, schrieb Cole Robinson: > Commit 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 changed the type of > 'opaque' for link properties, but missed updating this call site. > Reproducer: > > ./x86_64-softmmu/qemu-system-x86_64 -qmp unix:./qmp.sock,server & > ./scripts/qmp/qmp-shell ./qmp.sock > (QEMU) qom-list path=//machine/i440fx/pci.0/child[2] I would much prefer if we could give the path as just path=/machine/i440fx/pci.0/child[2] (without double slash). Peter can you improve when applying? > > Reported-by: Marcin Gibuła <m.gibula@beyond.pl> > Signed-off-by: Cole Robinson <crobinso@redhat.com> > --- > qom/object.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Andreas Färber <afaerber@suse.de> I would like to remark that specifically for regression-testing link<> properties I had extended qom-test, and it succeeds currently. So Cole, if you have any suggestion on how to extend it to catch this regression that would be welcome. Thanks, Andreas
On 04/11/2014 12:57 PM, Andreas Färber wrote: > Am 10.04.2014 20:47, schrieb Cole Robinson: >> Commit 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 changed the type of >> 'opaque' for link properties, but missed updating this call site. >> Reproducer: >> >> ./x86_64-softmmu/qemu-system-x86_64 -qmp unix:./qmp.sock,server & >> ./scripts/qmp/qmp-shell ./qmp.sock >> (QEMU) qom-list path=//machine/i440fx/pci.0/child[2] > > I would much prefer if we could give the path as just > path=/machine/i440fx/pci.0/child[2] (without double slash). Peter can > you improve when applying? > >> >> Reported-by: Marcin Gibuła <m.gibula@beyond.pl> >> Signed-off-by: Cole Robinson <crobinso@redhat.com> >> --- >> qom/object.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > I would like to remark that specifically for regression-testing link<> > properties I had extended qom-test, and it succeeds currently. So Cole, > if you have any suggestion on how to extend it to catch this regression > that would be welcome. > I haven't looked closely at tests/, I see *qmp*.c files, but is there infrastructure for running qmp commands? If so we could turn the above reproducer into a test case. - Cole
On 11 April 2014 17:57, Andreas Färber <afaerber@suse.de> wrote: > Am 10.04.2014 20:47, schrieb Cole Robinson: >> Commit 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 changed the type of >> 'opaque' for link properties, but missed updating this call site. >> Reproducer: >> >> ./x86_64-softmmu/qemu-system-x86_64 -qmp unix:./qmp.sock,server & >> ./scripts/qmp/qmp-shell ./qmp.sock >> (QEMU) qom-list path=//machine/i440fx/pci.0/child[2] > > I would much prefer if we could give the path as just > path=/machine/i440fx/pci.0/child[2] (without double slash). Peter can > you improve when applying? Sorry, had already pushed before I read this. >> >> Reported-by: Marcin Gibuła <m.gibula@beyond.pl> >> Signed-off-by: Cole Robinson <crobinso@redhat.com> >> --- >> qom/object.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > I would like to remark that specifically for regression-testing link<> > properties I had extended qom-test, and it succeeds currently. So Cole, > if you have any suggestion on how to extend it to catch this regression > that would be welcome. Applied, thanks. -- PMM
Am 11.04.2014 19:05, schrieb Cole Robinson: > On 04/11/2014 12:57 PM, Andreas Färber wrote: >> Am 10.04.2014 20:47, schrieb Cole Robinson: >>> Commit 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 changed the type of >>> 'opaque' for link properties, but missed updating this call site. >>> Reproducer: >>> >>> ./x86_64-softmmu/qemu-system-x86_64 -qmp unix:./qmp.sock,server & >>> ./scripts/qmp/qmp-shell ./qmp.sock >>> (QEMU) qom-list path=//machine/i440fx/pci.0/child[2] >> >> I would much prefer if we could give the path as just >> path=/machine/i440fx/pci.0/child[2] (without double slash). Peter can >> you improve when applying? >> >>> >>> Reported-by: Marcin Gibuła <m.gibula@beyond.pl> >>> Signed-off-by: Cole Robinson <crobinso@redhat.com> >>> --- >>> qom/object.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> Reviewed-by: Andreas Färber <afaerber@suse.de> >> >> I would like to remark that specifically for regression-testing link<> >> properties I had extended qom-test, and it succeeds currently. So Cole, >> if you have any suggestion on how to extend it to catch this regression >> that would be welcome. >> > > I haven't looked closely at tests/, I see *qmp*.c files, but is there > infrastructure for running qmp commands? If so we could turn the above > reproducer into a test case. Please see the mentioned tests/qom-test.c, it is already doing qom-list + qom-get for all properties - therefore wondering why we didn't catch this. Regards, Andreas
diff --git a/qom/object.c b/qom/object.c index f4de619..9a730e7 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1225,7 +1225,8 @@ Object *object_resolve_path_component(Object *parent, const gchar *part) } if (object_property_is_link(prop)) { - return *(Object **)prop->opaque; + LinkProperty *lprop = prop->opaque; + return *lprop->child; } else if (object_property_is_child(prop)) { return prop->opaque; } else {
Commit 9561fda8d90e176bef598ba87c42a1bd6ad03ef7 changed the type of 'opaque' for link properties, but missed updating this call site. Reproducer: ./x86_64-softmmu/qemu-system-x86_64 -qmp unix:./qmp.sock,server & ./scripts/qmp/qmp-shell ./qmp.sock (QEMU) qom-list path=//machine/i440fx/pci.0/child[2] Reported-by: Marcin Gibuła <m.gibula@beyond.pl> Signed-off-by: Cole Robinson <crobinso@redhat.com> --- qom/object.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)