| Message ID | 20210202124956.63146-9-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: deal with errp: part I | expand |
On Tue 02 Feb 2021 01:49:50 PM CET, Vladimir Sementsov-Ogievskiy wrote: > -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 **tail = &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); > info->granularity = 1U << bm->granularity_bits; > info->name = g_strdup(bm->name); > info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS); > - QAPI_LIST_APPEND(tail, info); > + QAPI_LIST_APPEND(info_list, info); > } > > bitmap_list_free(bm_list); > > - return list; > + return true; > } Maybe I'm reading this wrong but... In the original code you had the head and tail of the list ('list' and 'tail') then you would append items to the tail and finally return the head. However the new code only uses and updates 'info_list' and it does not keep the head anywhere, so what the caller gets is a pointer to the tail. Berto
05.02.2021 14:43, Alberto Garcia wrote: > On Tue 02 Feb 2021 01:49:50 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> -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 **tail = &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); >> info->granularity = 1U << bm->granularity_bits; >> info->name = g_strdup(bm->name); >> info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS); >> - QAPI_LIST_APPEND(tail, info); >> + QAPI_LIST_APPEND(info_list, info); >> } >> >> bitmap_list_free(bm_list); >> >> - return list; >> + return true; >> } > > Maybe I'm reading this wrong but... > > In the original code you had the head and tail of the list ('list' and > 'tail') then you would append items to the tail and finally return the > head. > > However the new code only uses and updates 'info_list' and it does not > keep the head anywhere, so what the caller gets is a pointer to the > tail. > No. *info_list is modified only on the first loop iteration. And than info_list is switched to &(*(info_list))->next, so on second iteration we will modify @next field of first element, not original *info_list.
On Fri 05 Feb 2021 12:52:03 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> However the new code only uses and updates 'info_list' and it does not >> keep the head anywhere, so what the caller gets is a pointer to the >> tail. >> > > No. *info_list is modified only on the first loop iteration. And than > info_list is switched to &(*(info_list))->next, so on second iteration > we will modify @next field of first element, not original *info_list. Right, I see! I find it a bit confusing, 'info_list' is at the same time a return value and a local variable that you use to iterate over the list. Anyway, Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On 2/5/21 5:52 AM, Vladimir Sementsov-Ogievskiy wrote: > 05.02.2021 14:43, Alberto Garcia wrote: >> On Tue 02 Feb 2021 01:49:50 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>> -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 **tail = &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); >>> info->granularity = 1U << bm->granularity_bits; >>> info->name = g_strdup(bm->name); >>> info->flags = get_bitmap_info_flags(bm->flags & >>> ~BME_RESERVED_FLAGS); >>> - QAPI_LIST_APPEND(tail, info); >>> + QAPI_LIST_APPEND(info_list, info); >>> } >>> bitmap_list_free(bm_list); >>> - return list; >>> + return true; >>> } >> >> Maybe I'm reading this wrong but... >> >> In the original code you had the head and tail of the list ('list' and >> 'tail') then you would append items to the tail and finally return the >> head. >> >> However the new code only uses and updates 'info_list' and it does not >> keep the head anywhere, so what the caller gets is a pointer to the >> tail. >> > > No. *info_list is modified only on the first loop iteration. And than > info_list is switched to &(*(info_list))->next, so on second iteration > we will modify @next field of first element, not original *info_list. Elsewhere when making these types of conversions, Markus suggested that I keep a separate tail variable, initialized by the parameter info_list, to make it more apparent. As in squashing the patch below: With that, it looks this series is reviewed, so I'm planning on taking it through my dirty-bitmaps tree (perhaps I'm stretching the fact that patch 10/14 is definitely dirty-bitmaps into taking the whole series, but I doubt I'll hear any complaints from other block maintainers) diff --git i/block/qcow2-bitmap.c w/block/qcow2-bitmap.c index e50da1ee7da3..f417f9ccb195 100644 --- i/block/qcow2-bitmap.c +++ w/block/qcow2-bitmap.c @@ -1103,6 +1103,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, BDRVQcow2State *s = bs->opaque; Qcow2BitmapList *bm_list; Qcow2Bitmap *bm; + Qcow2BitmapInfoList **tail; if (s->nb_bitmaps == 0) { *info_list = NULL; @@ -1116,13 +1117,14 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, } *info_list = NULL; + tail = info_list; QSIMPLEQ_FOREACH(bm, bm_list, entry) { Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1); info->granularity = 1U << bm->granularity_bits; info->name = g_strdup(bm->name); info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS); - QAPI_LIST_APPEND(info_list, info); + QAPI_LIST_APPEND(tail, info); } bitmap_list_free(bm_list);
diff --git a/block/qcow2.h b/block/qcow2.h index 0678073b74..a6bf2881bb 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -979,8 +979,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 5eef82fa55..c95d6e37e6 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1089,41 +1089,41 @@ 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 info_list set (note, that if there are no + * bitmaps, info_list is set to NULL). + * 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 **tail = &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); info->granularity = 1U << bm->granularity_bits; info->name = g_strdup(bm->name); info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS); - QAPI_LIST_APPEND(tail, info); + QAPI_LIST_APPEND(info_list, info); } 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 e8dd42d73b..1e83c6cebe 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -5063,12 +5063,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; } } @@ -5085,9 +5083,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;
Don't use error propagation in qcow2_get_specific_info(). For this refactor qcow2_get_bitmap_info_list, its current interface is rather weird. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.h | 4 ++-- block/qcow2-bitmap.c | 26 +++++++++++++------------- block/qcow2.c | 10 +++------- 3 files changed, 18 insertions(+), 22 deletions(-)