Re: [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent()

[Paolo Bonzini <pbonzini@redhat.com>]

Il 11/09/2013 09:45, Stefan Hajnoczi ha scritto:
> 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.
> 
> 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.
> 
> I opted to keep a qdev wrapper function instead of making all callers
> use object_unparent(OBJECT(qdev)) directly due to the slightly ugly
> cast.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> This patch is based on the "[PATCH 0/6] qdev and blockdev refcount leak fixes"
> series that I sent yesterday.
> 
>  hw/acpi/piix4.c          |  2 +-
>  hw/core/qdev.c           | 10 +++++-----
>  hw/pci/pci-hotplug-old.c |  2 +-
>  hw/pci/pci_bridge.c      |  2 +-
>  hw/pci/pcie.c            |  2 +-
>  hw/pci/shpc.c            |  2 +-
>  hw/s390x/virtio-ccw.c    |  2 +-
>  hw/scsi/scsi-bus.c       |  6 +++---
>  hw/usb/bus.c             |  4 ++--
>  hw/usb/dev-storage.c     |  2 +-
>  hw/usb/host-legacy.c     |  2 +-
>  hw/virtio/virtio-bus.c   |  2 +-
>  hw/xen/xen_platform.c    |  2 +-
>  include/hw/qdev-core.h   |  2 +-
>  qdev-monitor.c           |  2 +-
>  15 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 0b8d1d9..2ec9ada 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -326,7 +326,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
>              if (pc->no_hotplug) {
>                  slot_free = false;
>              } else {
> -                qdev_free(qdev);
> +                qdev_unparent(qdev);
>              }
>          }
>      }
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 533f6dd..ac2352b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -164,7 +164,7 @@ int qdev_init(DeviceState *dev)
>      if (local_err != NULL) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> -        qdev_free(dev);
> +        qdev_unparent(dev);
>          return -1;
>      }
>      return 0;
> @@ -258,7 +258,7 @@ void qbus_reset_all_fn(void *opaque)
>  int qdev_simple_unplug_cb(DeviceState *dev)
>  {
>      /* just zap it */
> -    qdev_free(dev);
> +    qdev_unparent(dev);
>      return 0;
>  }
>  
> @@ -280,8 +280,8 @@ void qdev_init_nofail(DeviceState *dev)
>      }
>  }
>  
> -/* Unlink device from bus and free the structure.  */
> -void qdev_free(DeviceState *dev)
> +/* Unlink device from bus.  dev is freed if refcount reaches zero. */
> +void qdev_unparent(DeviceState *dev)
>  {
>      object_unparent(OBJECT(dev));
>  }
> @@ -458,7 +458,7 @@ static void bus_unparent(Object *obj)
>  
>      while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>          DeviceState *dev = kid->child;
> -        qdev_free(dev);
> +        qdev_unparent(dev);
>      }
>      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..f197f5f 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);
> +            qdev_unparent(&dev->qdev);
>              dev = NULL;
>              break;
>          }
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index e6b22b8..68e0bcb 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -391,7 +391,7 @@ void pci_bridge_exitfn(PCIDevice *pci_dev)
>      pci_bridge_region_cleanup(s, s->windows);
>      memory_region_destroy(&s->address_space_mem);
>      memory_region_destroy(&s->address_space_io);
> -    /* qbus_free() is called automatically by qdev_free() */
> +    /* qbus_free() is called automatically by qdev_unparent() */
>  }
>  
>  /*
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 50af3c1..edd3f40 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -251,7 +251,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
>                                     PCI_EXP_SLTSTA_PDS);
>          pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
>      } else {
> -        qdev_free(&pci_dev->qdev);
> +        qdev_unparent(&pci_dev->qdev);
>          pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
>                                       PCI_EXP_SLTSTA_PDS);
>          pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index eb092fd..5ba2b17 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -254,7 +254,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
>           ++devfn) {
>          PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
>          if (affected_dev) {
> -            qdev_free(&affected_dev->qdev);
> +            qdev_unparent(&affected_dev->qdev);
>          }
>      }
>  }
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index cd67db5..249a0ec 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1239,7 +1239,7 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev)
>  
>      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0);
>  
> -    qdev_free(dev);
> +    qdev_unparent(dev);
>      return 0;
>  }
>  
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 5cd6137..cad3089 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -176,7 +176,7 @@ static int scsi_qdev_init(DeviceState *qdev)
>          d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
>          assert(d);
>          if (d->lun == dev->lun && dev != d) {
> -            qdev_free(&d->qdev);
> +            qdev_unparent(&d->qdev);
>          }
>      }
>  
> @@ -229,13 +229,13 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
>      }
>      if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) {
>          error_setg(errp, "Setting drive property failed");
> -        qdev_free(dev);
> +        qdev_unparent(dev);
>          return NULL;
>      }
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>      if (err != NULL) {
>          error_propagate(errp, err);
> -        qdev_free(dev);
> +        qdev_unparent(dev);
>          return NULL;
>      }
>      return SCSI_DEVICE(dev);
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 82ca6a1..6eb4dd4 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -353,7 +353,7 @@ void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
>  void usb_unregister_port(USBBus *bus, USBPort *port)
>  {
>      if (port->dev)
> -        qdev_free(&port->dev->qdev);
> +        qdev_unparent(&port->dev->qdev);
>      QTAILQ_REMOVE(&bus->free, port, next);
>      bus->nfree--;
>  }
> @@ -501,7 +501,7 @@ int usb_device_delete_addr(int busnr, int addr)
>          return -1;
>      dev = port->dev;
>  
> -    qdev_free(&dev->qdev);
> +    qdev_unparent(&dev->qdev);
>      return 0;
>  }
>  
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 1d81ac2..4b855f2 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -703,7 +703,7 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
>          return NULL;
>      }
>      if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) {
> -        qdev_free(&dev->qdev);
> +        qdev_unparent(&dev->qdev);
>          return NULL;
>      }
>      if (qdev_init(&dev->qdev) < 0)
> diff --git a/hw/usb/host-legacy.c b/hw/usb/host-legacy.c
> index 3a5f705..c6d28d2 100644
> --- a/hw/usb/host-legacy.c
> +++ b/hw/usb/host-legacy.c
> @@ -132,7 +132,7 @@ USBDevice *usb_host_device_open(USBBus *bus, const char *devname)
>      return dev;
>  
>  fail:
> -    qdev_free(&dev->qdev);
> +    qdev_unparent(&dev->qdev);
>      return NULL;
>  }
>  
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 6849a01..a690939 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -77,7 +77,7 @@ void virtio_bus_destroy_device(VirtioBusState *bus)
>              klass->device_unplug(qbus->parent);
>          }
>          qdev = DEVICE(bus->vdev);
> -        qdev_free(qdev);
> +        qdev_unparent(qdev);
>          bus->vdev = NULL;
>      }
>  }
> diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> index 79bf0b3..0cb2489 100644
> --- a/hw/xen/xen_platform.c
> +++ b/hw/xen/xen_platform.c
> @@ -95,7 +95,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
>      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
>              PCI_CLASS_NETWORK_ETHERNET
>              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> -        qdev_free(DEVICE(d));
> +        qdev_unparent(DEVICE(d));
>      }
>  }
>  
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a62f231..923e0d8 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -237,7 +237,7 @@ void qdev_init_nofail(DeviceState *dev);
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
>  void qdev_unplug(DeviceState *dev, Error **errp);
> -void qdev_free(DeviceState *dev);
> +void qdev_unparent(DeviceState *dev);
>  int qdev_simple_unplug_cb(DeviceState *dev);
>  void qdev_machine_creation_done(void);
>  bool qdev_machine_modified(void);
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5657cdc..eeb364f 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -511,7 +511,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          qdev->id = id;
>      }
>      if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
> -        qdev_free(qdev);
> +        qdev_unparent(qdev);
>          object_unref(OBJECT(qdev));
>          return NULL;
>      }
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>