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

* [Qemu-devel] [PATCH v3 0/3] coccinelle: Clean up error checks and return value variables
@ 2016-06-13 21:57 Eduardo Habkost
  2016-06-13 21:57 ` [Qemu-devel] [PATCH v3 1/3] error: Remove NULL checks on error_propagate() calls Eduardo Habkost
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Eduardo Habkost @ 2016-06-13 21:57 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel, Eric Blake; +Cc: kwolf, qemu-block, mreitz

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.

Changes v2 -> v3 on patch 2/3:
* Remove unused metavariable from script
 * Do changes only if errp is not touched before the error_setg()
   call (so we are sure *errp is not set and error_setg() won't
   abort)
 * Changes dropped from v2:
   * block.c:bdrv_create_co_entry()
   * block.c:bdrv_create_file()
   * blockdev.c:qmp_blockdev_mirror()

Changes v2 -> v3 on patch 3/3:
* Not a RFC anymore
* Used --keep-comments option
* Instead of using:
    - VAR = E;
    + return E;
  use:
    - VAR =
    + return
      E
  This makes Coccinelle keep the existing formatting on some
  cases.
* Manual fixups

Diff from v2 below:

  diff --git a/scripts/coccinelle/remove_local_err.cocci b/scripts/coccinelle/remove_local_err.cocci
  index 5f0b6c0..9261c99 100644
  --- a/scripts/coccinelle/remove_local_err.cocci
  +++ b/scripts/coccinelle/remove_local_err.cocci
  @@ -2,18 +2,20 @@
   // direct usage of errp argument

   @@
  +identifier F;
   expression list ARGS;
   expression F2;
   identifier LOCAL_ERR;
  -expression ERRP;
  +identifier ERRP;
   idexpression V;
   typedef Error;
  -expression I;
   @@
  + F(..., Error **ERRP)
    {
        ...
   -    Error *LOCAL_ERR;
        ... when != LOCAL_ERR
  +         when != ERRP
   (
   -    F2(ARGS, &LOCAL_ERR);
   -    error_propagate(ERRP, LOCAL_ERR);
  diff --git a/scripts/coccinelle/return_directly.cocci b/scripts/coccinelle/return_directly.cocci
  index 7b0b6ac..c52f4fc 100644
  --- a/scripts/coccinelle/return_directly.cocci
  +++ b/scripts/coccinelle/return_directly.cocci
  @@ -12,8 +12,10 @@ identifier F;
        ...
   -    T VAR;
        ... when != VAR
  --    VAR = E;
  +
  +-    VAR =
  ++    return
  +     E;
   -    return VAR;
  -+    return E;
        ... when != VAR
    }
  diff --git a/block.c b/block.c
  index c537307..ecca55a 100644
  --- a/block.c
  +++ b/block.c
  @@ -294,12 +294,14 @@ 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, &cco->err);
  +    ret = cco->drv->bdrv_create(cco->filename, cco->opts, &local_err);
  +    error_propagate(&cco->err, local_err);
       cco->ret = ret;
   }

  @@ -351,13 +353,17 @@ 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);
       if (drv == NULL) {
           return -ENOENT;
       }

  -    return bdrv_create(drv, filename, opts, errp);
  +    ret = bdrv_create(drv, filename, opts, &local_err);
  +    error_propagate(errp, local_err);
  +    return ret;
   }

   /**
  diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
  index e0e5d9e..afc8d6b 100644
  --- a/block/qcow2-cluster.c
  +++ b/block/qcow2-cluster.c
  @@ -155,7 +155,7 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
   {
       BDRVQcow2State *s = bs->opaque;
       return qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
  -                           (void **)l2_table);
  +                           (void **) l2_table);
   }

   /*
  diff --git a/blockdev.c b/blockdev.c
  index 3b6d242..028dba3 100644
  --- a/blockdev.c
  +++ b/blockdev.c
  @@ -3654,6 +3654,7 @@ 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) {
  @@ -3677,11 +3678,16 @@ 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, errp);
  +    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);

       aio_context_release(aio_context);
   }
  diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
  index a0ccb61..ae40db8 100644
  --- a/hw/ppc/spapr_vio.c
  +++ b/hw/ppc/spapr_vio.c
  @@ -57,6 +57,8 @@ static char *spapr_vio_get_dev_name(DeviceState *qdev)
   {
       VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
       VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
  +
  +    /* Device tree style name device@reg */
       return g_strdup_printf("%s@%x", pc->dt_name, dev->reg);
   }

  diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
  index 178c4e7..d177218 100644
  --- a/hw/scsi/megasas.c
  +++ b/hw/scsi/megasas.c
  @@ -412,7 +412,12 @@ static uint64_t megasas_fw_time(void)
       struct tm curtime;

       qemu_get_timedate(&curtime, 0);
  -    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);
  +    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/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
  index c212990..f4e333e 100644
  --- a/hw/timer/mc146818rtc.c
  +++ b/hw/timer/mc146818rtc.c
  @@ -107,7 +107,8 @@ static uint64_t get_guest_rtc_ns(RTCState *s)
   {
       uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);

  -    return s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - s->last_update + s->offset;
  +    return s->base_rtc * NANOSECONDS_PER_SECOND +
  +        guest_clock - s->last_update + s->offset;
   }

   #ifdef TARGET_I386
  diff --git a/qga/commands-win32.c b/qga/commands-win32.c
  index b00a891..9c9be12 100644
  --- a/qga/commands-win32.c
  +++ b/qga/commands-win32.c
  @@ -1163,7 +1163,8 @@ int64_t qmp_guest_get_time(Error **errp)
           return -1;
       }

  -    return ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100;
  +    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/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
  index 7cbda0b..df7d220 100644
  --- a/target-mips/dsp_helper.c
  +++ b/target-mips/dsp_helper.c
  @@ -3274,11 +3274,14 @@ 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);
  -    return (temp[1] << 63) | (temp[0] >> 1);
  +    result = (temp[1] << 63) | (temp[0] >> 1);
  +
  +    return result;
   }

   target_ulong helper_dextr_r_l(target_ulong ac, target_ulong shift,
  @@ -3286,6 +3289,7 @@ 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);
  @@ -3305,7 +3309,9 @@ target_ulong helper_dextr_r_l(target_ulong ac, target_ulong shift,
           set_DSPControl_overflow_flag(1, 23, env);
       }

  -    return (temp[1] << 63) | (temp[0] >> 1);
  +    result = (temp[1] << 63) | (temp[0] >> 1);
  +
  +    return result;
   }

   target_ulong helper_dextr_rs_l(target_ulong ac, target_ulong shift,
  @@ -3313,6 +3319,7 @@ 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);
  @@ -3338,7 +3345,9 @@ target_ulong helper_dextr_rs_l(target_ulong ac, target_ulong shift,
           }
           set_DSPControl_overflow_flag(1, 23, env);
       }
  -    return (temp[1] << 63) | (temp[0] >> 1);
  +    result = (temp[1] << 63) | (temp[0] >> 1);
  +
  +    return result;
   }
   #endif

  diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
  index 25310b5..6e8b83a 100644
  --- a/ui/qemu-pixman.c
  +++ b/ui/qemu-pixman.c
  @@ -180,8 +180,10 @@ 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)
   {
  -    return pixman_image_create_bits(format, pixman_image_get_width(image),
  -                                    pixman_image_get_height(image), NULL,
  +    return pixman_image_create_bits(format,
  +                                    pixman_image_get_width(image),
  +                                    pixman_image_get_height(image),
  +                                    NULL,
                                       pixman_image_get_stride(image));
   }

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

 scripts/coccinelle/error_propagate_null.cocci | 10 +++++++
 scripts/coccinelle/remove_local_err.cocci     | 29 ++++++++++++++++++
 scripts/coccinelle/return_directly.cocci      | 21 ++++++++++++++
 audio/audio.c                                 | 10 ++-----
 block.c                                       | 20 ++++---------
 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                                   |  4 +--
 block/snapshot.c                              |  4 +--
 block/vmdk.c                                  |  6 ++--
 block/vvfat.c                                 |  5 +---
 blockdev.c                                    | 12 ++------
 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                            |  5 +---
 hw/s390x/s390-virtio-ccw.c                    |  5 +---
 hw/s390x/virtio-ccw.c                         | 42 +++++----------------------
 hw/scsi/megasas.c                             |  5 +---
 hw/scsi/scsi-generic.c                        |  5 +---
 hw/timer/mc146818rtc.c                        |  4 +--
 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                          | 13 ++-------
 qobject/qlist.c                               |  5 +---
 qom/object.c                                  |  4 +--
 target-i386/cpu.c                             |  4 +--
 target-i386/fpu_helper.c                      | 10 ++-----
 target-i386/kvm.c                             |  5 ++--
 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                              | 13 ++++-----
 util/module.c                                 |  6 +---
 vl.c                                          |  5 +---
 61 files changed, 159 insertions(+), 346 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

^ permalink raw reply	[flat|nested] 14+ messages in thread