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

[Qemu-devel] [PATCH v5 0/6] 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 a new ImageInfoSpecific union type to
ImageInfo which may be used by block drivers as a template for new types
dedicated to the specific information they can provide, as well as a
function bdrv_get_specific_info for retrieving this information. It also
adds support to qemu-img info and qemu-io -c info to print the content
of these specific structures.

v5:
 - In consideration of Stefan's argument about bdrv_round_to_cluster
   being called in the I/O path, removed ImageInfoSpecific from
   BlockDriverInfo and introduced a new function bdrv_get_specific_info
   instead; this also removes the need for bdrv_put_info.
   Resulting changes in regard to the single patches:
    - patch 2: completely different
    - patch 3: qemu-io-cmds.c:info_f now needs to call
      bdrv_get_specific_info in order to receive the new information
    - patch 4: moved the creation of ImageInfoSpecific from qcow2_get_info
      (which now remains unmodified) to the new qcow2_get_specific_info
      function; additionally, register this function as
      bdrv_qcow2.bdrv_get_specific_info
 - rebased on Kevin's block branch, renamed test from 064 to 065 (to
   avoid conflict with his blockdev-add series; affects patch 6)
 - patches 1 and 5 remain unmodified

v4:
 - changed dirty "grep -A 42" for grepping all lines until EOF in test
   064 to cleaner "sed -n '//,$p'" (patch 6)
 - rebased on Kevin's block branch (affects line numbers in patches 2, 3
   and 4 as well as the group file change in patch 6)

v3:
 - implemented Fam's remarks:
   - bdrv_get_info already initializes all fields to NULL, no need to do
     this manually (patch 2)
   - implemented bdrv_put_info as a wrapper to
     qapi_free_ImageInfoSpecific, though this may change with further
     extensions to BlockDriverInfo (patch 2)
   - changed one occurence of puts("foo") to printf("foo\n") in order to
     be consistent with the surrounding code (patch 3)
   - other patches (1, 4, 5, 6) remain unmodified

v2:
 - following Eric's recommendation: changed the representation of the
   format specific information from an uninterpreted blobbed string to a
   union of format specific types


Max Reitz (6):
  qapi: Add ImageInfoSpecific type
  block: Add bdrv_get_specific_info
  block/qapi: Human-readable ImageInfoSpecific dump
  qcow2: Add support for ImageInfoSpecific
  qemu-iotests: Discard specific info in _img_info
  qemu-iotests: Additional info from qemu-img info

 block.c                      |   9 ++++
 block/qapi.c                 | 124 +++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c                |  19 +++++++
 include/block/block.h        |   1 +
 include/block/block_int.h    |   1 +
 include/block/qapi.h         |   2 +
 qapi-schema.json             |  34 +++++++++++-
 qemu-io-cmds.c               |   9 ++++
 tests/qemu-iotests/065       |  72 +++++++++++++++++++++++++
 tests/qemu-iotests/065.out   |  22 ++++++++
 tests/qemu-iotests/common.rc |  19 ++++++-
 tests/qemu-iotests/group     |   1 +
 12 files changed, 311 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/065
 create mode 100644 tests/qemu-iotests/065.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 1/6] qapi: Add ImageInfoSpecific type

[Max Reitz]

Add a new type ImageInfoSpecific as a union for image format specific
information in ImageInfo.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi-schema.json | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 145eca8..cbad705 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -210,6 +210,18 @@
             'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
 
 ##
+# @ImageInfoSpecific:
+#
+# A discriminated record of image format specific information structures.
+#
+# Since: 1.7
+##
+
+{ 'union': 'ImageInfoSpecific',
+  'data': {
+  } }
+
+##
 # @ImageInfo:
 #
 # Information about a QEMU image file
@@ -238,6 +250,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 +263,8 @@
            '*cluster-size': 'int', '*encrypted': 'bool',
            '*backing-filename': 'str', '*full-backing-filename': 'str',
            '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
-           '*backing-image': 'ImageInfo' } }
+           '*backing-image': 'ImageInfo',
+           '*format-specific': 'ImageInfoSpecific' } }
 
 ##
 # @ImageCheck:
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 2/6] block: Add bdrv_get_specific_info

