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);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
