[PATCH v3 0/3] Dump QCOW2 metadata

[PATCH v3 0/3] Dump QCOW2 metadata

[Andrey Shinkevich]

The information about QCOW2 metadata allocations in an image ELF-file is
helpful for finding issues with the image data integrity.

v3: Descriptions of the new key option were added. The names of identifiers
    were amended. The QEMU-IMG key options were put in the sorted order in
    the code. All suggested by Eric. Discussed in the email thread with ID
    <1577447039-400109-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Snapshots dump example:

$ sudo ./qemu-img check /.../.../harddisk.hdd -M --output=json
{
    "image-end-offset": 24820842496,
    "total-clusters": 153600,
    "check-errors": 0,
    "viscera": {
        "refcount-table": {
            "location": {
                "offset": 3845128192,
                "size": 1048576
            }
        },
        "active-l1": {
            "name": "L1 active table",
            "location": {
                "offset": 4194304,
                "size": 16
            },
            "l2-list": [
                {
                    "offset": 619708416,
                    "size": 1048576
                },
                {
                    "offset": 1156579328,
                    "size": 1048576
                }
            ]
        },
        "qcow2-header": {
            "location": {
                "offset": 0,
                "size": 1048576
            },
            "version": 3
        },
        "snapshot-table": {
            "location": {
                "offset": 648019968,
                "size": 191
            },
            "l1-list": [
                {
                    "name": "{3036f6c5-3a1f-44cb-af1f-653cc87fba04}",
                    "location": {
                        "offset": 14680064,
                        "size": 16
                    },
                    "l2-list": [
                        {
                            "offset": 3957325824,
                            "size": 1048576
                        },
                        {
                            "offset": 7025459200,
                            "size": 1048576
                        }
                    ]
                },
                {
                    "name": "{0aa1a7d6-16ee-4b44-a515-b5ecc571c959}",
                    "location": {
                        "offset": 638582784,
                        "size": 16
                    },
                    "l2-list": [
                        {
                            "offset": 3957325824,
                            "size": 1048576
                        },
                        {
                            "offset": 7025459200,
                            "size": 1048576
                        }
                    ]
                }
            ]
        }
    },
    "allocated-clusters": 22485,
    "filename": "/.../.../harddisk.hdd",
    "format": "qcow2",
    "fragmented-clusters": 3549
}

Bitmaps dump example:

$ ./qemu-img check /home/disk -M --output=json
{
    "image-end-offset": 1441792,
    "total-clusters": 16,
    "check-errors": 0,
    "viscera": {
        "refcount-table": {
            "location": {
                "offset": 65536,
                "size": 65536
            }
        },
        "active-l1": {
            "name": "L1 active table",
            "location": {
                "offset": 196608,
                "size": 8
            },
            "l2-list": [
                {
                    "offset": 262144,
                    "size": 65536
                }
            ]
        },
        "bitmaps": {
            "bitmap-dir": {
                "location": {
                    "offset": 1048576,
                    "size": 64
                },
                "dir-entries": [
                    {
                        "bitmap-table": {
                            "location": {
                                "offset": 589824,
                                "size": 8
                            },
                            "table-entries": [
                                {
                                    "type": "all-zeros"
                                }
                            ]
                        },
                        "bitmap-name": "bitmap-1"
                    },
                    {
                        "bitmap-table": {
                            "location": {
                                "offset": 983040,
                                "size": 8
                            },
                            "table-entries": [
                                {
                                    "cluster": {
                                        "offset": 655360,
                                        "size": 65536
                                    },
                                    "type": "serialized"
                                }
                            ]
                        },
                        "bitmap-name": "bitmap-2"
                    }
                ]
            },
            "nb-bitmaps": 2
        },
        "qcow2-header": {
            "location": {
                "offset": 0,
                "size": 65536
            },
            "version": 3
        }
    },
    "allocated-clusters": 12,
    "filename": "/home/disk",
    "format": "qcow2",
    "fragmented-clusters": 2
}

Andrey Shinkevich (3):
  qcow2: introduce Qcow2Metadata structure
  qemu-img: sort key options alphabetically
  qcow2: dump QCOW2 metadata

 block/qcow2-bitmap.c   |  54 ++++++++++++-
 block/qcow2-refcount.c |  84 ++++++++++++++++----
 block/qcow2.c          |  30 +++++++
 block/qcow2.h          |   6 +-
 include/block/block.h  |   3 +-
 qapi/block-core.json   | 209 ++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-img.c             |  50 +++++++++---
 qemu-img.texi          |   7 +-
 8 files changed, 408 insertions(+), 35 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/3] qcow2: introduce Qcow2Metadata structure

[Andrey Shinkevich]

The preliminary patch to provide an extendable structure for dumping
QCOW2 metadata allocations in image. The command line optional key is
introduced in the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 qapi/block-core.json | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 208 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ff5e5e..fab7435 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -176,6 +176,209 @@
            '*backing-image': 'ImageInfo',
            '*format-specific': 'ImageInfoSpecific' } }
 