[Max Reitz]

Add a function for retrieving an ImageInfoSpecific object from a block
driver.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 9 +++++++++
 block/qapi.c              | 3 +++
 include/block/block.h     | 1 +
 include/block/block_int.h | 1 +
 4 files changed, 14 insertions(+)

diff --git a/block.c b/block.c
index a2ea39a..4360767 100644
--- a/block.c
+++ b/block.c
@@ -3340,6 +3340,15 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return drv->bdrv_get_info(bs, bdi);
 }
 
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (drv && drv->bdrv_get_specific_info) {
+        return drv->bdrv_get_specific_info(bs);
+    }
+    return NULL;
+}
+
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
diff --git a/block/qapi.c b/block/qapi.c
index 782051c..ab1dd24 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -134,6 +134,9 @@ void bdrv_query_image_info(BlockDriverState *bs,
         info->dirty_flag = bdi.is_dirty;
         info->has_dirty_flag = true;
     }
+    info->format_specific     = bdrv_get_specific_info(bs);
+    info->has_format_specific = info->format_specific != NULL;
+
     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
         info->backing_filename = g_strdup(backing_filename);
diff --git a/include/block/block.h b/include/block/block.h
index f808550..e265124 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,6 +335,7 @@ int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
                           const uint8_t *buf, int nb_sectors);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
                             int64_t sector_num, int nb_sectors,
                             int64_t *cluster_sector_num,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3eeb6fe..5de5b0e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -162,6 +162,7 @@ struct BlockDriver {
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
                                   const char *snapshot_name);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
+    ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
 
     int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
                              int64_t pos);
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 3/6] block/qapi: Human-readable ImageInfoSpecific dump

[Max Reitz]

Add a function for generically dumping the ImageInfoSpecific information
in a human-readable format to block/qapi.c.

Use this function in bdrv_image_info_dump and qemu-io-cmds.c:info_f to
allow qemu-img info resp. qemu-io -c info to print that format specific
information.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qapi.c         | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/qapi.h |   2 +
 qemu-io-cmds.c       |   9 ++++
 3 files changed, 132 insertions(+)

diff --git a/block/qapi.c b/block/qapi.c
index ab1dd24..e7d1591 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -25,6 +25,9 @@
 #include "block/qapi.h"
 #include "block/block_int.h"
 #include "qmp-commands.h"
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
+#include "qapi/qmp/types.h"
 
 /*
  * Returns 0 on success, with *p_list either set to describe snapshot
@@ -426,6 +429,119 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
     }
 }
 
+static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
+                       QDict *dict);
+static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
+                       QList *list);
+
+static void dump_qobject(fprintf_function func_fprintf, void *f,
+                         int comp_indent, QObject *obj)
+{
+    switch (qobject_type(obj)) {
+        case QTYPE_QINT: {
+            QInt *value = qobject_to_qint(obj);
+            func_fprintf(f, "%" PRId64, qint_get_int(value));
+            break;
+        }
+        case QTYPE_QSTRING: {
+            QString *value = qobject_to_qstring(obj);
+            func_fprintf(f, "%s", qstring_get_str(value));
+            break;
+        }
+        case QTYPE_QDICT: {
+            QDict *value = qobject_to_qdict(obj);
+            dump_qdict(func_fprintf, f, comp_indent, value);
+            break;
+        }
+        case QTYPE_QLIST: {
+            QList *value = qobject_to_qlist(obj);
+            dump_qlist(func_fprintf, f, comp_indent, value);
+            break;
+        }
+        case QTYPE_QFLOAT: {
+            QFloat *value = qobject_to_qfloat(obj);
+            func_fprintf(f, "%g", qfloat_get_double(value));
+            break;
+        }
+        case QTYPE_QBOOL: {
+            QBool *value = qobject_to_qbool(obj);
+            func_fprintf(f, "%s", qbool_get_int(value) ? "true" : "false");
+            break;
+        }
+        case QTYPE_QERROR: {
+            QString *value = qerror_human((QError *)obj);
+            func_fprintf(f, "%s", qstring_get_str(value));
+            break;
+        }
+        case QTYPE_NONE:
+            break;
+        case QTYPE_MAX:
+        default:
+            abort();
+    }
+}
+
+static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
+                       QList *list)
+{
+    const QListEntry *entry;
+    int i = 0;
+
+    for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
+        qtype_code type = qobject_type(entry->value);
+        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
+        const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: ";
+
+        func_fprintf(f, format, indentation * 4, "", i);
+        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
+        if (!composite) {
+            func_fprintf(f, "\n");
+        }
+    }
+}
+
+static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
+                       QDict *dict)
+{
+    const QDictEntry *entry;
+
+    for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
+        qtype_code type = qobject_type(entry->value);
+        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
+        const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
+        char key[strlen(entry->key) + 1];
+        int i;
+
+        /* replace dashes with spaces in key (variable) names */
+        for (i = 0; entry->key[i]; i++) {
+            key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
+        }
+        key[i] = 0;
+
+        func_fprintf(f, format, indentation * 4, "", key);
+        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
+        if (!composite) {
+            func_fprintf(f, "\n");
+        }
+    }
+}
+
+void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
+                                   ImageInfoSpecific *info_spec)
+{
+    Error *local_err = NULL;
+    QmpOutputVisitor *ov = qmp_output_visitor_new();
+    QObject *obj, *data;
+
+    visit_type_ImageInfoSpecific(qmp_output_get_visitor(ov), &info_spec, NULL,
+                                 &local_err);
+    obj = qmp_output_get_qobject(ov);
+    assert(qobject_type(obj) == QTYPE_QDICT);
+    data = qdict_get(qobject_to_qdict(obj), "data");
+    dump_qobject(func_fprintf, f, 0, data);
+    qmp_output_visitor_cleanup(ov);
+}
+
 void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
                           ImageInfo *info)
 {
@@ -496,4 +612,9 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
             func_fprintf(f, "\n");
         }
     }
