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

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

[Tuguoyi]

Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
---
 block/qcow2.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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);
-- 
2.7.4

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 <tu.guoyi@h3c.com>

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

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

[Vladimir Sementsov-Ogievskiy]

18.12.2019 5:26, Tuguoyi wrote:
> 
> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/qcow2.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> 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);
> 


-- 
Best regards,
Vladimir

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

[Tuguoyi]

On 18.12.2019 17:33 Kevin Wolf wrote:
> 
> Am 18.12.2019 um 03:26 hat Tuguoyi geschrieben:
> >
> > Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
> 
> 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

Thanks for pointing it out, I will send the v2 patch

--
Best regards,
Guoyi