Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
On 10/11/21 23:00, Eric Blake wrote:
On Fri, Oct 08, 2021 at 03:34:36PM +0200, Kevin Wolf wrote:
> From: Damien Hedde <damien.hedde(a)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(a)greensocs.com>
> Signed-off-by: Kevin Wolf <kwolf(a)redhat.com>
> ---
> + } else {
> + error_setg(errp, "Duplicate device ID '%s'", id);
> + return NULL;
id is not freed on this error path...
>
> 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.
Since we strdup the id just before calling qdev_set_id.
Maybe we should do the the strdup in qdev_set_id (and free things on
error there too). It seems simplier than freeing things on the callee
side just in case of an error.
Damien