+
+##
+# @Qcow2Metadata:
+#
+# Encapsulates QCOW2 metadata information
+#
+# @qcow2-header: QCOW2 header info
+#
+# @active-l1: L1 active table info
+#
+# @refcount-table: refcount table info
+#
+# @crypt-header: encryption header info
+#
+# @bitmaps: bitmap tables info
+#
+# @snapshot-table: snapshot tables info
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2Metadata',
+  'data': { 'qcow2-header': 'Qcow2Header',
+            'refcount-table': 'Qcow2RefcountTable',
+            'active-l1': 'Qcow2L1Table',
+            '*crypt-header': 'Qcow2EncryptionHeader',
+            '*bitmaps': 'Qcow2Bitmaps',
+            '*snapshot-table': 'Qcow2SnapshotsTable' } }
+
+##
+# @Qcow2Header:
+#
+# QCOW2 header information
+#
+# @version: version number
+#
+# @location: header offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2Header',
+  'data': {'version': 'int',
+           'location': 'Qcow2Allocation' } }
+
+##
+# @Qcow2EncryptionHeader:
+#
+# QCOW2 encryption header information
+#
+# @location: header offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2EncryptionHeader',
+  'data': {'location': 'Qcow2Allocation' } }
+
+##
+# @Qcow2RefcountTable:
+#
+# QCOW2 refcount table information
+#
+# @location: refcount table offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2RefcountTable',
+  'data': {'location': 'Qcow2Allocation' } }
+
+##
+# @Qcow2L1Table:
+#
+# L1 table information
+#
+# @l2-list: list of L2 table locations
+#
+# @location: L1 table offset and size in image
+#
+# @name: entity name the table relates to
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2L1Table',
+  'data': {'l2-list': ['Qcow2Allocation'],
+           'location': 'Qcow2Allocation',
+           'name': 'str'} }
+
+##
+# @Qcow2Allocation:
+#
+# QCOW2 data location in image
+#
+# @offset: data offset
+#
+# @size: data size
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2Allocation',
+  'data': {'offset': 'uint64', 'size': 'uint64' } }
+
+##
+# @Qcow2Bitmaps:
+#
+# QCOW2 bitmaps information
+#
+# @nb-bitmaps: the number of bitmaps contained in the image
+#
+# @bitmap-dir: bitmap directory information
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2Bitmaps',
+  'data': {'nb-bitmaps': 'int',
+           'bitmap-dir': 'Qcow2BitmapDir' } }
+
+##
+# @Qcow2BitmapDir:
+#
+# QCOW2 bitmap directory information
+#
+# @dir-entries: list of bitmap directory entries
+#
+# @location: bitmap directory offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2BitmapDir',
+  'data': {'dir-entries': ['Qcow2BitmapDirectoryEntry'],
+           'location': 'Qcow2Allocation' } }
+
+##
+# @Qcow2BitmapDirectoryEntry:
+#
+# QCOW2 bitmap directory entry information
+#
+# @bitmap-table: bitmap table offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2BitmapDirectoryEntry',
+  'data': {'bitmap-table': 'Qcow2BitmapTableInfo',
+           'bitmap-name': 'str' } }
+
+##
+# @Qcow2BitmapTableInfo:
+#
+# QCOW2 bitmap table information
+#
+# @table-entries: list of bitmap table entries
+#
+# @location: bitmap table offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2BitmapTableInfo',
+  'data': {'table-entries': ['Qcow2BitmapTableInfoEntry'],
+           'location': 'Qcow2Allocation' } }
+
+##
+# @Qcow2BitmapTableInfoEntry:
+#
+# QCOW2 bitmap table entry information
+#
+# @type: bitmap table entry type
+#
+# @cluster: bitmap table entry offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2BitmapTableInfoEntry',
+  'data': {'type': 'Qcow2BitmapTableInfoEntryType',
+           '*cluster': 'Qcow2Allocation' } }
+
+##
+# @Qcow2BitmapTableInfoEntryType:
+#
+# An enumeration of cluster types in bitmap table
+#
+# @all-zeroes: cluster should be read as all zeroes
+#
+# @all-ones: cluster should be read as all ones
+#
+# @serialized: cluster data are written on disk
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2BitmapTableInfoEntryType',
+  'data': ['all-zeroes', 'all-ones', 'serialized'] }
+
+##
+# @Qcow2SnapshotsTable:
+#
+# Snapshots table location in image file.
+#
+# @location: offset and size of snapshot table
+#
+# @l1-list: list of snapshots L1 tables
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2SnapshotsTable',
+  'data': {'location': 'Qcow2Allocation',
+           'l1-list': ['Qcow2L1Table'] } }
+
 ##
 # @ImageCheck:
 #
@@ -215,6 +418,9 @@
 #                       field is present if the driver for the image format
 #                       supports it
 #
+# @metadata: encapsulates QCOW2 tables allocation information (default: none,
+#            turned on with the command line optional key; since 5.0)
+#
 # Since: 1.4
 #
 ##
@@ -223,7 +429,8 @@
            '*image-end-offset': 'int', '*corruptions': 'int', '*leaks': 'int',
            '*corruptions-fixed': 'int', '*leaks-fixed': 'int',
            '*total-clusters': 'int', '*allocated-clusters': 'int',
-           '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
+           '*fragmented-clusters': 'int', '*compressed-clusters': 'int',
+           '*metadata': 'Qcow2Metadata' } }
 
 ##
 # @MapEntry:
-- 
1.8.3.1

[PATCH v3 2/3] qemu-img: sort key options alphabetically

[Andrey Shinkevich]

Put qemu-img key options in the alphabetical order in the code
for a faster finding.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 qemu-img.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6233b8c..c86f760 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -703,7 +703,7 @@ static int img_check(int argc, char **argv)
             {"force-share", no_argument, 0, 'U'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hf:r:T:qU",
+        c = getopt_long(argc, argv, ":f:hqr:T:U",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -715,11 +715,14 @@ static int img_check(int argc, char **argv)
         case '?':
             unrecognized_option(argv[optind - 1]);
             break;
+        case 'f':
+            fmt = optarg;
+            break;
         case 'h':
             help();
             break;
-        case 'f':
-            fmt = optarg;
+        case 'q':
+            quiet = true;
             break;
         case 'r':
             flags |= BDRV_O_RDWR;
@@ -733,18 +736,15 @@ static int img_check(int argc, char **argv)
                            "(expecting 'leaks' or 'all'): %s", optarg);
             }
             break;
