Patchwork [v2,2/2] qom/object.c: Allow itf cast with num_itfs = 0

login
register
mail settings
Submitter Peter Crosthwaite
Date Feb. 19, 2013, 4:02 a.m.
Message ID <a8c2db3b9b1f3c4bb81aca352b69e33260f36545.1361246206.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/221611/
State New
Headers show

Comments

Peter Crosthwaite - Feb. 19, 2013, 4:02 a.m.
num_interfaces only tells you how many interfaces the concrete child class has
(as defined in the TypeInfo). This means if you have a child class which defines
no interfaces of its own, but its parent has interfaces you cannot cast to those
parent interfaces.

Fixed changing the guard to check the class->interfaces list instead (which is
a complete flattened list of implemented interfaces).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed from v1:
Change guard implementation rather than removed it (Paolo review)

 qom/object.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Paolo Bonzini - Feb. 19, 2013, 9:07 a.m.
Il 19/02/2013 05:02, Peter Crosthwaite ha scritto:
> num_interfaces only tells you how many interfaces the concrete child class has
> (as defined in the TypeInfo). This means if you have a child class which defines
> no interfaces of its own, but its parent has interfaces you cannot cast to those
> parent interfaces.
> 
> Fixed changing the guard to check the class->interfaces list instead (which is
> a complete flattened list of implemented interfaces).
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed from v1:
> Change guard implementation rather than removed it (Paolo review)
> 
>  qom/object.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 4b72a64..3d638ff 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -449,7 +449,8 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>      TypeImpl *type = class->type;
>      ObjectClass *ret = NULL;
>  
> -    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
> +    if (type->class->interfaces &&
> +            type_is_ancestor(target_type, type_interface)) {
>          int found = 0;
>          GSList *i;
>  
> 

Series looks good, thanks!

Paolo
Peter Crosthwaite - Feb. 21, 2013, 8:08 a.m.
Hi Paolo,

On Tue, Feb 19, 2013 at 7:07 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 19/02/2013 05:02, Peter Crosthwaite ha scritto:
>> num_interfaces only tells you how many interfaces the concrete child class has
>> (as defined in the TypeInfo). This means if you have a child class which defines
>> no interfaces of its own, but its parent has interfaces you cannot cast to those
>> parent interfaces.
>>
>> Fixed changing the guard to check the class->interfaces list instead (which is
>> a complete flattened list of implemented interfaces).
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> changed from v1:
>> Change guard implementation rather than removed it (Paolo review)
>>
>>  qom/object.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 4b72a64..3d638ff 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -449,7 +449,8 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>>      TypeImpl *type = class->type;
>>      ObjectClass *ret = NULL;
>>
>> -    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
>> +    if (type->class->interfaces &&
>> +            type_is_ancestor(target_type, type_interface)) {
>>          int found = 0;
>>          GSList *i;
>>
>>
>
> Series looks good, thanks!
>
Can I take that as an RB or acked-by? :-)

Regards,
Peter

> Paolo
>
Paolo Bonzini - Feb. 21, 2013, 12:17 p.m.
> >> diff --git a/qom/object.c b/qom/object.c
> >> index 4b72a64..3d638ff 100644
> >> --- a/qom/object.c
> >> +++ b/qom/object.c
> >> @@ -449,7 +449,8 @@ ObjectClass
> >> *object_class_dynamic_cast(ObjectClass *class,
> >>      TypeImpl *type = class->type;
> >>      ObjectClass *ret = NULL;
> >>
> >> -    if (type->num_interfaces && type_is_ancestor(target_type,
> >> type_interface)) {
> >> +    if (type->class->interfaces &&
> >> +            type_is_ancestor(target_type, type_interface)) {
> >>          int found = 0;
> >>          GSList *i;
> >>
> >>
> >
> > Series looks good, thanks!
>
> Can I take that as an RB or acked-by? :-)

I try to stay as far as possible from QOM these days, but people
don't let me... :)  Yes, it is a Reviewed-by.

Paolo
Anthony Liguori - Feb. 21, 2013, 8:01 p.m.
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> num_interfaces only tells you how many interfaces the concrete child class has
> (as defined in the TypeInfo). This means if you have a child class which defines
> no interfaces of its own, but its parent has interfaces you cannot cast to those
> parent interfaces.
>
> Fixed changing the guard to check the class->interfaces list instead (which is
> a complete flattened list of implemented interfaces).
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

I must admit I don't understand it yet, but this breaks hot unplug.

Here is the qemu-test output:

[aliguori@ccnode4 qemu-test]$ QEMU_TEST_SEED=45240 ./qemu-test ~/build/qemu/x86_64-softmmu/qemu-system-x86_64 tests/device-del.sh
Using RANDOM seed 45240
Testing nic
Formatting '.tmp-31849/disk.img', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off 
/home/aliguori/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel /usr/local/share/qemu-jeos/kernel-x86_64-pc -initrd .tmp-31849/initramfs-31849.img.gz -device isa-debug-exit -append console=ttyS0 seed=45240 -nographic -enable-kvm -device virtio-balloon-pci -pidfile .tmp-31849/pidfile-31849.pid -qmp unix:.tmp-31849/qmpsock-31849.sock,server,nowait
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Linux version 3.4.0 (root@ccnode4) (gcc version 4.6.4 20120830 (prerelease) (GCC) ) #2 SMP Mon Dec 3 19:40:41 CST 2012
[    0.000000] Command line: console=ttyS0 seed=45240
[    0.000000] BIOS-provided physical RAM map:

<- snip ->

[    0.865631] Freeing unused kernel memory: 584k freed
[    0.866320] Write protecting the kernel read-only data: 12288k
[    0.868626] Freeing unused kernel memory: 640k freed
[    0.873366] Freeing unused kernel memory: 1724k freed
Setting guest RANDOM seed to 45240
*** Running tests ***
/tests/device-del.sh
Running test /tests/device-del.sh...
** waiting for hotplug **
./tests/device-del.sh: line 36: test: !=: unary operator expected
** waiting for remove **
** waiting for guest to see device **
./qemu-test: line 99: 31883 Segmentation fault      (core dumped) "$@"

The backtrace is:

Program terminated with signal 11, Segmentation fault.
#0  qemu_bh_delete (bh=0x0) at /home/aliguori/git/qemu/async.c:116
116	    bh->scheduled = 0;
Missing separate debuginfos, use: debuginfo-install bzip2-libs-1.0.6-7.fc18.x86_64 glib2-2.34.2-2.fc18.x86_64 glibc-2.16-28.fc18.x86_64 libaio-0.3.109-6.fc18.x86_64 libgcc-4.7.2-8.fc18.x86_64 pixman-0.26.2-5.fc18.x86_64 xen-libs-4.2.1-5.fc18.x86_64 xz-libs-5.1.2-2alpha.fc18.x86_64 zlib-1.2.7-9.fc18.x86_64
(gdb) bt
#0  qemu_bh_delete (bh=0x0) at /home/aliguori/git/qemu/async.c:116
#1  0x00007f55cac9bd95 in virtio_net_exit (vdev=0x7f55cd6eb960)
    at /home/aliguori/git/qemu/hw/virtio-net.c:1413
#2  0x00007f55cabde7f9 in virtio_net_exit_pci (pci_dev=0x7f55cd668260)
    at /home/aliguori/git/qemu/hw/virtio-pci.c:1016
#3  0x00007f55cab92e4a in pci_unregister_device (dev=<optimized out>)
    at /home/aliguori/git/qemu/hw/pci/pci.c:889
#4  0x00007f55caba7485 in device_unparent (obj=<optimized out>)
    at /home/aliguori/git/qemu/hw/qdev.c:773
#5  0x00007f55cac1ea0f in object_unparent (obj=0x7f55cd668260)
    at /home/aliguori/git/qemu/qom/object.c:370
#6  0x00007f55caba7a4d in qdev_free (dev=<optimized out>)
    at /home/aliguori/git/qemu/hw/qdev.c:270
#7  0x00007f55cab38da0 in acpi_piix_eject_slot (s=0x7f55cd68fd50, 
    slots=<optimized out>) at /home/aliguori/git/qemu/hw/acpi_piix4.c:306
#8  0x00007f55caca70f2 in access_with_adjusted_size (addr=addr@entry=8, 
    value=value@entry=0x7f55c8104c28, size=4, access_size_min=<optimized out>, 
    access_size_max=<optimized out>, access=access@entry=
    0x7f55caca7710 <memory_region_write_accessor>, opaque=opaque@entry=
    0x7f55cd690540) at /home/aliguori/git/qemu/memory.c:364
#9  0x00007f55caca8767 in memory_region_iorange_write (
    iorange=<optimized out>, offset=8, width=4, data=32)
    at /home/aliguori/git/qemu/memory.c:439

