Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()

[Vladimir Sementsov-Ogievskiy]

24.09.2021 12:04, Kevin Wolf wrote:
> object_property_add_child() fails (with &error_abort) if an object with
> the same name already exists. As long as QemuOpts is in use for -device
> and device_add, it catches duplicate IDs before qdev_set_id() is even
> called. However, for enabling non-QemuOpts code paths, we need to make
> sure that the condition doesn't cause a crash, but fails gracefully.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/monitor/qdev.h      |  2 +-
>   hw/xen/xen-legacy-backend.c |  3 ++-
>   softmmu/qdev-monitor.c      | 16 ++++++++++------
>   3 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 389287eb44..7961308c75 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, char *id);
> +void qdev_set_id(DeviceState *dev, char *id, Error **errp);
>   
>   #endif
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index dd8ae1452d..17aca85ddc 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char 
> *type, int dom,
>       xendev = g_malloc0(ops->size);
>       object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
>       OBJECT(xendev)->free = g_free;
> -    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
> +    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
> +                &error_abort);
>       qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
>       object_unref(OBJECT(xendev));
>   
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 1207e57a46..c2af906df0 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path, Error **errp)
>   }
>   
>   /* Takes ownership of @id, will be freed when deleting the device */
> -void qdev_set_id(DeviceState *dev, char *id)
> +void qdev_set_id(DeviceState *dev, char *id, Error **errp)

According to recommendations in error.h, worth adding also return value (for 
example true=success false=failure), so [..]

>   {
>       if (id) {
>           dev->id = id;
>       }

Unrelated but.. What's the strange logic?

Is it intended that with passed id=NULL we don't update dev->id variable but try 
to do following logic with old dev->id?

>   
>       if (dev->id) {
> -        object_property_add_child(qdev_get_peripheral(), dev->id,
> -                                  OBJECT(dev));
> +        object_property_try_add_child(qdev_get_peripheral(), dev->id,
> +                                      OBJECT(dev), errp);
>       } else {
>           static int anon_count;
>           gchar *name = g_strdup_printf("device[%d]", anon_count++);
> -        object_property_add_child(qdev_get_peripheral_anon(), name,
> -                                  OBJECT(dev));
> +        object_property_try_add_child(qdev_get_peripheral_anon(), name,
> +                                      OBJECT(dev), errp);
>           g_free(name);
>       }
>   }
>   
>   DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>   {
> +    ERRP_GUARD();
>       DeviceClass *dc;
>       const char *driver, *path;
>       DeviceState *dev = NULL;
> @@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>           }
>       }
>   
> -    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
> +    qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
> +    if (*errp) {
> +        goto err_del_dev;
> +    }

[..] here we'll have

if (!qdev_set_id(...)) {
  goto err_del_dev;
}

and no need for ERRP_GUARD.

>   
>       /* set properties */
>       if (qemu_opt_foreach(opts, set_property, dev, errp)) {


--
Best regards,
Vladimir