Re: [PATCH] qcow2: Move error check of local_err near its assignment

[Kevin Wolf]

Am 18.12.2019 um 03:26 hat Tuguoyi geschrieben:
> 
> Signed-off-by: Guoyi Tu <address@hidden>

Empty commit messages are rarely acceptable. You should explain here
why you are making the change and why it's correct (for example by
comparing with when it was introduced).

In this case, the local_err check outside of the if block was necessary
when it was introduced in commit d1258dd0c87 because it needed to be
executed even if qcow2_load_autoloading_dirty_bitmaps() returned false.

After some modifications that all required the error check to remain
where it is, commit 9c98f145dfb finally moved the
qcow2_load_dirty_bitmaps() call into the if block, so now the error
check should be there, too.

This is information that should be in the commit message rather than
requiring each reader to do the research.

Please also make sure to CC the author of the code that you're
modifying, in this case Vladimir.

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7c18721..ce3db29 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1705,14 +1705,14 @@ static int coroutine_fn 
> qcow2_do_open(BlockDriverState *bs, QDict *options,
>      if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
>          /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
>          bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>  
>          update_header = update_header && !header_updated;
>      }
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        ret = -EINVAL;
> -        goto fail;
> -    }
>  
>      if (update_header) {
>          ret = qcow2_update_header(bs);

The change itself looks good to me, but I'll let Vladimir have a look as
well. If there are no more comments, I'm looking forward to a v2 patch
with a non-empty commit message.

Kevin