qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Damien Hedde
Subject: Re: [PATCH v2 09/15] softmmu/qdev-monitor: add error handling in qdev_set_id
Date: Wed, 13 Oct 2021 15:10:38 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.2



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

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







reply via email to

[Prev in Thread] Current Thread [Next in Thread]