[Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eduardo Habkost]

error_propagate() already ignores local_err==NULL, so there's no
need to check it before calling.

Done using the following Coccinelle patch:

  @@
  identifier L;
  expression E;
  @@
  -if (L) {
       error_propagate(E, L);
  -}

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block.c               | 20 +++++---------------
 block/qcow2.c         |  4 +---
 block/quorum.c        |  4 +---
 block/raw-posix.c     | 16 ++++------------
 block/raw_bsd.c       |  4 +---
 block/snapshot.c      |  4 +---
 blockdev.c            | 12 +++---------
 bootdevice.c          |  4 +---
 dump.c                |  4 +---
 hw/ide/qdev.c         |  4 +---
 hw/net/ne2000-isa.c   |  4 +---
 hw/s390x/virtio-ccw.c | 28 +++++++---------------------
 hw/usb/dev-storage.c  |  4 +---
 qga/commands-win32.c  |  8 ++------
 qom/object.c          |  4 +---
 15 files changed, 31 insertions(+), 93 deletions(-)

diff --git a/block.c b/block.c
index f54bc25..ecca55a 100644
--- a/block.c
+++ b/block.c
@@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
     assert(cco->drv);
 
     ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err);
-    if (local_err) {
-        error_propagate(&cco->err, local_err);
-    }
+    error_propagate(&cco->err, local_err);
     cco->ret = ret;
 }
 
@@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     }
 
     ret = bdrv_create(drv, filename, opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     return ret;
 }
 
@@ -1760,18 +1756,14 @@ fail:
     QDECREF(options);
     bs->options = NULL;
     bdrv_unref(bs);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     return NULL;
 
 close_and_fail:
     bdrv_unref(bs);
     QDECREF(snapshot_options);
     QDECREF(options);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     return NULL;
 }
 
@@ -3591,9 +3583,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
 out:
     qemu_opts_del(opts);
     qemu_opts_free(create_opts);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6f5fb81..4504846 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2394,9 +2394,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
                         &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 
 finish:
     g_free(backing_file);
diff --git a/block/quorum.c b/block/quorum.c
index ec6f3b9..331b726 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -971,9 +971,7 @@ close_exit:
 exit:
     qemu_opts_del(opts);
     /* propagate error */
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ce2e20f..cb663d8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->type = FTYPE_FILE;
     ret = raw_open_common(bs, options, flags, 0, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     return ret;
 }
 
@@ -2239,9 +2237,7 @@ hdev_open_Mac_error:
 
     ret = raw_open_common(bs, options, flags, 0, &local_err);
     if (ret < 0) {
-        if (local_err) {
-            error_propagate(errp, local_err);
-        }
+        error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
         if (*bsd_path) {
             filename = bsd_path;
@@ -2453,9 +2449,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
     ret = raw_open_common(bs, options, flags, O_NONBLOCK, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     return ret;
 }
 
@@ -2573,9 +2567,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = raw_open_common(bs, options, flags, 0, &local_err);
     if (ret) {
-        if (local_err) {
-            error_propagate(errp, local_err);
-        }
+        error_propagate(errp, local_err);
         return ret;
     }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index b1d5237..5af11b6 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -194,9 +194,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
     int ret;
 
     ret = bdrv_create_file(filename, opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     return ret;
 }
 
diff --git a/block/snapshot.c b/block/snapshot.c
index da89d2b..bf5c2ca 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -358,9 +358,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
         ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, &local_err);
     }
 
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 
     return ret;
 }
diff --git a/blockdev.c b/blockdev.c
index 7fd515a..028dba3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3633,9 +3633,7 @@ void qmp_drive_mirror(const char *device, const char *target,
                            has_unmap, unmap,
                            &local_err);
     bdrv_unref(target_bs);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 out:
     aio_context_release(aio_context);
 }
@@ -3689,9 +3687,7 @@ void qmp_blockdev_mirror(const char *device, const char *target,
                            has_on_target_error, on_target_error,
                            true, true,
                            &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 
     aio_context_release(aio_context);
 }
