10:14 a.m.
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(a)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(a)virtuozzo.com>
--
Best regards,
Vladimir