Re: [Qemu-devel] [PATCH v10 2/3] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

On 30/01/2019 21:24, Eric Blake wrote:
> On 1/30/19 11:51 AM, Andrey Shinkevich wrote:
>> In the 'Format specific information' section of the 'qemu-img info'
>> command output, the supplemental information about existing QCOW2
>> bitmaps will be shown, such as a bitmap name, flags and granularity:
>>
> 
>> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
>> +{
>> +    Qcow2BitmapInfoFlagsList *list = NULL;
>> +    Qcow2BitmapInfoFlagsList **plist = &list;
>> +    int i;
>> +
>> +    static const struct {
>> +        int bme;  /* Bitmap directory entry flags */
>> +        int info; /* The flags to report to the user */
>> +    } map[] = {
>> +        { BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
>> +        { BME_FLAG_AUTO,   QCOW2_BITMAP_INFO_FLAGS_AUTO },
>> +    };
>> +
>> +    int map_size = sizeof(map) / sizeof(map[0]);
>> +
>> +    for (i = 0; i < map_size; ++i) {
>> +        if (flags & map[i].bme) {
>> +            Qcow2BitmapInfoFlagsList *entry =
>> +                g_new0(Qcow2BitmapInfoFlagsList, 1);
>> +            entry->value = map[i].info;
>> +            *plist = entry;
>> +            plist = &entry->next;
> 
> Here's an idea for having runtime verification that our mapping of BME_
> flags to QAPI flags is complete. At the spots marked [1], add:
> 
> flags &= ~map[i].bme;
> 
>> +        }
>> +    }
>> +
>> +    *plist = NULL;
> 
> Dead assignment. plist is originally pointing to list (which was
> NULL-initialized at declaration), and is only ever changed to point to
> the list's next entry->next (which were NULL-initialized thanks to g_new0).

Yes, it is redundant due to the trailing '0' at g_new.
Agreed absolutely ))

> 
> [1]
> assert(!flags);
> 
>> +
>> +    return list;
>> +}
>> +
>> +/*
>> + * 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 (for a 0-length list in the QMP).
>> + * When bitmap information can not be obtained, the function returns
>> + * NULL and the @errp parameter is set (for omitting the list in QMP).
> 
> Comment is a bit stale, now that we aren't going to omit the list in
> QMP, but instead fail the QMP command outright.

Ouch! Missed out to correct that comment ((

> 
>> + */
>> +Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>> +                                                Error **errp)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    Qcow2BitmapList *bm_list;
>> +    Qcow2Bitmap *bm;
>> +    Qcow2BitmapInfoList *list = NULL;
>> +    Qcow2BitmapInfoList **plist = &list;
>> +
>> +    if (s->nb_bitmaps == 0) {
>> +        return NULL;
>> +    }
>> +
>> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> +                               s->bitmap_directory_size, errp);
>> +    if (bm_list == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> +        Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
>> +        Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
>> +        info->granularity = 1U << bm->granularity_bits;
>> +        info->name = g_strdup(bm->name);
>> +        info->flags = get_bitmap_info_flags(bm->flags);
> 
> [1]
> get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS)

Thank you, we will discuss it...

> 
>> +        info->unknown_flags = bm->flags & BME_RESERVED_FLAGS;
>> +        info->has_unknown_flags = !!info->unknown_flags;
>> +        obj->value = info;
>> +        *plist = obj;
>> +        plist = &obj->next;
>> +    }
>> +
>> +    bitmap_list_free(bm_list);
>> +
>> +    return list;
>> +}
>> +
>>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool 
>> *header_updated,
>>                                    Error **errp)
>>   {
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 27e5a2c..4824ca8 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4394,6 +4394,14 @@ static ImageInfoSpecific 
>> *qcow2_get_specific_info(BlockDriverState *bs,
>>               .refcount_bits      = s->refcount_bits,
>>           };
>>       } 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);
>> +            g_free(spec_info->u.qcow2.data);
>> +            g_free(spec_info);
> 
> I think it is cleaner to use qapi_free_ImageInfoSpecific(spec_info),
> which takes care of recursion without you having to analyze whether two
> g_free() calls are sufficient.  Of course, for THAT to work, we need to
> fix the line above that does:
> 
> .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
> 
> to instead use g_new0(), since recursive freeing of uninitialized data
> is not a good idea (without your patch, g_new() was sufficient since all
> paths through the code either fully initialize or assert).  So maybe
> your patch is the shortest that works, after all.

Yea, the invocation to qapi_free_ImageInfoSpecific(spec_info) looks like
a better code style.

> 
> 
>> +##
>> +# @Qcow2BitmapInfo:
>> +#
>> +# Qcow2 bitmap information.
>> +#
>> +# @name: the name of the bitmap
>> +#
>> +# @granularity: granularity of the bitmap in bytes
>> +#
>> +# @flags: recognized flags of the bitmap
>> +#
>> +# @unknown-flags: any remaining flags not recognized by the current qemu 
>> version
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'Qcow2BitmapInfo',
>> +  'data': {'name': 'str', 'granularity': 'uint32',
>> +           'flags': ['Qcow2BitmapInfoFlags'],
>> +           '*unknown-flags': 'uint32' } }
> 
> Not for this patch, but how hard would it be to add a field showing the
> number of set bits in the bitmap?

I believe that is not too hard and would be happy to implement that with
another series, please.

> 
> Kevin's insistence that a failure to read bitmap headers should fail the
> overall 'qemu-img info' (on the grounds that we're going to have
> problems using the image anyways) is reasonable enough; thanks for
> putting up with competing demands (such as my earlier ideas of how best
> to ignore read failures while still reporting as much remaining useful
> information as possible), even if it has taken us through v10 to get to
> a consensus on the semantics we want to support.
> 
Both approaches looks reasonable to me.
"For by wise counsel thou shalt make thy war:
and in multitude of counselors there is safety."

-- 
With the best regards,
Andrey Shinkevich