It very much looks like a double free to me of the device.

The test is doing a simple device_del in the monitor.

Regards,

Anthony Liguori

> ---
> changed from v1:
> Change guard implementation rather than removed it (Paolo review)
>
>  qom/object.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 4b72a64..3d638ff 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -449,7 +449,8 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>      TypeImpl *type = class->type;
>      ObjectClass *ret = NULL;
>  
> -    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
> +    if (type->class->interfaces &&
> +            type_is_ancestor(target_type, type_interface)) {
>          int found = 0;
>          GSList *i;
>  
> -- 
> 1.7.0.4
Anthony Liguori - Feb. 21, 2013, 8:13 p.m.
Anthony Liguori <aliguori@us.ibm.com> writes:

> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> num_interfaces only tells you how many interfaces the concrete child class has
>> (as defined in the TypeInfo). This means if you have a child class which defines
>> no interfaces of its own, but its parent has interfaces you cannot cast to those
>> parent interfaces.
>>
>> Fixed changing the guard to check the class->interfaces list instead (which is
>> a complete flattened list of implemented interfaces).
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> I must admit I don't understand it yet, but this breaks hot unplug.

It was a bad bisect.  It doesn't always happen and I wasn't running
enough tests to catch it.  Am going again now.  Stay tuned.

Regards,

Anthony Liguori

>
> Here is the qemu-test output:
>
> [aliguori@ccnode4 qemu-test]$ QEMU_TEST_SEED=45240 ./qemu-test ~/build/qemu/x86_64-softmmu/qemu-system-x86_64 tests/device-del.sh
> Using RANDOM seed 45240
> Testing nic
> Formatting '.tmp-31849/disk.img', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off 
> /home/aliguori/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel /usr/local/share/qemu-jeos/kernel-x86_64-pc -initrd .tmp-31849/initramfs-31849.img.gz -device isa-debug-exit -append console=ttyS0 seed=45240 -nographic -enable-kvm -device virtio-balloon-pci -pidfile .tmp-31849/pidfile-31849.pid -qmp unix:.tmp-31849/qmpsock-31849.sock,server,nowait
> [    0.000000] Initializing cgroup subsys cpuset
> [    0.000000] Initializing cgroup subsys cpu
> [    0.000000] Linux version 3.4.0 (root@ccnode4) (gcc version 4.6.4 20120830 (prerelease) (GCC) ) #2 SMP Mon Dec 3 19:40:41 CST 2012
> [    0.000000] Command line: console=ttyS0 seed=45240
> [    0.000000] BIOS-provided physical RAM map:
>
> <- snip ->
>
> [    0.865631] Freeing unused kernel memory: 584k freed
> [    0.866320] Write protecting the kernel read-only data: 12288k
> [    0.868626] Freeing unused kernel memory: 640k freed
> [    0.873366] Freeing unused kernel memory: 1724k freed
> Setting guest RANDOM seed to 45240
> *** Running tests ***
> /tests/device-del.sh
> Running test /tests/device-del.sh...
> ** waiting for hotplug **
> ./tests/device-del.sh: line 36: test: !=: unary operator expected
> ** waiting for remove **
> ** waiting for guest to see device **
> ./qemu-test: line 99: 31883 Segmentation fault      (core dumped) "$@"
>
> The backtrace is:
>
> Program terminated with signal 11, Segmentation fault.
> #0  qemu_bh_delete (bh=0x0) at /home/aliguori/git/qemu/async.c:116
> 116	    bh->scheduled = 0;
> Missing separate debuginfos, use: debuginfo-install bzip2-libs-1.0.6-7.fc18.x86_64 glib2-2.34.2-2.fc18.x86_64 glibc-2.16-28.fc18.x86_64 libaio-0.3.109-6.fc18.x86_64 libgcc-4.7.2-8.fc18.x86_64 pixman-0.26.2-5.fc18.x86_64 xen-libs-4.2.1-5.fc18.x86_64 xz-libs-5.1.2-2alpha.fc18.x86_64 zlib-1.2.7-9.fc18.x86_64
> (gdb) bt
> #0  qemu_bh_delete (bh=0x0) at /home/aliguori/git/qemu/async.c:116
> #1  0x00007f55cac9bd95 in virtio_net_exit (vdev=0x7f55cd6eb960)
>     at /home/aliguori/git/qemu/hw/virtio-net.c:1413
> #2  0x00007f55cabde7f9 in virtio_net_exit_pci (pci_dev=0x7f55cd668260)
>     at /home/aliguori/git/qemu/hw/virtio-pci.c:1016
> #3  0x00007f55cab92e4a in pci_unregister_device (dev=<optimized out>)
>     at /home/aliguori/git/qemu/hw/pci/pci.c:889
> #4  0x00007f55caba7485 in device_unparent (obj=<optimized out>)
>     at /home/aliguori/git/qemu/hw/qdev.c:773
> #5  0x00007f55cac1ea0f in object_unparent (obj=0x7f55cd668260)
>     at /home/aliguori/git/qemu/qom/object.c:370
> #6  0x00007f55caba7a4d in qdev_free (dev=<optimized out>)
>     at /home/aliguori/git/qemu/hw/qdev.c:270
> #7  0x00007f55cab38da0 in acpi_piix_eject_slot (s=0x7f55cd68fd50, 
>     slots=<optimized out>) at /home/aliguori/git/qemu/hw/acpi_piix4.c:306
> #8  0x00007f55caca70f2 in access_with_adjusted_size (addr=addr@entry=8, 
>     value=value@entry=0x7f55c8104c28, size=4, access_size_min=<optimized out>, 
>     access_size_max=<optimized out>, access=access@entry=
>     0x7f55caca7710 <memory_region_write_accessor>, opaque=opaque@entry=
>     0x7f55cd690540) at /home/aliguori/git/qemu/memory.c:364
> #9  0x00007f55caca8767 in memory_region_iorange_write (
>     iorange=<optimized out>, offset=8, width=4, data=32)
>     at /home/aliguori/git/qemu/memory.c:439
>
> It very much looks like a double free to me of the device.
>
> The test is doing a simple device_del in the monitor.
>
> Regards,
>
> Anthony Liguori
>
>> ---
>> changed from v1:
>> Change guard implementation rather than removed it (Paolo review)
>>
>>  qom/object.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 4b72a64..3d638ff 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -449,7 +449,8 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>>      TypeImpl *type = class->type;
>>      ObjectClass *ret = NULL;
>>  
>> -    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
>> +    if (type->class->interfaces &&
>> +            type_is_ancestor(target_type, type_interface)) {
>>          int found = 0;
>>          GSList *i;
>>  
>> -- 
>> 1.7.0.4

Patch

diff --git a/qom/object.c b/qom/object.c
index 4b72a64..3d638ff 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -449,7 +449,8 @@  ObjectClass *object_class_dynamic_cast(ObjectClass *class,
     TypeImpl *type = class->type;
     ObjectClass *ret = NULL;
 
-    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
+    if (type->class->interfaces &&
+            type_is_ancestor(target_type, type_interface)) {
         int found = 0;
         GSList *i;