Re: [Qemu-devel] [PATCH v3 7/7] qdev: drop misleading qdev_free() function

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> The qdev_free() function name is misleading since all the function does
> is unlink the device from its parent.  The device is not necessarily
> freed.

Aha - you anticipated my comment on 2/7 :)

> 
> The device will be freed when its QObject refcount reaches zero.  It is
> usual for the parent (bus) to hold the final reference but there are
> cases where something else holds a reference so "free" is a misleading
> name.
> 
> Call object_unparent(obj) directly instead of having a qdev wrapper
> function.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

>      while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>          DeviceState *dev = kid->child;
> -        qdev_free(dev);
> +        object_unparent(OBJECT(dev));

In most cases, you are just expanding the old call inline...

>      }
>      if (bus->parent) {
>          QLIST_REMOVE(bus, sibling);
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index 619fe47..8dbc3c1 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -248,7 +248,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
>          }
>          dev = pci_create(bus, devfn, "virtio-blk-pci");
>          if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) {
> -            qdev_free(&dev->qdev);
> +            object_unparent(OBJECT(dev));

...but in this case, you are unparenting OBJECT(dev) in the new code
while the old code unparented OBJECT(&dev->qdev).  Ah, I see - qdev is
the first member of dev, so it is the same pointer address.

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: 621 bytes --]