[Qemu-devel] [PATCH v2 0/3] coccinelle: Clean up error checks and return value variables

[Qemu-devel] [PATCH v2 0/3] coccinelle: Clean up error checks and return value variables

[Eduardo Habkost]

v2 of the previous "error: Remove NULL checks on
error_propagate() calls" patch, now it became a series.

Changes v1 -> v2:
* The Coccinelle scripts were simplified by using "when"
  constraints to detect when a variable is not used elsewhere
  inside the function.
* Added script to remove unnecessary variables for function
  return value.
* Coccinelle scripts added to scripts/coccinelle.

Eduardo Habkost (3):
  error: Remove NULL checks on error_propagate() calls
  error: Remove unnecessary local_err variables
  [RFC] Remove unnecessary variables for function return value

 audio/audio.c                                 | 10 ++-----
 block.c                                       | 26 ++++-------------
 block/archipelago.c                           |  4 +--
 block/qcow2-cluster.c                         |  7 ++---
 block/qcow2-refcount.c                        |  7 ++---
 block/qcow2.c                                 |  4 +--
 block/quorum.c                                |  4 +--
 block/raw-posix.c                             | 24 +++------------
 block/raw_bsd.c                               |  9 +-----
 block/rbd.c                                   |  5 +---
 block/snapshot.c                              |  4 +--
 block/vmdk.c                                  |  6 ++--
 block/vvfat.c                                 |  5 +---
 blockdev.c                                    | 26 +++++------------
 bootdevice.c                                  |  4 +--
 dump.c                                        |  4 +--
 hw/acpi/aml-build.c                           | 13 ++-------
 hw/audio/intel-hda.c                          |  5 +---
 hw/display/vga.c                              |  4 +--
 hw/ide/qdev.c                                 |  4 +--
 hw/intc/s390_flic_kvm.c                       |  5 ++--
 hw/net/ne2000-isa.c                           |  4 +--
 hw/pci-host/uninorth.c                        |  5 +---
 hw/ppc/spapr_vio.c                            |  7 +----
 hw/s390x/s390-virtio-ccw.c                    |  5 +---
 hw/s390x/virtio-ccw.c                         | 42 +++++----------------------
 hw/scsi/megasas.c                             | 10 +------
 hw/scsi/scsi-generic.c                        |  5 +---
 hw/timer/mc146818rtc.c                        |  5 +---
 hw/usb/dev-storage.c                          |  4 +--
 hw/virtio/virtio-pci.c                        |  4 +--
 linux-user/signal.c                           | 15 +++-------
 page_cache.c                                  |  5 +---
 qga/commands-posix.c                          |  4 +--
 qga/commands-win32.c                          | 14 ++-------
 qobject/qlist.c                               |  5 +---
 qom/object.c                                  |  4 +--
 scripts/coccinelle/error_propagate_null.cocci | 10 +++++++
 scripts/coccinelle/remove_local_err.cocci     | 27 +++++++++++++++++
 scripts/coccinelle/return_directly.cocci      | 19 ++++++++++++
 target-i386/cpu.c                             |  4 +--
 target-i386/fpu_helper.c                      | 10 ++-----
 target-i386/kvm.c                             |  5 ++--
 target-mips/dsp_helper.c                      | 15 ++--------
 target-mips/op_helper.c                       |  4 +--
 target-s390x/helper.c                         |  6 +---
 target-sparc/cc_helper.c                      | 25 ++++------------
 target-tricore/op_helper.c                    | 13 +++------
 tests/display-vga-test.c                      |  6 +---
 tests/endianness-test.c                       |  5 +---
 tests/i440fx-test.c                           |  4 +--
 tests/intel-hda-test.c                        |  6 +---
 tests/test-filter-redirector.c                |  6 +---
 tests/virtio-blk-test.c                       |  5 +---
 tests/virtio-console-test.c                   |  6 +---
 tests/virtio-net-test.c                       |  6 +---
 tests/virtio-scsi-test.c                      |  6 +---
 tests/wdt_ib700-test.c                        |  6 +---
 ui/cursor.c                                   | 10 ++-----
 ui/qemu-pixman.c                              | 11 ++-----
 util/module.c                                 |  6 +---
 vl.c                                          |  5 +---
 62 files changed, 160 insertions(+), 384 deletions(-)
 create mode 100644 scripts/coccinelle/error_propagate_null.cocci
 create mode 100644 scripts/coccinelle/remove_local_err.cocci
 create mode 100644 scripts/coccinelle/return_directly.cocci

-- 
2.5.5

[Qemu-devel] [PATCH v2 1/3] 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.

Coccinelle patch used to perform the changes added to
scripts/coccinelle/error_propagate_null.cocci.

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 +---
 scripts/coccinelle/error_propagate_null.cocci | 10 ++++++++++
 16 files changed, 41 insertions(+), 93 deletions(-)
 create mode 100644 scripts/coccinelle/error_propagate_null.cocci

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;
 }
diff --git a/scripts/coccinelle/error_propagate_null.cocci b/scripts/coccinelle/error_propagate_null.cocci
new file mode 100644
index 0000000..c236380
--- /dev/null
+++ b/scripts/coccinelle/error_propagate_null.cocci
@@ -0,0 +1,10 @@
+// error_propagate() already ignores local_err==NULL, so there's
+// no need to check it before calling.
+
+@@
+identifier L;
+expression E;
+@@
+-if (L) {
+     error_propagate(E, L);
+-}
-- 
2.5.5

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

[Eric Blake]

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

On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> error_propagate() already ignores local_err==NULL, so there's no
> need to check it before calling.
> 
> Coccinelle patch used to perform the changes added to
> scripts/coccinelle/error_propagate_null.cocci.
> 
> 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 +---
>  scripts/coccinelle/error_propagate_null.cocci | 10 ++++++++++
>  16 files changed, 41 insertions(+), 93 deletions(-)
>  create mode 100644 scripts/coccinelle/error_propagate_null.cocci

You can do:
git config diff.orderFile /path/to/file

and then set up a list of globs in /path/to/file in order to influence
your diffs; in my case, I stuck 'scripts/coccinelle/*' near the top of
my order file, as I find that to be a more useful part of the patch than
the churn from running it.  But it doesn't affect patch correctness,
just ease of review.

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/scripts/coccinelle/error_propagate_null.cocci
> @@ -0,0 +1,10 @@
> +// error_propagate() already ignores local_err==NULL, so there's
> +// no need to check it before calling.
> +
> +@@
> +identifier L;
> +expression E;
> +@@
> +-if (L) {
> +     error_propagate(E, L);
> +-}
> 

-- 
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 v2 1/3] error: Remove NULL checks on error_propagate() calls

[Cornelia Huck]

On Fri, 10 Jun 2016 17:12:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> error_propagate() already ignores local_err==NULL, so there's no
> need to check it before calling.
> 
> Coccinelle patch used to perform the changes added to
> scripts/coccinelle/error_propagate_null.cocci.
> 
> 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 +++++++--------------------

For virtio-ccw:

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

>  hw/usb/dev-storage.c                          |  4 +---
>  qga/commands-win32.c                          |  8 ++------
>  qom/object.c                                  |  4 +---
>  scripts/coccinelle/error_propagate_null.cocci | 10 ++++++++++
>  16 files changed, 41 insertions(+), 93 deletions(-)
>  create mode 100644 scripts/coccinelle/error_propagate_null.cocci

[Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

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

Coccinelle patch used to perform the changes added to
scripts/coccinelle/remove_local_err.cocci.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block.c                                   |  8 ++------
 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 +++++++---------------------
 scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++
 target-i386/cpu.c                         |  4 +---
 8 files changed, 46 insertions(+), 54 deletions(-)
 create mode 100644 scripts/coccinelle/remove_local_err.cocci

diff --git a/block.c b/block.c
index ecca55a..d516ab6 100644
--- a/block.c
+++ b/block.c
@@ -294,14 +294,12 @@ typedef struct CreateCo {
 
 static void coroutine_fn bdrv_create_co_entry(void *opaque)
 {
-    Error *local_err = NULL;
     int ret;
 
     CreateCo *cco = opaque;
     assert(cco->drv);
 
-    ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err);
-    error_propagate(&cco->err, local_err);
+    ret = cco->drv->bdrv_create(cco->filename, cco->opts, &cco->err);
     cco->ret = ret;
 }
 
@@ -353,7 +351,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 +358,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/scripts/coccinelle/remove_local_err.cocci b/scripts/coccinelle/remove_local_err.cocci
new file mode 100644
index 0000000..5f0b6c0
--- /dev/null
+++ b/scripts/coccinelle/remove_local_err.cocci
@@ -0,0 +1,27 @@
+// Replace unnecessary usage of local_err variable with
+// direct usage of errp argument
+
+@@
+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
+ }
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 v2 2/3] error: Remove unnecessary local_err variables

[Eric Blake]

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