@@ -3901,9 +3897,7 @@ void qmp_change_backing_file(const char *device,
 
     if (ro) {
         bdrv_reopen(image_bs, open_flags, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err); /* will preserve prior errp */
-        }
+        error_propagate(errp, local_err);
     }
 
 out:
diff --git a/bootdevice.c b/bootdevice.c
index bb9c08e..33e3029 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -302,9 +302,7 @@ static void device_set_bootindex(Object *obj, Visitor *v, const char *name,
     add_boot_device_path(*prop->bootindex, prop->dev, prop->suffix);
 
 out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static void property_release_bootindex(Object *obj, const char *name,
diff --git a/dump.c b/dump.c
index 9726f1f..f7b80d8 100644
--- a/dump.c
+++ b/dump.c
@@ -918,9 +918,7 @@ static void write_dump_header(DumpState *s, Error **errp)
     } else {
         create_header64(s, &local_err);
     }
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static size_t dump_bitmap_get_bufsize(DumpState *s)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 4bc74a3..6842a55 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -233,9 +233,7 @@ static void ide_dev_set_bootindex(Object *obj, Visitor *v, const char *name,
                              d->unit ? "/disk@1" : "/disk@0");
     }
 out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static void ide_dev_instance_init(Object *obj)
diff --git a/hw/net/ne2000-isa.c b/hw/net/ne2000-isa.c
index a7f5a94..8fab7ae 100644
--- a/hw/net/ne2000-isa.c
+++ b/hw/net/ne2000-isa.c
@@ -127,9 +127,7 @@ static void isa_ne2000_set_bootindex(Object *obj, Visitor *v,
     s->c.bootindex = boot_index;
 
 out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static void isa_ne2000_instance_init(Object *obj)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 2b68e5e..464b091 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -905,9 +905,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
                                   object_get_typename(OBJECT(qdev)));
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void virtio_ccw_net_instance_init(Object *obj)
@@ -928,9 +926,7 @@ static void virtio_ccw_blk_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void virtio_ccw_blk_instance_init(Object *obj)
@@ -965,9 +961,7 @@ static void virtio_ccw_serial_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 
@@ -987,9 +981,7 @@ static void virtio_ccw_balloon_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void virtio_ccw_balloon_instance_init(Object *obj)
@@ -1025,9 +1017,7 @@ static void virtio_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void virtio_ccw_scsi_instance_init(Object *obj)
@@ -1049,9 +1039,7 @@ static void vhost_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void vhost_ccw_scsi_instance_init(Object *obj)
@@ -1872,9 +1860,7 @@ static void virtio_ccw_9p_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 248a580..9fd00df 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -818,9 +818,7 @@ static void usb_msd_set_bootindex(Object *obj, Visitor *v, const char *name,
     }
 
 out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static const TypeInfo usb_storage_dev_type_info = {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index c1a8588..ea23797 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -247,9 +247,7 @@ out:
     if (token) {
         CloseHandle(token);
     }
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static void execute_async(DWORD WINAPI (*func)(LPVOID), LPVOID opaque,
@@ -882,9 +880,7 @@ static void check_suspend_mode(GuestSuspendMode mode, Error **errp)
     }
 
 out:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static DWORD WINAPI do_suspend(LPVOID opaque)
diff --git a/qom/object.c b/qom/object.c
index 3bc8a00..7ba5917 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -541,9 +541,7 @@ Object *object_new_with_propv(const char *typename,
     return obj;
 
  error:
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
     object_unref(obj);
     return NULL;
 }
-- 
2.5.5

Re: [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 6145 bytes --]

On 06/09/2016 11:40 AM, Eduardo Habkost wrote:
> error_propagate() already ignores local_err==NULL, so there's no
> need to check it before calling.
> 
> Done using the following Coccinelle patch:
> 
>   @@
>   identifier L;
>   expression E;
>   @@
>   -if (L) {
>        error_propagate(E, L);
>   -}
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

> diff --git a/block.c b/block.c
> index f54bc25..ecca55a 100644
> --- a/block.c
> +++ b/block.c
> @@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>      assert(cco->drv);
>  
>      ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err);
> -    if (local_err) {
> -        error_propagate(&cco->err, local_err);
> -    }
> +    error_propagate(&cco->err, local_err);
>      cco->ret = ret;
>  }

