qemu-block
[Top][All Lists]
Advanced

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

Re: [PATCH 07/46] error: Avoid more error_propagate() when error is not


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 07/46] error: Avoid more error_propagate() when error is not used here
Date: Fri, 26 Jun 2020 20:21:24 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0

24.06.2020 19:43, Markus Armbruster wrote:
When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away.  The previous commit did that for simple cases with
Coccinelle.  Do it for a few more manually.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
  blockdev.c     |  5 +----
  hw/core/numa.c | 44 ++++++++++++++------------------------------
  qdev-monitor.c | 11 ++++-------
  3 files changed, 19 insertions(+), 41 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b66863c42a..73736a4eaf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1009,13 +1009,10 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type,
      }
/* Actual block device init: Functionality shared with blockdev-add */
-    blk = blockdev_init(filename, bs_opts, &local_err);
+    blk = blockdev_init(filename, bs_opts, errp);
      bs_opts = NULL;
      if (!blk) {
-        error_propagate(errp, local_err);
          goto fail;
-    } else {
-        assert(!local_err);
      }
/* Create legacy DriveInfo */
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 5f81900f88..aa8c6be210 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -449,40 +449,33 @@ void parse_numa_hmat_cache(MachineState *ms, 
NumaHmatCacheOptions *node,
void set_numa_options(MachineState *ms, NumaOptions *object, Error **errp)
  {
-    Error *err = NULL;
-
      if (!ms->numa_state) {
          error_setg(errp, "NUMA is not supported by this machine-type");
-        goto end;
+        return;
      }
switch (object->type) {
      case NUMA_OPTIONS_TYPE_NODE:
-        parse_numa_node(ms, &object->u.node, &err);
-        if (err) {
-            goto end;
-        }
+        parse_numa_node(ms, &object->u.node, errp);
          break;

Could we use return here and and for other "break" operators here, to stress, 
that we
are not going to do something more in case of failure (as well as in case of
success)? To prevent the future addition of some code after the switch without
handling the error carefully here.

      case NUMA_OPTIONS_TYPE_DIST:
-        parse_numa_distance(ms, &object->u.dist, &err);
-        if (err) {
-            goto end;
-        }
+        parse_numa_distance(ms, &object->u.dist, errp);
          break;
      case NUMA_OPTIONS_TYPE_CPU:
          if (!object->u.cpu.has_node_id) {
-            error_setg(&err, "Missing mandatory node-id property");
-            goto end;
+            error_setg(errp, "Missing mandatory node-id property");
+            return;
          }
          if (!ms->numa_state->nodes[object->u.cpu.node_id].present) {
-            error_setg(&err, "Invalid node-id=%" PRId64 ", NUMA node must be "
-                "defined with -numa node,nodeid=ID before it's used with "
-                "-numa cpu,node-id=ID", object->u.cpu.node_id);
-            goto end;
+            error_setg(errp, "Invalid node-id=%" PRId64 ", NUMA node must be "
+                       "defined with -numa node,nodeid=ID before it's used with 
"
+                       "-numa cpu,node-id=ID", object->u.cpu.node_id);
+            return;
          }
- machine_set_cpu_numa_node(ms, qapi_NumaCpuOptions_base(&object->u.cpu),
-                                  &err);
+        machine_set_cpu_numa_node(ms,
+                                  qapi_NumaCpuOptions_base(&object->u.cpu),
+                                  errp);
          break;
      case NUMA_OPTIONS_TYPE_HMAT_LB:
          if (!ms->numa_state->hmat_enabled) {
@@ -492,10 +485,7 @@ void set_numa_options(MachineState *ms, NumaOptions 
*object, Error **errp)
              return;
          }
- parse_numa_hmat_lb(ms->numa_state, &object->u.hmat_lb, &err);
-        if (err) {
-            goto end;
-        }
+        parse_numa_hmat_lb(ms->numa_state, &object->u.hmat_lb, errp);
          break;
      case NUMA_OPTIONS_TYPE_HMAT_CACHE:
          if (!ms->numa_state->hmat_enabled) {
@@ -505,17 +495,11 @@ void set_numa_options(MachineState *ms, NumaOptions 
*object, Error **errp)
              return;
          }
- parse_numa_hmat_cache(ms, &object->u.hmat_cache, &err);
-        if (err) {
-            goto end;
-        }
+        parse_numa_hmat_cache(ms, &object->u.hmat_cache, errp);
          break;
      default:
          abort();
      }
-
-end:
-    error_propagate(errp, err);
  }
static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e38030429b..40c34bb9cf 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -600,7 +600,6 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
      const char *driver, *path;
      DeviceState *dev = NULL;
      BusState *bus = NULL;
-    Error *err = NULL;
      bool hide;
driver = qemu_opt_get(opts, "driver");
@@ -655,15 +654,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
      dev = qdev_new(driver);
/* Check whether the hotplug is allowed by the machine */
-    if (qdev_hotplug && !qdev_hotplug_allowed(dev, &err)) {
+    if (qdev_hotplug && !qdev_hotplug_allowed(dev, errp)) {
          /* Error must be set in the machine hook */
-        assert(err);
          goto err_del_dev;
      }
if (!bus && qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
          /* No bus, no machine hotplug handler --> device is not hotpluggable 
*/
-        error_setg(&err, "Device '%s' can not be hotplugged on this machine",
+        error_setg(errp, "Device '%s' can not be hotplugged on this machine",
                     driver);
          goto err_del_dev;
      }
@@ -671,19 +669,18 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
      qdev_set_id(dev, qemu_opts_id(opts));
/* set properties */
-    if (qemu_opt_foreach(opts, set_property, dev, &err)) {
+    if (qemu_opt_foreach(opts, set_property, dev, errp)) {

Here is an example, what I was afraid, when we discussed introducing a lot more
bool functions (true is success).

Here are two functions with different semantics, and it looks a bit weird,
one if (func()) and one if (!func()). Still "goto err" makes it obvious
that it's all about error checking.

I don't remember, did we considered a convention to avoid if (func()) to check
errors, and use instead if (func() < 0) for such case? So here, update it to be

if (qemu_opt_foreach(opts, set_property, dev, errp) < 0)


(I don't insist to do it exactly in this patch, as its aim is another, I just
 want to remind about this problem)

          goto err_del_dev;
      }
dev->opts = opts;
-    if (!qdev_realize(DEVICE(dev), bus, &err)) {
+    if (!qdev_realize(DEVICE(dev), bus, errp)) {
          dev->opts = NULL;
          goto err_del_dev;
      }
      return dev;
err_del_dev:
-    error_propagate(errp, err);
      if (dev) {
          object_unparent(OBJECT(dev));
          object_unref(OBJECT(dev));



Also, suggest a squash-in, I've noted during previous patch review:

diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index dbe5a8aae6..3cdc406b0d 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -184,15 +184,13 @@ static void cryptodev_vhost_user_init(
 {
     int queues = backend->conf.peers.queues;
     size_t i;
-    Error *local_err = NULL;
     Chardev *chr;
     CryptoDevBackendClient *cc;
     CryptoDevBackendVhostUser *s =
                       CRYPTODEV_BACKEND_VHOST_USER(backend);
- chr = cryptodev_vhost_claim_chardev(s, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    chr = cryptodev_vhost_claim_chardev(s, errp);
+    if (!chr) {
         return;
     }


--
Best regards,
Vladimir



reply via email to

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