On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> This patch simplifies code that uses a local_err variable just to
> immediately use it for an error_propagate() call.
> 
> Coccinelle patch used to perform the changes added to
> scripts/coccinelle/remove_local_err.cocci.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block.c                                   |  8 ++------
>  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 +++++++---------------------
>  scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++
>  target-i386/cpu.c                         |  4 +---
>  8 files changed, 46 insertions(+), 54 deletions(-)
>  create mode 100644 scripts/coccinelle/remove_local_err.cocci
> 

> +++ b/block.c
> @@ -294,14 +294,12 @@ typedef struct CreateCo {
>  
>  static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  {
> -    Error *local_err = NULL;
>      int ret;
>  
>      CreateCo *cco = opaque;
>      assert(cco->drv);
>  
> -    ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err);
> -    error_propagate(&cco->err, local_err);
> +    ret = cco->drv->bdrv_create(cco->filename, cco->opts, &cco->err);
>      cco->ret = ret;

This hunk doesn't get simplified by 3/3; you may want to consider a
manual followup to drop 'int ret' and just assign
cco->drv->bdrv_create() directly to cco->ret.  But doesn't change this
patch.


> +++ 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);

Coccinelle messes a bit with the formatting (the old way explicitly
tried to pair related has_foo with foo). But I'm going to mess with it
again with my qapi patches for passing a boxed parameter rather than
lots of arguments, so don't worry about it.

> +++ b/scripts/coccinelle/remove_local_err.cocci
> @@ -0,0 +1,27 @@
> +// Replace unnecessary usage of local_err variable with
> +// direct usage of errp argument
> +
> +@@
> +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
> + }

Looks good.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Fri, Jun 10, 2016 at 02:59:55PM -0600, Eric Blake wrote:
> On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> > This patch simplifies code that uses a local_err variable just to
> > immediately use it for an error_propagate() call.
> > 
> > Coccinelle patch used to perform the changes added to
> > scripts/coccinelle/remove_local_err.cocci.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  block.c                                   |  8 ++------
> >  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 +++++++---------------------
> >  scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++
> >  target-i386/cpu.c                         |  4 +---
> >  8 files changed, 46 insertions(+), 54 deletions(-)
> >  create mode 100644 scripts/coccinelle/remove_local_err.cocci
> > 
> 
> > +++ b/block.c
> > @@ -294,14 +294,12 @@ typedef struct CreateCo {
> >  
> >  static void coroutine_fn bdrv_create_co_entry(void *opaque)
> >  {
> > -    Error *local_err = NULL;
> >      int ret;
> >  
> >      CreateCo *cco = opaque;
> >      assert(cco->drv);
> >  
> > -    ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err);
> > -    error_propagate(&cco->err, local_err);
> > +    ret = cco->drv->bdrv_create(cco->filename, cco->opts, &cco->err);
> >      cco->ret = ret;
> 
> This hunk doesn't get simplified by 3/3; you may want to consider a
> manual followup to drop 'int ret' and just assign
> cco->drv->bdrv_create() directly to cco->ret.  But doesn't change this
> patch.

This could become yet another Coccinelle script, but we need to
be careful about type conversions, and tell it to do it only if
the types of 'ret', 'cc->drv->bdrv_create()' and 'cco->ret' are
the same.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Cornelia Huck]

On Fri, 10 Jun 2016 17:12:17 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This patch simplifies code that uses a local_err variable just to
> immediately use it for an error_propagate() call.
> 
> Coccinelle patch used to perform the changes added to
> scripts/coccinelle/remove_local_err.cocci.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block.c                                   |  8 ++------
>  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 +++++++---------------------

For the two virtio-ccw files:

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

>  scripts/coccinelle/remove_local_err.cocci | 27 +++++++++++++++++++++++++++
>  target-i386/cpu.c                         |  4 +---
>  8 files changed, 46 insertions(+), 54 deletions(-)
>  create mode 100644 scripts/coccinelle/remove_local_err.cocci

Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> This patch simplifies code that uses a local_err variable just to
> immediately use it for an error_propagate() call.
>
> Coccinelle patch used to perform the changes added to
> scripts/coccinelle/remove_local_err.cocci.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
[...]
> diff --git a/scripts/coccinelle/remove_local_err.cocci b/scripts/coccinelle/remove_local_err.cocci
> new file mode 100644
> index 0000000..5f0b6c0
> --- /dev/null
> +++ b/scripts/coccinelle/remove_local_err.cocci
> @@ -0,0 +1,27 @@
> +// Replace unnecessary usage of local_err variable with
> +// direct usage of errp argument
> +
> +@@
> +expression list ARGS;
> +expression F2;
> +identifier LOCAL_ERR;
> +expression ERRP;
> +idexpression V;
> +typedef Error;
> +expression I;

I isn't used.

> +@@
> + {
> +     ...
> +-    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
> + }

There is an (ugly) difference between

    error_setg(&local_err, ...);
    error_propagate(errp, &local_err);

and

    error_setg(errp, ...);

The latter aborts when @errp already contains an error, the former does
nothing.

Your transformation has the error_setg() or similar hidden in F2().  It
can add aborts.

I think it can be salvaged: we know that @errp must not contain an error
on function entry.  If @errp doesn't occur elsewhere in this function,
it cannot pick up an error on the way to the transformed spot.  Can you
add that to your when constraints?

[...]

Re: [Qemu-devel] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 01:42:15PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > This patch simplifies code that uses a local_err variable just to
> > immediately use it for an error_propagate() call.
> >
> > Coccinelle patch used to perform the changes added to
> > scripts/coccinelle/remove_local_err.cocci.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> [...]
> > diff --git a/scripts/coccinelle/remove_local_err.cocci b/scripts/coccinelle/remove_local_err.cocci
> > new file mode 100644
> > index 0000000..5f0b6c0
> > --- /dev/null
> > +++ b/scripts/coccinelle/remove_local_err.cocci
> > @@ -0,0 +1,27 @@
> > +// Replace unnecessary usage of local_err variable with
> > +// direct usage of errp argument
> > +
> > +@@
> > +expression list ARGS;
> > +expression F2;
> > +identifier LOCAL_ERR;
> > +expression ERRP;
> > +idexpression V;
> > +typedef Error;
> > +expression I;
> 
> I isn't used.
> 
> > +@@
> > + {
> > +     ...
> > +-    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
> > + }
> 
> There is an (ugly) difference between
> 
>     error_setg(&local_err, ...);
>     error_propagate(errp, &local_err);
> 
> and
> 
>     error_setg(errp, ...);
> 
> The latter aborts when @errp already contains an error, the former does
> nothing.

Why the difference? Couldn't we change that so that both are equivalent?

> 
> Your transformation has the error_setg() or similar hidden in F2().  It
> can add aborts.
> 
> I think it can be salvaged: we know that @errp must not contain an error
> on function entry.  If @errp doesn't occur elsewhere in this function,
> it cannot pick up an error on the way to the transformed spot.  Can you
> add that to your when constraints?

Do we really know that *errp is NULL on entry? Aren't we allowed to call
functions with a non-NULL *errp?

See, e.g.:

void qmp_guest_suspend_disk(Error **errp)
{
    Error *local_err = NULL;
    GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);

    *mode = GUEST_SUSPEND_MODE_DISK;
    check_suspend_mode(*mode, &local_err);
    acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
    execute_async(do_suspend, mode, &local_err);

    if (local_err) {
        error_propagate(errp, local_err);
        g_free(mode);
    }
}


-- 
Eduardo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eric Blake]

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

On 06/13/2016 09:52 AM, Eduardo Habkost wrote:

>>
>> There is an (ugly) difference between
>>
>>     error_setg(&local_err, ...);
>>     error_propagate(errp, &local_err);
>>
>> and
>>
>>     error_setg(errp, ...);
>>
>> The latter aborts when @errp already contains an error, the former does
>> nothing.
> 
> Why the difference? Couldn't we change that so that both are equivalent?

Maybe, but I think it weakens our position. An abort() on an attempt to
incorrectly set an error twice helps catch errors where we are throwing
away a more useful first error message.  The documentation for
error_propagate() already mentioned that it was an exception to the rule.

> 
>>
>> Your transformation has the error_setg() or similar hidden in F2().  It
>> can add aborts.
>>
>> I think it can be salvaged: we know that @errp must not contain an error
>> on function entry.  If @errp doesn't occur elsewhere in this function,
>> it cannot pick up an error on the way to the transformed spot.  Can you
>> add that to your when constraints?
> 
> Do we really know that *errp is NULL on entry? Aren't we allowed to call
> functions with a non-NULL *errp?

Except for error_propagate(), no.