This is a case where we can go one step further:

assert(cco->drv);
cco->ret = cco->drv->bdrv_create(cco->filename, cco->opts, &cco->err);

and ditch local_err and ret altogether.

>  
> @@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>      }
>  
>      ret = bdrv_create(drv, filename, opts, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>      return ret;
>  }

Another place where we could ditch local_err, and directly call

return bdrv_create(drv, filename, opts, errp);

Of course, unless you tweak the Coccinelle script to do that cleanup
automatically, or improve the commit message to mention that you did
further cleanups beyond Coccinelle, then it would belong better as a
followup patch, leaving this conversion to be fine to commit as is.

>  
> @@ -1760,18 +1756,14 @@ fail:
>      QDECREF(options);
>      bs->options = NULL;
>      bdrv_unref(bs);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>      return NULL;

This one's fine, along with any others I don't comment on.

> +++ b/block/qcow2.c
> @@ -2394,9 +2394,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>      ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
>                          cluster_size, prealloc, opts, version, refcount_order,
>                          &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);

Another case where we could ditch local_err, OR, we could do:

     prealloc = qapi_enum_parse(..., &local_err);
     if (local_err) {
-        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto finish;
     }
...
     ret = qcow2_create2(..., &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }

 finish:
+    error_propagate(errp, local_err);

> +++ b/block/raw-posix.c
> @@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->type = FTYPE_FILE;
>      ret = raw_open_common(bs, options, flags, 0, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>      return ret;
>  }

Another case where we could ditch local_err.

> @@ -2453,9 +2449,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
>      ret = raw_open_common(bs, options, flags, O_NONBLOCK, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>      return ret;

and another.

>  }
>  
> @@ -2573,9 +2567,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      ret = raw_open_common(bs, options, flags, 0, &local_err);
>      if (ret) {
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -        }
> +        error_propagate(errp, local_err);
>          return ret;
>      }

and another

>  
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index b1d5237..5af11b6 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -194,9 +194,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
>      int ret;
>  
>      ret = bdrv_create_file(filename, opts, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>      return ret;

and another

>  }
>  
> diff --git a/block/snapshot.c b/block/snapshot.c
> index da89d2b..bf5c2ca 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -358,9 +358,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
>          ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, &local_err);
>      }
>  
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>  
>      return ret;

and another

>  }
> diff --git a/blockdev.c b/blockdev.c
> index 7fd515a..028dba3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3633,9 +3633,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>                             has_unmap, unmap,
>                             &local_err);
>      bdrv_unref(target_bs);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>  out:
>      aio_context_release(aio_context);
>  }

and another.

Hmm - it seems like in most of the cases where the ONLY thing done in
the if (local_err) block is to propagate the error, we should instead be
directly assigning to errp instead of wasting a local variable.  At this
point, my review is repetitive enough that I'll stop looking, and leave
it up to you and Markus whether to attempt a more ambitious Coccinelle
script.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
[...]
> 
> Hmm - it seems like in most of the cases where the ONLY thing done in
> the if (local_err) block is to propagate the error, we should instead be
> directly assigning to errp instead of wasting a local variable.  At this
> point, my review is repetitive enough that I'll stop looking, and leave
> it up to you and Markus whether to attempt a more ambitious Coccinelle
> script.

If it happens immediately before the function end or a return
statement it should be easy, but it would still require some
manual work to remove the unused variable declaration. Probably
easier to do that in a follow-up patch.

