* [PATCH 0/2] Dump QCOW2 metadata
@ 2019-12-27 11:43 Andrey Shinkevich
2019-12-27 11:43 ` [PATCH 1/2] qcow2: introduce Qcow2Metadata structure Andrey Shinkevich
2019-12-27 11:43 ` [PATCH 2/2] qcow2: dump QCOW2 metadata Andrey Shinkevich
0 siblings, 2 replies; 10+ messages in thread
From: Andrey Shinkevich @ 2019-12-27 11:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, andrey.shinkevich, den, mreitz
The information about QCOW2 metadata allocations in an image ELF-file is
helpful for finding issues with the image data integrity.
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 (2):
qcow2: introduce Qcow2Metadata structure
qcow2: dump QCOW2 metadata
block/qcow2-bitmap.c | 53 ++++++++++++-
block/qcow2-refcount.c | 84 ++++++++++++++++----
block/qcow2.c | 30 +++++++
block/qcow2.h | 6 +-
include/block/block.h | 3 +-
qapi/block-core.json | 208 ++++++++++++++++++++++++++++++++++++++++++++++++-
qemu-img.c | 29 ++++++-
qemu-img.texi | 2 +-
8 files changed, 390 insertions(+), 25 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] qcow2: introduce Qcow2Metadata structure
2019-12-27 11:43 [PATCH 0/2] Dump QCOW2 metadata Andrey Shinkevich
@ 2019-12-27 11:43 ` Andrey Shinkevich
2020-01-07 22:07 ` Eric Blake
2019-12-27 11:43 ` [PATCH 2/2] qcow2: dump QCOW2 metadata Andrey Shinkevich
1 sibling, 1 reply; 10+ messages in thread
From: Andrey Shinkevich @ 2019-12-27 11:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, andrey.shinkevich, den, mreitz
The preliminary patch to provide an extendable structure for dumping
QCOW2 metadata allocations in image.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
qapi/block-core.json | 208 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 207 insertions(+), 1 deletion(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0cf68fe..3eaac13 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': 'Qcow2BitmapTbl',
+ 'bitmap-name': 'str' } }
+
+##
+# @Qcow2BitmapTbl:
+#
+# QCOW2 bitmap table information
+#
+# @table-entries: list of bitmap table entries
+#
+# @location: bitmap table offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2BitmapTbl',
+ 'data': {'table-entries': ['Qcow2BitmapTblEntry'],
+ 'location': 'Qcow2Allocation' } }
+
+##
+# @Qcow2BitmapTblEntry:
+#
+# QCOW2 bitmap table entry information
+#
+# @type: bitmap table entry type
+#
+# @cluster: bitmap table entry offset and size in image
+#
+# Since: 5.0
+##
+{ 'struct': 'Qcow2BitmapTblEntry',
+ 'data': {'type': 'Qcow2BitmapTblEntryType',
+ '*cluster': 'Qcow2Allocation' } }
+
+##
+# @Qcow2BitmapTblEntryType:
+#
+# An enumeration of cluster types in bitmap table
+#
+# @all-zeros: cluster should be read as all zeros
+#
+# @all-ones: cluster should be read as all ones
+#
+# @serialized: cluster data are written on disk
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2BitmapTblEntryType',
+ 'data': ['all-zeros', '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,8 @@
# field is present if the driver for the image format
# supports it
#
+# @viscera: encapsulates QCOW2 tables allocation information
+#
# Since: 1.4
#
##
@@ -223,7 +428,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',
+ '*viscera': 'Qcow2Metadata' } }
##
# @MapEntry:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] qcow2: dump QCOW2 metadata
2019-12-27 11:43 [PATCH 0/2] Dump QCOW2 metadata Andrey Shinkevich
2019-12-27 11:43 ` [PATCH 1/2] qcow2: introduce Qcow2Metadata structure Andrey Shinkevich
@ 2019-12-27 11:43 ` Andrey Shinkevich
2020-01-07 22:11 ` Eric Blake
1 sibling, 1 reply; 10+ messages in thread
From: Andrey Shinkevich @ 2019-12-27 11:43 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, andrey.shinkevich, den, mreitz
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 | 53 ++++++++++++++++++++++++++++---
block/qcow2-refcount.c | 84 +++++++++++++++++++++++++++++++++++++++++---------
block/qcow2.c | 30 ++++++++++++++++++
block/qcow2.h | 6 ++--
include/block/block.h | 3 +-
qemu-img.c | 29 ++++++++++++++++-
qemu-img.texi | 2 +-
7 files changed, 183 insertions(+), 24 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index c6c8ebb..4f7b375 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;
+ Qcow2BitmapTblEntryList **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(Qcow2BitmapTbl, 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,31 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
continue;
}
+ Qcow2BitmapTblEntry *bmte = NULL;
+ if (bmde) {
+ bmte = g_new0(Qcow2BitmapTblEntry, 1);
+ bmte->type = offset ? QCOW2_BITMAP_TBL_ENTRY_TYPE_SERIALIZED :
+ entry & BME_TABLE_ENTRY_FLAG_ALL_ONES;
+ if (offset) {
+ bmte->cluster = g_new0(Qcow2Allocation, 1);
+ }
+ bmte->has_cluster = !!(bmte->cluster);
+ Qcow2BitmapTblEntryList *elem =
+ g_new0(Qcow2BitmapTblEntryList, 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..ac1ae42 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->viscera && res->viscera->snapshot_table;
+ Qcow2L1TableList **plist = has_snapshots ?
+ &res->viscera->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->viscera) {
+ res->viscera->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->viscera ?
+ res->viscera->qcow2_header->location : NULL);
if (ret < 0) {
return ret;
}
/* current L1 table */
+ if (res->viscera) {
+ res->viscera->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->viscera ? res->viscera->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->viscera->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->viscera ?
+ res->viscera->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->viscera ?
+ res->viscera->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->viscera ? res->viscera->bitmaps
+ : NULL);
if (ret < 0) {
return ret;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c18721..39cf918 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->viscera = g_new0(Qcow2Metadata, 1);
+ result->viscera->qcow2_header = g_new0(Qcow2Header, 1);
+ result->viscera->qcow2_header->location = g_new0(Qcow2Allocation, 1);
+ result->viscera->active_l1 = g_new0(Qcow2L1Table, 1);
+ result->viscera->active_l1->location = g_new0(Qcow2Allocation, 1);
+ result->viscera->refcount_table = g_new0(Qcow2RefcountTable, 1);
+ result->viscera->refcount_table->location = g_new0(Qcow2Allocation, 1);
+
+ refcount_res.viscera = result->viscera;
+
+ if (s->crypto_header.length) {
+ result->viscera->crypt_header = g_new0(Qcow2EncryptionHeader, 1);
+ result->viscera->crypt_header->location =
+ g_new0(Qcow2Allocation, 1);
+ }
+ if (s->nb_bitmaps) {
+ result->viscera->bitmaps = g_new0(Qcow2Bitmaps, 1);
+ result->viscera->bitmaps->bitmap_dir = g_new0(Qcow2BitmapDir, 1);
+ result->viscera->bitmaps->bitmap_dir->location =
+ g_new0(Qcow2Allocation, 1);
+ }
+ if (s->nb_snapshots) {
+ result->viscera->snapshot_table = g_new0(Qcow2SnapshotsTable, 1);
+ result->viscera->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 1df9848..b1fc275 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -373,7 +373,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;
@@ -382,11 +381,13 @@ typedef struct BdrvCheckResult {
int leaks_fixed;
int64_t image_end_offset;
BlockFragInfo bfi;
+ Qcow2Metadata *viscera;
} 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 95a24b9..840eee2 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,14 @@ 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->viscera = result.viscera;
+ check->has_viscera = !!(result.viscera);
+
+ if (check->has_viscera) {
+ check->viscera->has_crypt_header = !!(check->viscera->crypt_header);
+ check->viscera->has_bitmaps = !!(check->viscera->bitmaps);
+ check->viscera->has_snapshot_table = !!(check->viscera->snapshot_table);
+ }
return 0;
}
@@ -701,9 +710,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, ":hf:r:T:qU",
+ c = getopt_long(argc, argv, ":hf:r:T:qU:M",
long_options, &option_index);
if (c == -1) {
break;
@@ -745,6 +755,9 @@ static int img_check(int argc, char **argv)
case 'U':
force_share = true;
break;
+ case 'M':
+ fix |= BDRV_DUMP_META;
+ break;
case OPTION_OBJECT: {
QemuOpts *opts;
opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -772,6 +785,11 @@ static int img_check(int argc, char **argv)
return 1;
}
+ if ((fix & BDRV_DUMP_META) && output_format != OFORMAT_JSON) {
+ error_report("Metadata output 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 +810,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..873e80a 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}.
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] qcow2: introduce Qcow2Metadata structure
2019-12-27 11:43 ` [PATCH 1/2] qcow2: introduce Qcow2Metadata structure Andrey Shinkevich
@ 2020-01-07 22:07 ` Eric Blake
2020-01-13 9:49 ` Andrey Shinkevich
0 siblings, 1 reply; 10+ messages in thread
From: Eric Blake @ 2020-01-07 22:07 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, den, mreitz
On 12/27/19 5:43 AM, Andrey Shinkevich wrote:
> The preliminary patch to provide an extendable structure for dumping
> QCOW2 metadata allocations in image.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> qapi/block-core.json | 208 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 207 insertions(+), 1 deletion(-)
>
> +
> +##
> +# @Qcow2BitmapTbl:
> +#
Any reason we must abbreviate instead of spelling this out as Table?
> +# QCOW2 bitmap table information
> +#
> +# @table-entries: list of bitmap table entries
> +#
> +# @location: bitmap table offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapTbl',
> + 'data': {'table-entries': ['Qcow2BitmapTblEntry'],
> + 'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2BitmapTblEntry:
Similar question
> +{ 'struct': 'Qcow2BitmapTblEntry',
> + 'data': {'type': 'Qcow2BitmapTblEntryType',
> + '*cluster': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2BitmapTblEntryType:
> +#
> +# An enumeration of cluster types in bitmap table
> +#
> +# @all-zeros: cluster should be read as all zeros
While there are multiple 'zeros' in the code base, 'zeroes' appears to
be the more common spelling.
> @@ -215,6 +418,8 @@
> # field is present if the driver for the image format
> # supports it
> #
> +# @viscera: encapsulates QCOW2 tables allocation information
Missing a '(since 5.0)' tag. Interesting choice of name; not one I
would have picked out of the air. Would 'metadata' be any more of a
reasonable name?
> +#
> # Since: 1.4
> #
> ##
> @@ -223,7 +428,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',
> + '*viscera': 'Qcow2Metadata' } }
The field is listed as optional, but the docs don't describe what
controls whether it is present or absent. Is that worth adding?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
2019-12-27 11:43 ` [PATCH 2/2] qcow2: dump QCOW2 metadata Andrey Shinkevich
@ 2020-01-07 22:11 ` Eric Blake
2020-01-13 10:30 ` Andrey Shinkevich
0 siblings, 1 reply; 10+ messages in thread
From: Eric Blake @ 2020-01-07 22:11 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, vsementsov, armbru, qemu-devel, den, mreitz
On 12/27/19 5:43 AM, Andrey Shinkevich wrote:
> 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>
> ---
> +++ 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"
Should QCOW2 be all caps?
> }
> @@ -701,9 +710,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, ":hf:r:T:qU",
> + c = getopt_long(argc, argv, ":hf:r:T:qU:M",
We are already inconsistent, but I tend to add options in alphabetical
order, both here...
> long_options, &option_index);
> if (c == -1) {
> break;
> @@ -745,6 +755,9 @@ static int img_check(int argc, char **argv)
> case 'U':
> force_share = true;
> break;
> + case 'M':
...and here, as it is then easier to find a given option for later editing.
> + fix |= BDRV_DUMP_META;
> + break;
> case OPTION_OBJECT: {
> QemuOpts *opts;
> opts = qemu_opts_parse_noisily(&qemu_object_opts,
> @@ -772,6 +785,11 @@ static int img_check(int argc, char **argv)
> return 1;
> }
>
> + if ((fix & BDRV_DUMP_META) && output_format != OFORMAT_JSON) {
> + error_report("Metadata output in JSON format only");
> + return 1;
Why this restriction?
> + }
> +
> if (qemu_opts_foreach(&qemu_object_opts,
> user_creatable_add_opts_foreach,
> qemu_img_object_print_help, &error_fatal)) {
> @@ -792,6 +810,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;
This one makes sense, I guess - we may relax it in later versions if
more file formats gain the ability to dump extra metadata.
> +++ 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}
This mentions that -M is valid, but has no further documentation on what
-M means. Without that, it's anyone's guess.
>
> 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}.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] qcow2: introduce Qcow2Metadata structure
2020-01-07 22:07 ` Eric Blake
@ 2020-01-13 9:49 ` Andrey Shinkevich
0 siblings, 0 replies; 10+ messages in thread
From: Andrey Shinkevich @ 2020-01-13 9:49 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: kwolf, Vladimir Sementsov-Ogievskiy, Denis Lunev, armbru,
qemu-devel, mreitz
On 08/01/2020 01:07, Eric Blake wrote:
> On 12/27/19 5:43 AM, Andrey Shinkevich wrote:
>> The preliminary patch to provide an extendable structure for dumping
>> QCOW2 metadata allocations in image.
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> qapi/block-core.json | 208
>> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 207 insertions(+), 1 deletion(-)
>>
>
>> +
>> +##
>> +# @Qcow2BitmapTbl:
>> +#
>
> Any reason we must abbreviate instead of spelling this out as Table?
>
Thank you Eric for your comments. The reason for the abbreviation is
that the name 'Qcow2BitmapTable' is in conflict with the existing
structure name in block/qcow2-bitmap.c. I can rename it to
'Qcow2BitmapTableInfo'
>> +# QCOW2 bitmap table information
>> +#
>> +# @table-entries: list of bitmap table entries
>> +#
>> +# @location: bitmap table offset and size in image
>> +#
>> +# Since: 5.0
>> +##
>> +{ 'struct': 'Qcow2BitmapTbl',
>> + 'data': {'table-entries': ['Qcow2BitmapTblEntry'],
>> + 'location': 'Qcow2Allocation' } }
>> +
>> +##
>> +# @Qcow2BitmapTblEntry:
>
> Similar question
>
and to 'Qcow2BitmapTableInfoEntry' here.
>
>> +{ 'struct': 'Qcow2BitmapTblEntry',
>> + 'data': {'type': 'Qcow2BitmapTblEntryType',
>> + '*cluster': 'Qcow2Allocation' } }
>> +
>> +##
>> +# @Qcow2BitmapTblEntryType:
>> +#
>> +# An enumeration of cluster types in bitmap table
>> +#
>> +# @all-zeros: cluster should be read as all zeros
>
> While there are multiple 'zeros' in the code base, 'zeroes' appears to
> be the more common spelling.
>
>
>> @@ -215,6 +418,8 @@
>> # field is present if the driver for the image
>> format
>> # supports it
>> #
>> +# @viscera: encapsulates QCOW2 tables allocation information
>
> Missing a '(since 5.0)' tag. Interesting choice of name; not one I
> would have picked out of the air. Would 'metadata' be any more of a
> reasonable name?
>
>> +#
>> # Since: 1.4
>> #
>> ##
>> @@ -223,7 +428,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',
>> + '*viscera': 'Qcow2Metadata' } }
>
> The field is listed as optional, but the docs don't describe what
> controls whether it is present or absent. Is that worth adding?
>
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
2020-01-07 22:11 ` Eric Blake
@ 2020-01-13 10:30 ` Andrey Shinkevich
2020-01-13 16:16 ` Eric Blake
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Shinkevich @ 2020-01-13 10:30 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: kwolf, Vladimir Sementsov-Ogievskiy, Denis Lunev, armbru,
qemu-devel, mreitz
On 08/01/2020 01:11, Eric Blake wrote:
> On 12/27/19 5:43 AM, Andrey Shinkevich wrote:
>> 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>
>> ---
>
>> +++ 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"
>
> Should QCOW2 be all caps?
>
>
>> }
>> @@ -701,9 +710,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, ":hf:r:T:qU",
>> + c = getopt_long(argc, argv, ":hf:r:T:qU:M",
>
> We are already inconsistent, but I tend to add options in alphabetical
> order, both here...
>
If I merely move 'M' forward like ':hf:M:r:T:qU', will it be OK?
>> long_options, &option_index);
>> if (c == -1) {
>> break;
>> @@ -745,6 +755,9 @@ static int img_check(int argc, char **argv)
>> case 'U':
>> force_share = true;
>> break;
>> + case 'M':
>
> ...and here, as it is then easier to find a given option for later editing.
>
...and similar here without changing the existing code. Have I
understood you correctly?
>> + fix |= BDRV_DUMP_META;
>> + break;
>> case OPTION_OBJECT: {
>> QemuOpts *opts;
>> opts = qemu_opts_parse_noisily(&qemu_object_opts,
>> @@ -772,6 +785,11 @@ static int img_check(int argc, char **argv)
>> return 1;
>> }
>> + if ((fix & BDRV_DUMP_META) && output_format != OFORMAT_JSON) {
>> + error_report("Metadata output in JSON format only");
>> + return 1;
>
> Why this restriction?
>
This is to remind a user that '-M' can be effective with the option
'--output=json' only. Do you think that a comment in the qemu-img.texi
would be enough and the restriction should be omitted here?
>> + }
>> +
>> if (qemu_opts_foreach(&qemu_object_opts,
>> user_creatable_add_opts_foreach,
>> qemu_img_object_print_help, &error_fatal)) {
>> @@ -792,6 +810,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;
>
> This one makes sense, I guess - we may relax it in later versions if
> more file formats gain the ability to dump extra metadata.
>
>
>> +++ 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}
>
> This mentions that -M is valid, but has no further documentation on what
> -M means. Without that, it's anyone's guess.
>
Thank you Eric, I really missed to supply a comment for the new option
here and am going to put it below. Should I mention that option in
qapi/block-core.json file also with this patch of the series?
>> 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}.
>>
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
2020-01-13 10:30 ` Andrey Shinkevich
@ 2020-01-13 16:16 ` Eric Blake
2020-01-13 17:02 ` Andrey Shinkevich
0 siblings, 1 reply; 10+ messages in thread
From: Eric Blake @ 2020-01-13 16:16 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, Vladimir Sementsov-Ogievskiy, Denis Lunev, armbru,
qemu-devel, mreitz
On 1/13/20 4:30 AM, Andrey Shinkevich wrote:
>>> - c = getopt_long(argc, argv, ":hf:r:T:qU",
>>> + c = getopt_long(argc, argv, ":hf:r:T:qU:M",
>>
>> We are already inconsistent, but I tend to add options in alphabetical
>> order, both here...
>>
>
> If I merely move 'M' forward like ':hf:M:r:T:qU', will it be OK?
>
If you don't mind writing a pre-requisite patch that sorts the existing
options, then the patch adding your option in sorted order is easy. But
that's asking you to do extra work, which I'm not going to insist on, so
I can also live with your patch being in any order as it is no worse
than existing code and anyone that wants to do a cleanup patch to sort
things has roughly the same level of effort whether or not your patch
without sorting lands in the meantime.
>>> + if ((fix & BDRV_DUMP_META) && output_format != OFORMAT_JSON) {
>>> + error_report("Metadata output in JSON format only");
>>> + return 1;
>>
>> Why this restriction?
>>
>
> This is to remind a user that '-M' can be effective with the option
> '--output=json' only. Do you think that a comment in the qemu-img.texi
> would be enough and the restriction should be omitted here?
Rather, why can't we come up with some sort of sane human output, so
that we don't have to limit the flag to just --output=json?
>>> +++ 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}
>>
>> This mentions that -M is valid, but has no further documentation on what
>> -M means. Without that, it's anyone's guess.
>>
>
> Thank you Eric, I really missed to supply a comment for the new option
> here and am going to put it below. Should I mention that option in
> qapi/block-core.json file also with this patch of the series?
Mentioning that the qapi type exists to facilitate a qemu-img option
might not hurt. But more important is that the qemu-img documentation
mentions what -M does; that documentation can point to the qapi docs for
how the output will be structured when --output=json is in effect.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
2020-01-13 16:16 ` Eric Blake
@ 2020-01-13 17:02 ` Andrey Shinkevich
2020-01-13 17:27 ` Eric Blake
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Shinkevich @ 2020-01-13 17:02 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: kwolf, Vladimir Sementsov-Ogievskiy, Denis Lunev, armbru,
qemu-devel, mreitz
On 13/01/2020 19:16, Eric Blake wrote:
> On 1/13/20 4:30 AM, Andrey Shinkevich wrote:
>
>>>> - c = getopt_long(argc, argv, ":hf:r:T:qU",
>>>> + c = getopt_long(argc, argv, ":hf:r:T:qU:M",
>>>
>>> We are already inconsistent, but I tend to add options in alphabetical
>>> order, both here...
>>>
>>
>> If I merely move 'M' forward like ':hf:M:r:T:qU', will it be OK?
>>
>
> If you don't mind writing a pre-requisite patch that sorts the existing
> options, then the patch adding your option in sorted order is easy. But
> that's asking you to do extra work, which I'm not going to insist on, so
> I can also live with your patch being in any order as it is no worse
> than existing code and anyone that wants to do a cleanup patch to sort
> things has roughly the same level of effort whether or not your patch
> without sorting lands in the meantime.
>
>
Yes, I don't mind. It is easy.
>>>> + if ((fix & BDRV_DUMP_META) && output_format != OFORMAT_JSON) {
>>>> + error_report("Metadata output in JSON format only");
>>>> + return 1;
>>>
>>> Why this restriction?
>>>
>>
>> This is to remind a user that '-M' can be effective with the option
>> '--output=json' only. Do you think that a comment in the qemu-img.texi
>> would be enough and the restriction should be omitted here?
>
> Rather, why can't we come up with some sort of sane human output, so
> that we don't have to limit the flag to just --output=json?
>
It is not as easy and will incur much more coding. The json output is
priority-driven format and can be converted to a human readable one with
some other tools if needed. This option is for developers only.
>>>> +++ 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}
>>>
>>> This mentions that -M is valid, but has no further documentation on what
>>> -M means. Without that, it's anyone's guess.
>>>
>>
>> Thank you Eric, I really missed to supply a comment for the new option
>> here and am going to put it below. Should I mention that option in
>> qapi/block-core.json file also with this patch of the series?
>
> Mentioning that the qapi type exists to facilitate a qemu-img option
> might not hurt. But more important is that the qemu-img documentation
> mentions what -M does; that documentation can point to the qapi docs for
> how the output will be structured when --output=json is in effect.
>
Would you please specify the qemu-img and qapi documentation files to
modify? Thank you.
Andrey
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
2020-01-13 17:02 ` Andrey Shinkevich
@ 2020-01-13 17:27 ` Eric Blake
0 siblings, 0 replies; 10+ messages in thread
From: Eric Blake @ 2020-01-13 17:27 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block
Cc: kwolf, Vladimir Sementsov-Ogievskiy, Denis Lunev, armbru,
qemu-devel, mreitz
On 1/13/20 11:02 AM, Andrey Shinkevich wrote:
>>>>> +++ 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}
>>>>
>>>> This mentions that -M is valid, but has no further documentation on what
>>>> -M means. Without that, it's anyone's guess.
>>>>
>>>
>>> Thank you Eric, I really missed to supply a comment for the new option
>>> here and am going to put it below. Should I mention that option in
>>> qapi/block-core.json file also with this patch of the series?
>>
>> Mentioning that the qapi type exists to facilitate a qemu-img option
>> might not hurt. But more important is that the qemu-img documentation
>> mentions what -M does; that documentation can point to the qapi docs for
>> how the output will be structured when --output=json is in effect.
>>
>
> Would you please specify the qemu-img and qapi documentation files to
> modify? Thank you.
I'm thinking that qemu-img.texi can simply mention something like "see
type XYZ in the QAPI docs for how the JSON output will be formatted",
and then your QAPI documentation already added in patch 1/2 is then
sufficient to cover the details of what -M exposes here.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-13 17:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-27 11:43 [PATCH 0/2] Dump QCOW2 metadata Andrey Shinkevich
2019-12-27 11:43 ` [PATCH 1/2] qcow2: introduce Qcow2Metadata structure Andrey Shinkevich
2020-01-07 22:07 ` Eric Blake
2020-01-13 9:49 ` Andrey Shinkevich
2019-12-27 11:43 ` [PATCH 2/2] qcow2: dump QCOW2 metadata Andrey Shinkevich
2020-01-07 22:11 ` Eric Blake
2020-01-13 10:30 ` Andrey Shinkevich
2020-01-13 16:16 ` Eric Blake
2020-01-13 17:02 ` Andrey Shinkevich
2020-01-13 17:27 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.