> 
> See, e.g.:
> 
> void qmp_guest_suspend_disk(Error **errp)
> {
>     Error *local_err = NULL;
>     GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
> 
>     *mode = GUEST_SUSPEND_MODE_DISK;
>     check_suspend_mode(*mode, &local_err);
>     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>     execute_async(do_suspend, mode, &local_err);

That usage is a bug.  A Coccinelle script to root out such buggy
instances would be nice.

-- 
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] [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/13/2016 09:52 AM, Eduardo Habkost wrote:
>
>>>
>>> There is an (ugly) difference between
>>>
>>>     error_setg(&local_err, ...);
>>>     error_propagate(errp, &local_err);
>>>
>>> and
>>>
>>>     error_setg(errp, ...);
>>>
>>> The latter aborts when @errp already contains an error, the former does
>>> nothing.
>> 
>> Why the difference? Couldn't we change that so that both are equivalent?
>
> Maybe, but I think it weakens our position. An abort() on an attempt to
> incorrectly set an error twice helps catch errors where we are throwing
> away a more useful first error message.  The documentation for
> error_propagate() already mentioned that it was an exception to the rule.

For what it's worth, both g_set_error(err, ...) and
g_propagate_error(err, ...) require !err || !*err.  To accumulate
multiple errors so that the first one wins, you have to do something
like

    if (local_err) {
        g_clear_error(errp);
        g_propagate_error(errp, local_err);
    }

error.h happend before we got GLib.  Its designers chose to deviate from
GLib and made error_propagate() accumulate errors.  This trades the
ability to detect misuse for less boilerplate:

    error_propagate(errp, local_err);

Tightening error_propagate() now would lead to a rather tedious hunt for
callers that rely on its accumulating behavior.

We could do it incrementally by renaming error_propagate() to
error_accumulate(), and have a new error_propagate() that's consistent
with error_setg().  Not sure it's worth it.

Loosening error_setg() & friends is also possible, but we'd detect fewer
misuse, and we'd be left with some superfluous code to clean up.  Not
sure that's smart.

>>> Your transformation has the error_setg() or similar hidden in F2().  It
>>> can add aborts.
>>>
>>> I think it can be salvaged: we know that @errp must not contain an error
>>> on function entry.  If @errp doesn't occur elsewhere in this function,
>>> it cannot pick up an error on the way to the transformed spot.  Can you
>>> add that to your when constraints?
>> 
>> Do we really know that *errp is NULL on entry? Aren't we allowed to call
>> functions with a non-NULL *errp?
>
> Except for error_propagate(), no.

By convention, all functions taking an Error **errp argument expect one
that can be safely passed to error_setg().  That means !errp || errp ==
&error_abort || errp == &error_fatal || !*errp.

>> 
>> See, e.g.:
>> 
>> void qmp_guest_suspend_disk(Error **errp)
>> {
>>     Error *local_err = NULL;
>>     GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
>> 
>>     *mode = GUEST_SUSPEND_MODE_DISK;
>>     check_suspend_mode(*mode, &local_err);
>>     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>     execute_async(do_suspend, mode, &local_err);
>
> That usage is a bug.  A Coccinelle script to root out such buggy
> instances would be nice.

We've discussed this before.  See for instance commit 297a364:

    qapi: Replace uncommon use of the error API by the common one
    
    We commonly use the error API like this:
    
        err = NULL;
        foo(..., &err);
        if (err) {
            goto out;
        }
        bar(..., &err);
    
    Every error source is checked separately.  The second function is only
    called when the first one succeeds.  Both functions are free to pass
    their argument to error_set().  Because error_set() asserts no error
    has been set, this effectively means they must not be called with an
    error set.
    
    The qapi-generated code uses the error API differently:
    
        // *errp was initialized to NULL somewhere up the call chain
        frob(..., errp);
        gnat(..., errp);
    
    Errors accumulate in *errp: first error wins, subsequent errors get
    dropped.  To make this work, the second function does nothing when
    called with an error set.  Requires non-null errp, or else the second
    function can't see the first one fail.
    
    This usage has also bled into visitor tests, and two device model
    object property getters rtc_get_date() and balloon_stats_get_all().
    
    With the "accumulate" technique, you need fewer error checks in
    callers, and buy that with an error check in every callee.  Can be
    nice.
    
    However, mixing the two techniques is confusing.  You can't use the
    "accumulate" technique with functions designed for the "check
    separately" technique.  You can use the "check separately" technique
    with functions designed for the "accumulate" technique, but then
    error_set() can't catch you setting an error more than once.
    
    Standardize on the "check separately" technique for now, because it's
    overwhelmingly prevalent.

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 08:49:37PM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
[...]
> >> 
> >> See, e.g.:
> >> 
> >> void qmp_guest_suspend_disk(Error **errp)
> >> {
> >>     Error *local_err = NULL;
> >>     GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
> >> 
> >>     *mode = GUEST_SUSPEND_MODE_DISK;
> >>     check_suspend_mode(*mode, &local_err);
> >>     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> >>     execute_async(do_suspend, mode, &local_err);
> >
> > That usage is a bug.  A Coccinelle script to root out such buggy
> > instances would be nice.
> 
> We've discussed this before.  See for instance commit 297a364:
> 
>     qapi: Replace uncommon use of the error API by the common one

That explains why I was confused: I remember seeing that QAPI
visitors could be called with *errp set.

I will try to change the script as suggested, to only apply the
changes if errp is never touched before the error_setg() call.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Jun 13, 2016 at 08:49:37PM +0200, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
> [...]
>> >> 
>> >> See, e.g.:
>> >> 
>> >> void qmp_guest_suspend_disk(Error **errp)
>> >> {
>> >>     Error *local_err = NULL;
>> >>     GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
>> >> 
>> >>     *mode = GUEST_SUSPEND_MODE_DISK;
>> >>     check_suspend_mode(*mode, &local_err);
>> >>     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>> >>     execute_async(do_suspend, mode, &local_err);
>> >
>> > That usage is a bug.  A Coccinelle script to root out such buggy
>> > instances would be nice.
>> 
>> We've discussed this before.  See for instance commit 297a364:
>> 
>>     qapi: Replace uncommon use of the error API by the common one
>
> That explains why I was confused: I remember seeing that QAPI
> visitors could be called with *errp set.

Nothing confuses as effectively as bad examples.

> I will try to change the script as suggested, to only apply the
> changes if errp is never touched before the error_setg() call.

Thanks!

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 10:01:16AM -0600, Eric Blake wrote:
> On 06/13/2016 09:52 AM, Eduardo Habkost wrote:
[...]
> > 
> > See, e.g.:
> > 
> > void qmp_guest_suspend_disk(Error **errp)
> > {
> >     Error *local_err = NULL;
> >     GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
> > 
> >     *mode = GUEST_SUSPEND_MODE_DISK;
> >     check_suspend_mode(*mode, &local_err);
> >     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> >     execute_async(do_suspend, mode, &local_err);
> 
> That usage is a bug.  A Coccinelle script to root out such buggy
> instances would be nice.

I've tried to write one, but got lots of false positives due to
error-checking using the function return value, not local_err.
For reference, this is the script I have used:

@@
typedef Error;
identifier F1 !~ "hmp_handle_error|error_free_or_abort";
identifier F2 !~ "hmp_handle_error|error_free_or_abort";
idexpression Error *ERR;
@@
-F1(..., &ERR)
+FIXME_incorrect_error_usage1()
... when != ERR
-F2(..., &ERR)
+FIXME_incorrect_error_usage2()

@@
identifier F1 !~ "hmp_handle_error|error_free_or_abort";
identifier F2 !~ "hmp_handle_error|error_free_or_abort";
idexpression Error **ERRP;
@@
-F1(..., ERRP)
+FIXME_incorrect_error_usage1()
 ... when != ERRP
-F2(..., ERRP)
+FIXME_incorrect_error_usage2()

-- 
Eduardo

[Qemu-devel] [RFC v2 3/3] Remove unnecessary variables for function return value

[Eduardo Habkost]

Use Coccinelle script to replace 'ret = E; return ret' with
'return E'. The script will do the substitution only when the
function return type and variable type are the same.

Sending as RFC because the patch looks more intrusive than the
others. Probably better to split it per subsystem and let each
maintainer review and apply it?

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 audio/audio.c                            | 10 ++--------
 block.c                                  |  4 +---
 block/archipelago.c                      |  4 +---
 block/qcow2-cluster.c                    |  7 ++-----
 block/qcow2-refcount.c                   |  7 ++-----
 block/raw-posix.c                        |  8 ++------
 block/raw_bsd.c                          |  5 +----
 block/rbd.c                              |  5 +----
 block/vmdk.c                             |  6 ++----
 block/vvfat.c                            |  5 +----
 hw/acpi/aml-build.c                      | 13 +++----------
 hw/audio/intel-hda.c                     |  5 +----
 hw/display/vga.c                         |  4 +---
 hw/intc/s390_flic_kvm.c                  |  5 ++---
 hw/pci-host/uninorth.c                   |  5 +----
 hw/ppc/spapr_vio.c                       |  7 +------
 hw/scsi/megasas.c                        | 10 +---------
 hw/scsi/scsi-generic.c                   |  5 +----
 hw/timer/mc146818rtc.c                   |  5 +----
 hw/virtio/virtio-pci.c                   |  4 +---
 linux-user/signal.c                      | 15 ++++-----------
 page_cache.c                             |  5 +----
 qga/commands-posix.c                     |  4 +---
 qga/commands-win32.c                     |  6 +-----
 qobject/qlist.c                          |  5 +----
 scripts/coccinelle/return_directly.cocci | 19 +++++++++++++++++++
 target-i386/fpu_helper.c                 | 10 ++--------
 target-i386/kvm.c                        |  5 ++---
 target-mips/dsp_helper.c                 | 15 +++------------
 target-mips/op_helper.c                  |  4 +---
 target-s390x/helper.c                    |  6 +-----
 target-sparc/cc_helper.c                 | 25 +++++--------------------
 target-tricore/op_helper.c               | 13 ++++---------
 tests/display-vga-test.c                 |  6 +-----
 tests/endianness-test.c                  |  5 +----
 tests/i440fx-test.c                      |  4 +---
 tests/intel-hda-test.c                   |  6 +-----
 tests/test-filter-redirector.c           |  6 +-----
 tests/virtio-blk-test.c                  |  5 +----
 tests/virtio-console-test.c              |  6 +-----
 tests/virtio-net-test.c                  |  6 +-----
 tests/virtio-scsi-test.c                 |  6 +-----
 tests/wdt_ib700-test.c                   |  6 +-----
 ui/cursor.c                              | 10 ++--------
 ui/qemu-pixman.c                         | 11 +++--------
 util/module.c                            |  6 +-----
 vl.c                                     |  5 +----
 47 files changed, 90 insertions(+), 254 deletions(-)
 create mode 100644 scripts/coccinelle/return_directly.cocci

diff --git a/audio/audio.c b/audio/audio.c
index e60c124..9d4dcc7 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1131,8 +1131,6 @@ static void audio_timer (void *opaque)
  */
 int AUD_write (SWVoiceOut *sw, void *buf, int size)
 {
-    int bytes;
-
     if (!sw) {
         /* XXX: Consider options */
         return size;
@@ -1143,14 +1141,11 @@ int AUD_write (SWVoiceOut *sw, void *buf, int size)
         return 0;
     }
 
-    bytes = sw->hw->pcm_ops->write (sw, buf, size);
-    return bytes;
+    return sw->hw->pcm_ops->write(sw, buf, size);
 }
 
 int AUD_read (SWVoiceIn *sw, void *buf, int size)
 {
-    int bytes;
-
     if (!sw) {
         /* XXX: Consider options */
         return size;
@@ -1161,8 +1156,7 @@ int AUD_read (SWVoiceIn *sw, void *buf, int size)
         return 0;
     }
 
-    bytes = sw->hw->pcm_ops->read (sw, buf, size);
-    return bytes;
+    return sw->hw->pcm_ops->read(sw, buf, size);
 }
 
 int AUD_get_buffer_size_out (SWVoiceOut *sw)
diff --git a/block.c b/block.c
index d516ab6..c537307 100644
--- a/block.c
+++ b/block.c
@@ -351,15 +351,13 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
     BlockDriver *drv;
-    int ret;
 
     drv = bdrv_find_protocol(filename, true, errp);
     if (drv == NULL) {
         return -ENOENT;
     }
 
-    ret = bdrv_create(drv, filename, opts, errp);
-    return ret;
+    return bdrv_create(drv, filename, opts, errp);
 }
 
 /**
diff --git a/block/archipelago.c b/block/archipelago.c
index b9f5e69..37b8aca 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -974,11 +974,9 @@ err_exit2:
 
 static int64_t qemu_archipelago_getlength(BlockDriverState *bs)
 {
-    int64_t ret;
     BDRVArchipelagoState *s = bs->opaque;
 
-    ret = archipelago_volume_info(s);
-    return ret;
+    return archipelago_volume_info(s);
 }
 
 static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index b04bfaf..e0e5d9e 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -154,11 +154,8 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
     uint64_t **l2_table)
 {
     BDRVQcow2State *s = bs->opaque;
-    int ret;
-
-    ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset, (void**) l2_table);
-
-    return ret;
+    return qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
+                           (void **)l2_table);
 }
 
 /*
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 66f187a..3bef410 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -218,13 +218,10 @@ static int load_refcount_block(BlockDriverState *bs,
                                void **refcount_block)
 {
     BDRVQcow2State *s = bs->opaque;
-    int ret;
 
     BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_LOAD);
-    ret = qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset,
-        refcount_block);
-
-    return ret;
+    return qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset,
+                           refcount_block);
 }
 
 /*
diff --git a/block/raw-posix.c b/block/raw-posix.c
index d7397bf..6a88276 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -582,11 +582,9 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     BDRVRawState *s = bs->opaque;
-    int ret;
 
     s->type = FTYPE_FILE;
-    ret = raw_open_common(bs, options, flags, 0, errp);
-    return ret;
+    return raw_open_common(bs, options, flags, 0, errp);
 }
 
 static int raw_reopen_prepare(BDRVReopenState *state,
@@ -2440,13 +2438,11 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
     BDRVRawState *s = bs->opaque;
-    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, errp);
-    return ret;
+    return raw_open_common(bs, options, flags, O_NONBLOCK, errp);
 }
 
 static int cdrom_probe_device(const char *filename)
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index b51ac98..7f63791 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -190,10 +190,7 @@ static int raw_has_zero_init(BlockDriverState *bs)
 
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-    int ret;
-
-    ret = bdrv_create_file(filename, opts, errp);
-    return ret;
+    return bdrv_create_file(filename, opts, errp);
 }
 
 static int raw_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/block/rbd.c b/block/rbd.c
index 5bc5b32..b94ce40 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -875,10 +875,7 @@ static int qemu_rbd_snap_rollback(BlockDriverState *bs,
                                   const char *snapshot_name)
 {
     BDRVRBDState *s = bs->opaque;
-    int r;
-
-    r = rbd_snap_rollback(s->image, snapshot_name);
-    return r;
+    return rbd_snap_rollback(s->image, snapshot_name);
 }
 
 static int qemu_rbd_snap_list(BlockDriverState *bs,
diff --git a/block/vmdk.c b/block/vmdk.c
index ee09423..2901692 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1260,15 +1260,13 @@ static VmdkExtent *find_extent(BDRVVmdkState *s,
 static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
                                                    int64_t offset)
 {
-    uint64_t offset_in_cluster, extent_begin_offset, extent_relative_offset;
+    uint64_t extent_begin_offset, extent_relative_offset;
     uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
 
     extent_begin_offset =
         (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
     extent_relative_offset = offset - extent_begin_offset;
-    offset_in_cluster = extent_relative_offset % cluster_size;
-
-    return offset_in_cluster;
+    return extent_relative_offset % cluster_size;
 }
 
 static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
diff --git a/block/vvfat.c b/block/vvfat.c
index 6d2e21c..5569450 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -114,15 +114,12 @@ static inline int array_ensure_allocated(array_t* array, int index)
 
 static inline void* array_get_next(array_t* array) {
     unsigned int next = array->next;
-    void* result;
 
     if (array_ensure_allocated(array, next) < 0)
 	return NULL;
 
     array->next = next + 1;
-    result = array_get(array, next);
-
-    return result;
+    return array_get(array, next);
 }
 
 static inline void* array_insert(array_t* array,unsigned int index,unsigned int count) {
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 123160a..874e473 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -324,12 +324,9 @@ static void aml_free(gpointer data, gpointer user_data)
 
 Aml *init_aml_allocator(void)
 {
-    Aml *var;
-
     assert(!alloc_list);
     alloc_list = g_ptr_array_new();
-    var = aml_alloc();
-    return var;
+    return aml_alloc();
 }
 
 void free_aml_allocator(void)
@@ -451,12 +448,10 @@ Aml *aml_name_decl(const char *name, Aml *val)
 /* ACPI 1.0b: 16.2.6.1 Arg Objects Encoding */
 Aml *aml_arg(int pos)
 {
-    Aml *var;
     uint8_t op = 0x68 /* ARG0 op */ + pos;
 
     assert(pos <= 6);
-    var = aml_opcode(op);
-    return var;
+    return aml_opcode(op);
 }
 
 /* ACPI 2.0a: 17.2.4.4 Type 2 Opcodes Encoding: DefToInteger */
@@ -1082,12 +1077,10 @@ Aml *aml_string(const char *name_format, ...)
 /* ACPI 1.0b: 16.2.6.2 Local Objects Encoding */
 Aml *aml_local(int num)
 {
-    Aml *var;
     uint8_t op = 0x60 /* Local0Op */ + num;
 
     assert(num <= 7);
-    var = aml_opcode(op);
-    return var;
+    return aml_opcode(op);
 }
 
 /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 93d7669..098b17d 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -219,10 +219,7 @@ static void intel_hda_reset(DeviceState *dev);
 
 static hwaddr intel_hda_addr(uint32_t lbase, uint32_t ubase)
 {
-    hwaddr addr;
-
-    addr = ((uint64_t)ubase << 32) | lbase;
-    return addr;
+    return ((uint64_t)ubase << 32) | lbase;
 }
 
 static void intel_hda_update_int_sts(IntelHDAState *d)
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 9ebc54f..2a88b3c 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -700,9 +700,7 @@ static void vbe_update_vgaregs(VGACommonState *s)
 static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr)
 {
     VGACommonState *s = opaque;
-    uint32_t val;
-    val = s->vbe_index;
-    return val;
+    return s->vbe_index;
 }
 
 uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index eed6325..8ab77e5 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -176,7 +176,7 @@ static int kvm_s390_register_io_adapter(S390FLICState *fs, uint32_t id,
         .swap = swap,
     };
     KVMS390FLICState *flic = KVM_S390_FLIC(fs);
-    int r, ret;
+    int r;
     struct kvm_device_attr attr = {
         .group = KVM_DEV_FLIC_ADAPTER_REGISTER,
         .addr = (uint64_t)&adapter,
@@ -189,8 +189,7 @@ static int kvm_s390_register_io_adapter(S390FLICState *fs, uint32_t id,
 
     r = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
 
-    ret = r ? -errno : 0;
-    return ret;
+    return r ? -errno : 0;
 }
 
 static int kvm_s390_io_adapter_map(S390FLICState *fs, uint32_t id,
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index 15b1054..7aac4d6 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -62,12 +62,9 @@ typedef struct UNINState {
 
 static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
 {
-    int retval;
     int devfn = pci_dev->devfn & 0x00FFFFFF;
 
-    retval = (((devfn >> 11) & 0x1F) + irq_num) & 3;
-
-    return retval;
+    return (((devfn >> 11) & 0x1F) + irq_num) & 3;
 }
 
 static void pci_unin_set_irq(void *opaque, int irq_num, int level)
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 3d9b9c6..a0ccb61 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -57,12 +57,7 @@ static char *spapr_vio_get_dev_name(DeviceState *qdev)
 {
     VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
     VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
-    char *name;
-
-    /* Device tree style name device@reg */
-    name = g_strdup_printf("%s@%x", pc->dt_name, dev->reg);
-
-    return name;
+    return g_strdup_printf("%s@%x", pc->dt_name, dev->reg);
 }
 
 static void spapr_vio_bus_class_init(ObjectClass *klass, void *data)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index a9ffc32..178c4e7 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -410,17 +410,9 @@ static void megasas_encode_lba(uint8_t *cdb, uint64_t lba,
 static uint64_t megasas_fw_time(void)
 {
     struct tm curtime;
-    uint64_t bcd_time;
 
     qemu_get_timedate(&curtime, 0);
-    bcd_time = ((uint64_t)curtime.tm_sec & 0xff) << 48 |
-        ((uint64_t)curtime.tm_min & 0xff)  << 40 |
-        ((uint64_t)curtime.tm_hour & 0xff) << 32 |
-        ((uint64_t)curtime.tm_mday & 0xff) << 24 |
-        ((uint64_t)curtime.tm_mon & 0xff)  << 16 |
-        ((uint64_t)(curtime.tm_year + 1900) & 0xffff);
-
-    return bcd_time;
+    return ((uint64_t)curtime.tm_sec & 0xff) << 48 | ((uint64_t)curtime.tm_min & 0xff) << 40 | ((uint64_t)curtime.tm_hour & 0xff) << 32 | ((uint64_t)curtime.tm_mday & 0xff) << 24 | ((uint64_t)curtime.tm_mon & 0xff) << 16 | ((uint64_t)(curtime.tm_year + 1900) & 0xffff);
 }
 
 /*
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 71372a8..6a2d89a 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -579,10 +579,7 @@ const SCSIReqOps scsi_generic_req_ops = {
 static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun,
                                      uint8_t *buf, void *hba_private)
 {
-    SCSIRequest *req;
-
-    req = scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private);
-    return req;
+    return scsi_req_alloc(&scsi_generic_req_ops, d, tag, lun, hba_private);
 }
 
 static Property scsi_generic_properties[] = {
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index a11b8b4..c212990 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -105,12 +105,9 @@ static inline bool rtc_running(RTCState *s)
 
 static uint64_t get_guest_rtc_ns(RTCState *s)
 {
-    uint64_t guest_rtc;
     uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
 
-    guest_rtc = s->base_rtc * NANOSECONDS_PER_SECOND +
-        guest_clock - s->last_update + s->offset;
-    return guest_rtc;
+    return s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - s->last_update + s->offset;
 }
 
 #ifdef TARGET_I386
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index bfedbbf..1a02783 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -761,9 +761,7 @@ static int kvm_virtio_pci_irqfd_use(VirtIOPCIProxy *proxy,
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
     VirtQueue *vq = virtio_get_queue(vdev, queue_no);
     EventNotifier *n = virtio_queue_get_guest_notifier(vq);
-    int ret;
-    ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, irqfd->virq);
-    return ret;
+    return kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, irqfd->virq);
 }
 
 static void kvm_virtio_pci_irqfd_release(VirtIOPCIProxy *proxy,
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 61c1145..1dadddf 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -195,7 +195,6 @@ int block_signals(void)
 {
     TaskState *ts = (TaskState *)thread_cpu->opaque;
     sigset_t set;
-    int pending;
 
     /* It's OK to block everything including SIGSEGV, because we won't
      * run any further guest code before unblocking signals in
@@ -204,9 +203,7 @@ int block_signals(void)
     sigfillset(&set);
     sigprocmask(SIG_SETMASK, &set, 0);
 
-    pending = atomic_xchg(&ts->signal_pending, 1);
-
-    return pending;
+    return atomic_xchg(&ts->signal_pending, 1);
 }
 
 /* Wrapper for sigprocmask function
@@ -3956,9 +3953,7 @@ static void setup_sigcontext(struct target_sigcontext *sc,
 
 static inline unsigned long align_sigframe(unsigned long sp)
 {
-    unsigned long i;
-    i = sp & ~3UL;
-    return i;
+    return sp & ~3UL;
 }
 
 static inline abi_ulong get_sigframe(struct target_sigaction *ka,
@@ -4555,7 +4550,7 @@ static target_ulong get_sigframe(struct target_sigaction *ka,
                                  CPUPPCState *env,
                                  int frame_size)
 {
-    target_ulong oldsp, newsp;
+    target_ulong oldsp;
 
     oldsp = env->gpr[1];
 
@@ -4565,9 +4560,7 @@ static target_ulong get_sigframe(struct target_sigaction *ka,
                  + target_sigaltstack_used.ss_size);
     }
 
-    newsp = (oldsp - frame_size) & ~0xFUL;
-
-    return newsp;
+    return (oldsp - frame_size) & ~0xFUL;
 }
 
 static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
diff --git a/page_cache.c b/page_cache.c
index a2809db..5f85787 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -111,11 +111,8 @@ void cache_fini(PageCache *cache)
 static size_t cache_get_cache_pos(const PageCache *cache,
                                   uint64_t address)
 {
-    size_t pos;
-
     g_assert(cache->max_num_items);
-    pos = (address / cache->page_size) & (cache->max_num_items - 1);
-    return pos;
+    return (address / cache->page_size) & (cache->max_num_items - 1);
 }
 
 static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index eaef7be..ea37c09 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -127,7 +127,6 @@ int64_t qmp_guest_get_time(Error **errp)
 {
    int ret;
    qemu_timeval tq;
-   int64_t time_ns;
 
    ret = qemu_gettimeofday(&tq);
    if (ret < 0) {
@@ -135,8 +134,7 @@ int64_t qmp_guest_get_time(Error **errp)
        return -1;
    }
 
-   time_ns = tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
-   return time_ns;
+   return tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
 }
 
 void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index ea23797..b00a891 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1150,7 +1150,6 @@ out:
 int64_t qmp_guest_get_time(Error **errp)
 {
     SYSTEMTIME ts = {0};
-    int64_t time_ns;
     FILETIME tf;
 
     GetSystemTime(&ts);
@@ -1164,10 +1163,7 @@ int64_t qmp_guest_get_time(Error **errp)
         return -1;
     }
 
-    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
-                - W32_FT_OFFSET) * 100;
-
-    return time_ns;
+    return ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100;
 }
 
 void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
diff --git a/qobject/qlist.c b/qobject/qlist.c
index 1ec74de..86b60cb 100644
--- a/qobject/qlist.c
+++ b/qobject/qlist.c
@@ -100,7 +100,6 @@ QObject *qlist_pop(QList *qlist)
 QObject *qlist_peek(QList *qlist)
 {
     QListEntry *entry;
-    QObject *ret;
 
     if (qlist == NULL || QTAILQ_EMPTY(&qlist->head)) {
         return NULL;
@@ -108,9 +107,7 @@ QObject *qlist_peek(QList *qlist)
 
     entry = QTAILQ_FIRST(&qlist->head);
 
-    ret = entry->value;
-
-    return ret;
+    return entry->value;
 }
 
 int qlist_empty(const QList *qlist)
diff --git a/scripts/coccinelle/return_directly.cocci b/scripts/coccinelle/return_directly.cocci
new file mode 100644
index 0000000..7b0b6ac
--- /dev/null
+++ b/scripts/coccinelle/return_directly.cocci
@@ -0,0 +1,19 @@
+// replace 'R = X; return R;' with 'return R;'
+
+// remove assignment
+@ removal @
+identifier VAR;
+expression E;
+type T;
+identifier F;
+@@
+ T F(...)
+ {
+     ...
+-    T VAR;
+     ... when != VAR
+-    VAR = E;
+-    return VAR;
++    return E;
+     ... when != VAR
+ }
diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
index 206e60f..929489b 100644
--- a/target-i386/fpu_helper.c
+++ b/target-i386/fpu_helper.c
@@ -298,18 +298,12 @@ int32_t helper_fistt_ST0(CPUX86State *env)
 
 int32_t helper_fisttl_ST0(CPUX86State *env)
 {
-    int32_t val;
-
-    val = floatx80_to_int32_round_to_zero(ST0, &env->fp_status);
-    return val;
+    return floatx80_to_int32_round_to_zero(ST0, &env->fp_status);
 }
 
 int64_t helper_fisttll_ST0(CPUX86State *env)
 {
-    int64_t val;
-
-    val = floatx80_to_int64_round_to_zero(ST0, &env->fp_status);
-    return val;
+    return floatx80_to_int64_round_to_zero(ST0, &env->fp_status);
 }
 
 void helper_fldt_ST0(CPUX86State *env, target_ulong ptr)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index abf50e6..98f7544 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1342,7 +1342,7 @@ static int kvm_put_xsave(X86CPU *cpu)
     CPUX86State *env = &cpu->env;
     X86XSaveArea *xsave = env->kvm_xsave_buf;
     uint16_t cwd, swd, twd;
-    int i, r;
+    int i;
 
     if (!has_xsave) {
         return kvm_put_fpu(cpu);
@@ -1391,8 +1391,7 @@ static int kvm_put_xsave(X86CPU *cpu)
             16 * sizeof env->xmm_regs[16]);
     memcpy(&xsave->pkru_state, &env->pkru, sizeof env->pkru);
 #endif
-    r = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_XSAVE, xsave);
-    return r;
+    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_XSAVE, xsave);
 }
 
 static int kvm_put_xcrs(X86CPU *cpu)
diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
index df7d220..7cbda0b 100644
--- a/target-mips/dsp_helper.c
+++ b/target-mips/dsp_helper.c
@@ -3274,14 +3274,11 @@ target_ulong helper_dextr_l(target_ulong ac, target_ulong shift,
                             CPUMIPSState *env)
 {
     uint64_t temp[3];
-    target_ulong result;
 
     shift = shift & 0x3F;
 
     mipsdsp_rndrashift_acc(temp, ac, shift, env);
-    result = (temp[1] << 63) | (temp[0] >> 1);
-
-    return result;
+    return (temp[1] << 63) | (temp[0] >> 1);
 }
 
 target_ulong helper_dextr_r_l(target_ulong ac, target_ulong shift,
@@ -3289,7 +3286,6 @@ target_ulong helper_dextr_r_l(target_ulong ac, target_ulong shift,
 {
     uint64_t temp[3];
     uint32_t temp128;
-    target_ulong result;
 
     shift = shift & 0x3F;
     mipsdsp_rndrashift_acc(temp, ac, shift, env);
@@ -3309,9 +3305,7 @@ target_ulong helper_dextr_r_l(target_ulong ac, target_ulong shift,
         set_DSPControl_overflow_flag(1, 23, env);
     }
 
-    result = (temp[1] << 63) | (temp[0] >> 1);
-
-    return result;
+    return (temp[1] << 63) | (temp[0] >> 1);
 }
 
 target_ulong helper_dextr_rs_l(target_ulong ac, target_ulong shift,
@@ -3319,7 +3313,6 @@ target_ulong helper_dextr_rs_l(target_ulong ac, target_ulong shift,
 {
     uint64_t temp[3];
     uint32_t temp128;
-    target_ulong result;
 
     shift = shift & 0x3F;
     mipsdsp_rndrashift_acc(temp, ac, shift, env);
@@ -3345,9 +3338,7 @@ target_ulong helper_dextr_rs_l(target_ulong ac, target_ulong shift,
         }
         set_DSPControl_overflow_flag(1, 23, env);
     }
-    result = (temp[1] << 63) | (temp[0] >> 1);
-
-    return result;
+    return (temp[1] << 63) | (temp[0] >> 1);
 }
 #endif
 
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 7cf9807..1ae1dda 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -133,10 +133,8 @@ static inline uint64_t get_HILO(CPUMIPSState *env)
 
 static inline target_ulong set_HIT0_LO(CPUMIPSState *env, uint64_t HILO)
 {
-    target_ulong tmp;
     env->active_tc.LO[0] = (int32_t)(HILO & 0xFFFFFFFF);
-    tmp = env->active_tc.HI[0] = (int32_t)(HILO >> 32);
-    return tmp;
+    return env->active_tc.HI[0] = (int32_t)(HILO >> 32);
 }
 
 static inline target_ulong set_HI_LOT0(CPUMIPSState *env, uint64_t HILO)
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 9a744df..54a5177 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -70,11 +70,7 @@ void s390x_cpu_timer(void *opaque)
 
 S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp)
 {
-    S390CPU *cpu;
-
-    cpu = S390_CPU(object_new(TYPE_S390_CPU));
-
-    return cpu;
+    return S390_CPU(object_new(TYPE_S390_CPU));
 }
 
 S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp)
diff --git a/target-sparc/cc_helper.c b/target-sparc/cc_helper.c
index 44c4409..a410a0b 100644
--- a/target-sparc/cc_helper.c
+++ b/target-sparc/cc_helper.c
@@ -200,10 +200,7 @@ static uint32_t compute_all_addx_xcc(CPUSPARCState *env)
 
 static uint32_t compute_C_addx_xcc(CPUSPARCState *env)
 {
-    uint32_t ret;
-
-    ret = get_C_addx_xcc(CC_DST, CC_SRC, CC_SRC2);
-    return ret;
+    return get_C_addx_xcc(CC_DST, CC_SRC, CC_SRC2);
 }
 #endif
 
@@ -219,10 +216,7 @@ static uint32_t compute_all_addx(CPUSPARCState *env)
 
 static uint32_t compute_C_addx(CPUSPARCState *env)
 {
-    uint32_t ret;
-
-    ret = get_C_addx_icc(CC_DST, CC_SRC, CC_SRC2);
-    return ret;
+    return get_C_addx_icc(CC_DST, CC_SRC, CC_SRC2);
 }
 
 static inline uint32_t get_V_tag_icc(target_ulong src1, target_ulong src2)
@@ -365,10 +359,7 @@ static uint32_t compute_all_subx_xcc(CPUSPARCState *env)
 
 static uint32_t compute_C_subx_xcc(CPUSPARCState *env)
 {
-    uint32_t ret;
-
-    ret = get_C_subx_xcc(CC_DST, CC_SRC, CC_SRC2);
-    return ret;
+    return get_C_subx_xcc(CC_DST, CC_SRC, CC_SRC2);
 }
 #endif
 
@@ -384,10 +375,7 @@ static uint32_t compute_all_subx(CPUSPARCState *env)
 
 static uint32_t compute_C_subx(CPUSPARCState *env)
 {
-    uint32_t ret;
-
-    ret = get_C_subx_icc(CC_DST, CC_SRC, CC_SRC2);
-    return ret;
+    return get_C_subx_icc(CC_DST, CC_SRC, CC_SRC2);
 }
 
 static uint32_t compute_all_tsub(CPUSPARCState *env)
@@ -479,8 +467,5 @@ void helper_compute_psr(CPUSPARCState *env)
 
 uint32_t helper_compute_C_icc(CPUSPARCState *env)
 {
-    uint32_t ret;
-
-    ret = icc_table[CC_OP].compute_c(env) >> PSR_CARRY_SHIFT;
-    return ret;
+    return icc_table[CC_OP].compute_c(env) >> PSR_CARRY_SHIFT;
 }
diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c
index a73ed53..3382a12 100644
--- a/target-tricore/op_helper.c
+++ b/target-tricore/op_helper.c
@@ -2117,7 +2117,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2)
     int32_t eq_pos = x_sign & ((r1 >> 32) == r2);
     int32_t eq_neg = x_sign & ((r1 >> 32) == -r2);
     uint32_t quotient;
-    uint64_t ret, remainder;
+    uint64_t remainder;
 
     if ((q_sign & ~eq_neg) | eq_pos) {
         quotient = (r1 + 1) & 0xffffffff;
@@ -2130,8 +2130,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2)
     } else {
         remainder = (r1 & 0xffffffff00000000ull);
     }
-    ret = remainder|quotient;
-    return ret;
+    return remainder | quotient;
 }
 
 uint64_t helper_dvstep(uint64_t r1, uint32_t r2)
@@ -2236,7 +2235,6 @@ uint64_t helper_divide_u(CPUTriCoreState *env, uint32_t r1, uint32_t r2)
 uint64_t helper_mul_h(uint32_t arg00, uint32_t arg01,
                       uint32_t arg10, uint32_t arg11, uint32_t n)
 {
-    uint64_t ret;
     uint32_t result0, result1;
 
     int32_t sc1 = ((arg00 & 0xffff) == 0x8000) &&
@@ -2253,8 +2251,7 @@ uint64_t helper_mul_h(uint32_t arg00, uint32_t arg01,
     } else {
         result0 = (((uint32_t)(arg01 * arg11)) << n);
     }
-    ret = (((uint64_t)result1 << 32)) | result0;
-    return ret;
+    return (((uint64_t)result1 << 32)) | result0;
 }
 
 uint64_t helper_mulm_h(uint32_t arg00, uint32_t arg01,
@@ -2308,11 +2305,9 @@ uint32_t helper_mulr_h(uint32_t arg00, uint32_t arg01,
 uint32_t helper_crc32(uint32_t arg0, uint32_t arg1)
 {
     uint8_t buf[4];
-    uint32_t ret;
     stl_be_p(buf, arg0);
 
-    ret = crc32(arg1, buf, 4);
-    return ret;
+    return crc32(arg1, buf, 4);
 }
 
 /* context save area (CSA) related helpers */
diff --git a/tests/display-vga-test.c b/tests/display-vga-test.c
index 06b244e..9146021 100644
--- a/tests/display-vga-test.c
+++ b/tests/display-vga-test.c
@@ -50,8 +50,6 @@ static void pci_virtio_vga(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("/display/pci/cirrus", pci_cirrus);
@@ -62,7 +60,5 @@ int main(int argc, char **argv)
 #ifdef CONFIG_VIRTIO_VGA
     qtest_add_func("/display/pci/virtio-vga", pci_virtio_vga);
 #endif
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/endianness-test.c b/tests/endianness-test.c
index 2197972..b7a120e 100644
--- a/tests/endianness-test.c
+++ b/tests/endianness-test.c
@@ -282,7 +282,6 @@ static void test_endianness_combine(gconstpointer data)
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
-    int ret;
     int i;
 
     g_test_init(&argc, &argv, NULL);
@@ -305,7 +304,5 @@ int main(int argc, char **argv)
         qtest_add_data_func(path, &test_cases[i], test_endianness_combine);
     }
 
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index bff999c..fa4f1a0 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -395,7 +395,6 @@ static void request_pflash(FirmwareTestFixture *fixture,
 int main(int argc, char **argv)
 {
     TestData data;
-    int ret;
 
     g_test_init(&argc, &argv, NULL);
 
@@ -406,6 +405,5 @@ int main(int argc, char **argv)
     add_firmware_test("i440fx/firmware/bios", request_bios);
     add_firmware_test("i440fx/firmware/pflash", request_pflash);
 
-    ret = g_test_run();
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/intel-hda-test.c b/tests/intel-hda-test.c
index b0ca7e0..b782b2e 100644
--- a/tests/intel-hda-test.c
+++ b/tests/intel-hda-test.c
@@ -31,13 +31,9 @@ static void ich9_test(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/intel-hda/ich6", ich6_test);
     qtest_add_func("/intel-hda/ich9", ich9_test);
 
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index 280e4b6..c63b68f 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -209,12 +209,8 @@ static void test_redirector_rx(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/netfilter/redirector_tx", test_redirector_tx);
     qtest_add_func("/netfilter/redirector_rx", test_redirector_rx);
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 8272ba8..06dd7fc 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -763,7 +763,6 @@ static void mmio_basic(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
     const char *arch = qtest_get_arch();
 
     g_test_init(&argc, &argv, NULL);
@@ -779,7 +778,5 @@ int main(int argc, char **argv)
         qtest_add_func("/virtio/blk/mmio/basic", mmio_basic);
     }
 
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/virtio-console-test.c b/tests/virtio-console-test.c
index 6d6414d..1c3de07 100644
--- a/tests/virtio-console-test.c
+++ b/tests/virtio-console-test.c
@@ -27,13 +27,9 @@ static void serialport_pci_nop(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/console/pci/nop", console_pci_nop);
     qtest_add_func("/virtio/serialport/pci/nop", serialport_pci_nop);
 
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index e5c1448..f4f6a4a 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -248,8 +248,6 @@ static void hotplug(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
 #ifndef _WIN32
     qtest_add_data_func("/virtio/net/pci/basic", send_recv_test, pci_basic);
@@ -258,7 +256,5 @@ int main(int argc, char **argv)
 #endif
     qtest_add_func("/virtio/net/pci/hotplug", hotplug);
 
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 5f1a8ae..b712652 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -260,15 +260,11 @@ static void test_unaligned_write_same(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/scsi/pci/nop", pci_nop);
     qtest_add_func("/virtio/scsi/pci/hotplug", hotplug);
     qtest_add_func("/virtio/scsi/pci/scsi-disk/unaligned-write-same",
                    test_unaligned_write_same);
 
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/tests/wdt_ib700-test.c b/tests/wdt_ib700-test.c
index 9c1d78b..49f4f0c 100644
--- a/tests/wdt_ib700-test.c
+++ b/tests/wdt_ib700-test.c
@@ -117,15 +117,11 @@ static void ib700_none(void)
 
 int main(int argc, char **argv)
 {
-    int ret;
-
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/wdt_ib700/pause", ib700_pause);
     qtest_add_func("/wdt_ib700/reset", ib700_reset);
     qtest_add_func("/wdt_ib700/shutdown", ib700_shutdown);
     qtest_add_func("/wdt_ib700/none", ib700_none);
 
-    ret = g_test_run();
-
-    return ret;
+    return g_test_run();
 }
diff --git a/ui/cursor.c b/ui/cursor.c
index a276e01..5155b39 100644
--- a/ui/cursor.c
+++ b/ui/cursor.c
@@ -81,18 +81,12 @@ void cursor_print_ascii_art(QEMUCursor *c, const char *prefix)
 
 QEMUCursor *cursor_builtin_hidden(void)
 {
-    QEMUCursor *c;
-
-    c = cursor_parse_xpm(cursor_hidden_xpm);
-    return c;
+    return cursor_parse_xpm(cursor_hidden_xpm);
 }
 
 QEMUCursor *cursor_builtin_left_ptr(void)
 {
-    QEMUCursor *c;
-
-    c = cursor_parse_xpm(cursor_left_ptr_xpm);
-    return c;
+    return cursor_parse_xpm(cursor_left_ptr_xpm);
 }
 
 QEMUCursor *cursor_alloc(int width, int height)
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index c9f8dce..25310b5 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -180,14 +180,9 @@ void qemu_pixman_linebuf_copy(pixman_image_t *fb, int width, int x, int y,
 pixman_image_t *qemu_pixman_mirror_create(pixman_format_code_t format,
                                           pixman_image_t *image)
 {
-    pixman_image_t *mirror;
-
-    mirror = pixman_image_create_bits(format,
-                                      pixman_image_get_width(image),
-                                      pixman_image_get_height(image),
-                                      NULL,
-                                      pixman_image_get_stride(image));
-    return mirror;
+    return pixman_image_create_bits(format, pixman_image_get_width(image),
+                                    pixman_image_get_height(image), NULL,
+                                    pixman_image_get_stride(image));
 }
 
 void qemu_pixman_image_unref(pixman_image_t *image)
diff --git a/util/module.c b/util/module.c
index ce058ae..86e3f7a 100644
--- a/util/module.c
+++ b/util/module.c
@@ -55,13 +55,9 @@ static void init_lists(void)
 
 static ModuleTypeList *find_type(module_init_type type)
 {
-    ModuleTypeList *l;
-
     init_lists();
 
-    l = &init_type_list[type];
-
-    return l;
+    return &init_type_list[type];
 }
 
 void register_module_init(void (*fn)(void), module_init_type type)
diff --git a/vl.c b/vl.c
index b0bcc25..ba06d27 100644
--- a/vl.c
+++ b/vl.c
@@ -2361,10 +2361,7 @@ static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp)
 #ifdef CONFIG_VIRTFS
 static int fsdev_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
-    int ret;
-    ret = qemu_fsdev_add(opts);
-
-    return ret;
+    return qemu_fsdev_add(opts);
 }
 #endif
 
-- 
2.5.5

Re: [Qemu-devel] [RFC v2 3/3] Remove unnecessary variables for function return value

[Eric Blake]

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

On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> Use Coccinelle script to replace 'ret = E; return ret' with
> 'return E'. The script will do the substitution only when the
> function return type and variable type are the same.
> 
> Sending as RFC because the patch looks more intrusive than the
> others. Probably better to split it per subsystem and let each
> maintainer review and apply it?

Borderline on size, so yeah, splitting it across several subsystems may
ease review (although then the patch will be committed in piecemeal
fashion, and you'd have to ensure the script/coccinelle/ patch goes in
first...)

At any rate, it's fairly mechanical, so I'll review it as is:

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

>  47 files changed, 90 insertions(+), 254 deletions(-)
>  create mode 100644 scripts/coccinelle/return_directly.cocci

Nice diffstat.

> +++ b/block/qcow2-cluster.c
> @@ -154,11 +154,8 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
>      uint64_t **l2_table)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    int ret;
> -
> -    ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset, (void**) l2_table);
> -
> -    return ret;
> +    return qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
> +                           (void **)l2_table);

Coccinelle changed spacing of the cast. I don't care strongly enough to
require a touchup if this is the only thing, but may be worth fixing if
you have to respin (for example to split up by submaintainers).

> +++ b/block/raw_bsd.c
> @@ -190,10 +190,7 @@ static int raw_has_zero_init(BlockDriverState *bs)
>  
>  static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
>  {
> -    int ret;
> -
> -    ret = bdrv_create_file(filename, opts, errp);
> -    return ret;
> +    return bdrv_create_file(filename, opts, errp);
>  }

Potential followup patch: delete raw_create(), and:
- .bdrv_create = &raw_create,
+ .bdrv_create = bdrv_create_file,

but doesn't affect this patch.

> +++ b/block/rbd.c
> @@ -875,10 +875,7 @@ static int qemu_rbd_snap_rollback(BlockDriverState *bs,
>                                    const char *snapshot_name)
>  {
>      BDRVRBDState *s = bs->opaque;
> -    int r;
> -
> -    r = rbd_snap_rollback(s->image, snapshot_name);
> -    return r;
> +    return rbd_snap_rollback(s->image, snapshot_name);

Coccinelle lost the blank line between declarations and statements;
might be nice to manually touch that up and add it back in.

> +++ b/hw/ppc/spapr_vio.c
> @@ -57,12 +57,7 @@ static char *spapr_vio_get_dev_name(DeviceState *qdev)
>  {
>      VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
>      VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> -    char *name;
> -
> -    /* Device tree style name device@reg */
> -    name = g_strdup_printf("%s@%x", pc->dt_name, dev->reg);
> -
> -    return name;
> +    return g_strdup_printf("%s@%x", pc->dt_name, dev->reg);

Coccinelle lost the comment; might be worth keeping it.

> +++ b/hw/scsi/megasas.c
> @@ -410,17 +410,9 @@ static void megasas_encode_lba(uint8_t *cdb, uint64_t lba,
>  static uint64_t megasas_fw_time(void)
>  {
>      struct tm curtime;
> -    uint64_t bcd_time;
>  
>      qemu_get_timedate(&curtime, 0);
> -    bcd_time = ((uint64_t)curtime.tm_sec & 0xff) << 48 |
> -        ((uint64_t)curtime.tm_min & 0xff)  << 40 |
> -        ((uint64_t)curtime.tm_hour & 0xff) << 32 |
> -        ((uint64_t)curtime.tm_mday & 0xff) << 24 |
> -        ((uint64_t)curtime.tm_mon & 0xff)  << 16 |
> -        ((uint64_t)(curtime.tm_year + 1900) & 0xffff);
> -
> -    return bcd_time;
> +    return ((uint64_t)curtime.tm_sec & 0xff) << 48 | ((uint64_t)curtime.tm_min & 0xff) << 40 | ((uint64_t)curtime.tm_hour & 0xff) << 32 | ((uint64_t)curtime.tm_mday & 0xff) << 24 | ((uint64_t)curtime.tm_mon & 0xff) << 16 | ((uint64_t)(curtime.tm_year + 1900) & 0xffff);

Eww. Coccinelle botched that formatting.  You'll need to manually fix
this one.

> +++ b/hw/timer/mc146818rtc.c
> @@ -105,12 +105,9 @@ static inline bool rtc_running(RTCState *s)
>  
>  static uint64_t get_guest_rtc_ns(RTCState *s)
>  {
> -    uint64_t guest_rtc;
>      uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
>  
> -    guest_rtc = s->base_rtc * NANOSECONDS_PER_SECOND +
> -        guest_clock - s->last_update + s->offset;
> -    return guest_rtc;
> +    return s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - s->last_update + s->offset;
>  }

Worth wrapping that line again (not as bad as the megasas one, though).

> +++ b/qga/commands-win32.c
> @@ -1150,7 +1150,6 @@ out:
>  int64_t qmp_guest_get_time(Error **errp)
>  {
>      SYSTEMTIME ts = {0};
> -    int64_t time_ns;
>      FILETIME tf;
>  
>      GetSystemTime(&ts);
> @@ -1164,10 +1163,7 @@ int64_t qmp_guest_get_time(Error **errp)
>          return -1;
>      }
>  
> -    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
> -                - W32_FT_OFFSET) * 100;
> -
> -    return time_ns;
> +    return ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100;

and again

> +++ b/scripts/coccinelle/return_directly.cocci
> @@ -0,0 +1,19 @@
> +// replace 'R = X; return R;' with 'return R;'
> +
> +// remove assignment
> +@ removal @
> +identifier VAR;
> +expression E;
> +type T;
> +identifier F;
> +@@
> + T F(...)
> + {
> +     ...
> +-    T VAR;
> +     ... when != VAR
> +-    VAR = E;
> +-    return VAR;
> ++    return E;
> +     ... when != VAR
> + }

Looks reasonable; I like that you constrained the type to be the same
(so that we aren't having to also worry about promotion rules while
reviewing it).

> +++ b/target-tricore/op_helper.c
> @@ -2117,7 +2117,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2)
>      int32_t eq_pos = x_sign & ((r1 >> 32) == r2);
>      int32_t eq_neg = x_sign & ((r1 >> 32) == -r2);
>      uint32_t quotient;
> -    uint64_t ret, remainder;
> +    uint64_t remainder;
>  
>      if ((q_sign & ~eq_neg) | eq_pos) {
>          quotient = (r1 + 1) & 0xffffffff;
> @@ -2130,8 +2130,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2)
>      } else {
>          remainder = (r1 & 0xffffffff00000000ull);
>      }
> -    ret = remainder|quotient;
> -    return ret;
> +    return remainder | quotient;

Coccinelle fixed a checkpatch violation :)

Minor tweaks, and your idea of splitting may be worth while, but all the
changes look correct, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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] [RFC v2 3/3] Remove unnecessary variables for function return value

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> Use Coccinelle script to replace 'ret = E; return ret' with
> 'return E'. The script will do the substitution only when the
> function return type and variable type are the same.
>
> Sending as RFC because the patch looks more intrusive than the
> others. Probably better to split it per subsystem and let each
> maintainer review and apply it?

As far as I'm concerned, obvious mechanical cleanups like this one can
go in as a single tree-wide patch.  I'd consider making you split it up,
then chase maintainers a waste of your time[*].

checkpatch.pl is unhappy:

529: WARNING: line over 80 characters
563: WARNING: line over 80 characters
695: WARNING: line over 80 characters
811: ERROR: return is not a function, parentheses are not required
830: ERROR: return is not a function, parentheses are not required
849: ERROR: return is not a function, parentheses are not required


[*] Been there, done that, but if it is what it takes...

Re: [Qemu-devel] [RFC v2 3/3] Remove unnecessary variables for function return value

[Eduardo Habkost]

On Mon, Jun 13, 2016 at 01:29:47PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > Use Coccinelle script to replace 'ret = E; return ret' with
> > 'return E'. The script will do the substitution only when the
> > function return type and variable type are the same.
> >
> > Sending as RFC because the patch looks more intrusive than the
> > others. Probably better to split it per subsystem and let each
> > maintainer review and apply it?
> 
> As far as I'm concerned, obvious mechanical cleanups like this one can
> go in as a single tree-wide patch.  I'd consider making you split it up,
> then chase maintainers a waste of your time[*].

Not wasting my time sounds like a good idea. :)

Once the issues below are fixed and Eric's comments are
addressed, should it go through your error reporting tree?

> 
> checkpatch.pl is unhappy:
> 
> 529: WARNING: line over 80 characters
> 563: WARNING: line over 80 characters
> 695: WARNING: line over 80 characters
> 811: ERROR: return is not a function, parentheses are not required
> 830: ERROR: return is not a function, parentheses are not required
> 849: ERROR: return is not a function, parentheses are not required
> 
> 
> [*] Been there, done that, but if it is what it takes...

-- 
Eduardo

Re: [Qemu-devel] [RFC v2 3/3] Remove unnecessary variables for function return value

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Jun 13, 2016 at 01:29:47PM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > Use Coccinelle script to replace 'ret = E; return ret' with
>> > 'return E'. The script will do the substitution only when the
>> > function return type and variable type are the same.
>> >
>> > Sending as RFC because the patch looks more intrusive than the
>> > others. Probably better to split it per subsystem and let each
>> > maintainer review and apply it?
>> 
>> As far as I'm concerned, obvious mechanical cleanups like this one can
>> go in as a single tree-wide patch.  I'd consider making you split it up,
>> then chase maintainers a waste of your time[*].
>
> Not wasting my time sounds like a good idea. :)
>
> Once the issues below are fixed and Eric's comments are
> addressed, should it go through your error reporting tree?

Yes.