Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls
On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote: [...] > > 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. If it happens immediately before the function end or a return statement it should be easy, but it would still require some manual work to remove the unused variable declaration. Probably easier to do that in a follow-up patch. It's harder (impossible?) to make Coccinelle avoid matching if local_err is used somewhere else in the function. But it's probably doable with some manual work, in a follow-up patch. -- Eduardo
Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls
On Thu, Jun 09, 2016 at 02:11:23PM -0600, Eric Blake wrote: > On 06/09/2016 01:50 PM, Eduardo Habkost wrote: > > On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote: > > [...] > >> > >> 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. > > > > If it happens immediately before the function end or a return > > statement it should be easy, but it would still require some > > manual work to remove the unused variable declaration. Probably > > easier to do that in a follow-up patch. > > I think Coccinelle can be used to eliminate unused local variables, but > don't know the recipe off-hand; maybe a web search will turn up something? I found something called "when constraints", but the documentation isn't very clear. There's an example here: http://coccinelle.lip6.fr/rules/unused.cocci > > > > > It's harder (impossible?) to make Coccinelle avoid matching if > > local_err is used somewhere else in the function. But it's > > probably doable with some manual work, in a follow-up patch. > > I don't know - Coccinelle is rather powerful, and there may indeed be a > way to flag conditions for a variable that is not used anywhere except > in the lines mentioned in the recipe, vs. a variable that can also be > used in the ... portion of the recipe. I found a hackish way to do that. Sending a follow-up patch in 1 minute. -- Eduardo
Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls
On 06/09/2016 01:50 PM, Eduardo Habkost wrote: > On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote: > [...] >> >> 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. > > If it happens immediately before the function end or a return > statement it should be easy, but it would still require some > manual work to remove the unused variable declaration. Probably > easier to do that in a follow-up patch. I think Coccinelle can be used to eliminate unused local variables, but don't know the recipe off-hand; maybe a web search will turn up something? > > It's harder (impossible?) to make Coccinelle avoid matching if > local_err is used somewhere else in the function. But it's > probably doable with some manual work, in a follow-up patch. I don't know - Coccinelle is rather powerful, and there may indeed be a way to flag conditions for a variable that is not used anywhere except in the lines mentioned in the recipe, vs. a variable that can also be used in the ... portion of the recipe. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls
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
> ---
> 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/bloc