-        case OPTION_OUTPUT:
-            output = optarg;
-            break;
         case 'T':
             cache = optarg;
             break;
-        case 'q':
-            quiet = true;
-            break;
         case 'U':
             force_share = true;
             break;
+        case OPTION_IMAGE_OPTS:
+            image_opts = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -753,8 +753,8 @@ static int img_check(int argc, char **argv)
                 return 1;
             }
         }   break;
-        case OPTION_IMAGE_OPTS:
-            image_opts = true;
+        case OPTION_OUTPUT:
+            output = optarg;
             break;
         }
     }
-- 
1.8.3.1

[PATCH v3 3/3] qcow2: dump QCOW2 metadata

[Andrey Shinkevich]

Let QEMU-IMG CHECK command show QCOW2 structure to inform a user about
metadata allocations on disk. Introduce '-M'('--dump-meta') key option.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/qcow2-bitmap.c   | 54 +++++++++++++++++++++++++++++---
 block/qcow2-refcount.c | 84 +++++++++++++++++++++++++++++++++++++++++---------
 block/qcow2.c          | 30 ++++++++++++++++++
 block/qcow2.h          |  6 ++--
 include/block/block.h  |  3 +-
 qemu-img.c             | 30 +++++++++++++++++-
 qemu-img.texi          |  7 ++++-
 7 files changed, 190 insertions(+), 24 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d41f5d0..15e035a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -659,20 +659,29 @@ fail:
 
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
-                                  int64_t *refcount_table_size)
+                                  int64_t *refcount_table_size,
+                                  Qcow2Bitmaps *bitmaps)
 {
     int ret;
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
     Qcow2Bitmap *bm;
+    Qcow2BitmapDirectoryEntryList **pp_dir =
+        bitmaps ? &bitmaps->bitmap_dir->dir_entries : NULL;
 
     if (s->nb_bitmaps == 0) {
         return 0;
     }
 
+    if (bitmaps) {
+        bitmaps->nb_bitmaps = s->nb_bitmaps;
+    }
+
     ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
                                    s->bitmap_directory_offset,
-                                   s->bitmap_directory_size);
+                                   s->bitmap_directory_size,
+                                   bitmaps ? bitmaps->bitmap_dir->location
+                                   : NULL);
     if (ret < 0) {
         return ret;
     }
@@ -686,12 +695,28 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
         uint64_t *bitmap_table = NULL;
+        Qcow2BitmapTableInfoEntryList **pp_table;
         int i;
 
+        Qcow2BitmapDirectoryEntry *bmde = NULL;
+        if (bitmaps) {
+            bmde = g_new0(Qcow2BitmapDirectoryEntry, 1);
+            bmde->bitmap_name = g_strdup(bm->name);
+            bmde->bitmap_table = g_new0(Qcow2BitmapTableInfo, 1);
+            bmde->bitmap_table->location = g_new0(Qcow2Allocation, 1);
+            Qcow2BitmapDirectoryEntryList *obj =
+                g_new0(Qcow2BitmapDirectoryEntryList, 1);
+            obj->value = bmde;
+            *pp_dir = obj;
+            pp_dir = &obj->next;
+        }
+
         ret = qcow2_inc_refcounts_imrt(bs, res,
                                        refcount_table, refcount_table_size,
                                        bm->table.offset,
-                                       bm->table.size * sizeof(uint64_t));
+                                       bm->table.size * sizeof(uint64_t),
+                                       bmde ? bmde->bitmap_table->location
+                                       : NULL);
         if (ret < 0) {
             goto out;
         }
@@ -702,6 +727,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
             goto out;
         }
 
+        pp_table = bmde ? &bmde->bitmap_table->table_entries : NULL;
+
         for (i = 0; i < bm->table.size; ++i) {
             uint64_t entry = bitmap_table[i];
             uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
@@ -711,13 +738,32 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                 continue;
             }
 
+            Qcow2BitmapTableInfoEntry *bmte = NULL;
+            if (bmde) {
+                bmte = g_new0(Qcow2BitmapTableInfoEntry, 1);
+                bmte->type = offset ?
+                    QCOW2_BITMAP_TABLE_INFO_ENTRY_TYPE_SERIALIZED :
+                    entry & BME_TABLE_ENTRY_FLAG_ALL_ONES;
+                if (offset) {
+                    bmte->cluster = g_new0(Qcow2Allocation, 1);
+                }
+                bmte->has_cluster = !!(bmte->cluster);
+                Qcow2BitmapTableInfoEntryList *elem =
+                    g_new0(Qcow2BitmapTableInfoEntryList, 1);
+                elem->value = bmte;
+                *pp_table = elem;
+                pp_table = &elem->next;
+            }
+
             if (offset == 0) {
                 continue;
             }
 
             ret = qcow2_inc_refcounts_imrt(bs, res,
                                            refcount_table, refcount_table_size,
-                                           offset, s->cluster_size);
+                                           offset, s->cluster_size,
+                                           bmte && bmte->cluster ? bmte->cluster
+                                           : NULL);
             if (ret < 0) {
                 g_free(bitmap_table);
                 goto out;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f67ac6b..f5444fc 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1521,7 +1521,8 @@ static int realloc_refcount_array(BDRVQcow2State *s, void **array,
 int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
                              void **refcount_table,
                              int64_t *refcount_table_size,
-                             int64_t offset, int64_t size)
+                             int64_t offset, int64_t size,
+                             Qcow2Allocation *qcow2_alloc)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t start, last, cluster_offset, k, refcount;
@@ -1550,6 +1551,11 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
         return 0;
     }
 