It's harder (impossible?) to make Coccinelle avoid matching if
local_err is used somewhere else in the function. But it's
probably doable with some manual work, in a follow-up patch.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

On 06/09/2016 01:50 PM, Eduardo Habkost wrote:
> On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
> [...]
>>
>> Hmm - it seems like in most of the cases where the ONLY thing done in
>> the if (local_err) block is to propagate the error, we should instead be
>> directly assigning to errp instead of wasting a local variable.  At this
>> point, my review is repetitive enough that I'll stop looking, and leave
>> it up to you and Markus whether to attempt a more ambitious Coccinelle
>> script.
> 
> If it happens immediately before the function end or a return
> statement it should be easy, but it would still require some
> manual work to remove the unused variable declaration. Probably
> easier to do that in a follow-up patch.

I think Coccinelle can be used to eliminate unused local variables, but
don't know the recipe off-hand; maybe a web search will turn up something?

> 
> It's harder (impossible?) to make Coccinelle avoid matching if
> local_err is used somewhere else in the function. But it's
> probably doable with some manual work, in a follow-up patch.

I don't know - Coccinelle is rather powerful, and there may indeed be a
way to flag conditions for a variable that is not used anywhere except
in the lines mentioned in the recipe, vs. a variable that can also be
used in the ... portion of the recipe.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:11:23PM -0600, Eric Blake wrote:
> On 06/09/2016 01:50 PM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
> > [...]
> >>
> >> Hmm - it seems like in most of the cases where the ONLY thing done in
> >> the if (local_err) block is to propagate the error, we should instead be
> >> directly assigning to errp instead of wasting a local variable.  At this
> >> point, my review is repetitive enough that I'll stop looking, and leave
> >> it up to you and Markus whether to attempt a more ambitious Coccinelle
> >> script.
> > 
> > If it happens immediately before the function end or a return
> > statement it should be easy, but it would still require some
> > manual work to remove the unused variable declaration. Probably
> > easier to do that in a follow-up patch.
> 
> I think Coccinelle can be used to eliminate unused local variables, but
> don't know the recipe off-hand; maybe a web search will turn up something?

I found something called "when constraints", but the
documentation isn't very clear.

There's an example here:
http://coccinelle.lip6.fr/rules/unused.cocci

> 
> > 
> > It's harder (impossible?) to make Coccinelle avoid matching if
> > local_err is used somewhere else in the function. But it's
> > probably doable with some manual work, in a follow-up patch.
> 
> I don't know - Coccinelle is rather powerful, and there may indeed be a
> way to flag conditions for a variable that is not used anywhere except
> in the lines mentioned in the recipe, vs. a variable that can also be
> used in the ... portion of the recipe.

I found a hackish way to do that. Sending a follow-up patch in 1
minute.

-- 
Eduardo

[Qemu-devel] [PATCH] error: Avoid redudant error_propagate() usage

[Eduardo Habkost]

This patch simplifies code that uses a local_err variable just to immediately
use it for an error_propagate() call.

Done using the following Coccinelle patch:

  @@
  expression R;
  expression list ARGS;
  type T;
  identifier F1, F2;
  identifier LOCAL_ERR;
  identifier ERRP;
  idexpression V;
  typedef Error;
  @@
   T F1(..., Error **ERRP)
   {
  (
       // this slice guarantees that the code won't be changed
       // if LOCAL_ERR is used elsewhere in the function
       // (i.e. if it appears 3+ times in the function body)
       ...
       Error *LOCAL_ERR;
       ...
       LOCAL_ERR
       ...
       LOCAL_ERR
       ...
       LOCAL_ERR
       ...
  |
       ...
  -    Error *LOCAL_ERR;
       ...
  (
  -    F2(ARGS, &LOCAL_ERR);
  -    error_propagate(ERRP, LOCAL_ERR);
  +    F2(ARGS, ERRP);
  |
  -    V = F2(ARGS, &LOCAL_ERR);
  -    error_propagate(ERRP, LOCAL_ERR);
  +    V = F2(ARGS, ERRP);
  )
       ...
  )
   }

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
In the end, I found a way to avoid matching cases where local_err
is used elsewhere in the function.
---
 block.c                    |  4 +---
 block/raw-posix.c          |  8 ++------
 block/raw_bsd.c            |  4 +---
 blockdev.c                 | 16 +++++-----------
 hw/s390x/s390-virtio-ccw.c |  5 +----
 hw/s390x/virtio-ccw.c      | 28 +++++++---------------------
 target-i386/cpu.c          |  4 +---
 7 files changed, 18 insertions(+), 51 deletions(-)

