On Fri, Oct 08, 2021 at 03:34:36PM +0200, Kevin Wolf wrote:
> From: Damien Hedde <damien.hedde@xxxxxxxxxxxxx>
>
> 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>
> Signed-off-by: Kevin Wolf <kwolf@xxxxxxxxxx>
> ---
> +++ b/softmmu/qdev-monitor.c
> @@ -593,22 +593,34 @@ 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)
> +const char *qdev_set_id(DeviceState *dev, char *id, Error **errp)
> {
> - if (id) {
> - dev->id = id;
> - }
> + ObjectProperty *prop;
>
> - if (dev->id) {
> - object_property_add_child(qdev_get_peripheral(), dev->id,
> - OBJECT(dev));
> + assert(!dev->id && !dev->realized);
> +
> + /*
> + * object_property_[try_]add_child() below will assert the device
> + * has no parent
> + */
> + if (id) {
> + 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;
id is not freed on this error path...
> + }
> } 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)
> @@ -691,7 +703,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> }
> }
>
> - qdev_set_id(dev, g_strdup(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, g_strdup(qemu_opts_id(opts)), errp)) {
> + goto err_del_dev;
...nor on this, which means the g_strdup() leaks on failure.
> + }
>
> /* set properties */
> if (qemu_opt_foreach(opts, set_property, dev, errp)) {
> --
> 2.31.1
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
