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.gu...@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
