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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6034 bytes --]

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).

[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.

> + */
> +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)

> +        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.


> +##
> +# @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?

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]