Re: [PATCH 05/11] qdev: Make DeviceState.id independent of QemuOpts

[Vladimir Sementsov-Ogievskiy]

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