+    if (qcow2_alloc) {
+        qcow2_alloc->offset = offset;
+        qcow2_alloc->size = size;
+    }
+
     start = start_of_cluster(s, offset);
     last = start_of_cluster(s, offset + size - 1);
     for(cluster_offset = start; cluster_offset <= last;
@@ -1643,7 +1649,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
             ret = qcow2_inc_refcounts_imrt(
                 bs, res, refcount_table, refcount_table_size,
                 l2_entry & QCOW2_COMPRESSED_SECTOR_MASK,
-                nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE);
+                nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE, NULL);
             if (ret < 0) {
                 goto fail;
             }
@@ -1731,7 +1737,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
             if (!has_data_file(bs)) {
                 ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table,
                                                refcount_table_size,
-                                               offset, s->cluster_size);
+                                               offset, s->cluster_size, NULL);
                 if (ret < 0) {
                     goto fail;
                 }
@@ -1769,17 +1775,20 @@ static int check_refcounts_l1(BlockDriverState *bs,
                               void **refcount_table,
                               int64_t *refcount_table_size,
                               int64_t l1_table_offset, int l1_size,
-                              int flags, BdrvCheckMode fix, bool active)
+                              int flags, BdrvCheckMode fix, bool active,
+                              Qcow2L1Table *l1t)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t *l1_table = NULL, l2_offset, l1_size2;
+    Qcow2AllocationList **plist = l1t ? &l1t->l2_list : NULL;
     int i, ret;
 
     l1_size2 = l1_size * sizeof(uint64_t);
 
     /* Mark L1 table as used */
     ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
-                                   l1_table_offset, l1_size2);
+                                   l1_table_offset, l1_size2,
+                                   l1t ? l1t->location : NULL);
     if (ret < 0) {
         goto fail;
     }
@@ -1808,9 +1817,19 @@ static int check_refcounts_l1(BlockDriverState *bs,
         if (l2_offset) {
             /* Mark L2 table as used */
             l2_offset &= L1E_OFFSET_MASK;
+
+            Qcow2Allocation *l2t = NULL;
+            if (l1t) {
+                l2t = g_new0(Qcow2Allocation, 1);
+                Qcow2AllocationList *obj = g_new0(Qcow2AllocationList, 1);
+                obj->value = l2t;
+                *plist = obj;
+                plist = &obj->next;
+            }
+
             ret = qcow2_inc_refcounts_imrt(bs, res,
                                            refcount_table, refcount_table_size,
-                                           l2_offset, s->cluster_size);
+                                           l2_offset, s->cluster_size, l2t);
             if (ret < 0) {
                 goto fail;
             }
@@ -2047,7 +2066,7 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
                 res->corruptions_fixed++;
                 ret = qcow2_inc_refcounts_imrt(bs, res,
                                                refcount_table, nb_clusters,
-                                               offset, s->cluster_size);
+                                               offset, s->cluster_size, NULL);
                 if (ret < 0) {
                     return ret;
                 }
@@ -2066,7 +2085,7 @@ resize_fail:
 
         if (offset != 0) {
             ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
-                                           offset, s->cluster_size);
+                                           offset, s->cluster_size, NULL);
             if (ret < 0) {
                 return ret;
             }
@@ -2093,6 +2112,9 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     BDRVQcow2State *s = bs->opaque;
     int64_t i;
     QCowSnapshot *sn;
+    bool has_snapshots = res->metadata && res->metadata->snapshot_table;
+    Qcow2L1TableList **plist = has_snapshots ?
+        &res->metadata->snapshot_table->l1_list : NULL;
     int ret;
 
     if (!*refcount_table) {
@@ -2106,16 +2128,25 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     }
 
     /* header */
+    if (res->metadata) {
+        res->metadata->qcow2_header->version = s->qcow_version;
+    }
     ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
-                                   0, s->cluster_size);
+                                   0, s->cluster_size,
+                                   res->metadata ?
+                                   res->metadata->qcow2_header->location : NULL);
     if (ret < 0) {
         return ret;
     }
 
     /* current L1 table */
+    if (res->metadata) {
+        res->metadata->active_l1->name = g_strdup("L1 active table");
+    }
     ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
                              s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO,
-                             fix, true);
+                             fix, true,
+                             res->metadata ? res->metadata->active_l1 : NULL);
     if (ret < 0) {
         return ret;
     }
@@ -2143,15 +2174,30 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
             res->corruptions++;
             continue;
         }
+
+        Qcow2L1Table *l1t = NULL;
+        if (has_snapshots) {
+            l1t = g_new0(Qcow2L1Table, 1);
+            l1t->location = g_new0(Qcow2Allocation, 1);
+            l1t->name = g_strdup(sn->name);
+            Qcow2L1TableList *obj = g_new0(Qcow2L1TableList, 1);
+            obj->value = l1t;
+            *plist = obj;
+            plist = &obj->next;
+        }
+
         ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
                                  sn->l1_table_offset, sn->l1_size, 0, fix,
-                                 false);
+                                 false, l1t);
         if (ret < 0) {
             return ret;
         }
     }
     ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
-                                   s->snapshots_offset, s->snapshots_size);
+                                   s->snapshots_offset, s->snapshots_size,
+                                   has_snapshots ?
+                                   res->metadata->snapshot_table->location
+                                   : NULL);
     if (ret < 0) {
         return ret;
     }
