| Message ID | 1548870690-647481-3-git-send-email-andrey.shinkevich@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-img info lists bitmap directory entries | expand |
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.
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."
30.01.2019 22:26, Andrey Shinkevich wrote: > > > 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 [...] >>> +## >>> +# @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. > In common case, we should load all bitmaps for it, which is bad idea for qmp query when vm is running. On the other hand, in some meaningful cases (when open read-only, with qemu-img info), we can reuse information obtained during loading bitmaps on open(). However, for in-use bitmaps we'll have to load them to count dirty bits. Which is not a problem for qemu-img, but again, may be not good idea while vm is running (as it may take too much time). Also, exporting dirty-count for in-use bitmaps while vm is running may be ambiguous, as some of the in-use bitmaps (or all) will be bitmaps in-use by exactly this vm run, so actual dirty-count is in BdrvDirtyBitmap info. And, another idea: we have extra_data field in bitmap list in qcow2, which may be safely ignored if extra_data_compatible is set for this bitmap. So, it may be a good reason to implement first type of compatible extra data.. But I don't sure that it's worth it.
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index b946301..17651cb 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1006,6 +1006,83 @@ fail: return false; } + +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; + } + } + + *plist = NULL; + + 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). + */ +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); + 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); + return NULL; + } *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){ .compat = g_strdup("1.1"), .lazy_refcounts = s->compatible_features & @@ -4403,6 +4411,8 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs, QCOW2_INCOMPAT_CORRUPT, .has_corrupt = true, .refcount_bits = s->refcount_bits, + .has_bitmaps = !!bitmaps, + .bitmaps = bitmaps, }; } else { /* if this assertion fails, this probably means a new version was diff --git a/block/qcow2.h b/block/qcow2.h index 438a1de..13e8964 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -684,6 +684,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, void **refcount_table, int64_t *refcount_table_size); bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); +Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, + Error **errp); int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); diff --git a/qapi/block-core.json b/qapi/block-core.json index 91685be..271e0df 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -69,6 +69,8 @@ # @encrypt: details about encryption parameters; only set if image # is encrypted (since 2.10) # +# @bitmaps: A list of qcow2 bitmap details (since 4.0) +# # Since: 1.7 ## { 'struct': 'ImageInfoSpecificQCow2', @@ -77,7 +79,8 @@ '*lazy-refcounts': 'bool', '*corrupt': 'bool', 'refcount-bits': 'int', - '*encrypt': 'ImageInfoSpecificQCow2Encryption' + '*encrypt': 'ImageInfoSpecificQCow2Encryption', + '*bitmaps': ['Qcow2BitmapInfo'] } } ## @@ -454,6 +457,41 @@ 'status': 'DirtyBitmapStatus'} } ## +# @Qcow2BitmapInfoFlags: +# +# An enumeration of flags that a bitmap can report to the user. +# +# @in-use: The bitmap was not saved correctly and may be inconsistent. +# +# @auto: The bitmap must reflect all changes of the virtual disk by any +# application that would write to this qcow2 file. +# +# Since: 4.0 +## +{ 'enum': 'Qcow2BitmapInfoFlags', + 'data': ['in-use', 'auto'] } + +## +# @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' } } + +## # @BlockLatencyHistogramInfo: # # Block latency histogram.
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: image: /vz/vmprivate/VM1/harddisk.hdd file format: qcow2 virtual size: 64G (68719476736 bytes) disk size: 3.0M cluster_size: 1048576 Format specific information: compat: 1.1 lazy refcounts: true bitmaps: [0]: flags: [0]: in-use [1]: auto name: back-up1 unknown flags: 4 granularity: 65536 [1]: flags: [0]: in-use [1]: auto name: back-up2 unknown flags: 8 granularity: 65536 refcount bits: 16 corrupt: false Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/qcow2-bitmap.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 10 +++++++ block/qcow2.h | 2 ++ qapi/block-core.json | 40 ++++++++++++++++++++++++++- 4 files changed, 128 insertions(+), 1 deletion(-)