diff mbox series

[v2,37/44] error: Reduce unnecessary error propagation

Message ID 20200702155000.3455325-38-armbru@redhat.com (mailing list archive)
State New, archived
Headers show
Series Less clumsy error checking | expand

Commit Message

Markus Armbruster July 2, 2020, 3:49 p.m. UTC 
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, even when we need to keep error_propagate() for other
error paths.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c             | 2 +-
 block/gluster.c     | 8 ++++----
 block/parallels.c   | 2 +-
 block/quorum.c      | 2 +-
 block/replication.c | 3 +--
 block/vxhs.c        | 4 ++--
 hw/core/qdev.c      | 2 +-
 hw/net/virtio-net.c | 4 ++--
 8 files changed, 13 insertions(+), 14 deletions(-)

Comments

Eric Blake July 2, 2020, 6:27 p.m. UTC | #1 
On 7/2/20 10:49 AM, 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, even when we need to keep error_propagate() for other
> error paths.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/block/replication.c
> @@ -85,7 +85,6 @@ static int replication_open(BlockDriverState *bs, QDict *options,
>   {
>       int ret;
>       BDRVReplicationState *s = bs->opaque;
> -    Error *local_err = NULL;
>       QemuOpts *opts = NULL;
>       const char *mode;
>       const char *top_id;
> @@ -99,7 +98,7 @@ static int replication_open(BlockDriverState *bs, QDict *options,
>   
>       ret = -EINVAL;
>       opts = qemu_opts_create(&replication_runtime_opts, NULL, 0, &error_abort);
> -    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
> +    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
>           goto fail;
>       }

Does this one belong in 36/44, given that removal of 'local_err' is 
evidence that no other error path needed it?

Either way, it belongs in the series, and the result of the two patches 
together is fine.

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster July 3, 2020, 7:09 a.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 7/2/20 10:49 AM, 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, even when we need to keep error_propagate() for other
>> error paths.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/block/replication.c
>> @@ -85,7 +85,6 @@ static int replication_open(BlockDriverState *bs, QDict *options,
>>   {
>>       int ret;
>>       BDRVReplicationState *s = bs->opaque;
>> -    Error *local_err = NULL;
>>       QemuOpts *opts = NULL;
>>       const char *mode;
>>       const char *top_id;
>> @@ -99,7 +98,7 @@ static int replication_open(BlockDriverState *bs, QDict *options,
>>         ret = -EINVAL;
>>       opts = qemu_opts_create(&replication_runtime_opts, NULL, 0, &error_abort);
>> -    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
>> +    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
>>           goto fail;
>>       }
>
> Does this one belong in 36/44, given that removal of 'local_err' is
> evidence that no other error path needed it?
>
> Either way, it belongs in the series, and the result of the two
> patches together is fine.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Actually, this hunk needs to go before PATCH 33 to keep it correct.
I'll find out how to best reshuffle hunks.  The end result will be the
same.

Thanks!
diff mbox series

Patch

diff --git a/block.c b/block.c
index 60d2945c2c..2dcf9afd61 100644
--- a/block.c
+++ b/block.c
@@ -6073,7 +6073,7 @@  void bdrv_img_create(const char *filename, const char *fmt,
 
     /* Parse -o options */
     if (options) {
-        if (!qemu_opts_do_parse(opts, options, NULL, &local_err)) {
+        if (!qemu_opts_do_parse(opts, options, NULL, errp)) {
             goto out;
         }
     }
diff --git a/block/gluster.c b/block/gluster.c
index c620880f27..4f1448e2bc 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -523,7 +523,7 @@  static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
     /* create opts info from runtime_json_opts list */
     opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
-    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         goto out;
     }
 
@@ -554,7 +554,7 @@  static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         /* create opts info from runtime_type_opts list */
         opts = qemu_opts_create(&runtime_type_opts, NULL, 0, &error_abort);
-        if (!qemu_opts_absorb_qdict(opts, backing_options, &local_err)) {
+        if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) {
             goto out;
         }
 
@@ -584,7 +584,7 @@  static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         if (gsconf->type == SOCKET_ADDRESS_TYPE_INET) {
             /* create opts info from runtime_inet_opts list */
             opts = qemu_opts_create(&runtime_inet_opts, NULL, 0, &error_abort);
-            if (!qemu_opts_absorb_qdict(opts, backing_options, &local_err)) {
+            if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) {
                 goto out;
             }
 
@@ -632,7 +632,7 @@  static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
         } else {
             /* create opts info from runtime_unix_opts list */
             opts = qemu_opts_create(&runtime_unix_opts, NULL, 0, &error_abort);
-            if (!qemu_opts_absorb_qdict(opts, backing_options, &local_err)) {
+            if (!qemu_opts_absorb_qdict(opts, backing_options, errp)) {
                 goto out;
             }
 
diff --git a/block/parallels.c b/block/parallels.c
index 324085ccea..c259799111 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -827,7 +827,7 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail_options;
     }
 
-    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         goto fail_options;
     }
 
diff --git a/block/quorum.c b/block/quorum.c
index 5d52e605db..6df9449fc2 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -921,7 +921,7 @@  static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     opts = qemu_opts_create(&quorum_runtime_opts, NULL, 0, &error_abort);
-    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         ret = -EINVAL;
         goto exit;
     }
diff --git a/block/replication.c b/block/replication.c
index dcd430624e..0c70215784 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -85,7 +85,6 @@  static int replication_open(BlockDriverState *bs, QDict *options,
 {
     int ret;
     BDRVReplicationState *s = bs->opaque;
-    Error *local_err = NULL;
     QemuOpts *opts = NULL;
     const char *mode;
     const char *top_id;
@@ -99,7 +98,7 @@  static int replication_open(BlockDriverState *bs, QDict *options,
 
     ret = -EINVAL;
     opts = qemu_opts_create(&replication_runtime_opts, NULL, 0, &error_abort);
-    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         goto fail;
     }
 
diff --git a/block/vxhs.c b/block/vxhs.c
index fecaeb82c9..dc0e254730 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -318,7 +318,7 @@  static int vxhs_open(BlockDriverState *bs, QDict *options,
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort);
 
-    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         ret = -EINVAL;
         goto out;
     }
@@ -345,7 +345,7 @@  static int vxhs_open(BlockDriverState *bs, QDict *options,
     /* get the 'server.' arguments */
     qdict_extract_subqdict(options, &backing_options, VXHS_OPT_SERVER".");
 
-    if (!qemu_opts_absorb_qdict(tcp_opts, backing_options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(tcp_opts, backing_options, errp)) {
         ret = -EINVAL;
         goto out;
     }
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 17bd8fc2ec..62b45af8e9 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -869,7 +869,7 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
     }
 
     if (value && !dev->realized) {
-        if (!check_only_migratable(obj, &local_err)) {
+        if (!check_only_migratable(obj, errp)) {
             goto fail;
         }
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 04b012e487..7502ff0b1e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3119,8 +3119,8 @@  static bool failover_replug_primary(VirtIONet *n, Error **errp)
     qdev_set_parent_bus(n->primary_dev, n->primary_bus);
     n->primary_should_be_hidden = false;
     if (!qemu_opt_set_bool(n->primary_device_opts,
-                           "partially_hotplugged", true, &err)) {
-        goto out;
+                           "partially_hotplugged", true, errp)) {
+        return false;
     }
     hotplug_ctrl = qdev_get_hotplug_handler(n->primary_dev);
     if (hotplug_ctrl) {