| Message ID | 20200909185930.26524-1-vsementsov@virtuozzo.com |
|---|---|
| Headers | show |
| Series | block: deal with errp: part I | expand |
On Wed, 9 Sep 2020 21:59:18 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > Now bdrv_append returns status and we can drop all the local_err things > around it. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> Just one suggestion for a follow-up below... > block.c | 5 +---- > block/backup-top.c | 20 ++++++++------------ > block/commit.c | 5 +---- > block/mirror.c | 6 ++---- > blockdev.c | 4 +--- > tests/test-bdrv-graph-mod.c | 6 +++--- > 6 files changed, 16 insertions(+), 30 deletions(-) > > diff --git a/block.c b/block.c > index 6d35449027..9b624b2535 100644 > --- a/block.c > +++ b/block.c > @@ -3156,7 +3156,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, > int64_t total_size; > QemuOpts *opts = NULL; > BlockDriverState *bs_snapshot = NULL; > - Error *local_err = NULL; > int ret; > > /* if snapshot, we create a temporary backing file and open it > @@ -3203,9 +3202,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, > * order to be able to return one, we have to increase > * bs_snapshot's refcount here */ > bdrv_ref(bs_snapshot); > - bdrv_append(bs_snapshot, bs, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + if (bdrv_append(bs_snapshot, bs, errp) < 0) { > bs_snapshot = NULL; > goto out; > } > diff --git a/block/backup-top.c b/block/backup-top.c > index af2f20f346..de9d5e1634 100644 > --- a/block/backup-top.c > +++ b/block/backup-top.c > @@ -192,7 +192,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, > BlockCopyState **bcs, > Error **errp) > { > - Error *local_err = NULL; > + ERRP_GUARD(); > BDRVBackupTopState *state; > BlockDriverState *top; > bool appended = false; > @@ -225,9 +225,8 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, > bdrv_drained_begin(source); > > bdrv_ref(top); > - bdrv_append(top, source, &local_err); > - if (local_err) { > - error_prepend(&local_err, "Cannot append backup-top filter: "); > + if (bdrv_append(top, source, errp) < 0) { > + error_prepend(errp, "Cannot append backup-top filter: "); > goto fail; > } > appended = true; > @@ -237,18 +236,16 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, > * we want. > */ > state->active = true; > - bdrv_child_refresh_perms(top, top->backing, &local_err); > - if (local_err) { > - error_prepend(&local_err, > - "Cannot set permissions for backup-top filter: "); > + if (bdrv_child_refresh_perms(top, top->backing, errp) < 0) { > + error_prepend(errp, "Cannot set permissions for backup-top filter: "); > goto fail; > } > > state->cluster_size = cluster_size; > state->bcs = block_copy_state_new(top->backing, state->target, > - cluster_size, write_flags, &local_err); > - if (local_err) { > - error_prepend(&local_err, "Cannot create block-copy-state: "); > + cluster_size, write_flags, errp); > + if (!state->bcs) { > + error_prepend(errp, "Cannot create block-copy-state: "); > goto fail; > } > *bcs = state->bcs; > @@ -266,7 +263,6 @@ fail: > } > > bdrv_drained_end(source); > - error_propagate(errp, local_err); > > return NULL; > } > diff --git a/block/commit.c b/block/commit.c > index 7732d02dfe..7720d4729b 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs, > CommitBlockJob *s; > BlockDriverState *iter; > BlockDriverState *commit_top_bs = NULL; > - Error *local_err = NULL; > int ret; > ... this is unrelated but while reviewing I've noticed that the ret variable isn't really needed. > assert(top != bs); > @@ -292,10 +291,8 @@ void commit_start(const char *job_id, BlockDriverState *bs, > > commit_top_bs->total_sectors = top->total_sectors; > > - bdrv_append(commit_top_bs, top, &local_err); > - if (local_err) { > + if (bdrv_append(commit_top_bs, top, errp) < 0) { > commit_top_bs = NULL; > - error_propagate(errp, local_err); > goto fail; > } > > diff --git a/block/mirror.c b/block/mirror.c > index e8e8844afc..ca250f765d 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1557,7 +1557,6 @@ static BlockJob *mirror_start_job( > BlockDriverState *mirror_top_bs; > bool target_graph_mod; > bool target_is_backing; > - Error *local_err = NULL; > int ret; > > if (granularity == 0) { > @@ -1606,12 +1605,11 @@ static BlockJob *mirror_start_job( > * it alive until block_job_create() succeeds even if bs has no parent. */ > bdrv_ref(mirror_top_bs); > bdrv_drained_begin(bs); > - bdrv_append(mirror_top_bs, bs, &local_err); > + ret = bdrv_append(mirror_top_bs, bs, errp); > bdrv_drained_end(bs); > > - if (local_err) { > + if (ret < 0) { > bdrv_unref(mirror_top_bs); > - error_propagate(errp, local_err); > return NULL; > } > > diff --git a/blockdev.c b/blockdev.c > index 3848a9c8ab..36bef6b188 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1576,9 +1576,7 @@ static void external_snapshot_prepare(BlkActionState *common, > * can fail, so we need to do it in .prepare; undoing it for abort is > * always possible. */ > bdrv_ref(state->new_bs); > - bdrv_append(state->new_bs, state->old_bs, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + if (bdrv_append(state->new_bs, state->old_bs, errp) < 0) { > goto out; > } > state->overlay_appended = true; > diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c > index 8cff13830e..2b71601c24 100644 > --- a/tests/test-bdrv-graph-mod.c > +++ b/tests/test-bdrv-graph-mod.c > @@ -101,7 +101,7 @@ static BlockDriverState *pass_through_node(const char *name) > */ > static void test_update_perm_tree(void) > { > - Error *local_err = NULL; > + int ret; > > BlockBackend *root = blk_new(qemu_get_aio_context(), > BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ, > @@ -114,8 +114,8 @@ static void test_update_perm_tree(void) > bdrv_attach_child(filter, bs, "child", &child_of_bds, > BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort); > > - bdrv_append(filter, bs, &local_err); > - error_free_or_abort(&local_err); > + ret = bdrv_append(filter, bs, NULL); > + assert(ret < 0); > > blk_unref(root); > }
On Wed, 9 Sep 2020 21:59:25 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > Don't use error propagation in qcow2_get_specific_info(). For this > refactor qcow2_get_bitmap_info_list, its current interface rather ... interface is rather > weird. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > block/qcow2.h | 4 ++-- > block/qcow2-bitmap.c | 27 +++++++++++++-------------- > block/qcow2.c | 10 +++------- > 3 files changed, 18 insertions(+), 23 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 065ec3df0b..ac6a2d3e2a 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -967,8 +967,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > void **refcount_table, > int64_t *refcount_table_size); > bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); > -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, > - Error **errp); > +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > + Qcow2BitmapInfoList **info_list, Error **errp); > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 8c34b2aef7..9b14c0791f 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1090,30 +1090,29 @@ static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags) > /* > * qcow2_get_bitmap_info_list() > * Returns a list of QCOW2 bitmap details. > - * In case of no bitmaps, the function returns NULL and > - * the @errp parameter is not set. > - * When bitmap information can not be obtained, the function returns > - * NULL and the @errp parameter is set. > + * On success return true with bm_list set (probably to NULL, if no bitmaps), > + * on failure return false with errp set. > */ > -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, > - Error **errp) > +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > + Qcow2BitmapInfoList **info_list, Error **errp) > { > BDRVQcow2State *s = bs->opaque; > Qcow2BitmapList *bm_list; > Qcow2Bitmap *bm; > - Qcow2BitmapInfoList *list = NULL; > - Qcow2BitmapInfoList **plist = &list; > > if (s->nb_bitmaps == 0) { > - return NULL; > + *info_list = NULL; > + return true; > } > > bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > s->bitmap_directory_size, errp); > - if (bm_list == NULL) { > - return NULL; > + if (!bm_list) { > + return false; > } > > + *info_list = NULL; > + > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1); > Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1); > @@ -1121,13 +1120,13 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, > info->name = g_strdup(bm->name); > info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS); > obj->value = info; > - *plist = obj; > - plist = &obj->next; > + *info_list = obj; > + info_list = &obj->next; > } > > bitmap_list_free(bm_list); > > - return list; > + return true; > } > > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) > diff --git a/block/qcow2.c b/block/qcow2.c > index 10175fa399..eb7c82120c 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -5056,12 +5056,10 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs, > BDRVQcow2State *s = bs->opaque; > ImageInfoSpecific *spec_info; > QCryptoBlockInfo *encrypt_info = NULL; > - Error *local_err = NULL; > > if (s->crypto != NULL) { > - encrypt_info = qcrypto_block_get_info(s->crypto, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + encrypt_info = qcrypto_block_get_info(s->crypto, errp); > + if (!encrypt_info) { > return NULL; > } > } > @@ -5078,9 +5076,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs, > }; > } else if (s->qcow_version == 3) { > Qcow2BitmapInfoList *bitmaps; > - bitmaps = qcow2_get_bitmap_info_list(bs, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + if (!qcow2_get_bitmap_info_list(bs, &bitmaps, errp)) { > qapi_free_ImageInfoSpecific(spec_info); > qapi_free_QCryptoBlockInfo(encrypt_info); > return NULL;
s/startus/status On Wed, 9 Sep 2020 21:59:27 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > It's better to return status together with setting errp. It makes > possible to avoid error propagation. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 2 +- > block/qcow2-bitmap.c | 13 ++++++------- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index e7e662533b..49824be5c6 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > Qcow2BitmapInfoList **info_list, Error **errp); > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); > -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > bool release_stored, Error **errp); > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); > bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index f58923fce3..5eeff1cb1c 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1524,9 +1524,10 @@ out: > * readonly to begin with, and whether we opened directly or reopened to that > * state shouldn't matter for the state we get afterward. > */ > -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > bool release_stored, Error **errp) > { > + ERRP_GUARD(); Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes an error_prepend(errp, ...) not visible in the patch context. Anyway, Reviewed-by: Greg Kurz <groug@kaod.org> > BdrvDirtyBitmap *bitmap; > BDRVQcow2State *s = bs->opaque; > uint32_t new_nb_bitmaps = s->nb_bitmaps; > @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > s->bitmap_directory_size, errp); > if (bm_list == NULL) { > - return; > + return false; > } > } > > @@ -1661,7 +1662,7 @@ success: > } > > bitmap_list_free(bm_list); > - return; > + return true; > > fail: > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > @@ -1679,16 +1680,14 @@ fail: > } > > bitmap_list_free(bm_list); > + return false; > } > > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) > { > BdrvDirtyBitmap *bitmap; > - Error *local_err = NULL; > > - qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err); > - if (local_err != NULL) { > - error_propagate(errp, local_err); > + if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) { > return -EINVAL; > } >
11.09.2020 14:21, Greg Kurz wrote: > On Fri, 11 Sep 2020 13:18:32 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> 11.09.2020 12:38, Greg Kurz wrote: >>> s/startus/status >>> >>> On Wed, 9 Sep 2020 21:59:27 +0300 >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>> >>>> It's better to return status together with setting errp. It makes >>>> possible to avoid error propagation. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/qcow2.h | 2 +- >>>> block/qcow2-bitmap.c | 13 ++++++------- >>>> 2 files changed, 7 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>> index e7e662533b..49824be5c6 100644 >>>> --- a/block/qcow2.h >>>> +++ b/block/qcow2.h >>>> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, >>>> Qcow2BitmapInfoList **info_list, Error **errp); >>>> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>>> int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>> bool release_stored, Error **errp); >>>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>>> bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>> index f58923fce3..5eeff1cb1c 100644 >>>> --- a/block/qcow2-bitmap.c >>>> +++ b/block/qcow2-bitmap.c >>>> @@ -1524,9 +1524,10 @@ out: >>>> * readonly to begin with, and whether we opened directly or reopened to that >>>> * state shouldn't matter for the state we get afterward. >>>> */ >>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>> bool release_stored, Error **errp) >>>> { >>>> + ERRP_GUARD(); >>> >>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes >>> an error_prepend(errp, ...) not visible in the patch context. >> >> Ah yes. Actually this is occasional thing I didn't want to include into this patch >> (and int this part I). So we can just drop it and leave for part II or part III, >> or add a note into commit message >> >>> >>> Anyway, >>> >>> Reviewed-by: Greg Kurz <groug@kaod.org> >> >> Thanks a lot for reviewing :) >> > > Don't mention it :) > >> Hmm.. With this series I understand the following: >> >> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because: >> >> - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series >> - reviewing is the hardest part of the process >> >> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations. >> >> 2. So, the process turns into following steps: >> >> - apply scripts/coccinelle/errp-guard.cocci >> - look through patches and do obvious refactorings (like this series) >> - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple) >> > > I've started to follow this process for the spapr code and, indeed, I > can come up with better changes by refactoring some code manually. > Some of these changes are not that obvious that they could be made > by someone who doesn't know the code, so I tend to agree with your > arguments in 1. > > This is also the reason I didn't review patches 10, 13 and 14 because > they looked like I should understand the corresponding code a bit more. > >> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:) >> > > Ha ha :D ... as a consolation prize, maybe we can reach a fair number > of r-b by reviewing each other's _simple_ cleanups ;-) > >> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers). >> > > This sounds nice. My only concern would be to end up fixing code nobody > uses or cares for, so I guess it would be better that active maintainers > or supporters give impetus on that. > >> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements.. >> > > I don't :) but the very massive series that were posted on the topic > the last few months look like an announcement to me, at least for > active maintainers and supporters. Aha, I know. Better than announcement is improving checkpatch. > >>> >>>> BdrvDirtyBitmap *bitmap; >>>> BDRVQcow2State *s = bs->opaque; >>>> uint32_t new_nb_bitmaps = s->nb_bitmaps; >>>> @@ -1546,7 +1547,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>> bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, >>>> s->bitmap_directory_size, errp); >>>> if (bm_list == NULL) { >>>> - return; >>>> + return false; >>>> } >>>> } >>>> >>>> @@ -1661,7 +1662,7 @@ success: >>>> } >>>> >>>> bitmap_list_free(bm_list); >>>> - return; >>>> + return true; >>>> >>>> fail: >>>> QSIMPLEQ_FOREACH(bm, bm_list, entry) { >>>> @@ -1679,16 +1680,14 @@ fail: >>>> } >>>> >>>> bitmap_list_free(bm_list); >>>> + return false; >>>> } >>>> >>>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) >>>> { >>>> BdrvDirtyBitmap *bitmap; >>>> - Error *local_err = NULL; >>>> >>>> - qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err); >>>> - if (local_err != NULL) { >>>> - error_propagate(errp, local_err); >>>> + if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) { >>>> return -EINVAL; >>>> } >>>> >>> >> >> >
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 11.09.2020 14:21, Greg Kurz wrote: >> On Fri, 11 Sep 2020 13:18:32 +0300 >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> >>> 11.09.2020 12:38, Greg Kurz wrote: >>>> s/startus/status >>>> >>>> On Wed, 9 Sep 2020 21:59:27 +0300 >>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>>> >>>>> It's better to return status together with setting errp. It makes >>>>> possible to avoid error propagation. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> block/qcow2.h | 2 +- >>>>> block/qcow2-bitmap.c | 13 ++++++------- >>>>> 2 files changed, 7 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>>> index e7e662533b..49824be5c6 100644 >>>>> --- a/block/qcow2.h >>>>> +++ b/block/qcow2.h >>>>> @@ -972,7 +972,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, >>>>> Qcow2BitmapInfoList **info_list, Error **errp); >>>>> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>>>> int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>>> bool release_stored, Error **errp); >>>>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>>>> bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>>> index f58923fce3..5eeff1cb1c 100644 >>>>> --- a/block/qcow2-bitmap.c >>>>> +++ b/block/qcow2-bitmap.c >>>>> @@ -1524,9 +1524,10 @@ out: >>>>> * readonly to begin with, and whether we opened directly or reopened to that >>>>> * state shouldn't matter for the state we get afterward. >>>>> */ >>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>>> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >>>>> bool release_stored, Error **errp) >>>>> { >>>>> + ERRP_GUARD(); >>>> >>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes >>>> an error_prepend(errp, ...) not visible in the patch context. >>> >>> Ah yes. Actually this is occasional thing I didn't want to include into this patch >>> (and int this part I). So we can just drop it and leave for part II or part III, >>> or add a note into commit message Either works for me. >>>> >>>> Anyway, >>>> >>>> Reviewed-by: Greg Kurz <groug@kaod.org> >>> >>> Thanks a lot for reviewing :) >>> >> Don't mention it :) >> >>> Hmm.. With this series I understand the following: >>> >>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because: >>> >>> - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series >>> - reviewing is the hardest part of the process >>> >>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations. Yes, going the extra mile is better. I recommend it for code that is actively maintained. Making the code simpler and thus easier to maintain is an investment that'll pay off. We may have code where it won't pay off. Do you think a blind application of errp-guard.cocci might be better than nothing there? >>> 2. So, the process turns into following steps: >>> >>> - apply scripts/coccinelle/errp-guard.cocci >>> - look through patches and do obvious refactorings (like this series) >>> - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple) >>> >> I've started to follow this process for the spapr code and, indeed, >> I >> can come up with better changes by refactoring some code manually. >> Some of these changes are not that obvious that they could be made >> by someone who doesn't know the code, so I tend to agree with your >> arguments in 1. >> This is also the reason I didn't review patches 10, 13 and 14 >> because >> they looked like I should understand the corresponding code a bit more. >> >>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:) LOL A lesser craftsman than you would've peddled the generated commits anyway. Props! >> Ha ha :D ... as a consolation prize, maybe we can reach a fair >> number >> of r-b by reviewing each other's _simple_ cleanups ;-) >> >>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers). If people care enough about [2] to submit patches, I'll feel obliged to help merging them. >> This sounds nice. My only concern would be to end up fixing code >> nobody >> uses or cares for, so I guess it would be better that active maintainers >> or supporters give impetus on that. A bit of weeding on the side is always appreciated, but please don't feel obliged to sink lots of time into code you don't care about. >>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements.. > >> I don't :) but the very massive series that were posted on the topic >> the last few months look like an announcement to me, at least for >> active maintainers and supporters. > > Aha, I know. Better than announcement is improving checkpatch. Yes, automated feedback works best. Relentless pressure from reviewers can also work in the long, long run. But it's tiresome. Of course, checkpatch.pl checks only new or changed code. Any ideas on how to make people aware of the opportunity to simplify their existing code? Obvious: posting more patches simplifying existing code we care about. Any other smart ideas?
On Wed 09 Sep 2020 08:59:18 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > Now bdrv_append returns status and we can drop all the local_err things > around it. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 1. Drop extra error propagation > > 2. Set errp always on failure. Generic bdrv_open_driver supports driver > functions which can return negative value and forget to set errp. > That's a strange thing.. Let's improve qcow2_do_open to not behave this > way. This allows to simplify code in qcow2_co_invalidate_cache(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 31dd28d19e..cc4e7dd461 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp) > static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, > int flags, Error **errp) > { > + ERRP_GUARD(); Why is this necessary? > BDRVQcow2State *s = bs->opaque; > unsigned int len, i; > int ret = 0; > @@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, > report_unsupported_feature(errp, feature_table, > s->incompatible_features & > ~QCOW2_INCOMPAT_MASK); > + error_setg(errp, > + "qcow2 header contains unknown > incompatible_feature bits"); I think that this is a mistake because the previous call to report_unsupported_feature() already calls error_setg(); > @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs) > static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, > Error **errp) > { > + ERRP_GUARD(); Again, why is this necessary? Berto
On Wed 09 Sep 2020 08:59:25 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > + * On success return true with bm_list set (probably to NULL, if no bitmaps), " probably " ? :-) > + * on failure return false with errp set. > */ > -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, > - Error **errp) > +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, > + Qcow2BitmapInfoList **info_list, Error **errp) > { > BDRVQcow2State *s = bs->opaque; > Qcow2BitmapList *bm_list; > Qcow2Bitmap *bm; > - Qcow2BitmapInfoList *list = NULL; > - Qcow2BitmapInfoList **plist = &list; So here 'list' points at NULL and 'plist' at &list. > - *plist = obj; > - plist = &obj->next; In the original code 'plist' is updated when you add a new element, so it always points at the end of the list. But 'list' is unchanged and it still points at the first element. So the caller receives a pointer to the first element. > + *info_list = obj; > + info_list = &obj->next; But in the new code there is only one variable (passed by the caller), which always points at the end of the list. Berto
17.09.2020 19:23, Alberto Garcia wrote: > On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> 1. Drop extra error propagation >> >> 2. Set errp always on failure. Generic bdrv_open_driver supports driver >> functions which can return negative value and forget to set errp. >> That's a strange thing.. Let's improve qcow2_do_open to not behave this >> way. This allows to simplify code in qcow2_co_invalidate_cache(). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2.c | 16 +++++++--------- >> 1 file changed, 7 insertions(+), 9 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 31dd28d19e..cc4e7dd461 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp) >> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, >> int flags, Error **errp) >> { >> + ERRP_GUARD(); > > Why is this necessary? Because error_append_hint() used in the function. Without ERRP_GUARD, error_append_hint won't work if errp = &error_fatal Read more in include/qapi/error.h near ERRP_GUARD definition. But yes, it's good to not it in commit message. > >> BDRVQcow2State *s = bs->opaque; >> unsigned int len, i; >> int ret = 0; >> @@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, >> report_unsupported_feature(errp, feature_table, >> s->incompatible_features & >> ~QCOW2_INCOMPAT_MASK); >> + error_setg(errp, >> + "qcow2 header contains unknown >> incompatible_feature bits"); > > I think that this is a mistake because the previous call to > report_unsupported_feature() already calls error_setg(); Oops, you are right. > >> @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs) >> static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, >> Error **errp) >> { >> + ERRP_GUARD(); > > Again, why is this necessary? > Because it uses error_prepend() after conversion (same reason as for error_append_hint()). Thanks for review! I'll post v2 soon.
17.09.2020 19:32, Alberto Garcia wrote: > On Wed 09 Sep 2020 08:59:25 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> + * On success return true with bm_list set (probably to NULL, if no bitmaps), > > " probably " ? :-) I note this as "set to NULL" is not obvious thing (is it "unset" ? :).. And by "probably" I mean "may be", i.e. NULL is just one of possible cases. Probably I use "probably" in a wrong way? > >> + * on failure return false with errp set. >> */ >> -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, >> - Error **errp) >> +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, >> + Qcow2BitmapInfoList **info_list, Error **errp) >> { >> BDRVQcow2State *s = bs->opaque; >> Qcow2BitmapList *bm_list; >> Qcow2Bitmap *bm; >> - Qcow2BitmapInfoList *list = NULL; >> - Qcow2BitmapInfoList **plist = &list; > > So here 'list' points at NULL and 'plist' at &list. Hmm, to be precise, list _is_ NULL (and points nowhere), and plist points to list. > >> - *plist = obj; >> - plist = &obj->next; > > In the original code 'plist' is updated when you add a new element, so > it always points at the end of the list. But 'list' is unchanged and it > still points at the first element. > > So the caller receives a pointer to the first element. > >> + *info_list = obj; >> + info_list = &obj->next; > > But in the new code there is only one variable (passed by the caller), > which always points at the end of the list. > No: at first "*info_list = obj", we set the result which user will get, users pointer now points to the first object in the list. Then, at "info_list = &obj->next", we reassign info_list to another pointer: to "next" field of first list item. So, all further "*info_list = obj" are note visible to the caller. Actually, the logic is not changed, just instead of plist we use info_list, and instead of list - a variable which should be defined in the caller. Look: in old code, first "*plist = obj" sets "list" variable, but all further "*plist = obj" don't change "list" variable.
10.09.2020 19:10, Greg Kurz wrote: > On Wed, 9 Sep 2020 21:59:18 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> Now bdrv_append returns status and we can drop all the local_err things >> around it. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- > > Reviewed-by: Greg Kurz <groug@kaod.org> > > Just one suggestion for a follow-up below... > >> block.c | 5 +---- >> block/backup-top.c | 20 ++++++++------------ [..] >> @@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs, >> CommitBlockJob *s; >> BlockDriverState *iter; >> BlockDriverState *commit_top_bs = NULL; >> - Error *local_err = NULL; >> int ret; >> > > ... this is unrelated but while reviewing I've noticed that the ret > variable isn't really needed. > Looking at this now, I'm not quite agreed. I think that avoiding multi-line if conditions is a good reason for additional variables, like this ret. A kind of taste of course, so if you want you may post a patch, but I don't want do it :) -- Best regards, Vladimir
Hi all! I've start to apply scripts/coccinelle/errp-guard.cocci to block subsystem, but it turned out that in most cases error propagation can be simply avoided. So, here are some improvements to block layer error reporting to avoid error propagation. It's not complete, so its called part I, I don't want to create huge series. Vladimir Sementsov-Ogievskiy (14): block: return status from bdrv_append and friends block: use return status of bdrv_append() block: check return value of bdrv_open_child and drop error propagation blockdev: fix drive_backup_prepare() missed error block: drop extra error propagation for bdrv_set_backing_hd block/mirror: drop extra error propagation in commit_active_start() block/blklogwrites: drop error propagation blockjob: return status from block_job_set_speed() block/qcow2: qcow2_get_specific_info(): drop error propagation block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps block/qcow2: read_cache_sizes: return status value block/qcow2: qcow2_do_open: deal with errp block/qed: bdrv_qed_do_open: deal with errp block/qcow2.h | 9 +++--- include/block/block.h | 12 +++---- include/block/blockjob.h | 2 +- block.c | 50 ++++++++++++++++-------------- block/backup-top.c | 20 +++++------- block/blkdebug.c | 6 ++-- block/blklogwrites.c | 33 +++++++++----------- block/blkreplay.c | 6 ++-- block/blkverify.c | 11 +++---- block/commit.c | 5 +-- block/mirror.c | 18 +++++------ block/qcow2-bitmap.c | 62 +++++++++++++++++-------------------- block/qcow2.c | 56 ++++++++++++++------------------- block/qed.c | 24 ++++++++------ block/quorum.c | 6 ++-- blockdev.c | 7 ++--- blockjob.c | 18 +++++------ tests/test-bdrv-graph-mod.c | 6 ++-- 18 files changed, 159 insertions(+), 192 deletions(-)