[Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

[Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

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

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

Re: [Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

[Marcel Apfelbaum]

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 {

Re: [Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

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

- Cole

Re: [Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-2.0] qom: Fix crash with qom-list and link properties

[Peter Maydell]

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