| Message ID | 1378382715-28132-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/05/2013 06:05 AM, Max Reitz wrote: > Add a string for additional information to ImageInfo and > BlockDriverInfo. Also, use this string to emit the compatibility level > and lazy_refcount value (on compat=1.1) for qcow2. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > +++ b/qapi-schema.json > @@ -238,6 +238,9 @@ > # > # @backing-image: #optional info of the backing image (since 1.6) > # > +# @info-string: #optional string supplying additional format-specific > +# information (since 1.7) > +# > # Since: 1.3 > # > ## > @@ -248,7 +251,7 @@ > '*cluster-size': 'int', '*encrypted': 'bool', > '*backing-filename': 'str', '*full-backing-filename': 'str', > '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'], > - '*backing-image': 'ImageInfo' } } > + '*backing-image': 'ImageInfo', '*info-string': 'str' } } This may work for HMP, but is LOUSY for use by QMP clients. If you are passing back more than a single piece of information, you are now requiring the QMP client to do a parse of a free-form string to learn those pieces of information. I'd much rather see a full JSON schema where EVERY piece of information passed back gets its own optional field, or even where the additional information is a union type discriminated by the image, so that we have full structure of the information being returned rather than just an ad-hoc blobbed string. Please rework this so that QMP clients like libvirt can easily probe what compat mode a qcow2 image uses, without having to parse a free-form string.
On 2013-09-05 14:25, Eric Blake wrote: > On 09/05/2013 06:05 AM, Max Reitz wrote: >> Add a string for additional information to ImageInfo and >> BlockDriverInfo. Also, use this string to emit the compatibility level >> and lazy_refcount value (on compat=1.1) for qcow2. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> +++ b/qapi-schema.json >> @@ -238,6 +238,9 @@ >> # >> # @backing-image: #optional info of the backing image (since 1.6) >> # >> +# @info-string: #optional string supplying additional format-specific >> +# information (since 1.7) >> +# >> # Since: 1.3 >> # >> ## >> @@ -248,7 +251,7 @@ >> '*cluster-size': 'int', '*encrypted': 'bool', >> '*backing-filename': 'str', '*full-backing-filename': 'str', >> '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'], >> - '*backing-image': 'ImageInfo' } } >> + '*backing-image': 'ImageInfo', '*info-string': 'str' } } > This may work for HMP, but is LOUSY for use by QMP clients. If you are > passing back more than a single piece of information, you are now > requiring the QMP client to do a parse of a free-form string to learn > those pieces of information. I'd much rather see a full JSON schema > where EVERY piece of information passed back gets its own optional > field, or even where the additional information is a union type > discriminated by the image, so that we have full structure of the > information being returned rather than just an ad-hoc blobbed string. > > Please rework this so that QMP clients like libvirt can easily probe > what compat mode a qcow2 image uses, without having to parse a free-form > string. > Seems very reasonable; I'll do my best. Max
On Thu, Sep 05, 2013 at 02:05:13PM +0200, Max Reitz wrote: > diff --git a/block.c b/block.c > index 26639e8..9fd9f3a 100644 > --- a/block.c > +++ b/block.c > @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > int64_t *cluster_sector_num, > int *cluster_nb_sectors) > { > - BlockDriverInfo bdi; > + BlockDriverInfo bdi = { .info_string = NULL }; > > if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { > *cluster_sector_num = sector_num; > @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, > *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + > nb_sectors, c); > } > + g_free(bdi.info_string); > } This function is called in the I/O path. It's not appropriate to generate an options string each time this gets called :). Together with Eric's comments it seems like this change needs a different interface that is QMP-friendly and not in the I/O path. Stefan
On 2013-09-18 16:59, Stefan Hajnoczi wrote: > On Thu, Sep 05, 2013 at 02:05:13PM +0200, Max Reitz wrote: >> diff --git a/block.c b/block.c >> index 26639e8..9fd9f3a 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >> int64_t *cluster_sector_num, >> int *cluster_nb_sectors) >> { >> - BlockDriverInfo bdi; >> + BlockDriverInfo bdi = { .info_string = NULL }; >> >> if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { >> *cluster_sector_num = sector_num; >> @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, >> *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + >> nb_sectors, c); >> } >> + g_free(bdi.info_string); >> } > This function is called in the I/O path. It's not appropriate to > generate an options string each time this gets called :). > > Together with Eric's comments it seems like this change needs a > different interface that is QMP-friendly and not in the I/O path. > > Stefan I've already sent a v2, v3 and v4 which address Eric's comment. However, your problem still remains. I think I might either add a new function for specifically requesting the format specific information (instead of generally including it in BDI) or introduce a flag parameter for bdrv_get_info which requests that BDI field to be filled (or, more generally, which will prevent bdrv_get_info from performing any relatively expensive operation). Max
diff --git a/block.c b/block.c index 26639e8..9fd9f3a 100644 --- a/block.c +++ b/block.c @@ -1921,7 +1921,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, int64_t *cluster_sector_num, int *cluster_nb_sectors) { - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .info_string = NULL }; if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) { *cluster_sector_num = sector_num; @@ -1932,6 +1932,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs, *cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num + nb_sectors, c); } + g_free(bdi.info_string); } static bool tracked_request_overlaps(BdrvTrackedRequest *req, diff --git a/block/mirror.c b/block/mirror.c index 86de458..296ad54 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -295,7 +295,7 @@ static void coroutine_fn mirror_run(void *opaque) BlockDriverState *bs = s->common.bs; int64_t sector_num, end, sectors_per_chunk, length; uint64_t last_pause_ns; - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .info_string = NULL }; char backing_filename[1024]; int ret = 0; int n; @@ -325,6 +325,7 @@ static void coroutine_fn mirror_run(void *opaque) s->buf_size = MAX(s->buf_size, bdi.cluster_size); s->cow_bitmap = bitmap_new(length); } + g_free(bdi.info_string); } end = s->common.len >> BDRV_SECTOR_BITS; @@ -544,13 +545,14 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, if (granularity == 0) { /* Choose the default granularity based on the target file's cluster * size, clamped between 4k and 64k. */ - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .info_string = NULL }; if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) { granularity = MAX(4096, bdi.cluster_size); granularity = MIN(65536, granularity); } else { granularity = 65536; } + g_free(bdi.info_string); } assert ((granularity & (granularity - 1)) == 0); diff --git a/block/qapi.c b/block/qapi.c index a4bc411..b63376c 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -110,7 +110,7 @@ void bdrv_query_image_info(BlockDriverState *bs, uint64_t total_sectors; const char *backing_filename; char backing_filename2[1024]; - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .info_string = NULL }; int ret; Error *err = NULL; ImageInfo *info = g_new0(ImageInfo, 1); @@ -133,6 +133,10 @@ void bdrv_query_image_info(BlockDriverState *bs, } info->dirty_flag = bdi.is_dirty; info->has_dirty_flag = true; + if (bdi.info_string) { + info->info_string = bdi.info_string; + info->has_info_string = true; + } } backing_filename = bs->backing_file; if (backing_filename[0] != '\0') { @@ -467,4 +471,8 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f, func_fprintf(f, "\n"); } } + + if (info->has_info_string) { + func_fprintf(f, "additional information: %s\n", info->info_string); + } } diff --git a/block/qcow2.c b/block/qcow2.c index 4bc679a..7351ab1 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1757,6 +1757,12 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) BDRVQcowState *s = bs->opaque; bdi->cluster_size = s->cluster_size; bdi->vm_state_offset = qcow2_vm_state_offset(s); + if (s->qcow_version == 2) { + bdi->info_string = g_strdup("compat=0.10"); + } else if (s->qcow_version == 3) { + bdi->info_string = g_strdup_printf("compat=1.1,lazy_refcounts=%s", + s->use_lazy_refcounts ? "on" : "off"); + } return 0; } diff --git a/include/block/block.h b/include/block/block.h index e6b391c..f8335a2 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -18,6 +18,8 @@ typedef struct BlockDriverInfo { /* offset at which the VM state can be saved (0 if not possible) */ int64_t vm_state_offset; bool is_dirty; + /* additional information; NULL if none */ + char *info_string; } BlockDriverInfo; typedef struct BlockFragInfo { diff --git a/qapi-schema.json b/qapi-schema.json index a51f7d2..15c4f02 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -238,6 +238,9 @@ # # @backing-image: #optional info of the backing image (since 1.6) # +# @info-string: #optional string supplying additional format-specific +# information (since 1.7) +# # Since: 1.3 # ## @@ -248,7 +251,7 @@ '*cluster-size': 'int', '*encrypted': 'bool', '*backing-filename': 'str', '*full-backing-filename': 'str', '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'], - '*backing-image': 'ImageInfo' } } + '*backing-image': 'ImageInfo', '*info-string': 'str' } } ## # @ImageCheck: diff --git a/qemu-img.c b/qemu-img.c index b9a848d..7081d99 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1125,7 +1125,7 @@ static int img_convert(int argc, char **argv) uint64_t bs_sectors; uint8_t * buf = NULL; const uint8_t *buf1; - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .info_string = NULL }; QEMUOptionParameter *param = NULL, *create_options = NULL; QEMUOptionParameter *out_baseimg_param; char *options = NULL; @@ -1369,6 +1369,7 @@ static int img_convert(int argc, char **argv) error_report("could not get block driver info"); goto out; } + g_free(bdi.info_string); cluster_size = bdi.cluster_size; if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { error_report("invalid cluster size"); diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index f91b6c4..fed0b4b 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1677,7 +1677,7 @@ static const cmdinfo_t length_cmd = { static int info_f(BlockDriverState *bs, int argc, char **argv) { - BlockDriverInfo bdi; + BlockDriverInfo bdi = { .info_string = NULL }; char s1[64], s2[64]; int ret; @@ -1699,6 +1699,11 @@ static int info_f(BlockDriverState *bs, int argc, char **argv) printf("cluster size: %s\n", s1); printf("vm state offset: %s\n", s2); + if (bdi.info_string) { + printf("additional information: %s\n", bdi.info_string); + g_free(bdi.info_string); + } + return 0; }
Add a string for additional information to ImageInfo and BlockDriverInfo. Also, use this string to emit the compatibility level and lazy_refcount value (on compat=1.1) for qcow2. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 3 ++- block/mirror.c | 6 ++++-- block/qapi.c | 10 +++++++++- block/qcow2.c | 6 ++++++ include/block/block.h | 2 ++ qapi-schema.json | 5 ++++- qemu-img.c | 3 ++- qemu-io-cmds.c | 7 ++++++- 8 files changed, 35 insertions(+), 7 deletions(-)