@@ -2159,7 +2205,10 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     /* refcount data */
     ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
                                    s->refcount_table_offset,
-                                   s->refcount_table_size * sizeof(uint64_t));
+                                   s->refcount_table_size * sizeof(uint64_t),
+                                   res->metadata ?
+                                   res->metadata->refcount_table->location
+                                   : NULL);
     if (ret < 0) {
         return ret;
     }
@@ -2168,14 +2217,19 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     if (s->crypto_header.length) {
         ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
                                        s->crypto_header.offset,
-                                       s->crypto_header.length);
+                                       s->crypto_header.length,
+                                       res->metadata ?
+                                       res->metadata->crypt_header->location
+                                       : NULL);
         if (ret < 0) {
             return ret;
         }
     }
 
     /* bitmaps */
-    ret = qcow2_check_bitmaps_refcounts(bs, res, refcount_table, nb_clusters);
+    ret = qcow2_check_bitmaps_refcounts(bs, res, refcount_table, nb_clusters,
+                                        res->metadata ? res->metadata->bitmaps
+                                        : NULL);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index cef9d72..634b642 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -590,6 +590,7 @@ static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
                                               BdrvCheckResult *result,
                                               BdrvCheckMode fix)
 {
+    BDRVQcow2State *s = bs->opaque;
     BdrvCheckResult snapshot_res = {};
     BdrvCheckResult refcount_res = {};
     int ret;
@@ -602,6 +603,35 @@ static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
         return ret;
     }
 
+    if (fix & BDRV_DUMP_META) {
+        result->metadata = g_new0(Qcow2Metadata, 1);
+        result->metadata->qcow2_header = g_new0(Qcow2Header, 1);
+        result->metadata->qcow2_header->location = g_new0(Qcow2Allocation, 1);
+        result->metadata->active_l1 = g_new0(Qcow2L1Table, 1);
+        result->metadata->active_l1->location = g_new0(Qcow2Allocation, 1);
+        result->metadata->refcount_table = g_new0(Qcow2RefcountTable, 1);
+        result->metadata->refcount_table->location = g_new0(Qcow2Allocation, 1);
+
+        refcount_res.metadata = result->metadata;
+
+        if (s->crypto_header.length) {
+            result->metadata->crypt_header = g_new0(Qcow2EncryptionHeader, 1);
+            result->metadata->crypt_header->location =
+                g_new0(Qcow2Allocation, 1);
+        }
+        if (s->nb_bitmaps) {
+            result->metadata->bitmaps = g_new0(Qcow2Bitmaps, 1);
+            result->metadata->bitmaps->bitmap_dir = g_new0(Qcow2BitmapDir, 1);
+            result->metadata->bitmaps->bitmap_dir->location =
+                g_new0(Qcow2Allocation, 1);
+        }
+        if (s->nb_snapshots) {
+            result->metadata->snapshot_table = g_new0(Qcow2SnapshotsTable, 1);
+            result->metadata->snapshot_table->location =
+                g_new0(Qcow2Allocation, 1);
+        }
+    }
+
     ret = qcow2_check_refcounts(bs, &refcount_res, fix);
     qcow2_add_check_result(result, &refcount_res, true);
     if (ret < 0) {
diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126..8d615e2 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -662,7 +662,8 @@ int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
 int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
                              void **refcount_table,
                              int64_t *refcount_table_size,
-                             int64_t offset, int64_t size);
+                             int64_t offset, int64_t size,
+                             Qcow2Allocation *qcow2_alloc);
 
 int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
                                 BlockDriverAmendStatusCB *status_cb,
@@ -751,7 +752,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);
 /* qcow2-bitmap.c functions */
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
-                                  int64_t *refcount_table_size);
+                                  int64_t *refcount_table_size,
+                                  Qcow2Bitmaps *bitmaps);
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
                                                 Error **errp);
diff --git a/include/block/block.h b/include/block/block.h
index e9dcfef..cfc9d68 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -374,7 +374,6 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
                               Error **errp);
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
 
