diff mbox

error: Avoid redudant error_propagate() usage

Message ID 20160609202134.GZ18662@thinpad.lan.raisama.net
State New
Headers show

Commit Message

Eduardo Habkost June 9, 2016, 8:21 p.m. UTC 
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(-)

Comments

Eduardo Habkost June 9, 2016, 8:47 p.m. UTC | #1 
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
 }
Eric Blake June 9, 2016, 8:54 p.m. UTC | #2 
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 June 9, 2016, 8:57 p.m. UTC | #3 
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.
Eduardo Habkost June 9, 2016, 9:57 p.m. UTC | #4 
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 Habkost June 9, 2016, 9:59 p.m. UTC | #5 
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.
diff mbox

Patch

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,