Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
Am 13.10.2021 um 23:37 hat Eric Blake geschrieben:
> On Wed, Oct 13, 2021 at 03:10:38PM +0200, Damien Hedde wrote:
> > > > @@ -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.
>
> Indeed. If we expected qdev_set_id() to be passed something that it
> can later free, we would have used 'char *'; but because we used
> 'const char *' for that parameter, it really does make more sense for
> the callers to pass in any string and for qdev_set_id() to do the
> necessary strdup()ing, as well as clean up on error.
Since this seems to be the only thing in the series that needs to be
addressed, I'll do the minimal fix while applying (adding g_free() to
the error path in qemu_opts_id()) and then we can clean up on top.
Kevin
Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
On Wed, Oct 13, 2021 at 03:10:38PM +0200, Damien Hedde wrote:
> > > @@ -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.
Indeed. If we expected qdev_set_id() to be passed something that it
can later free, we would have used 'char *'; but because we used
'const char *' for that parameter, it really does make more sense for
the callers to pass in any string and for qdev_set_id() to do the
necessary strdup()ing, as well as clean up on error.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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
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
Signed-off-by: Kevin Wolf
---
+} 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
Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
On Fri, Oct 08, 2021 at 03:34:36PM +0200, Kevin Wolf wrote:
> From: Damien Hedde
>
> 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
> Signed-off-by: Kevin Wolf
> ---
> +++ 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
[PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
From: Damien Hedde
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
Signed-off-by: Kevin Wolf
---
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 389287eb44..74e6c55a2b 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, 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, char *id, Error **errp);
#endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index be3cf4a195..085fd31ef7 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(>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),
+_fatal);
qdev_realize(DEVICE(xendev), xen_sysbus, _fatal);
object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..feb15818e6 100644
--- a/softmmu/qdev-monitor.c
+++ 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;
+}
} 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;
+}