-
 typedef struct BdrvCheckResult {
     int corruptions;
     int leaks;
@@ -383,11 +382,13 @@ typedef struct BdrvCheckResult {
     int leaks_fixed;
     int64_t image_end_offset;
     BlockFragInfo bfi;
+    Qcow2Metadata *metadata;
 } BdrvCheckResult;
 
 typedef enum {
     BDRV_FIX_LEAKS    = 1,
     BDRV_FIX_ERRORS   = 2,
+    BDRV_DUMP_META    = 4,
 } BdrvCheckMode;
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
diff --git a/qemu-img.c b/qemu-img.c
index c86f760..f534f44 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -173,6 +173,7 @@ static void QEMU_NORETURN help(void)
            "       '-r leaks' repairs only cluster leaks, whereas '-r all' fixes all\n"
            "       kinds of errors, with a higher risk of choosing the wrong fix or\n"
            "       hiding corruption that has already occurred.\n"
+           "  '-M' Dump qcow2 metadata to stdout in JSON format.\n"
            "\n"
            "Parameters to convert subcommand:\n"
            "  '-m' specifies how many coroutines work in parallel during the convert\n"
@@ -659,6 +660,15 @@ static int collect_image_check(BlockDriverState *bs,
     check->has_fragmented_clusters  = result.bfi.fragmented_clusters != 0;
     check->compressed_clusters      = result.bfi.compressed_clusters;
     check->has_compressed_clusters  = result.bfi.compressed_clusters != 0;
+    check->metadata                  = result.metadata;
+    check->has_metadata              = !!(result.metadata);
+
+    if (check->has_metadata) {
+        check->metadata->has_crypt_header = !!(check->metadata->crypt_header);
+        check->metadata->has_bitmaps = !!(check->metadata->bitmaps);
+        check->metadata->has_snapshot_table =
+            !!(check->metadata->snapshot_table);
+    }
 
     return 0;
 }
@@ -701,9 +711,10 @@ static int img_check(int argc, char **argv)
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {"force-share", no_argument, 0, 'U'},
+            {"dump-meta", no_argument, 0, 'M'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":f:hqr:T:U",
+        c = getopt_long(argc, argv, ":f:hMqr:T:U",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -724,6 +735,9 @@ static int img_check(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'M':
+            fix |= BDRV_DUMP_META;
+            break;
         case 'r':
             flags |= BDRV_O_RDWR;
 
@@ -772,6 +786,11 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
+    if ((fix & BDRV_DUMP_META) && output_format != OFORMAT_JSON) {
+        error_report("Metadata output is in JSON format only");
+        return 1;
+    }
+
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
                           qemu_img_object_print_help, &error_fatal)) {
@@ -792,6 +811,15 @@ static int img_check(int argc, char **argv)
     bs = blk_bs(blk);
 
     check = g_new0(ImageCheck, 1);
+
+    if (fix & BDRV_DUMP_META) {
+        if (strcmp(bs->drv->format_name, "qcow2")) {
+            error_report("Metadata output supported for QCOW2 format only");
+            ret = -ENOTSUP;
+            goto fail;
+        }
+    }
+
     ret = collect_image_check(bs, check, filename, fmt, fix);
 
     if (ret == -ENOTSUP) {
diff --git a/qemu-img.texi b/qemu-img.texi
index b5156d6..352b458 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -230,7 +230,7 @@ specified as well.
 For write tests, by default a buffer filled with zeros is written. This can be
 overridden with a pattern byte specified by @var{pattern}.
 
-@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] @var{filename}
+@item check [--object @var{objectdef}] [--image-opts] [-M] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] @var{filename}
 
 Perform a consistency check on the disk image @var{filename}. The command can
 output in the format @var{ofmt} which is either @code{human} or @code{json}.
@@ -241,6 +241,11 @@ during the check. @code{-r leaks} repairs only cluster leaks, whereas
 @code{-r all} fixes all kinds of errors, with a higher risk of choosing the
 wrong fix or hiding corruption that has already occurred.
 
+If @code{-M} is specified, qemu-img dumps metadata allocations in the image.
+This option works with @code{json} format output and is effective for the
+@code{qcow2} format only. See type Qcow2Metadata in the QAPI docs for how
+the JSON output will be formatted.
+
 Only the formats @code{qcow2}, @code{qed} and @code{vdi} support
 consistency checks.
 
-- 
1.8.3.1

Re: [PATCH v3 1/3] qcow2: introduce Qcow2Metadata structure

[Vladimir Sementsov-Ogievskiy]

14.01.2020 11:22, Andrey Shinkevich wrote:
> The preliminary patch to provide an extendable structure for dumping
> QCOW2 metadata allocations in image. The command line optional key is
> introduced in the patch that follows.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   qapi/block-core.json | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 208 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ff5e5e..fab7435 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -176,6 +176,209 @@
>              '*backing-image': 'ImageInfo',
>              '*format-specific': 'ImageInfoSpecific' } }
>   
> +
> +##
> +# @Qcow2Metadata:
> +#
> +# Encapsulates QCOW2 metadata information
> +#
> +# @qcow2-header: QCOW2 header info
> +#
> +# @active-l1: L1 active table info
> +#
> +# @refcount-table: refcount table info
> +#
> +# @crypt-header: encryption header info
> +#
> +# @bitmaps: bitmap tables info
> +#
> +# @snapshot-table: snapshot tables info
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Metadata',
> +  'data': { 'qcow2-header': 'Qcow2Header',
> +            'refcount-table': 'Qcow2RefcountTable',
> +            'active-l1': 'Qcow2L1Table',
> +            '*crypt-header': 'Qcow2EncryptionHeader',
> +            '*bitmaps': 'Qcow2Bitmaps',
> +            '*snapshot-table': 'Qcow2SnapshotsTable' } }
> +
> +##
> +# @Qcow2Header:
> +#
> +# QCOW2 header information
> +#
> +# @version: version number
> +#
> +# @location: header offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Header',
> +  'data': {'version': 'int',
> +           'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2EncryptionHeader:
> +#
> +# QCOW2 encryption header information
> +#
> +# @location: header offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2EncryptionHeader',
> +  'data': {'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2RefcountTable:
> +#
> +# QCOW2 refcount table information
> +#
> +# @location: refcount table offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2RefcountTable',
> +  'data': {'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2L1Table:
> +#
> +# L1 table information
> +#
> +# @l2-list: list of L2 table locations
> +#
> +# @location: L1 table offset and size in image
> +#
> +# @name: entity name the table relates to
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2L1Table',
> +  'data': {'l2-list': ['Qcow2Allocation'],

If we list only allocated l2, we lost information about their
idexes.

I think we need instead

'entries': ['Qcow2L1TableEntry'],


> +           'location': 'Qcow2Allocation',
> +           'name': 'str'} }

L1 table doesn't has name. Name is a property of snapshot.
I'd prefer to follow object model defined in docs/interop/qcow2.txt as
close as possible, with the only exception for "location" property, which
may be addeded to data structures like Qcow2L1Table.. (and I don't think
we need to add location into each Qcow2L1TableEntry), and dropping
corresponding offset and size fields, which are mirrored to the location
object.

Generic location object is good to automatically parse resulting json, to
make a view of it.

> +
> +##
> +# @Qcow2Allocation:
> +#
> +# QCOW2 data location in image
> +#
> +# @offset: data offset
> +#
> +# @size: data size
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Allocation',
> +  'data': {'offset': 'uint64', 'size': 'uint64' } }
> +
> +##
> +# @Qcow2Bitmaps:

This may be called Qcow2BitmapsExtension, to follow qcow2.txt

> +#
> +# QCOW2 bitmaps information
> +#
> +# @nb-bitmaps: the number of bitmaps contained in the image
> +#
> +# @bitmap-dir: bitmap directory information
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Bitmaps',
> +  'data': {'nb-bitmaps': 'int',
> +           'bitmap-dir': 'Qcow2BitmapDir' } }

