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

[Cornelia Huck]

On Fri, 10 Jun 2016 17:12:17 -0300
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 
> ---
>  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 

>  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-block] [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 
> > ---
> >  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, _err);
> > -error_propagate(>err, local_err);
> > +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >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-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Eric Blake]

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 
> ---
>  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, _err);
> -error_propagate(>err, local_err);
> +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >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,
> -   _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, _ERR);
> +-error_propagate(ERRP, LOCAL_ERR);
> ++F2(ARGS, ERRP);
> +|
> +-V = F2(ARGS, _ERR);
> +-error_propagate(ERRP, LOCAL_ERR);
> ++V = F2(ARGS, ERRP);
> +)
> + ... when != LOCAL_ERR
> + }

Looks good.
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature