| Message ID | c9d3fc7246ec4c1f9a6cde9ed3c3ae57@h3c.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: Move error check of local_err near its assignment | expand |
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
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); >
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
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);
Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com> --- block/qcow2.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)