Don't use error propagation in qcow2_get_specific_info(). For this
refactor qcow2_get_bitmap_info_list, its current interface rather
weird.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
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;
--
2.21.3
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;
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: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.
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.