+
+    if (info->has_format_specific) {
+        func_fprintf(f, "Format specific information:\n");
+        bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
+    }
 }
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 0496cc9..9518ee4 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -42,6 +42,8 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 
 void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
                         QEMUSnapshotInfo *sn);
+void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
+                                   ImageInfoSpecific *info_spec);
 void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
                           ImageInfo *info);
 #endif
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 8565d49..667f4e4 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -10,6 +10,7 @@
 
 #include "qemu-io.h"
 #include "block/block_int.h"
+#include "block/qapi.h"
 #include "qemu/main-loop.h"
 
 #define CMD_NOFILE_OK   0x01
@@ -1678,6 +1679,7 @@ static const cmdinfo_t length_cmd = {
 static int info_f(BlockDriverState *bs, int argc, char **argv)
 {
     BlockDriverInfo bdi;
+    ImageInfoSpecific *spec_info;
     char s1[64], s2[64];
     int ret;
 
@@ -1699,6 +1701,13 @@ static int info_f(BlockDriverState *bs, int argc, char **argv)
     printf("cluster size: %s\n", s1);
     printf("vm state offset: %s\n", s2);
 
+    spec_info = bdrv_get_specific_info(bs);
+    if (spec_info) {
+        printf("Format specific information:\n");
+        bdrv_image_info_specific_dump(fprintf, stdout, spec_info);
+        qapi_free_ImageInfoSpecific(spec_info);
+    }
+
     return 0;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 4/6] qcow2: Add support for ImageInfoSpecific

[Max Reitz]

Add a new ImageInfoSpecificQCow2 type as a subtype of ImageInfoSpecific.
This contains the compatibility level as a string and an optional
lazy_refcounts boolean (optional means mandatory for compat >= 1.1 and
not available for compat == 0.10).

Also, add qcow2_get_specific_info, which returns this information.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c    | 19 +++++++++++++++++++
 qapi-schema.json | 16 ++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 318d95d..f080a8a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1810,6 +1810,24 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
+static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    ImageInfoSpecific *spec_info = g_new0(ImageInfoSpecific, 1);
+
+    spec_info->kind = IMAGE_INFO_SPECIFIC_KIND_QCOW2;
+    spec_info->qcow2 = g_new0(ImageInfoSpecificQCow2, 1);
+    if (s->qcow_version == 2) {
+        spec_info->qcow2->compat = g_strdup("0.10");
+    } else if (s->qcow_version == 3) {
+        spec_info->qcow2->compat = g_strdup("1.1");
+        spec_info->qcow2->lazy_refcounts = s->use_lazy_refcounts;
+        spec_info->qcow2->has_lazy_refcounts = true;
+    }
+
+    return spec_info;
+}
+
 #if 0
 static void dump_refcounts(BlockDriverState *bs)
 {
@@ -2130,6 +2148,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_snapshot_list     = qcow2_snapshot_list,
     .bdrv_snapshot_load_tmp     = qcow2_snapshot_load_tmp,
     .bdrv_get_info      = qcow2_get_info,
+    .bdrv_get_specific_info = qcow2_get_specific_info,
 
     .bdrv_save_vmstate    = qcow2_save_vmstate,
     .bdrv_load_vmstate    = qcow2_load_vmstate,
diff --git a/qapi-schema.json b/qapi-schema.json
index cbad705..d697da2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -210,6 +210,21 @@
             'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
 
 ##
+# @ImageInfoSpecificQCow2:
+#
+# @compat: compatibility level
+#
+# @lazy-refcounts: #optional on or off; only valid for compat >= 1.1
+#
+# Since: 1.7
+##
+{ 'type': 'ImageInfoSpecificQCow2',
+  'data': {
+      'compat': 'str',
+      '*lazy-refcounts': 'bool'
+  } }
+
+##
 # @ImageInfoSpecific:
 #
 # A discriminated record of image format specific information structures.
@@ -219,6 +234,7 @@
 
 { 'union': 'ImageInfoSpecific',
   'data': {
+      'qcow2': 'ImageInfoSpecificQCow2'
   } }
 
 ##
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info

[Max Reitz]

In _img_info, filter out additional information specific to the image
format provided by qemu-img info, since tests designed for multiple
image formats would produce different outputs for every image format
else.

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

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 28b39e4..12d8882 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -181,12 +181,29 @@ _check_test_img()
 
 _img_info()
 {
+    discard=0
     $QEMU_IMG info "$@" $TEST_IMG 2>&1 | \
         sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
             -e "s#$TEST_DIR#TEST_DIR#g" \
             -e "s#$IMGFMT#IMGFMT#g" \
             -e "/^disk size:/ D" \
-            -e "/actual-size/ D"
+            -e "/actual-size/ D" | \
+        while IFS='' read line; do
+            if [ "$line" == "Format specific information:" ]; then
+                discard=1
+            elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then
+                discard=2
+                json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"
+            fi
+            if [ $discard == 0 ]; then
+                echo "$line"
+            elif [ $discard == 1 -a -z "$line" ]; then
+                echo
+                discard=0
+            elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then
+                discard=0
+            fi
+        done
 }
 
 _get_pids_by_name()
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 6/6] 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/065     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/065.out | 22 ++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 95 insertions(+)
 create mode 100755 tests/qemu-iotests/065
 create mode 100644 tests/qemu-iotests/065.out

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
new file mode 100755
index 0000000..5c56b56
--- /dev/null
+++ b/tests/qemu-iotests/065
@@ -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
+# additional information we're about to test for
+$QEMU_IMG info "$TEST_IMG" | sed -n '/^Format specific information:$/,$p'
+
+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" | sed -n '/^Format specific information:$/,$p'
+
+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" | sed -n '/^Format specific information:$/,$p'
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/065.out b/tests/qemu-iotests/065.out
new file mode 100644
index 0000000..4dd7f2b
--- /dev/null
+++ b/tests/qemu-iotests/065.out
@@ -0,0 +1,22 @@
+QA output created by 065
+
+=== Testing qcow2 image with -o compat=0.10 ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+Format specific information:
+compat: 0.10
+
+=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=off ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+Format specific information:
+compat: 1.1
+lazy refcounts: false
+
+=== Testing qcow2 image with -o compat=1.1,lazy_refcounts=on ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+Format specific information:
+compat: 1.1
+lazy refcounts: true
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1ad02e5..f1a68b0 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -69,3 +69,4 @@
 061 rw auto
 062 rw auto
 063 rw auto
