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

[Eric Blake <eblake@redhat.com>]

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

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

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

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

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

and ditch local_err and ret altogether.

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

Another place where we could ditch local_err, and directly call

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

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

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

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

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

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

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

 finish:
+    error_propagate(errp, local_err);

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

Another case where we could ditch local_err.

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

and another.

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

and another

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

and another

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

and another

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

and another.

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

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


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