Hmm I don't like these abbreviations. Qcow2BitmapDirectory will
interfere with existing type, I understand but let's at least keep
full field names, like bitmap-directory.

Also, wouldn't existence of types Qcow2BitmapDir and Qcow2BitmapDirectory
be confusing? I think it will.

Maybe, for qapi, we need QapiQcow2BitmapDirectory, to make it obvious?

[upd, after looking ahead], it should be called Qcow2BitmapDirectoryInfo.

> +
> +##
> +# @Qcow2BitmapDir:
> +#
> +# QCOW2 bitmap directory information
> +#
> +# @dir-entries: list of bitmap directory entries
> +#
> +# @location: bitmap directory offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapDir',
> +  'data': {'dir-entries': ['Qcow2BitmapDirectoryEntry'],
> +           'location': 'Qcow2Allocation' } }

I'd call them just 'entries'. It inconsistent with locations,
and I didn't see somewhere in qapi prefixing fields by structure
name abbreviation.

> +
> +##
> +# @Qcow2BitmapDirectoryEntry:
> +#
> +# QCOW2 bitmap directory entry information
> +#
> +# @bitmap-table: bitmap table offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapDirectoryEntry',
> +  'data': {'bitmap-table': 'Qcow2BitmapTableInfo',
> +           'bitmap-name': 'str' } }

and here, I'd keep "bitmap-table", as it matches qcow2.txt,
but s/bitmap-name/name.

> +
> +##
> +# @Qcow2BitmapTableInfo:
> +#
> +# QCOW2 bitmap table information
> +#
> +# @table-entries: list of bitmap table entries
> +#
> +# @location: bitmap table offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapTableInfo',
> +  'data': {'table-entries': ['Qcow2BitmapTableInfoEntry'],
> +           'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2BitmapTableInfoEntry:

Keep Info the last word, Qcow2BitmapTableEntryInfo

> +#
> +# QCOW2 bitmap table entry information
> +#
> +# @type: bitmap table entry type
> +#
> +# @cluster: bitmap table entry offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapTableInfoEntry',
> +  'data': {'type': 'Qcow2BitmapTableInfoEntryType',
> +           '*cluster': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2BitmapTableInfoEntryType:
> +#
> +# An enumeration of cluster types in bitmap table
> +#
> +# @all-zeroes: cluster should be read as all zeroes
> +#
> +# @all-ones: cluster should be read as all ones
> +#
> +# @serialized: cluster data are written on disk
> +#
> +# Since: 5.0
> +##
> +{ 'enum': 'Qcow2BitmapTableInfoEntryType',
> +  'data': ['all-zeroes', 'all-ones', 'serialized'] }
> +
> +##
> +# @Qcow2SnapshotsTable:
> +#
> +# Snapshots table location in image file.
> +#
> +# @location: offset and size of snapshot table
> +#
> +# @l1-list: list of snapshots L1 tables
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2SnapshotsTable',

in qcow2.txt it called snapshot table.. So, I don't think we need 's'.

> +  'data': {'location': 'Qcow2Allocation',
> +           'l1-list': ['Qcow2L1Table'] } }


instead, we need 'entries': ['Qcow2SnapshotTableEntry']

And the entry will contain at least l1-table and name.

> +
>   ##
>   # @ImageCheck:
>   #
> @@ -215,6 +418,9 @@
>   #                       field is present if the driver for the image format
>   #                       supports it
>   #
> +# @metadata: encapsulates QCOW2 tables allocation information (default: none,
> +#            turned on with the command line optional key; since 5.0)
> +#
>   # Since: 1.4
>   #
>   ##
> @@ -223,7 +429,8 @@
>              '*image-end-offset': 'int', '*corruptions': 'int', '*leaks': 'int',
>              '*corruptions-fixed': 'int', '*leaks-fixed': 'int',
>              '*total-clusters': 'int', '*allocated-clusters': 'int',
> -           '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
> +           '*fragmented-clusters': 'int', '*compressed-clusters': 'int',
> +           '*metadata': 'Qcow2Metadata' } }
>   
>   ##
>   # @MapEntry:
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 0/3] Dump QCOW2 metadata

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1174 bytes --]

On 14.01.20 09:22, Andrey Shinkevich wrote:
> The information about QCOW2 metadata allocations in an image ELF-file is
> helpful for finding issues with the image data integrity.

Sorry that I’m replying only so late – but I don’t know why we need this
in qemu, and this cover letter doesn’t provide a justification.  I mean,
it isn’t too complex (from the diffstat), but wouldn’t it be better to
just have a script for this?