+065 rw auto
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v5 1/6] qapi: Add ImageInfoSpecific type

[Eric Blake]

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

On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a new type ImageInfoSpecific as a union for image format specific
> information in ImageInfo.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi-schema.json | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)

> @@ -238,6 +250,9 @@
>  #
>  # @backing-image: #optional info of the backing image (since 1.6)
>  #
> +# @info-string: #optional string supplying additional format-specific

s/string/struct/

With that one word fixed, I'm fine with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v5 2/6] block: Add bdrv_get_specific_info

[Eric Blake]

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

On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a function for retrieving an ImageInfoSpecific object from a block
> driver.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   | 9 +++++++++
>  block/qapi.c              | 3 +++
>  include/block/block.h     | 1 +
>  include/block/block_int.h | 1 +
>  4 files changed, 14 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v5 3/6] block/qapi: Human-readable ImageInfoSpecific dump

[Eric Blake]

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

On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a function for generically dumping the ImageInfoSpecific information
> in a human-readable format to block/qapi.c.
> 
> Use this function in bdrv_image_info_dump and qemu-io-cmds.c:info_f to
> allow qemu-img info resp. qemu-io -c info to print that format specific
> information.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qapi.c         | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/qapi.h |   2 +
>  qemu-io-cmds.c       |   9 ++++
>  3 files changed, 132 insertions(+)
> 

