[Qemu-devel] [PATCH 0/3] Provide additional info through qemu-img info

[Qemu-devel] [PATCH 0/3] Provide additional info through qemu-img info

[Max Reitz]

qemu-img info provides only pretty general information about an image.
For any image format, there might be specific options which cannot be
represented in a universal way; for instance, qcow2 provides the
compatibility and lazy_refcount options whose values are certainly
interesting but currently cannot be output by qemu-img info.

Therefore, this series adds an info-string field to ImageInfo resp.
info_string to BlockDriverInfo which may be used by block drivers to
hand an uninterpreted string to the user. It also adds support to
qemu-img info and qemu-io -c info to dump that field's value.

Max Reitz (3):
  block: Additional info string in ImageInfo and BDI
  qemu-iotests: info-string filter in _img_info
  qemu-iotests: Additional info from qemu-img info

 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 ++++-
 tests/qemu-iotests/064       | 72 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/064.out   | 17 +++++++++++
 tests/qemu-iotests/common.rc |  4 ++-
 tests/qemu-iotests/group     |  1 +
 12 files changed, 128 insertions(+), 8 deletions(-)
 create mode 100755 tests/qemu-iotests/064
 create mode 100644 tests/qemu-iotests/064.out

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] block: Additional info string in ImageInfo and BDI

[Max Reitz]

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

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;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] qemu-iotests: info-string filter in _img_info

[Max Reitz]

Filter out additional information specific to the image format provided
by qemu-img info in _img_info.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.rc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 5e077c3..d2dae4b 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -186,7 +186,9 @@ _img_info()
             -e "s#$TEST_DIR#TEST_DIR#g" \
             -e "s#$IMGFMT#IMGFMT#g" \
             -e "/^disk size:/ D" \
-            -e "/actual-size/ D"
+            -e "/actual-size/ D" \
+            -e "/additional information:/ D" \
+            -e "/info-string/ D"
 }
 
 _get_pids_by_name()
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] qemu-iotests: Additional info from qemu-img info

[Max Reitz]

Add a test for the additional information now provided by qemu-img info
when used on qcow2 images.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/064     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/064.out | 17 +++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 90 insertions(+)
 create mode 100755 tests/qemu-iotests/064
 create mode 100644 tests/qemu-iotests/064.out

diff --git a/tests/qemu-iotests/064 b/tests/qemu-iotests/064
new file mode 100755
index 0000000..1ab2a64
--- /dev/null
+++ b/tests/qemu-iotests/064
@@ -0,0 +1,72 @@
+#!/bin/bash
+#
+# Test for additional information emitted by qemu-img info on qcow2
+# images
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+IMG_SIZE=64M
+
+echo
+echo "=== Testing qcow2 image with -o compat=0.10 ==="
+echo
+IMGOPTS="compat=0.10" _make_test_img $IMG_SIZE
+# don't use _img_info, since that function will filter out the
+# info-string
+$QEMU_IMG info "$TEST_IMG" | grep "additional information"
+
+echo
+echo "=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=off ==="
+echo
+IMGOPTS="compat=1.1,lazy_refcounts=off" _make_test_img $IMG_SIZE
+$QEMU_IMG info "$TEST_IMG" | grep "additional information"
+
+echo
+echo "=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=on ==="
+echo
+IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img $IMG_SIZE
+$QEMU_IMG info "$TEST_IMG" | grep "additional information"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/064.out b/tests/qemu-iotests/064.out
new file mode 100644
index 0000000..fcaa670
--- /dev/null
+++ b/tests/qemu-iotests/064.out
@@ -0,0 +1,17 @@
+QA output created by 064
+
+=== Testing qcow2 image with -o compat=0.10 ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+additional information: compat=0.10
+
+=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=off ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+additional information: compat=1.1,lazy_refcounts=off
+
+=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=on ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+additional information: compat=1.1,lazy_refcounts=on
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b696242..740cd84 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -66,3 +66,4 @@
 059 rw auto
 060 rw auto
 062 rw auto
+064 rw auto
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/3] block: Additional info string in ImageInfo and BDI

[Eric Blake]

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

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 1/3] block: Additional info string in ImageInfo and BDI

[Max Reitz]

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

Re: [Qemu-devel] [PATCH 1/3] block: Additional info string in ImageInfo and BDI

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH 1/3] block: Additional info string in ImageInfo and BDI

[Max Reitz]

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