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