> +
> +static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
> +                       QDict *dict)
> +{
> +    const QDictEntry *entry;
> +
> +    for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
> +        qtype_code type = qobject_type(entry->value);
> +        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> +        const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
> +        char key[strlen(entry->key) + 1];
> +        int i;
> +
> +        /* replace dashes with spaces in key (variable) names */
> +        for (i = 0; entry->key[i]; i++) {
> +            key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];

I wonder if this should replace underscores, too.  For now, this
function is only being used on types that happen to use - in key names,
but it seems generic enough that we may want to use it on older types
that followed older conventions.  But I won't force a respin for just
that (if someone DOES reuse this function in the future, they can tweak
underscores at that time, if you don't do it now).

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v5 4/6] qcow2: Add support for ImageInfoSpecific

[Eric Blake]

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

On 09/23/2013 06:09 AM, Max Reitz wrote:
> Add a new ImageInfoSpecificQCow2 type as a subtype of ImageInfoSpecific.
> This contains the compatibility level as a string and an optional
> lazy_refcounts boolean (optional means mandatory for compat >= 1.1 and
> not available for compat == 0.10).
> 
> Also, add qcow2_get_specific_info, which returns this information.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c    | 19 +++++++++++++++++++
>  qapi-schema.json | 16 ++++++++++++++++
>  2 files changed, 35 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v5 5/6] qemu-iotests: Discard specific info in _img_info

[Eric Blake]

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

On 09/23/2013 06:09 AM, Max Reitz wrote:
> In _img_info, filter out additional information specific to the image
> format provided by qemu-img info, since tests designed for multiple
> image formats would produce different outputs for every image format
> else.