diff --git a/block.c b/block.c
index ecca55a..5cc83e5 100644
--- a/block.c
+++ b/block.c
@@ -353,7 +353,6 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
     BlockDriver *drv;
-    Error *local_err = NULL;
     int ret;
 
     drv = bdrv_find_protocol(filename, true, errp);
@@ -361,8 +360,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
         return -ENOENT;
     }
 
-    ret = bdrv_create(drv, filename, opts, &local_err);
-    error_propagate(errp, local_err);
+    ret = bdrv_create(drv, filename, opts, errp);
     return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index cb663d8..d7397bf 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -582,12 +582,10 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     BDRVRawState *s = bs->opaque;
-    Error *local_err = NULL;
     int ret;
 
     s->type = FTYPE_FILE;
-    ret = raw_open_common(bs, options, flags, 0, &local_err);
-    error_propagate(errp, local_err);
+    ret = raw_open_common(bs, options, flags, 0, errp);
     return ret;
 }
 
@@ -2442,14 +2440,12 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
     BDRVRawState *s = bs->opaque;
-    Error *local_err = NULL;
     int ret;
 
     s->type = FTYPE_CD;
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-    ret = raw_open_common(bs, options, flags, O_NONBLOCK, &local_err);
-    error_propagate(errp, local_err);
+    ret = raw_open_common(bs, options, flags, O_NONBLOCK, errp);
     return ret;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5af11b6..b51ac98 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -190,11 +190,9 @@ static int raw_has_zero_init(BlockDriverState *bs)
 
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-    Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_create_file(filename, opts, &local_err);
-    error_propagate(errp, local_err);
+    ret = bdrv_create_file(filename, opts, errp);
     return ret;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 028dba3..3b6d242 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char *target,
     BlockBackend *blk;
     BlockDriverState *target_bs;
     AioContext *aio_context;
-    Error *local_err = NULL;
 
     blk = blk_by_name(device);
     if (!blk) {
@@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const char *target,
 
     bdrv_set_aio_context(target_bs, aio_context);
 
-    blockdev_mirror_common(bs, target_bs,
-                           has_replaces, replaces, sync,
-                           has_speed, speed,
-                           has_granularity, granularity,
-                           has_buf_size, buf_size,
-                           has_on_source_error, on_source_error,
-                           has_on_target_error, on_target_error,
-                           true, true,
-                           &local_err);
-    error_propagate(errp, local_err);
+    blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync,
+                           has_speed, speed, has_granularity, granularity,
+                           has_buf_size, buf_size, has_on_source_error,
+                           on_source_error, has_on_target_error,
+                           on_target_error, true, true, errp);
 
     aio_context_release(aio_context);
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 95ff5e3..b7112d0 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -180,10 +180,7 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
 static void s390_hot_add_cpu(const int64_t id, Error **errp)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
-    Error *err = NULL;
-
-    s390x_new_cpu(machine->cpu_model, id, &err);
-    error_propagate(errp, err);
+    s390x_new_cpu(machine->cpu_model, id, errp);
 }
 
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 464b091..50b0935 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -899,13 +899,11 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     DeviceState *qdev = DEVICE(ccw_dev);
     VirtIONetCcw *dev = VIRTIO_NET_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    Error *err = NULL;
 
     virtio_net_set_netclient_name(&dev->vdev, qdev->id,
                                   object_get_typename(OBJECT(qdev)));
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
-    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    error_propagate(errp, err);
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
 static void virtio_ccw_net_instance_init(Object *obj)
