Re: [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id

[Alistair Francis <alistair23@xxxxxxxxx>]

On Thu, Sep 23, 2021 at 2:29 AM Damien Hedde <damien.hedde@xxxxxxxxxxxxx> wrote:
>
> qdev_set_id() is mostly used when the user adds a device (using
> -device cli option or device_add qmp command). This commit adds
> an error parameter to handle the case where the given id is
> already taken.
>
> Also document the function and add a return value in order to
> be able to capture success/failure: the function now returns the
> id in case of success, or NULL in case of failure.
>
> The commit modifies the 2 calling places (qdev-monitor and
> xen-legacy-backend) to add the error object parameter.
>
> Note that the id is, right now, guaranteed to be unique because
> all ids came from the "device" QemuOptsList where the id is used
> as key. This addition is a preparation for a future commit which
> will relax the uniqueness.
>
> Signed-off-by: Damien Hedde <damien.hedde@xxxxxxxxxxxxx>

Reviewed-by: Alistair Francis <alistair.francis@xxxxxxx>

Alistair

> ---
>  include/monitor/qdev.h      | 25 +++++++++++++++++++++++-
>  hw/xen/xen-legacy-backend.c |  3 ++-
>  softmmu/qdev-monitor.c      | 38 +++++++++++++++++++++++++++----------
>  3 files changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index eaa947d73a..23c31f5296 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -9,6 +9,29 @@ 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);
> +
> +/**
> + * qdev_set_id: parent the device and set its id if provided.
> + * @dev: device to handle
> + * @id: id to be given to the device, or NULL.
> + *
> + * Returns: the id of the device in case of success; otherwise NULL.
> + *
> + * @dev must be unrealized, unparented and must not have an id.
> + *
> + * If @id is non-NULL, this function tries to setup @dev qom path as
> + * "/peripheral/id". If @id is already taken, it fails. If it succeeds,
> + * the id field of @dev is set to @id (@dev now owns the given @id
> + * parameter).
> + *
> + * If @id is NULL, this function generates a unique name and setups @dev
> + * qom path as "/peripheral-anon/name". This name is not set as the id
> + * of @dev.
> + *
> + * Upon success, it returns the id/name (generated or provided). The
> + * returned string is owned by the corresponding child property and must
> + * not be freed by the caller.
> + */
> +const char *qdev_set_id(DeviceState *dev, const char *id, Error **errp);
>
>  #endif
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index dd8ae1452d..f541cfa0e9 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_fatal);
>      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 25275984bd..0007698ff3 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -578,22 +578,34 @@ static BusState *qbus_find(const char *path, Error 
> **errp)
>      return bus;
>  }
>
> -void qdev_set_id(DeviceState *dev, const char *id)
> +const char *qdev_set_id(DeviceState *dev, const char *id, Error **errp)
>  {
> +    ObjectProperty *prop;
> +
> +    assert(!dev->id && !dev->realized);
> +
> +    /*
> +     * object_property_[try_]add_child() below will assert the device
> +     * has no parent
> +     */
>      if (id) {
> -        dev->id = id;
> -    }
> -
> -    if (dev->id) {
> -        object_property_add_child(qdev_get_peripheral(), dev->id,
> -                                  OBJECT(dev));
> +        prop = object_property_try_add_child(qdev_get_peripheral(), id,
> +                                             OBJECT(dev), NULL);
> +        if (prop) {
> +            dev->id = id;
> +        } else {
> +            error_setg(errp, "Duplicate device ID '%s'", id);
> +            return NULL;
> +        }
>      } 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));
> +        prop = object_property_add_child(qdev_get_peripheral_anon(), name,
> +                                         OBJECT(dev));
>          g_free(name);
>      }
> +
> +    return prop->name;
>  }
>
>  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> @@ -677,7 +689,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>          }
>      }
>
> -    qdev_set_id(dev, qemu_opts_id(opts));
> +    /*
> +     * set dev's parent and register its id.
> +     * If it fails it means the id is already taken.
> +     */
> +    if (!qdev_set_id(dev, qemu_opts_id(opts), errp)) {
> +        goto err_del_dev;
> +    }
>
>      /* set properties */
>      if (qemu_opt_foreach(opts, set_property, dev, errp)) {
> --
> 2.33.0
>
>