s/else/otherwise/

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/common.rc | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 28b39e4..12d8882 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -181,12 +181,29 @@ _check_test_img()
>  
>  _img_info()
>  {
> +    discard=0
>      $QEMU_IMG info "$@" $TEST_IMG 2>&1 | \
>          sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
>              -e "s#$TEST_DIR#TEST_DIR#g" \
>              -e "s#$IMGFMT#IMGFMT#g" \
>              -e "/^disk size:/ D" \
> -            -e "/actual-size/ D"
> +            -e "/actual-size/ D" | \
> +        while IFS='' read line; do
> +            if [ "$line" == "Format specific information:" ]; then

[ ... == ...] is a bashism (thank goodness this is already a bash
script); but I generally prefer you either stick to portable syntax:

if [ "$line" = "Format specific information:" ]

or make it obvious that you know you are using bash:

if [[ $line == "Format specific information:" ]]

> +                discard=1
> +            elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then

Use $(), not ``.

This script is already a bash script; why not exploit that and avoid a fork:

elif [[ $line =~ '"format-specific": {' ]]

> +                discard=2
> +                json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"

Use $(), not ``.

Exploit bash to avoid a fork:

json_indent=${line%%[! ]*}

> +            fi
> +            if [ $discard == 0 ]; then

Again, I don't like the bashism of [ == ].

> +                echo "$line"
> +            elif [ $discard == 1 -a -z "$line" ]; then

[ ... -a ... ] is flat out non-portable.  Even when you are already
requiring bash.  For example:

[ "$str1" -a "$str2" ]

gives status 0 for most pairs of non-empty strings, but could give
status 1 for str1="!" and str2=".".  Using a bashism, on the other hand,
is unambiguous:

elif [[ $discard == 1 && ! $line ]]

> +                echo
> +                discard=0
> +            elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then

Huh?  If we detected json output, then compare whether the current line
with trailing whitespace stripped is now identical to $json_indent; but
based on the above, you set $json_indent to contain JUST whitespace.  A
line with trailing whitespace removed will NEVER match a variable that
contains just whitespace, so this condition will never trigger.  Not to
mention the deprecated `` and non-portable use of == and -a inside [].

> +                discard=0
> +            fi
> +        done

For the human output case, sed can already do everything your 'while
read' loop did:

sed ...
             -e "/^disk size:/ D" \
             -e "/actual-size/ D" \
             -e "/Format specific information/,/^$/ D"

but for the JSON output case, while I'm sure that sed could probably be
coerced into stripping lines until the first line that does not have at
least as much indentation as the line containing "format-specific", the
resulting script wouldn't be very pretty to read (I couldn't quickly
produce one, at any rate - maybe Paolo has more expertise at writing
arcane sed scripts).

This patch might be easier to review if you provided a sample in the
commit message of what input is being stripped by this patch.

-- 
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 v5 6/6] qemu-iotests: Additional info from qemu-img info

[Eric Blake]

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

On 09/23/2013 06:09 AM, Max Reitz wrote:
> 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/065     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/065.out | 22 ++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 95 insertions(+)
>  create mode 100755 tests/qemu-iotests/065
>  create mode 100644 tests/qemu-iotests/065.out

This patch only tests human output, not JSON.