@@ -922,11 +920,9 @@ static void virtio_ccw_blk_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 {
     VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    Error *err = NULL;
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
-    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    error_propagate(errp, err);
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
 static void virtio_ccw_blk_instance_init(Object *obj)
@@ -946,7 +942,6 @@ static void virtio_ccw_serial_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     VirtioSerialCcw *dev = VIRTIO_SERIAL_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
     DeviceState *proxy = DEVICE(ccw_dev);
-    Error *err = NULL;
     char *bus_name;
 
     /*
@@ -960,8 +955,7 @@ static void virtio_ccw_serial_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     }
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
-    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    error_propagate(errp, err);
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
 
@@ -977,11 +971,9 @@ static void virtio_ccw_balloon_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 {
     VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    Error *err = NULL;
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
-    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    error_propagate(errp, err);
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
 static void virtio_ccw_balloon_instance_init(Object *obj)
@@ -1002,7 +994,6 @@ static void virtio_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     VirtIOSCSICcw *dev = VIRTIO_SCSI_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
     DeviceState *qdev = DEVICE(ccw_dev);
-    Error *err = NULL;
     char *bus_name;
 
     /*
@@ -1016,8 +1007,7 @@ static void virtio_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     }
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
-    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    error_propagate(errp, err);
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
 static void virtio_ccw_scsi_instance_init(Object *obj)
@@ -1035,11 +1025,9 @@ static void vhost_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 {
     VHostSCSICcw *dev = VHOST_SCSI_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    Error *err = NULL;
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
-    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    error_propagate(errp, err);
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
 static void vhost_ccw_scsi_instance_init(Object *obj)
@@ -1856,11 +1844,9 @@ static void virtio_ccw_9p_realize(VirtioCcwDevice *ccw_dev, Error **errp)
 {
     V9fsCCWState *dev = VIRTIO_9P_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    Error *err = NULL;
 
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
-    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
-    error_propagate(errp, err);
+    object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
 static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 895a386..2cea40a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1820,7 +1820,6 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
 {
     uint32_t *array = (uint32_t *)opaque;
     FeatureWord w;
-    Error *err = NULL;
     X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { };
     X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { };
     X86CPUFeatureWordInfoList *list = NULL;
@@ -1840,8 +1839,7 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
         list = &list_entries[w];
     }
 
-    visit_type_X86CPUFeatureWordInfoList(v, "feature-words", &list, &err);
-    error_propagate(errp, err);
+    visit_type_X86CPUFeatureWordInfoList(v, "feature-words", &list, errp);
 }
 
 static void x86_get_hv_spinlocks(Object *obj, Visitor *v, const char *name,
-- 
2.5.5

Re: [Qemu-devel] [PATCH] error: Avoid redudant error_propagate() usage

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 05:21:34PM -0300, Eduardo Habkost wrote:
> This patch simplifies code that uses a local_err variable just to immediately
> use it for an error_propagate() call.

Please ignore this one. I found a better way to tell Coccinelle
to do that. Updated Coccinelle patch is below, but I will send
the actual patch tomorrow.

@@
expression list ARGS;
expression F2;
identifier LOCAL_ERR;
expression ERRP;
idexpression V;
typedef Error;
expression I;
@@
 {
     ...
-    Error *LOCAL_ERR;
     ... when != LOCAL_ERR
(
-    F2(ARGS, &LOCAL_ERR);
-    error_propagate(ERRP, LOCAL_ERR);
+    F2(ARGS, ERRP);
|
-    V = F2(ARGS, &LOCAL_ERR);
-    error_propagate(ERRP, LOCAL_ERR);
+    V = F2(ARGS, ERRP);
)
     ... when != LOCAL_ERR
 }

-- 
Eduardo

Re: [Qemu-devel] [PATCH] error: Avoid redudant error_propagate() usage

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On 06/09/2016 02:47 PM, Eduardo Habkost wrote:
> On Thu, Jun 09, 2016 at 05:21:34PM -0300, Eduardo Habkost wrote:
>> This patch simplifies code that uses a local_err variable just to immediately
>> use it for an error_propagate() call.
> 
> Please ignore this one. I found a better way to tell Coccinelle
> to do that. Updated Coccinelle patch is below, but I will send
> the actual patch tomorrow.

Cool.  Worth sticking these in scripts/coccinelle/ as part of your
commit so that we can rerun them later (and refer to them for ideas when
writing new scripts)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: Avoid redudant error_propagate() usage

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

On 06/09/2016 02:21 PM, Eduardo Habkost wrote:
> This patch simplifies code that uses a local_err variable just to immediately
> use it for an error_propagate() call.
> 
> Done using the following Coccinelle patch:
> 

> +++ b/block.c
> @@ -353,7 +353,6 @@ out:
>  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>  {
>      BlockDriver *drv;
> -    Error *local_err = NULL;
>      int ret;
>  
>      drv = bdrv_find_protocol(filename, true, errp);
> @@ -361,8 +360,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>          return -ENOENT;
>      }
>  
> -    ret = bdrv_create(drv, filename, opts, &local_err);
> -    error_propagate(errp, local_err);
> +    ret = bdrv_create(drv, filename, opts, errp);
>      return ret;

And I _know_ there's a Coccinelle recipe for further shortening this
into 'return bdrv_create(...)' (since it was part of the tutorial class
at last year's KVM Forum) - again, I don't know the actual syntax to use
to get it, but it shouldn't be too hard to find in a web search.  Fine
as yet another followup patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: Avoid redudant error_propagate() usage

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:54:51PM -0600, Eric Blake wrote:
> On 06/09/2016 02:47 PM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 05:21:34PM -0300, Eduardo Habkost wrote:
> >> This patch simplifies code that uses a local_err variable just to immediately
> >> use it for an error_propagate() call.
> > 
> > Please ignore this one. I found a better way to tell Coccinelle
> > to do that. Updated Coccinelle patch is below, but I will send
> > the actual patch tomorrow.
> 
> Cool.  Worth sticking these in scripts/coccinelle/ as part of your
> commit so that we can rerun them later (and refer to them for ideas when
> writing new scripts)?

I didn't know about scripts/coccinelle. I will do!

-- 
Eduardo

Re: [Qemu-devel] [PATCH] error: Avoid redudant error_propagate() usage

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:57:08PM -0600, Eric Blake wrote:
> On 06/09/2016 02:21 PM, Eduardo Habkost wrote:
> > This patch simplifies code that uses a local_err variable just to immediately
> > use it for an error_propagate() call.
> > 
> > Done using the following Coccinelle patch:
> > 
> 
> > +++ b/block.c
> > @@ -353,7 +353,6 @@ out:
> >  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
> >  {
> >      BlockDriver *drv;
> > -    Error *local_err = NULL;
> >      int ret;
> >  
> >      drv = bdrv_find_protocol(filename, true, errp);
> > @@ -361,8 +360,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
> >          return -ENOENT;
> >      }
> >  
> > -    ret = bdrv_create(drv, filename, opts, &local_err);
> > -    error_propagate(errp, local_err);
> > +    ret = bdrv_create(drv, filename, opts, errp);
> >      return ret;
> 
> And I _know_ there's a Coccinelle recipe for further shortening this
> into 'return bdrv_create(...)' (since it was part of the tutorial class
> at last year's KVM Forum) - again, I don't know the actual syntax to use
> to get it, but it shouldn't be too hard to find in a web search.  Fine
> as yet another followup patch.

This is an easy one. I will give it a try.

Probably v2 will become a 3-patch series.

-- 
Eduardo