Re: [PATCH v7 08/14] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Eric Blake <eblake@redhat.com>]

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