I suppose one reason to put it in qemu/qemu-img is that a script
wouldn’t be packaged by distributions.  So if a user has a corrupted
image, with this series we could tell them to run qemu-img check -M and
put the output somewhere.  With a script, we’d first have to tell them
to download the script.  But then again, downloading a script (that
should be part of the qemu repository) doesn’t seem too much trouble to
me either.

So I’m curious as to whether you had a specific reason in mind when you
decided to implement this as part of qemu itself?

(I suppose the additional complexity is fully limited to the check
infrastructure, so it wouldn’t interfere with the rest of the qcow2
driver.  Hm.  Fair enough.)

Max


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

Re: [PATCH v3 0/3] Dump QCOW2 metadata

[Vladimir Sementsov-Ogievskiy]

20.02.2020 14:58, Max Reitz wrote:
> On 14.01.20 09:22, Andrey Shinkevich wrote:
>> The information about QCOW2 metadata allocations in an image ELF-file is
>> helpful for finding issues with the image data integrity.
> 
> Sorry that I’m replying only so late – but I don’t know why we need this
> in qemu, and this cover letter doesn’t provide a justification.  I mean,
> it isn’t too complex (from the diffstat), but wouldn’t it be better to
> just have a script for this?
> 
> I suppose one reason to put it in qemu/qemu-img is that a script
> wouldn’t be packaged by distributions.  So if a user has a corrupted
> image, with this series we could tell them to run qemu-img check -M and
> put the output somewhere.  With a script, we’d first have to tell them
> to download the script.  But then again, downloading a script (that
> should be part of the qemu repository) doesn’t seem too much trouble to
> me either.
> 
> So I’m curious as to whether you had a specific reason in mind when you
> decided to implement this as part of qemu itself?
> 
> (I suppose the additional complexity is fully limited to the check
> infrastructure, so it wouldn’t interfere with the rest of the qcow2
> driver.  Hm.  Fair enough.)
> 

Just not to parse qcow2 from scratch. Qemu already can read qcow2, and
it looks through all its structures during check, why not
to add an ability to represent these structures as a qobject?


-- 
Best regards,
Vladimir

Re: [PATCH v3 0/3] Dump QCOW2 metadata

[Kevin Wolf]

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

Am 20.02.2020 um 12:58 hat Max Reitz geschrieben:
> On 14.01.20 09:22, Andrey Shinkevich wrote:
> > The information about QCOW2 metadata allocations in an image ELF-file is
> > helpful for finding issues with the image data integrity.
> 
> Sorry that I’m replying only so late – but I don’t know why we need this
> in qemu, and this cover letter doesn’t provide a justification.  I mean,
> it isn’t too complex (from the diffstat), but wouldn’t it be better to
> just have a script for this?

Specifically, we could extend tests/qemu-iotests/qcow2.py. This seems to
be debugging output that would be in line with what the script is
already used for.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 0/3] Dump QCOW2 metadata

[Eric Blake]

On 2/20/20 6:28 AM, Kevin Wolf wrote:
> Am 20.02.2020 um 12:58 hat Max Reitz geschrieben:
>> On 14.01.20 09:22, Andrey Shinkevich wrote:
>>> The information about QCOW2 metadata allocations in an image ELF-file is
>>> helpful for finding issues with the image data integrity.
>>
>> Sorry that I’m replying only so late – but I don’t know why we need this
>> in qemu, and this cover letter doesn’t provide a justification.  I mean,
>> it isn’t too complex (from the diffstat), but wouldn’t it be better to
>> just have a script for this?
> 
> Specifically, we could extend tests/qemu-iotests/qcow2.py. This seems to
> be debugging output that would be in line with what the script is
> already used for.

I also just discovered GNU poke, http://jemarch.net/poke, which is an 
arbitrary binary-format editor with a fairly good example of how it can 
be used to inspect ELF files.  I'm wondering if it would be easier to 
write a pickle describing the qcow2 format that would make it easier to 
do interactive browsing/editing of a qcow2 file, at the expense of 
having to depend on poke (which has not yet hit the 1.0 release and is 
not yet bundled for Fedora).

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

Re: [PATCH v3 0/3] Dump QCOW2 metadata

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1639 bytes --]

On 20.02.20 13:10, Vladimir Sementsov-Ogievskiy wrote:
> 20.02.2020 14:58, Max Reitz wrote:
>> On 14.01.20 09:22, Andrey Shinkevich wrote:
>>> The information about QCOW2 metadata allocations in an image ELF-file is
>>> helpful for finding issues with the image data integrity.
>>
>> Sorry that I’m replying only so late – but I don’t know why we need this
>> in qemu, and this cover letter doesn’t provide a justification.  I mean,
>> it isn’t too complex (from the diffstat), but wouldn’t it be better to
>> just have a script for this?
>>
>> I suppose one reason to put it in qemu/qemu-img is that a script
>> wouldn’t be packaged by distributions.  So if a user has a corrupted
>> image, with this series we could tell them to run qemu-img check -M and
>> put the output somewhere.  With a script, we’d first have to tell them
>> to download the script.  But then again, downloading a script (that
>> should be part of the qemu repository) doesn’t seem too much trouble to
>> me either.
>>
>> So I’m curious as to whether you had a specific reason in mind when you
>> decided to implement this as part of qemu itself?
>>
>> (I suppose the additional complexity is fully limited to the check
>> infrastructure, so it wouldn’t interfere with the rest of the qcow2
>> driver.  Hm.  Fair enough.)
>>
> 
> Just not to parse qcow2 from scratch. Qemu already can read qcow2, and
> it looks through all its structures during check, why not
> to add an ability to represent these structures as a qobject?

Because it’d be code in qemu (i.e., a liability) that users are pretty
much never going to use.

Max


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