> +# creator
> +owner=mreitz@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`

Not your fault (copy-and-paste from other tests), but as long as we are
requiring bash, $PWD is much faster than `pwd`, and $() is nicer than ``
where we don't have even faster shortcuts like $PWD.


> +=== Testing qcow2 image with -o compat=0.10 ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
> +Format specific information:
> +compat: 0.10

Should we be indenting the human output, to make it obvious how many
remaining fields are being output as a result of format specific
information?

I'm still not happy with 5/6 in its current form, but can live with this
patch as-is if we don't bother with testing JSON form.  Does 5/6 even
need to worry about stripping JSON form, if you aren't going to test
JSON form?  Depending on the answer to that question:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v5 5/6] qemu-iotests: Discard specific info in _img_info

[Max Reitz]

On 2013-09-30 19:14, Eric Blake wrote:
> On 09/23/2013 06:09 AM, Max Reitz wrote:
>> In _img_info, filter out additional information specific to the image
>> format provided by qemu-img info, since tests designed for multiple
>> image formats would produce different outputs for every image format
>> else.
> s/else/otherwise/
>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/common.rc | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 28b39e4..12d8882 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -181,12 +181,29 @@ _check_test_img()
>>   
>>   _img_info()
>>   {
>> +    discard=0
>>       $QEMU_IMG info "$@" $TEST_IMG 2>&1 | \
>>           sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
>>               -e "s#$TEST_DIR#TEST_DIR#g" \
>>               -e "s#$IMGFMT#IMGFMT#g" \
>>               -e "/^disk size:/ D" \
>> -            -e "/actual-size/ D"
>> +            -e "/actual-size/ D" | \
>> +        while IFS='' read line; do
>> +            if [ "$line" == "Format specific information:" ]; then
> [ ... == ...] is a bashism (thank goodness this is already a bash
> script); but I generally prefer you either stick to portable syntax:
>
> if [ "$line" = "Format specific information:" ]
>
> or make it obvious that you know you are using bash:
>
> if [[ $line == "Format specific information:" ]]
>
>> +                discard=1
>> +            elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then
> Use $(), not ``.
>
> This script is already a bash script; why not exploit that and avoid a fork:
>
> elif [[ $line =~ '"format-specific": {' ]]
>
>> +                discard=2
>> +                json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"
> Use $(), not ``.
>
> Exploit bash to avoid a fork:
>
> json_indent=${line%%[! ]*}
>
>> +            fi
>> +            if [ $discard == 0 ]; then
> Again, I don't like the bashism of [ == ].
>
>> +                echo "$line"
>> +            elif [ $discard == 1 -a -z "$line" ]; then
> [ ... -a ... ] is flat out non-portable.  Even when you are already
> requiring bash.  For example:
>
> [ "$str1" -a "$str2" ]
>
> gives status 0 for most pairs of non-empty strings, but could give
> status 1 for str1="!" and str2=".".  Using a bashism, on the other hand,
> is unambiguous:
>
> elif [[ $discard == 1 && ! $line ]]
Okay, I'll rewrite all these to use bash syntax.

>> +                echo
>> +                discard=0
>> +            elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then
> Huh?  If we detected json output, then compare whether the current line
> with trailing whitespace stripped is now identical to $json_indent
Take a closer look: "${json_indent}}," is not "${json_indent}", mind the 
trailing "}," ;)
Therefore, this matches when the current line is the ending brace of the 
"format-specific" structure.

>> +                discard=0
>> +            fi
>> +        done
> For the human output case, sed can already do everything your 'while
> read' loop did:
>
> sed ...
>               -e "/^disk size:/ D" \
>               -e "/actual-size/ D" \
>               -e "/Format specific information/,/^$/ D"
Oh, nice; sorry, but multiline sed regexes are something I know 
basically nothing about. I'd leave the script as it is for now, anyway 
(while implementing your remarks), because of the JSON filter and since 
I don't think this to be the bottleneck in test cases.

In your review to patch 6 you asked whether the JSON filter is actually 
necessary: Test 043 is a shell script which simply queries the JSON 
output (in addition to the human-readable version) and is valid for 
multiple image formats (qcow2 and qed), therefore the format specific 
information has to be filtered out, both for human-readable and JSON output.

Max

Re: [Qemu-devel] [PATCH v5 6/6] qemu-iotests: Additional info from qemu-img info

[Max Reitz]

On 2013-09-30 19:19, Eric Blake wrote:
> On 09/23/2013 06:09 AM, Max Reitz wrote:
>> 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/065     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/065.out | 22 ++++++++++++++
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 95 insertions(+)
>>   create mode 100755 tests/qemu-iotests/065
>>   create mode 100644 tests/qemu-iotests/065.out
> This patch only tests human output, not JSON.
I'd not be happy at all with testing JSON output through a shell script 
(or rather, without properly parsing it). Since I don't really see the 
point of testing the JSON output as well: It is basically generated the 
same way as the human-readable output, however, the latter uses the new 
bdrv_image_info_specific_dump function, so I think the human-readable 
output is actually more "error-prone" (and if something breaks the JSON 
output that doesn't break other JSON tests, it should break the 
human-readable output as well).

So I'll leave the JSON test out and include a note in the commit message.

>> +# creator
>> +owner=mreitz@redhat.com
>> +
>> +seq=`basename $0`
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
> Not your fault (copy-and-paste from other tests), but as long as we are
> requiring bash, $PWD is much faster than `pwd`, and $() is nicer than ``
> where we don't have even faster shortcuts like $PWD.
Well, it's executed just once per test, so it shouldn't be that much of 
a performance killer, but I'll change it anyway, thanks. ;)

>> +=== Testing qcow2 image with -o compat=0.10 ===
>> +
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>> +Format specific information:
>> +compat: 0.10
> Should we be indenting the human output, to make it obvious how many
> remaining fields are being output as a result of format specific
> information?
Seems very reasonable. I'll have to change one of the other patches for 
this as well, but this should be a very minor change.

> I'm still not happy with 5/6 in its current form, but can live with this
> patch as-is if we don't bother with testing JSON form.  Does 5/6 even
> need to worry about stripping JSON form, if you aren't going to test
> JSON form?
As I've said in my answer to patch 5, stripping JSON isn't for this new 
test, but rather for compatibility with the old tests (specifically, 
test 043).

Max