* [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent()
@ 2013-09-11 7:45 Stefan Hajnoczi
2013-09-11 7:48 ` Paolo Bonzini
2013-09-11 8:23 ` Andreas Färber
0 siblings, 2 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2013-09-11 7:45 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Stefan Hajnoczi, Andreas Faerber, Anthony Liguori
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;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent()
2013-09-11 7:45 [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent() Stefan Hajnoczi
@ 2013-09-11 7:48 ` Paolo Bonzini
2013-09-11 8:23 ` Andreas Färber
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2013-09-11 7:48 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Anthony Liguori, Andreas Faerber
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent()
2013-09-11 7:45 [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent() Stefan Hajnoczi
2013-09-11 7:48 ` Paolo Bonzini
@ 2013-09-11 8:23 ` Andreas Färber
2013-09-11 11:52 ` Stefan Hajnoczi
1 sibling, 1 reply; 4+ messages in thread
From: Andreas Färber @ 2013-09-11 8:23 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Paolo Bonzini, qemu-devel, Anthony Liguori
Am 11.09.2013 09:45, schrieb Stefan Hajnoczi:
> 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);
This should be DEVICE(dev), making most callers outside qdev.c do a cast
after all.
Also we should name the function device_unparent if we decide to keep it
around.
Andreas
> 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;
> }
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent()
2013-09-11 8:23 ` Andreas Färber
@ 2013-09-11 11:52 ` Stefan Hajnoczi
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2013-09-11 11:52 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Paolo Bonzini, qemu-devel, Stefan Hajnoczi
On Wed, Sep 11, 2013 at 10:23 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 11.09.2013 09:45, schrieb Stefan Hajnoczi:
>> 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);
>
> This should be DEVICE(dev), making most callers outside qdev.c do a cast
> after all.
>
> Also we should name the function device_unparent if we decide to keep it
> around.
This is true. We should probably just use object_unref(OBJECT(foo)) everywhere.
I'll drop qdev_free() completely and resend.
Stefan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-11 11:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-11 7:45 [Qemu-devel] [PATCH] qdev: rename qdev_free() to qdev_unparent() Stefan Hajnoczi
2013-09-11 7:48 ` Paolo Bonzini
2013-09-11 8:23 ` Andreas Färber
2013-09-11 11:52 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.