Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id

[Eric Blake]

On Fri, Oct 08, 2021 at 03:34:36PM +0200, Kevin Wolf wrote:
> From: Damien Hedde <damien.hedde@greensocs.com>
> 
> 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@greensocs.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ 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