DeviceState.id is a pointer to a string that is stored in the QemuOpts
object DeviceState.opts and freed together with it. We want to create
devices without going through QemuOpts in the future, so make this a
separately allocated string.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/qdev-core.h | 2 +-
include/monitor/qdev.h | 2 +-
hw/arm/virt.c | 2 +-
hw/core/qdev.c | 1 +
hw/pci-bridge/pci_expander_bridge.c | 2 +-
hw/ppc/e500.c | 2 +-
softmmu/qdev-monitor.c | 5 +++--
7 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 34c8a7506a..1857d9698e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -176,7 +176,7 @@ struct DeviceState {
Object parent_obj;
/*< public >*/
- const char *id;
+ char *id;
char *canonical_path;
bool realized;
bool pending_deleted_event;
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..389287eb44 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+void qdev_set_id(DeviceState *dev, char *id);
#endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1d59f0e59f..f933d48d3b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
MemoryRegion *sysmem = get_system_memory();
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
- dev->id = TYPE_PLATFORM_BUS_DEVICE;
+ dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..d918b50a1d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
}
qemu_opts_del(dev->opts);
+ g_free(dev->id);
}
static void device_class_base_init(ObjectClass *class, void *data)
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 7112dc3062..10e6e7c2ab 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
} else {
bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
bds = qdev_new("pci-bridge");
- bds->id = dev_name;
+ bds->id = g_strdup(dev_name);
qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
}
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 95451414dd..960e7efcd3 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
/* Platform Bus Device */
if (pmc->has_platform_bus) {
dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
- dev->id = TYPE_PLATFORM_BUS_DEVICE;
+ dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 034b999401..1207e57a46 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
return bus;
}
-void qdev_set_id(DeviceState *dev, const char *id)
+/* Takes ownership of @id, will be freed when deleting the device */
+void qdev_set_id(DeviceState *dev, char *id)
{
if (id) {
dev->id = id;
@@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
}
}
- qdev_set_id(dev, qemu_opts_id(opts));
+ qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
--
2.31.1
24.09.2021 12:04, Kevin Wolf wrote:
> DeviceState.id is a pointer to a string that is stored in the QemuOpts
> object DeviceState.opts and freed together with it. We want to create
> devices without going through QemuOpts in the future, so make this a
> separately allocated string.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Interesting that in hw/xen/xen-legacy-backend.c g_strdup_printf-allocated id is passed to qdev_set_id prior this patch. So, the patch seems to fix that small leak. Worth to mention?
> ---
> include/hw/qdev-core.h | 2 +-
> include/monitor/qdev.h | 2 +-
> hw/arm/virt.c | 2 +-
> hw/core/qdev.c | 1 +
> hw/pci-bridge/pci_expander_bridge.c | 2 +-
> hw/ppc/e500.c | 2 +-
> softmmu/qdev-monitor.c | 5 +++--
> 7 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 34c8a7506a..1857d9698e 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -176,7 +176,7 @@ struct DeviceState {
> Object parent_obj;
> /*< public >*/
>
> - const char *id;
> + char *id;
> char *canonical_path;
> bool realized;
> bool pending_deleted_event;
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index eaa947d73a..389287eb44 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
>
> int qdev_device_help(QemuOpts *opts);
> DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
> -void qdev_set_id(DeviceState *dev, const char *id);
> +void qdev_set_id(DeviceState *dev, char *id);
>
> #endif
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 1d59f0e59f..f933d48d3b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
> MemoryRegion *sysmem = get_system_memory();
>
> dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> - dev->id = TYPE_PLATFORM_BUS_DEVICE;
> + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
> qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index cefc5eaa0a..d918b50a1d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
> }
>
> qemu_opts_del(dev->opts);
> + g_free(dev->id);
> }
>
> static void device_class_base_init(ObjectClass *class, void *data)
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 7112dc3062..10e6e7c2ab 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
> } else {
> bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
> bds = qdev_new("pci-bridge");
> - bds->id = dev_name;
> + bds->id = g_strdup(dev_name);
> qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
> qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
> }
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 95451414dd..960e7efcd3 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
> /* Platform Bus Device */
> if (pmc->has_platform_bus) {
> dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> - dev->id = TYPE_PLATFORM_BUS_DEVICE;
> + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
> qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 034b999401..1207e57a46 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
> return bus;
> }
>
> -void qdev_set_id(DeviceState *dev, const char *id)
> +/* Takes ownership of @id, will be freed when deleting the device */
> +void qdev_set_id(DeviceState *dev, char *id)
> {
> if (id) {
> dev->id = id;
> @@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> }
> }
>
> - qdev_set_id(dev, qemu_opts_id(opts));
> + qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
>
> /* set properties */
> if (qemu_opt_foreach(opts, set_property, dev, errp)) {
>
In hw/pci/pci_bridge.c
we do
br->bus_name = dev->qdev.id
It seems bad, as we now stole dynamic pointer, that may be freed later.
--
Best regards,
Vladimir
Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.09.2021 12:04, Kevin Wolf wrote:
> > DeviceState.id is a pointer to a string that is stored in the QemuOpts
> > object DeviceState.opts and freed together with it. We want to create
> > devices without going through QemuOpts in the future, so make this a
> > separately allocated string.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>
> Interesting that in hw/xen/xen-legacy-backend.c
> g_strdup_printf-allocated id is passed to qdev_set_id prior this
> patch. So, the patch seems to fix that small leak. Worth to mention?
Ok, I can mention it explicitly.
> > ---
> > include/hw/qdev-core.h | 2 +-
> > include/monitor/qdev.h | 2 +-
> > hw/arm/virt.c | 2 +-
> > hw/core/qdev.c | 1 +
> > hw/pci-bridge/pci_expander_bridge.c | 2 +-
> > hw/ppc/e500.c | 2 +-
> > softmmu/qdev-monitor.c | 5 +++--
> > 7 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 34c8a7506a..1857d9698e 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -176,7 +176,7 @@ struct DeviceState {
> > Object parent_obj;
> > /*< public >*/
> > - const char *id;
> > + char *id;
> > char *canonical_path;
> > bool realized;
> > bool pending_deleted_event;
> > diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> > index eaa947d73a..389287eb44 100644
> > --- a/include/monitor/qdev.h
> > +++ b/include/monitor/qdev.h
> > @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
> > int qdev_device_help(QemuOpts *opts);
> > DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
> > -void qdev_set_id(DeviceState *dev, const char *id);
> > +void qdev_set_id(DeviceState *dev, char *id);
> > #endif
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 1d59f0e59f..f933d48d3b 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
> > MemoryRegion *sysmem = get_system_memory();
> > dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> > - dev->id = TYPE_PLATFORM_BUS_DEVICE;
> > + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> > qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
> > qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index cefc5eaa0a..d918b50a1d 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
> > }
> > qemu_opts_del(dev->opts);
> > + g_free(dev->id);
> > }
> > static void device_class_base_init(ObjectClass *class, void *data)
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> > index 7112dc3062..10e6e7c2ab 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
> > } else {
> > bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
> > bds = qdev_new("pci-bridge");
> > - bds->id = dev_name;
> > + bds->id = g_strdup(dev_name);
> > qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
> > qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
> > }
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > index 95451414dd..960e7efcd3 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
> > /* Platform Bus Device */
> > if (pmc->has_platform_bus) {
> > dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
> > - dev->id = TYPE_PLATFORM_BUS_DEVICE;
> > + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> > qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
> > qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index 034b999401..1207e57a46 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
> > return bus;
> > }
> > -void qdev_set_id(DeviceState *dev, const char *id)
> > +/* Takes ownership of @id, will be freed when deleting the device */
> > +void qdev_set_id(DeviceState *dev, char *id)
> > {
> > if (id) {
> > dev->id = id;
> > @@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> > }
> > }
> > - qdev_set_id(dev, qemu_opts_id(opts));
> > + qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
> > /* set properties */
> > if (qemu_opt_foreach(opts, set_property, dev, errp)) {
> >
>
> In hw/pci/pci_bridge.c
>
> we do
>
> br->bus_name = dev->qdev.id
>
> It seems bad, as we now stole dynamic pointer, that may be freed later.
If it's bad now, it was as bad before because previously it was just a
pointer stolen from the QemuOpts that is freed in the same function as
dev->id after this patch.
I think the code is correct (it's just another field of the same device,
the real bus name is just a copy of it, and the bus can't outlive the
device that owns it anyway), but it might be bad style.
Kevin
24.09.2021 18:10, Kevin Wolf wrote:
> Am 24.09.2021 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 24.09.2021 12:04, Kevin Wolf wrote:
>>> DeviceState.id is a pointer to a string that is stored in the QemuOpts
>>> object DeviceState.opts and freed together with it. We want to create
>>> devices without going through QemuOpts in the future, so make this a
>>> separately allocated string.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>
>> Interesting that in hw/xen/xen-legacy-backend.c
>> g_strdup_printf-allocated id is passed to qdev_set_id prior this
>> patch. So, the patch seems to fix that small leak. Worth to mention?
>
> Ok, I can mention it explicitly.
>
>>> ---
>>> include/hw/qdev-core.h | 2 +-
>>> include/monitor/qdev.h | 2 +-
>>> hw/arm/virt.c | 2 +-
>>> hw/core/qdev.c | 1 +
>>> hw/pci-bridge/pci_expander_bridge.c | 2 +-
>>> hw/ppc/e500.c | 2 +-
>>> softmmu/qdev-monitor.c | 5 +++--
>>> 7 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>> index 34c8a7506a..1857d9698e 100644
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -176,7 +176,7 @@ struct DeviceState {
>>> Object parent_obj;
>>> /*< public >*/
>>> - const char *id;
>>> + char *id;
>>> char *canonical_path;
>>> bool realized;
>>> bool pending_deleted_event;
>>> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
>>> index eaa947d73a..389287eb44 100644
>>> --- a/include/monitor/qdev.h
>>> +++ b/include/monitor/qdev.h
>>> @@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
>>> int qdev_device_help(QemuOpts *opts);
>>> DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
>>> -void qdev_set_id(DeviceState *dev, const char *id);
>>> +void qdev_set_id(DeviceState *dev, char *id);
>>> #endif
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 1d59f0e59f..f933d48d3b 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1459,7 +1459,7 @@ static void create_platform_bus(VirtMachineState *vms)
>>> MemoryRegion *sysmem = get_system_memory();
>>> dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
>>> - dev->id = TYPE_PLATFORM_BUS_DEVICE;
>>> + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
>>> qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
>>> qdev_prop_set_uint32(dev, "mmio_size", vms->memmap[VIRT_PLATFORM_BUS].size);
>>> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index cefc5eaa0a..d918b50a1d 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -956,6 +956,7 @@ static void device_finalize(Object *obj)
>>> }
>>> qemu_opts_del(dev->opts);
>>> + g_free(dev->id);
>>> }
>>> static void device_class_base_init(ObjectClass *class, void *data)
>>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>>> index 7112dc3062..10e6e7c2ab 100644
>>> --- a/hw/pci-bridge/pci_expander_bridge.c
>>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>>> @@ -245,7 +245,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
>>> } else {
>>> bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
>>> bds = qdev_new("pci-bridge");
>>> - bds->id = dev_name;
>>> + bds->id = g_strdup(dev_name);
>>> qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
>>> qdev_prop_set_bit(bds, PCI_BRIDGE_DEV_PROP_SHPC, false);
>>> }
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index 95451414dd..960e7efcd3 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -1006,7 +1006,7 @@ void ppce500_init(MachineState *machine)
>>> /* Platform Bus Device */
>>> if (pmc->has_platform_bus) {
>>> dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
>>> - dev->id = TYPE_PLATFORM_BUS_DEVICE;
>>> + dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
>>> qdev_prop_set_uint32(dev, "num_irqs", pmc->platform_bus_num_irqs);
>>> qdev_prop_set_uint32(dev, "mmio_size", pmc->platform_bus_size);
>>> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>>> index 034b999401..1207e57a46 100644
>>> --- a/softmmu/qdev-monitor.c
>>> +++ b/softmmu/qdev-monitor.c
>>> @@ -592,7 +592,8 @@ static BusState *qbus_find(const char *path, Error **errp)
>>> return bus;
>>> }
>>> -void qdev_set_id(DeviceState *dev, const char *id)
>>> +/* Takes ownership of @id, will be freed when deleting the device */
>>> +void qdev_set_id(DeviceState *dev, char *id)
>>> {
>>> if (id) {
>>> dev->id = id;
>>> @@ -690,7 +691,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>>> }
>>> }
>>> - qdev_set_id(dev, qemu_opts_id(opts));
>>> + qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
>>> /* set properties */
>>> if (qemu_opt_foreach(opts, set_property, dev, errp)) {
>>>
>>
>> In hw/pci/pci_bridge.c
>>
>> we do
>>
>> br->bus_name = dev->qdev.id
>>
>> It seems bad, as we now stole dynamic pointer, that may be freed later.
>
> If it's bad now, it was as bad before because previously it was just a
> pointer stolen from the QemuOpts that is freed in the same function as
> dev->id after this patch.
>
Ah, right, OK.
> I think the code is correct (it's just another field of the same device,
> the real bus name is just a copy of it, and the bus can't outlive the
> device that owns it anyway), but it might be bad style.
>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.