* [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits
@ 2019-08-19 18:55 Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h Max Reitz
` (15 more replies)
0 siblings, 16 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Hi,
See the v1 cover letter here if you haven’t already:
https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg01290.html
I kept all patches from the previous version because Eric deemed the
patches I might have dropped useful.
I kept the way I implemented fixing overly long snapshot tables, namely
by discarding all snapshots past the maximum end. Eric criticized this,
because this is indiscriminate (and the last ones have been created the
most recently, but I’m not sure whether we’d generally rather keep the
oldest or the most recent ones. Maybe best would be to drop every
second snapshot? :-)).
I kept it as it is, because everything else would require user
interaction. We currently have no interactive check mode, and I feel
like this is not a place where we need it badly: We introduced our
current snapshot limits in such a way that no sane image would ever hit
them. If you do hit them, changes are that your image is already
corrupted, so you probably no longer care about your snapshots anyway
and just want to copy the active layer off.
Furthermore, I felt like an interactive mode would be pretty hard to
test thoroughly. I don’t want to introduce code that probably nobody
will ever use and that is hardly tested.
I also tried Eric’s suggestion of using bdrv_preadv() in
qcow2_read_snapshots() to read all variable data at once, and while it
was very promising, it doesn’t allow us to skip data (which we must do
when we want to truncate extra data down to a sane size). So all in all
I couldn’t make it work.
So, the changes in v2:
- Patch 1: Eric asked from some magic numbers to be removed (very
reasonable), and endof() will help with that.
- Patch 2: See, it helps!
- Patch 4: Rebased conflicts (because of patch 2), and I should use
BDRV_REQUEST_MAX_BYTES instead of INT_MAX when I mean the
former
- Patch 7: Drop magic number [Eric]
- Patch 8: Drop magic number [Eric]
- Patch 10: Instead of dropping all unknown extra data when there is too
much, just truncate it down to the maximum [Eric]
- Patch 11: When some snapshot table entry has too much extra data and
we truncate it, that shortens the snapshot table length, so
we should keep that in mind when calculating whether the
table is too long
- Patch 12: Rebase conflicts, and dropped a magic number [Eric]
- Patch 14: Drop magic number [Eric]
- Patch 16: Simplified padding calculation [Eric], fixed one test case
(I used “sn-size” instead of “sn_size”, oops), and added a
test case for what happens when a snapshot table entry is
too big and contains an entry with too much extra data (the
latter should be fixed first and then we should look into
whether the table is still too long)
git-backport-diff against v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/16:[down] 'include: Move endof() up from hw/virtio/virtio.h'
002/16:[down] 'qcow2: Use endof()'
003/16:[----] [--] 'qcow2: Add Error ** to qcow2_read_snapshots()'
004/16:[0012] [FC] 'qcow2: Keep unknown extra snapshot data'
005/16:[----] [--] 'qcow2: Make qcow2_write_snapshots() public'
006/16:[----] [--] 'qcow2: Put qcow2_upgrade() into an own function'
007/16:[0005] [FC] 'qcow2: Write v3-compliant snapshot list on upgrade'
008/16:[0004] [FC] 'qcow2: Separate qcow2_check_read_snapshot_table()'
009/16:[----] [--] 'qcow2: Add qcow2_check_fix_snapshot_table()'
010/16:[0038] [FC] 'qcow2: Fix broken snapshot table entries'
011/16:[down] 'qcow2: Keep track of the snapshot table length'
012/16:[0003] [FC] 'qcow2: Fix overly long snapshot tables'
013/16:[----] [--] 'qcow2: Repair snapshot table with too many entries'
014/16:[0005] [FC] 'qcow2: Fix v3 snapshot table entry compliancy'
015/16:[----] [--] 'iotests: Add peek_file* functions'
016/16:[0125] [FC] 'iotests: Test qcow2's snapshot table handling'
Max Reitz (16):
include: Move endof() up from hw/virtio/virtio.h
qcow2: Use endof()
qcow2: Add Error ** to qcow2_read_snapshots()
qcow2: Keep unknown extra snapshot data
qcow2: Make qcow2_write_snapshots() public
qcow2: Put qcow2_upgrade() into its own function
qcow2: Write v3-compliant snapshot list on upgrade
qcow2: Separate qcow2_check_read_snapshot_table()
qcow2: Add qcow2_check_fix_snapshot_table()
qcow2: Fix broken snapshot table entries
qcow2: Keep track of the snapshot table length
qcow2: Fix overly long snapshot tables
qcow2: Repair snapshot table with too many entries
qcow2: Fix v3 snapshot table entry compliancy
iotests: Add peek_file* functions
iotests: Test qcow2's snapshot table handling
block/qcow2.h | 15 +-
include/hw/virtio/virtio.h | 7 -
include/qemu/compiler.h | 7 +
block/qcow2-snapshot.c | 323 +++++++++++++++++++--
block/qcow2.c | 155 +++++++++--
hw/block/virtio-blk.c | 4 +-
hw/net/virtio-net.c | 10 +-
tests/qemu-iotests/261 | 523 +++++++++++++++++++++++++++++++++++
tests/qemu-iotests/261.out | 346 +++++++++++++++++++++++
tests/qemu-iotests/common.rc | 20 ++
tests/qemu-iotests/group | 1 +
11 files changed, 1354 insertions(+), 57 deletions(-)
create mode 100755 tests/qemu-iotests/261
create mode 100644 tests/qemu-iotests/261.out
--
2.21.0
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:06 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 02/16] qcow2: Use endof() Max Reitz
` (14 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
endof() is a useful macro, we can make use of it outside of virtio.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/hw/virtio/virtio.h | 7 -------
include/qemu/compiler.h | 7 +++++++
hw/block/virtio-blk.c | 4 ++--
hw/net/virtio-net.c | 10 +++++-----
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 48e8d04ff6..ef083af550 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -37,13 +37,6 @@ static inline hwaddr vring_align(hwaddr addr,
return QEMU_ALIGN_UP(addr, align);
}
-/*
- * Calculate the number of bytes up to and including the given 'field' of
- * 'container'.
- */
-#define virtio_endof(container, field) \
- (offsetof(container, field) + sizeof_field(container, field))
-
typedef struct VirtIOFeature {
uint64_t flags;
size_t end;
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 09fc44cca4..dab3d7fad4 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -60,6 +60,13 @@
#define sizeof_field(type, field) sizeof(((type *)0)->field)
+/*
+ * Calculate the number of bytes up to and including the given 'field' of
+ * 'container'.
+ */
+#define endof(container, field) \
+ (offsetof(container, field) + sizeof_field(container, field))
+
/* Convert from a base type to a parent type, with compile time checking. */
#ifdef __GNUC__
#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 18851601cb..1ad6a784f0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -41,9 +41,9 @@
*/
static VirtIOFeature feature_sizes[] = {
{.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
- .end = virtio_endof(struct virtio_blk_config, discard_sector_alignment)},
+ .end = endof(struct virtio_blk_config, discard_sector_alignment)},
{.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
- .end = virtio_endof(struct virtio_blk_config, write_zeroes_may_unmap)},
+ .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
{}
};
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9f11422337..2c4909c5f9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -90,15 +90,15 @@ static inline __virtio16 *virtio_net_rsc_ext_num_dupacks(
static VirtIOFeature feature_sizes[] = {
{.flags = 1ULL << VIRTIO_NET_F_MAC,
- .end = virtio_endof(struct virtio_net_config, mac)},
+ .end = endof(struct virtio_net_config, mac)},
{.flags = 1ULL << VIRTIO_NET_F_STATUS,
- .end = virtio_endof(struct virtio_net_config, status)},
+ .end = endof(struct virtio_net_config, status)},
{.flags = 1ULL << VIRTIO_NET_F_MQ,
- .end = virtio_endof(struct virtio_net_config, max_virtqueue_pairs)},
+ .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
{.flags = 1ULL << VIRTIO_NET_F_MTU,
- .end = virtio_endof(struct virtio_net_config, mtu)},
+ .end = endof(struct virtio_net_config, mtu)},
{.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
- .end = virtio_endof(struct virtio_net_config, duplex)},
+ .end = endof(struct virtio_net_config, duplex)},
{}
};
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 02/16] qcow2: Use endof()
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:09 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 03/16] qcow2: Add Error ** to qcow2_read_snapshots() Max Reitz
` (13 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-snapshot.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d0e7fa9311..752883e5c3 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -92,11 +92,12 @@ int qcow2_read_snapshots(BlockDriverState *bs)
}
offset += extra_data_size;
- if (extra_data_size >= 8) {
+ if (extra_data_size >= endof(QCowSnapshotExtraData,
+ vm_state_size_large)) {
sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
}
- if (extra_data_size >= 16) {
+ if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
sn->disk_size = be64_to_cpu(extra.disk_size);
} else {
sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
@@ -251,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
}
QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
- offsetof(QCowHeader, nb_snapshots) + sizeof(header_data.nb_snapshots));
+ endof(QCowHeader, nb_snapshots));
header_data.nb_snapshots = cpu_to_be32(s->nb_snapshots);
header_data.snapshots_offset = cpu_to_be64(snapshots_offset);
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 03/16] qcow2: Add Error ** to qcow2_read_snapshots()
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 02/16] qcow2: Use endof() Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data Max Reitz
` (12 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 2 +-
block/qcow2-snapshot.c | 7 ++++++-
block/qcow2.c | 3 +--
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index fc1b0d3c1e..175708cee0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -704,7 +704,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
Error **errp);
void qcow2_free_snapshots(BlockDriverState *bs);
-int qcow2_read_snapshots(BlockDriverState *bs);
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
/* qcow2-cache.c functions */
Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 752883e5c3..80d32e4504 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -43,7 +43,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
s->nb_snapshots = 0;
}
-int qcow2_read_snapshots(BlockDriverState *bs)
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
QCowSnapshotHeader h;
@@ -68,6 +68,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
offset = ROUND_UP(offset, 8);
ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to read snapshot table");
goto fail;
}
@@ -88,6 +89,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
ret = bdrv_pread(bs->file, offset, &extra,
MIN(sizeof(extra), extra_data_size));
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to read snapshot table");
goto fail;
}
offset += extra_data_size;
@@ -107,6 +109,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
sn->id_str = g_malloc(id_str_size + 1);
ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to read snapshot table");
goto fail;
}
offset += id_str_size;
@@ -116,6 +119,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
sn->name = g_malloc(name_size + 1);
ret = bdrv_pread(bs->file, offset, sn->name, name_size);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to read snapshot table");
goto fail;
}
offset += name_size;
@@ -123,6 +127,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
if (offset - s->snapshots_offset > QCOW_MAX_SNAPSHOTS_SIZE) {
ret = -EFBIG;
+ error_setg(errp, "Snapshot table is too big");
goto fail;
}
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c5a4859f7..dbb50bc2f8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1578,9 +1578,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
s->snapshots_offset = header.snapshots_offset;
s->nb_snapshots = header.nb_snapshots;
- ret = qcow2_read_snapshots(bs);
+ ret = qcow2_read_snapshots(bs, errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not read snapshots");
goto fail;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (2 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 03/16] qcow2: Add Error ** to qcow2_read_snapshots() Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:23 ` Eric Blake
2019-08-19 19:34 ` [Qemu-devel] " Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 05/16] qcow2: Make qcow2_write_snapshots() public Max Reitz
` (11 subsequent siblings)
15 siblings, 2 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
The qcow2 specification says to ignore unknown extra data fields in
snapshot table entries. Currently, we discard it whenever we update the
image, which is a bit different from "ignore".
This patch makes the qcow2 driver keep all unknown extra data fields
when updating an image's snapshot table.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.h | 5 ++++
block/qcow2-snapshot.c | 61 +++++++++++++++++++++++++++++++++++-------
2 files changed, 56 insertions(+), 10 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 175708cee0..290a48b77e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -61,6 +61,9 @@
* space for snapshot names and IDs */
#define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
+/* Maximum amount of extra data per snapshot table entry to accept */
+#define QCOW_MAX_SNAPSHOT_EXTRA_DATA 1024
+
/* Bitmap header extension constraints */
#define QCOW2_MAX_BITMAPS 65535
#define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
@@ -178,6 +181,8 @@ typedef struct QCowSnapshot {
uint32_t date_sec;
uint32_t date_nsec;
uint64_t vm_clock_nsec;
+ uint32_t extra_data_size;
+ void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */
} QCowSnapshot;
struct Qcow2Cache;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 80d32e4504..120cb7fa09 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -37,6 +37,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
for(i = 0; i < s->nb_snapshots; i++) {
g_free(s->snapshots[i].name);
g_free(s->snapshots[i].id_str);
+ g_free(s->snapshots[i].unknown_extra_data);
}
g_free(s->snapshots);
s->snapshots = NULL;
@@ -51,7 +52,6 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
QCowSnapshot *sn;
int i, id_str_size, name_size;
int64_t offset;
- uint32_t extra_data_size;
int ret;
if (!s->nb_snapshots) {
@@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
sn->date_sec = be32_to_cpu(h.date_sec);
sn->date_nsec = be32_to_cpu(h.date_nsec);
sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec);
- extra_data_size = be32_to_cpu(h.extra_data_size);
+ sn->extra_data_size = be32_to_cpu(h.extra_data_size);
id_str_size = be16_to_cpu(h.id_str_size);
name_size = be16_to_cpu(h.name_size);
- /* Read extra data */
+ if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
+ ret = -EFBIG;
+ error_setg(errp, "Too much extra metadata in snapshot table "
+ "entry %i", i);
+ goto fail;
+ }
+
+ /* Read known extra data */
ret = bdrv_pread(bs->file, offset, &extra,
- MIN(sizeof(extra), extra_data_size));
+ MIN(sizeof(extra), sn->extra_data_size));
if (ret < 0) {
error_setg_errno(errp, -ret, "Failed to read snapshot table");
goto fail;
}
- offset += extra_data_size;
+ offset += MIN(sizeof(extra), sn->extra_data_size);
- if (extra_data_size >= endof(QCowSnapshotExtraData,
- vm_state_size_large)) {
+ if (sn->extra_data_size >= endof(QCowSnapshotExtraData,
+ vm_state_size_large)) {
sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
}
- if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
+ if (sn->extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
sn->disk_size = be64_to_cpu(extra.disk_size);
} else {
sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
}
+ if (sn->extra_data_size > sizeof(extra)) {
+ /* Store unknown extra data */
+ size_t unknown_extra_data_size =
+ sn->extra_data_size - sizeof(extra);
+
+ sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
+ ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
+ unknown_extra_data_size);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to read snapshot table");
+ goto fail;
+ }
+ offset += unknown_extra_data_size;
+ }
+
/* Read snapshot ID */
sn->id_str = g_malloc(id_str_size + 1);
ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
@@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
sn = s->snapshots + i;
offset = ROUND_UP(offset, 8);
offset += sizeof(h);
- offset += sizeof(extra);
+ offset += MAX(sizeof(extra), sn->extra_data_size);
offset += strlen(sn->id_str);
offset += strlen(sn->name);
@@ -209,7 +231,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
h.date_sec = cpu_to_be32(sn->date_sec);
h.date_nsec = cpu_to_be32(sn->date_nsec);
h.vm_clock_nsec = cpu_to_be64(sn->vm_clock_nsec);
- h.extra_data_size = cpu_to_be32(sizeof(extra));
+ h.extra_data_size = cpu_to_be32(MAX(sizeof(extra),
+ sn->extra_data_size));
memset(&extra, 0, sizeof(extra));
extra.vm_state_size_large = cpu_to_be64(sn->vm_state_size);
@@ -234,6 +257,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
}
offset += sizeof(extra);
+ if (sn->extra_data_size > sizeof(extra)) {
+ size_t unknown_extra_data_size =
+ sn->extra_data_size - sizeof(extra);
+
+ /* qcow2_read_snapshots() ensures no unbounded allocation */
+ assert(unknown_extra_data_size <= BDRV_REQUEST_MAX_BYTES);
+ assert(sn->unknown_extra_data);
+
+ ret = bdrv_pwrite(bs->file, offset, sn->unknown_extra_data,
+ unknown_extra_data_size);
+ if (ret < 0) {
+ goto fail;
+ }
+ offset += unknown_extra_data_size;
+ }
+
ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
if (ret < 0) {
goto fail;
@@ -376,6 +415,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
sn->date_sec = sn_info->date_sec;
sn->date_nsec = sn_info->date_nsec;
sn->vm_clock_nsec = sn_info->vm_clock_nsec;
+ sn->extra_data_size = sizeof(QCowSnapshotExtraData);
/* Allocate the L1 table of the snapshot and copy the current one there. */
l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
@@ -647,6 +687,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
* The snapshot is now unused, clean up. If we fail after this point, we
* won't recover but just leak clusters.
*/
+ g_free(sn.unknown_extra_data);
g_free(sn.id_str);
g_free(sn.name);
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 05/16] qcow2: Make qcow2_write_snapshots() public
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (3 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 06/16] qcow2: Put qcow2_upgrade() into its own function Max Reitz
` (10 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Updating the snapshot list will be useful when upgrading a v2 image to
v3, so we will need to call this function in qcow2.c.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 1 +
block/qcow2-snapshot.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 290a48b77e..77586d81b9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -710,6 +710,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
void qcow2_free_snapshots(BlockDriverState *bs);
int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
+int qcow2_write_snapshots(BlockDriverState *bs);
/* qcow2-cache.c functions */
Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 120cb7fa09..e3bf4c9776 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -164,7 +164,7 @@ fail:
}
/* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs)
+int qcow2_write_snapshots(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
QCowSnapshot *sn;
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 06/16] qcow2: Put qcow2_upgrade() into its own function
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (4 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 05/16] qcow2: Make qcow2_write_snapshots() public Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade Max Reitz
` (9 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
This does not make sense right now, but it will make sense once we need
to do more than to just update s->qcow_version.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.c | 43 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index dbb50bc2f8..190a3874c1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4749,12 +4749,46 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
return 0;
}
+/*
+ * Upgrades an image's version. While newer versions encompass all
+ * features of older versions, some things may have to be presented
+ * differently.
+ */
+static int qcow2_upgrade(BlockDriverState *bs, int target_version,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ Error **errp)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int current_version = s->qcow_version;
+ int ret;
+
+ /* This is qcow2_upgrade(), not qcow2_downgrade() */
+ assert(target_version > current_version);
+
+ /* There are no other versions (yet) that you can upgrade to */
+ assert(target_version == 3);
+
+ status_cb(bs, 0, 1, cb_opaque);
+
+ s->qcow_version = target_version;
+ ret = qcow2_update_header(bs);
+ if (ret < 0) {
+ s->qcow_version = current_version;
+ error_setg_errno(errp, -ret, "Failed to update the image header");
+ return ret;
+ }
+ status_cb(bs, 1, 1, cb_opaque);
+
+ return 0;
+}
+
typedef enum Qcow2AmendOperation {
/* This is the value Qcow2AmendHelperCBInfo::last_operation will be
* statically initialized to so that the helper CB can discern the first
* invocation from an operation change */
QCOW2_NO_OPERATION = 0,
+ QCOW2_UPGRADING,
QCOW2_CHANGING_REFCOUNT_ORDER,
QCOW2_DOWNGRADING,
} Qcow2AmendOperation;
@@ -4937,17 +4971,16 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
helper_cb_info = (Qcow2AmendHelperCBInfo){
.original_status_cb = status_cb,
.original_cb_opaque = cb_opaque,
- .total_operations = (new_version < old_version)
+ .total_operations = (new_version != old_version)
+ (s->refcount_bits != refcount_bits)
};
/* Upgrade first (some features may require compat=1.1) */
if (new_version > old_version) {
- s->qcow_version = new_version;
- ret = qcow2_update_header(bs);
+ helper_cb_info.current_operation = QCOW2_UPGRADING;
+ ret = qcow2_upgrade(bs, new_version, &qcow2_amend_helper_cb,
+ &helper_cb_info, errp);
if (ret < 0) {
- s->qcow_version = old_version;
- error_setg_errno(errp, -ret, "Failed to update the image header");
return ret;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (5 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 06/16] qcow2: Put qcow2_upgrade() into its own function Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:25 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 08/16] qcow2: Separate qcow2_check_read_snapshot_table() Max Reitz
` (8 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
qcow2 v3 requires every snapshot table entry to have two extra data
fields: The 64-bit VM state size, and the virtual disk size. Both are
optional for v2 images, so they may not be present.
qcow2_upgrade() therefore should update the snapshot table to ensure all
entries have these extra data fields.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 190a3874c1..2219639e11 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4759,7 +4759,9 @@ static int qcow2_upgrade(BlockDriverState *bs, int target_version,
Error **errp)
{
BDRVQcow2State *s = bs->opaque;
+ bool need_snapshot_update;
int current_version = s->qcow_version;
+ int i;
int ret;
/* This is qcow2_upgrade(), not qcow2_downgrade() */
@@ -4768,7 +4770,33 @@ static int qcow2_upgrade(BlockDriverState *bs, int target_version,
/* There are no other versions (yet) that you can upgrade to */
assert(target_version == 3);
- status_cb(bs, 0, 1, cb_opaque);
+ status_cb(bs, 0, 2, cb_opaque);
+
+ /*
+ * In v2, snapshots do not need to have extra data. v3 requires
+ * the 64-bit VM state size and the virtual disk size to be
+ * present.
+ * qcow2_write_snapshots() will always write the list in the
+ * v3-compliant format.
+ */
+ need_snapshot_update = false;
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (s->snapshots[i].extra_data_size <
+ sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+ sizeof_field(QCowSnapshotExtraData, disk_size))
+ {
+ need_snapshot_update = true;
+ break;
+ }
+ }
+ if (need_snapshot_update) {
+ ret = qcow2_write_snapshots(bs);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to update the snapshot table");
+ return ret;
+ }
+ }
+ status_cb(bs, 1, 2, cb_opaque);
s->qcow_version = target_version;
ret = qcow2_update_header(bs);
@@ -4777,7 +4805,7 @@ static int qcow2_upgrade(BlockDriverState *bs, int target_version,
error_setg_errno(errp, -ret, "Failed to update the image header");
return ret;
}
- status_cb(bs, 1, 1, cb_opaque);
+ status_cb(bs, 2, 2, cb_opaque);
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 08/16] qcow2: Separate qcow2_check_read_snapshot_table()
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (6 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 09/16] qcow2: Add qcow2_check_fix_snapshot_table() Max Reitz
` (7 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Reading the snapshot table can fail. That is a problem when we want to
repair the image.
Therefore, stop reading the snapshot table in qcow2_do_open() in check
mode. Instead, add a new function qcow2_check_read_snapshot_table()
that reads the snapshot table at a later point. In the future, we want
to handle errors here and fix them.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 4 +++
block/qcow2-snapshot.c | 58 ++++++++++++++++++++++++++++++++
block/qcow2.c | 76 ++++++++++++++++++++++++++++++++----------
3 files changed, 120 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 77586d81b9..50c7eefb5b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -712,6 +712,10 @@ void qcow2_free_snapshots(BlockDriverState *bs);
int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
int qcow2_write_snapshots(BlockDriverState *bs);
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix);
+
/* qcow2-cache.c functions */
Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
unsigned table_size);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index e3bf4c9776..d667bfd935 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -322,6 +322,64 @@ fail:
return ret;
}
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix)
+{
+ BDRVQcow2State *s = bs->opaque;
+ Error *local_err = NULL;
+ int ret;
+ struct {
+ uint32_t nb_snapshots;
+ uint64_t snapshots_offset;
+ } QEMU_PACKED snapshot_table_pointer;
+
+ /* qcow2_do_open() discards this information in check mode */
+ ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
+ &snapshot_table_pointer, sizeof(snapshot_table_pointer));
+ if (ret < 0) {
+ result->check_errors++;
+ fprintf(stderr, "ERROR failed to read the snapshot table pointer from "
+ "the image header: %s\n", strerror(-ret));
+ return ret;
+ }
+
+ s->snapshots_offset = be64_to_cpu(snapshot_table_pointer.snapshots_offset);
+ s->nb_snapshots = be32_to_cpu(snapshot_table_pointer.nb_snapshots);
+
+ ret = qcow2_validate_table(bs, s->snapshots_offset, s->nb_snapshots,
+ sizeof(QCowSnapshotHeader),
+ sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
+ "snapshot table", &local_err);
+ if (ret < 0) {
+ result->check_errors++;
+ error_reportf_err(local_err, "ERROR ");
+
+ /* We did not read the snapshot table, so invalidate this information */
+ s->snapshots_offset = 0;
+ s->nb_snapshots = 0;
+
+ return ret;
+ }
+
+ qemu_co_mutex_unlock(&s->lock);
+ ret = qcow2_read_snapshots(bs, &local_err);
+ qemu_co_mutex_lock(&s->lock);
+ if (ret < 0) {
+ result->check_errors++;
+ error_reportf_err(local_err,
+ "ERROR failed to read the snapshot table: ");
+
+ /* We did not read the snapshot table, so invalidate this information */
+ s->snapshots_offset = 0;
+ s->nb_snapshots = 0;
+
+ return ret;
+ }
+
+ return 0;
+}
+
static void find_new_snapshot_id(BlockDriverState *bs,
char *id_str, int id_str_size)
{
diff --git a/block/qcow2.c b/block/qcow2.c
index 2219639e11..56c764de0b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -568,11 +568,40 @@ int qcow2_mark_consistent(BlockDriverState *bs)
return 0;
}
+static void qcow2_add_check_result(BdrvCheckResult *out,
+ const BdrvCheckResult *src,
+ bool set_allocation_info)
+{
+ out->corruptions += src->corruptions;
+ out->leaks += src->leaks;
+ out->check_errors += src->check_errors;
+ out->corruptions_fixed += src->corruptions_fixed;
+ out->leaks_fixed += src->leaks_fixed;
+
+ if (set_allocation_info) {
+ out->image_end_offset = src->image_end_offset;
+ out->bfi = src->bfi;
+ }
+}
+
static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
BdrvCheckResult *result,
BdrvCheckMode fix)
{
- int ret = qcow2_check_refcounts(bs, result, fix);
+ BdrvCheckResult snapshot_res = {};
+ BdrvCheckResult refcount_res = {};
+ int ret;
+
+ memset(result, 0, sizeof(*result));
+
+ ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
+ qcow2_add_check_result(result, &snapshot_res, false);
+ if (ret < 0) {
+ return ret;
+ }
+
+ ret = qcow2_check_refcounts(bs, &refcount_res, fix);
+ qcow2_add_check_result(result, &refcount_res, true);
if (ret < 0) {
return ret;
}
@@ -1404,17 +1433,22 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
goto fail;
}
- /* The total size in bytes of the snapshot table is checked in
- * qcow2_read_snapshots() because the size of each snapshot is
- * variable and we don't know it yet.
- * Here we only check the offset and number of snapshots. */
- ret = qcow2_validate_table(bs, header.snapshots_offset,
- header.nb_snapshots,
- sizeof(QCowSnapshotHeader),
- sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
- "Snapshot table", errp);
- if (ret < 0) {
- goto fail;
+ if (!(flags & BDRV_O_CHECK)) {
+ /*
+ * The total size in bytes of the snapshot table is checked in
+ * qcow2_read_snapshots() because the size of each snapshot is
+ * variable and we don't know it yet.
+ * Here we only check the offset and number of snapshots.
+ */
+ ret = qcow2_validate_table(bs, header.snapshots_offset,
+ header.nb_snapshots,
+ sizeof(QCowSnapshotHeader),
+ sizeof(QCowSnapshotHeader) *
+ QCOW_MAX_SNAPSHOTS,
+ "Snapshot table", errp);
+ if (ret < 0) {
+ goto fail;
+ }
}
/* read the level 1 table */
@@ -1574,13 +1608,19 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
s->image_backing_file = g_strdup(bs->auto_backing_file);
}
- /* Internal snapshots */
- s->snapshots_offset = header.snapshots_offset;
- s->nb_snapshots = header.nb_snapshots;
+ /*
+ * Internal snapshots; skip reading them in check mode, because
+ * we do not need them then, and we do not want to abort because
+ * of a broken table.
+ */
+ if (!(flags & BDRV_O_CHECK)) {
+ s->snapshots_offset = header.snapshots_offset;
+ s->nb_snapshots = header.nb_snapshots;
- ret = qcow2_read_snapshots(bs, errp);
- if (ret < 0) {
- goto fail;
+ ret = qcow2_read_snapshots(bs, errp);
+ if (ret < 0) {
+ goto fail;
+ }
}
/* Clear unknown autoclear feature bits */
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 09/16] qcow2: Add qcow2_check_fix_snapshot_table()
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (7 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 08/16] qcow2: Separate qcow2_check_read_snapshot_table() Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries Max Reitz
` (6 subsequent siblings)
15 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
qcow2_check_read_snapshot_table() can perform consistency checks, but it
cannot fix everything. Specifically, it cannot allocate new clusters,
because that should wait until the refcount structures are known to be
consistent (i.e., after qcow2_check_refcounts()). Thus, it cannot call
qcow2_write_snapshots().
Do that in qcow2_check_fix_snapshot_table(), which is called after
qcow2_check_refcounts().
Currently, there is nothing that would set result->corruptions, so this
is a no-op. A follow-up patch will change that.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 3 +++
block/qcow2-snapshot.c | 25 +++++++++++++++++++++++++
block/qcow2.c | 9 ++++++++-
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 50c7eefb5b..6823c37e82 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -715,6 +715,9 @@ int qcow2_write_snapshots(BlockDriverState *bs);
int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
BdrvCheckResult *result,
BdrvCheckMode fix);
+int coroutine_fn qcow2_check_fix_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix);
/* qcow2-cache.c functions */
Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d667bfd935..b526a8f819 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -380,6 +380,31 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
return 0;
}
+int coroutine_fn qcow2_check_fix_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+
+ if (result->corruptions && (fix & BDRV_FIX_ERRORS)) {
+ qemu_co_mutex_unlock(&s->lock);
+ ret = qcow2_write_snapshots(bs);
+ qemu_co_mutex_lock(&s->lock);
+ if (ret < 0) {
+ result->check_errors++;
+ fprintf(stderr, "ERROR failed to update snapshot table: %s\n",
+ strerror(-ret));
+ return ret;
+ }
+
+ result->corruptions_fixed += result->corruptions;
+ result->corruptions = 0;
+ }
+
+ return 0;
+}
+
static void find_new_snapshot_id(BlockDriverState *bs,
char *id_str, int id_str_size)
{
diff --git a/block/qcow2.c b/block/qcow2.c
index 56c764de0b..a32ff20458 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -595,13 +595,20 @@ static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
memset(result, 0, sizeof(*result));
ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
- qcow2_add_check_result(result, &snapshot_res, false);
if (ret < 0) {
+ qcow2_add_check_result(result, &snapshot_res, false);
return ret;
}
ret = qcow2_check_refcounts(bs, &refcount_res, fix);
qcow2_add_check_result(result, &refcount_res, true);
+ if (ret < 0) {
+ qcow2_add_check_result(result, &snapshot_res, false);
+ return ret;
+ }
+
+ ret = qcow2_check_fix_snapshot_table(bs, &snapshot_res, fix);
+ qcow2_add_check_result(result, &snapshot_res, false);
if (ret < 0) {
return ret;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (8 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 09/16] qcow2: Add qcow2_check_fix_snapshot_table() Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:37 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length Max Reitz
` (5 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
The only case where we currently reject snapshot table entries is when
they have too much extra data. Fix them with qemu-img check -r all by
counting it as a corruption, reducing their extra_data_size, and then
letting qcow2_check_fix_snapshot_table() do the rest.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-snapshot.c | 67 +++++++++++++++++++++++++++++++++++-------
1 file changed, 56 insertions(+), 11 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index b526a8f819..53dc1635ec 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -44,7 +44,23 @@ void qcow2_free_snapshots(BlockDriverState *bs)
s->nb_snapshots = 0;
}
-int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+/*
+ * If @repair is true, try to repair a broken snapshot table instead
+ * of just returning an error:
+ *
+ * - If there were snapshots with too much extra metadata, increment
+ * *extra_data_dropped for each.
+ * This requires the caller to eventually rewrite the whole snapshot
+ * table, which requires cluster allocation. Therefore, this should
+ * be done only after qcow2_check_refcounts() made sure the refcount
+ * structures are valid.
+ * (In the meantime, the image is still valid because
+ * qcow2_check_refcounts() does not do anything with snapshots'
+ * extra data.)
+ */
+static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+ int *extra_data_dropped,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
QCowSnapshotHeader h;
@@ -64,6 +80,8 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
for(i = 0; i < s->nb_snapshots; i++) {
+ bool truncate_unknown_extra_data = false;
+
/* Read statically sized part of the snapshot header */
offset = ROUND_UP(offset, 8);
ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
@@ -86,10 +104,21 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
name_size = be16_to_cpu(h.name_size);
if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
- ret = -EFBIG;
- error_setg(errp, "Too much extra metadata in snapshot table "
- "entry %i", i);
- goto fail;
+ if (!repair) {
+ ret = -EFBIG;
+ error_setg(errp, "Too much extra metadata in snapshot table "
+ "entry %i", i);
+ error_append_hint(errp, "You can force-remove this extra "
+ "metadata with qemu-img check -r all\n");
+ goto fail;
+ }
+
+ fprintf(stderr, "Discarding too much extra metadata in snapshot "
+ "table entry %i (%" PRIu32 " > %u)\n",
+ i, sn->extra_data_size, QCOW_MAX_SNAPSHOT_EXTRA_DATA);
+
+ (*extra_data_dropped)++;
+ truncate_unknown_extra_data = true;
}
/* Read known extra data */
@@ -113,18 +142,26 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
}
if (sn->extra_data_size > sizeof(extra)) {
- /* Store unknown extra data */
- size_t unknown_extra_data_size =
- sn->extra_data_size - sizeof(extra);
+ uint64_t extra_data_end;
+ size_t unknown_extra_data_size;
+
+ extra_data_end = offset + sn->extra_data_size - sizeof(extra);
+ if (truncate_unknown_extra_data) {
+ sn->extra_data_size = QCOW_MAX_SNAPSHOT_EXTRA_DATA;
+ }
+
+ /* Store unknown extra data */
+ unknown_extra_data_size = sn->extra_data_size - sizeof(extra);
sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
unknown_extra_data_size);
if (ret < 0) {
- error_setg_errno(errp, -ret, "Failed to read snapshot table");
+ error_setg_errno(errp, -ret,
+ "Failed to read snapshot table");
goto fail;
}
- offset += unknown_extra_data_size;
+ offset = extra_data_end;
}
/* Read snapshot ID */
@@ -163,6 +200,11 @@ fail:
return ret;
}
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+{
+ return qcow2_do_read_snapshots(bs, false, NULL, errp);
+}
+
/* add at the end of the file a new list of snapshots */
int qcow2_write_snapshots(BlockDriverState *bs)
{
@@ -328,6 +370,7 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
{
BDRVQcow2State *s = bs->opaque;
Error *local_err = NULL;
+ int extra_data_dropped = 0;
int ret;
struct {
uint32_t nb_snapshots;
@@ -363,7 +406,8 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
}
qemu_co_mutex_unlock(&s->lock);
- ret = qcow2_read_snapshots(bs, &local_err);
+ ret = qcow2_do_read_snapshots(bs, fix & BDRV_FIX_ERRORS,
+ &extra_data_dropped, &local_err);
qemu_co_mutex_lock(&s->lock);
if (ret < 0) {
result->check_errors++;
@@ -376,6 +420,7 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
return ret;
}
+ result->corruptions += extra_data_dropped;
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (9 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:40 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables Max Reitz
` (4 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
When repairing the snapshot table, we truncate entries that have too
much extra data. This frees up space that we do not have to count
towards the snapshot table size.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-snapshot.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 53dc1635ec..582eb3386a 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -68,6 +68,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
QCowSnapshot *sn;
int i, id_str_size, name_size;
int64_t offset;
+ uint64_t table_length = 0;
int ret;
if (!s->nb_snapshots) {
@@ -82,6 +83,8 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
for(i = 0; i < s->nb_snapshots; i++) {
bool truncate_unknown_extra_data = false;
+ table_length = ROUND_UP(table_length, 8);
+
/* Read statically sized part of the snapshot header */
offset = ROUND_UP(offset, 8);
ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
@@ -184,7 +187,16 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
offset += name_size;
sn->name[name_size] = '\0';
- if (offset - s->snapshots_offset > QCOW_MAX_SNAPSHOTS_SIZE) {
+ /* Note that the extra data may have been truncated */
+ table_length += sizeof(h) + sn->extra_data_size + id_str_size +
+ name_size;
+ if (!repair) {
+ assert(table_length == offset - s->snapshots_offset);
+ }
+
+ if (table_length > QCOW_MAX_SNAPSHOTS_SIZE ||
+ offset - s->snapshots_offset > INT_MAX)
+ {
ret = -EFBIG;
error_setg(errp, "Snapshot table is too big");
goto fail;
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (10 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:43 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries Max Reitz
` (3 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
We currently refuse to open qcow2 images with overly long snapshot
tables. This patch makes qemu-img check -r all drop all offending
entries past what we deem acceptable.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-snapshot.c | 88 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 78 insertions(+), 10 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 582eb3386a..366d9f574c 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -29,15 +29,24 @@
#include "qemu/error-report.h"
#include "qemu/cutils.h"
+static void qcow2_free_single_snapshot(BlockDriverState *bs, int i)
+{
+ BDRVQcow2State *s = bs->opaque;
+
+ assert(i >= 0 && i < s->nb_snapshots);
+ g_free(s->snapshots[i].name);
+ g_free(s->snapshots[i].id_str);
+ g_free(s->snapshots[i].unknown_extra_data);
+ memset(&s->snapshots[i], 0, sizeof(s->snapshots[i]));
+}
+
void qcow2_free_snapshots(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
int i;
for(i = 0; i < s->nb_snapshots; i++) {
- g_free(s->snapshots[i].name);
- g_free(s->snapshots[i].id_str);
- g_free(s->snapshots[i].unknown_extra_data);
+ qcow2_free_single_snapshot(bs, i);
}
g_free(s->snapshots);
s->snapshots = NULL;
@@ -48,6 +57,14 @@ void qcow2_free_snapshots(BlockDriverState *bs)
* If @repair is true, try to repair a broken snapshot table instead
* of just returning an error:
*
+ * - If the snapshot table was too long, set *nb_clusters_reduced to
+ * the number of snapshots removed off the end.
+ * The caller will update the on-disk nb_snapshots accordingly;
+ * this leaks clusters, but is safe.
+ * (The on-disk information must be updated before
+ * qcow2_check_refcounts(), because that function relies on
+ * s->nb_snapshots to reflect the on-disk value.)
+ *
* - If there were snapshots with too much extra metadata, increment
* *extra_data_dropped for each.
* This requires the caller to eventually rewrite the whole snapshot
@@ -59,6 +76,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
* extra data.)
*/
static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+ int *nb_clusters_reduced,
int *extra_data_dropped,
Error **errp)
{
@@ -67,7 +85,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
QCowSnapshotExtraData extra;
QCowSnapshot *sn;
int i, id_str_size, name_size;
- int64_t offset;
+ int64_t offset, pre_sn_offset;
uint64_t table_length = 0;
int ret;
@@ -83,6 +101,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
for(i = 0; i < s->nb_snapshots; i++) {
bool truncate_unknown_extra_data = false;
+ pre_sn_offset = offset;
table_length = ROUND_UP(table_length, 8);
/* Read statically sized part of the snapshot header */
@@ -197,9 +216,31 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
if (table_length > QCOW_MAX_SNAPSHOTS_SIZE ||
offset - s->snapshots_offset > INT_MAX)
{
- ret = -EFBIG;
- error_setg(errp, "Snapshot table is too big");
- goto fail;
+ if (!repair) {
+ ret = -EFBIG;
+ error_setg(errp, "Snapshot table is too big");
+ error_append_hint(errp, "You can force-remove all %u "
+ "overhanging snapshots with qemu-img check "
+ "-r all\n", s->nb_snapshots - i);
+ goto fail;
+ }
+
+ fprintf(stderr, "Discarding %u overhanging snapshots (snapshot "
+ "table is too big)\n", s->nb_snapshots - i);
+
+ *nb_clusters_reduced += (s->nb_snapshots - i);
+
+ /* Discard current snapshot also */
+ qcow2_free_single_snapshot(bs, i);
+
+ /*
+ * This leaks all the rest of the snapshot table and the
+ * snapshots' clusters, but we run in check -r all mode,
+ * so qcow2_check_refcounts() will take care of it.
+ */
+ s->nb_snapshots = i;
+ offset = pre_sn_offset;
+ break;
}
}
@@ -214,7 +255,7 @@ fail:
int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
{
- return qcow2_do_read_snapshots(bs, false, NULL, errp);
+ return qcow2_do_read_snapshots(bs, false, NULL, NULL, errp);
}
/* add at the end of the file a new list of snapshots */
@@ -382,6 +423,7 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
{
BDRVQcow2State *s = bs->opaque;
Error *local_err = NULL;
+ int nb_clusters_reduced = 0;
int extra_data_dropped = 0;
int ret;
struct {
@@ -419,7 +461,8 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
qemu_co_mutex_unlock(&s->lock);
ret = qcow2_do_read_snapshots(bs, fix & BDRV_FIX_ERRORS,
- &extra_data_dropped, &local_err);
+ &nb_clusters_reduced, &extra_data_dropped,
+ &local_err);
qemu_co_mutex_lock(&s->lock);
if (ret < 0) {
result->check_errors++;
@@ -432,7 +475,32 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
return ret;
}
- result->corruptions += extra_data_dropped;
+ result->corruptions += nb_clusters_reduced + extra_data_dropped;
+
+ if (nb_clusters_reduced) {
+ /*
+ * Update image header now, because:
+ * (1) qcow2_check_refcounts() relies on s->nb_snapshots to be
+ * the same as what the image header says,
+ * (2) this leaks clusters, but qcow2_check_refcounts() will
+ * fix that.
+ */
+ assert(fix & BDRV_FIX_ERRORS);
+
+ snapshot_table_pointer.nb_snapshots = cpu_to_be32(s->nb_snapshots);
+ ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
+ &snapshot_table_pointer.nb_snapshots,
+ sizeof(snapshot_table_pointer.nb_snapshots));
+ if (ret < 0) {
+ result->check_errors++;
+ fprintf(stderr, "ERROR failed to update the snapshot count in the "
+ "image header: %s\n", strerror(-ret));
+ return ret;
+ }
+
+ result->corruptions_fixed += nb_clusters_reduced;
+ result->corruptions -= nb_clusters_reduced;
+ }
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (11 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables Max Reitz
@ 2019-08-19 18:55 ` Max Reitz
2019-08-19 19:45 ` Eric Blake
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy Max Reitz
` (2 subsequent siblings)
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:55 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-snapshot.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 366d9f574c..dac8a778e4 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -444,6 +444,14 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
s->snapshots_offset = be64_to_cpu(snapshot_table_pointer.snapshots_offset);
s->nb_snapshots = be32_to_cpu(snapshot_table_pointer.nb_snapshots);
+ if (s->nb_snapshots > QCOW_MAX_SNAPSHOTS && (fix & BDRV_FIX_ERRORS)) {
+ fprintf(stderr, "Discarding %u overhanging snapshots\n",
+ s->nb_snapshots - QCOW_MAX_SNAPSHOTS);
+
+ nb_clusters_reduced += s->nb_snapshots - QCOW_MAX_SNAPSHOTS;
+ s->nb_snapshots = QCOW_MAX_SNAPSHOTS;
+ }
+
ret = qcow2_validate_table(bs, s->snapshots_offset, s->nb_snapshots,
sizeof(QCowSnapshotHeader),
sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
@@ -452,6 +460,12 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
result->check_errors++;
error_reportf_err(local_err, "ERROR ");
+ if (s->nb_snapshots > QCOW_MAX_SNAPSHOTS) {
+ fprintf(stderr, "You can force-remove all %u overhanging snapshots "
+ "with qemu-img check -r all\n",
+ s->nb_snapshots - QCOW_MAX_SNAPSHOTS);
+ }
+
/* We did not read the snapshot table, so invalidate this information */
s->snapshots_offset = 0;
s->nb_snapshots = 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (12 preceding siblings ...)
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries Max Reitz
@ 2019-08-19 18:56 ` Max Reitz
2019-08-19 19:46 ` Eric Blake
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 15/16] iotests: Add peek_file* functions Max Reitz
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling Max Reitz
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:56 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
qcow2 v3 images require every snapshot table entry to have at least 16
bytes of extra data. If they do not, let qemu-img check -r all fix it.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-snapshot.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index dac8a778e4..5ab64da1ec 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -516,6 +516,24 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
result->corruptions -= nb_clusters_reduced;
}
+ /*
+ * All of v3 images' snapshot table entries need to have at least
+ * 16 bytes of extra data.
+ */
+ if (s->qcow_version >= 3) {
+ int i;
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (s->snapshots[i].extra_data_size <
+ sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+ sizeof_field(QCowSnapshotExtraData, disk_size))
+ {
+ result->corruptions++;
+ fprintf(stderr, "%s snapshot table entry %i is incomplete\n",
+ fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+ }
+ }
+ }
+
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 15/16] iotests: Add peek_file* functions
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (13 preceding siblings ...)
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy Max Reitz
@ 2019-08-19 18:56 ` Max Reitz
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling Max Reitz
15 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:56 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/common.rc | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 5502c3da2f..78decfd5d5 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,26 @@ poke_file()
printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
}
+# peek_file_le 'test.img' 512 2 => 65534
+peek_file_le()
+{
+ # Wrap in echo $() to strip spaces
+ echo $(od -j"$2" -N"$3" --endian=little -An -vtu"$3" "$1")
+}
+
+# peek_file_be 'test.img' 512 2 => 65279
+peek_file_be()
+{
+ # Wrap in echo $() to strip spaces
+ echo $(od -j"$2" -N"$3" --endian=big -An -vtu"$3" "$1")
+}
+
+# peek_file_raw 'test.img' 512 2 => '\xff\xfe'
+peek_file_raw()
+{
+ dd if="$1" bs=1 skip="$2" count="$3" status=none
+}
+
if ! . ./common.config
then
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
` (14 preceding siblings ...)
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 15/16] iotests: Add peek_file* functions Max Reitz
@ 2019-08-19 18:56 ` Max Reitz
2019-08-19 20:25 ` Eric Blake
15 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-19 18:56 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Add a test how our qcow2 driver handles extra data in snapshot table
entries, and how it repairs overly long snapshot tables.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/261 | 523 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/261.out | 346 ++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 870 insertions(+)
create mode 100755 tests/qemu-iotests/261
create mode 100644 tests/qemu-iotests/261.out
diff --git a/tests/qemu-iotests/261 b/tests/qemu-iotests/261
new file mode 100755
index 0000000000..fb96bcfbe2
--- /dev/null
+++ b/tests/qemu-iotests/261
@@ -0,0 +1,523 @@
+#!/usr/bin/env bash
+#
+# Test case for qcow2's handling of extra data in snapshot table entries
+# (and more generally, how certain cases of broken snapshot tables are
+# handled)
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ rm -f "$TEST_IMG".v{2,3}.orig
+ rm -f "$TEST_DIR"/sn{0,1,2}{,-pre,-extra,-post}
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+# (1) We create a v2 image that supports nothing but refcount_bits=16
+# (2) We do some refcount management on our own which expects
+# refcount_bits=16
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+
+# Parameters:
+# $1: image filename
+# $2: snapshot table entry offset in the image
+snapshot_table_entry_size()
+{
+ id_len=$(peek_file_be "$1" $(($2 + 12)) 2)
+ name_len=$(peek_file_be "$1" $(($2 + 14)) 2)
+ extra_len=$(peek_file_be "$1" $(($2 + 36)) 4)
+
+ full_len=$((40 + extra_len + id_len + name_len))
+ echo $(((full_len + 7) / 8 * 8))
+}
+
+# Parameter:
+# $1: image filename
+print_snapshot_table()
+{
+ nb_entries=$(peek_file_be "$1" 60 4)
+ offset=$(peek_file_be "$1" 64 8)
+
+ echo "Snapshots in $1:" | _filter_testdir | _filter_imgfmt
+
+ for ((i = 0; i < nb_entries; i++)); do
+ id_len=$(peek_file_be "$1" $((offset + 12)) 2)
+ name_len=$(peek_file_be "$1" $((offset + 14)) 2)
+ extra_len=$(peek_file_be "$1" $((offset + 36)) 4)
+
+ extra_ofs=$((offset + 40))
+ id_ofs=$((extra_ofs + extra_len))
+ name_ofs=$((id_ofs + id_len))
+
+ echo " [$i]"
+ echo " ID: $(peek_file_raw "$1" $id_ofs $id_len)"
+ echo " Name: $(peek_file_raw "$1" $name_ofs $name_len)"
+ echo " Extra data size: $extra_len"
+ if [ $extra_len -ge 8 ]; then
+ echo " VM state size: $(peek_file_be "$1" $extra_ofs 8)"
+ fi
+ if [ $extra_len -ge 16 ]; then
+ echo " Disk size: $(peek_file_be "$1" $((extra_ofs + 8)) 8)"
+ fi
+ if [ $extra_len -gt 16 ]; then
+ echo ' Unknown extra data:' \
+ "$(peek_file_raw "$1" $((extra_ofs + 16)) $((extra_len - 16)) \
+ | tr -d '\0')"
+ fi
+
+ offset=$((offset + $(snapshot_table_entry_size "$1" $offset)))
+ done
+}
+
+# Mark clusters as allocated; works only in refblock 0 (i.e. before
+# cluster #32768).
+# Parameters:
+# $1: Start offset of what to allocate
+# $2: End offset (exclusive)
+refblock0_allocate()
+{
+ reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
+ refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
+
+ cluster=$(($1 / 65536))
+ ecluster=$((($2 + 65535) / 65536))
+
+ while [ $cluster -lt $ecluster ]; do
+ if [ $cluster -ge 32768 ]; then
+ echo "*** Abort: Cluster $cluster exceeds refblock 0 ***"
+ exit 1
+ fi
+ poke_file "$TEST_IMG" $((refblock_ofs + cluster * 2)) '\x00\x01'
+ cluster=$((cluster + 1))
+ done
+}
+
+
+echo
+echo '=== Create v2 template ==='
+echo
+
+# Create v2 image with a snapshot table with three entries:
+# [0]: No extra data (valid with v2, not valid with v3)
+# [1]: Has extra data unknown to qemu
+# [2]: Has the 64-bit VM state size, but not the disk size (again,
+# valid with v2, not valid with v3)
+
+TEST_IMG="$TEST_IMG.v2.orig" IMGOPTS='compat=0.10' _make_test_img 64M
+$QEMU_IMG snapshot -c sn0 "$TEST_IMG.v2.orig"
+$QEMU_IMG snapshot -c sn1 "$TEST_IMG.v2.orig"
+$QEMU_IMG snapshot -c sn2 "$TEST_IMG.v2.orig"
+
+# Copy out all existing snapshot table entries
+sn_table_ofs=$(peek_file_be "$TEST_IMG.v2.orig" 64 8)
+
+# ofs: Snapshot table entry offset
+# eds: Extra data size
+# ids: Name + ID size
+# len: Total entry length
+sn0_ofs=$sn_table_ofs
+sn0_eds=$(peek_file_be "$TEST_IMG.v2.orig" $((sn0_ofs + 36)) 4)
+sn0_ids=$(($(peek_file_be "$TEST_IMG.v2.orig" $((sn0_ofs + 12)) 2) +
+ $(peek_file_be "$TEST_IMG.v2.orig" $((sn0_ofs + 14)) 2)))
+sn0_len=$(snapshot_table_entry_size "$TEST_IMG.v2.orig" $sn0_ofs)
+sn1_ofs=$((sn0_ofs + sn0_len))
+sn1_eds=$(peek_file_be "$TEST_IMG.v2.orig" $((sn1_ofs + 36)) 4)
+sn1_ids=$(($(peek_file_be "$TEST_IMG.v2.orig" $((sn1_ofs + 12)) 2) +
+ $(peek_file_be "$TEST_IMG.v2.orig" $((sn1_ofs + 14)) 2)))
+sn1_len=$(snapshot_table_entry_size "$TEST_IMG.v2.orig" $sn1_ofs)
+sn2_ofs=$((sn1_ofs + sn1_len))
+sn2_eds=$(peek_file_be "$TEST_IMG.v2.orig" $((sn2_ofs + 36)) 4)
+sn2_ids=$(($(peek_file_be "$TEST_IMG.v2.orig" $((sn2_ofs + 12)) 2) +
+ $(peek_file_be "$TEST_IMG.v2.orig" $((sn2_ofs + 14)) 2)))
+sn2_len=$(snapshot_table_entry_size "$TEST_IMG.v2.orig" $sn2_ofs)
+
+# Data before extra data
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn0-pre" bs=1 skip=$sn0_ofs count=40 \
+ &> /dev/null
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn1-pre" bs=1 skip=$sn1_ofs count=40 \
+ &> /dev/null
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn2-pre" bs=1 skip=$sn2_ofs count=40 \
+ &> /dev/null
+
+# Extra data
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn0-extra" bs=1 \
+ skip=$((sn0_ofs + 40)) count=$sn0_eds &> /dev/null
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn1-extra" bs=1 \
+ skip=$((sn1_ofs + 40)) count=$sn1_eds &> /dev/null
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn2-extra" bs=1 \
+ skip=$((sn2_ofs + 40)) count=$sn2_eds &> /dev/null
+
+# Data after extra data
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn0-post" bs=1 \
+ skip=$((sn0_ofs + 40 + sn0_eds)) count=$sn0_ids \
+ &> /dev/null
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn1-post" bs=1 \
+ skip=$((sn1_ofs + 40 + sn1_eds)) count=$sn1_ids \
+ &> /dev/null
+dd if="$TEST_IMG.v2.orig" of="$TEST_DIR/sn2-post" bs=1 \
+ skip=$((sn2_ofs + 40 + sn2_eds)) count=$sn2_ids \
+ &> /dev/null
+
+# Amend them, one by one
+# Set sn0's extra data size to 0
+poke_file "$TEST_DIR/sn0-pre" 36 '\x00\x00\x00\x00'
+truncate -s 0 "$TEST_DIR/sn0-extra"
+# Grow sn0-post to pad
+truncate -s $(($(snapshot_table_entry_size "$TEST_DIR/sn0-pre") - 40)) \
+ "$TEST_DIR/sn0-post"
+
+# Set sn1's extra data size to 42
+poke_file "$TEST_DIR/sn1-pre" 36 '\x00\x00\x00\x2a'
+truncate -s 42 "$TEST_DIR/sn1-extra"
+poke_file "$TEST_DIR/sn1-extra" 16 'very important data'
+# Grow sn1-post to pad
+truncate -s $(($(snapshot_table_entry_size "$TEST_DIR/sn1-pre") - 82)) \
+ "$TEST_DIR/sn1-post"
+
+# Set sn2's extra data size to 8
+poke_file "$TEST_DIR/sn2-pre" 36 '\x00\x00\x00\x08'
+truncate -s 8 "$TEST_DIR/sn2-extra"
+# Grow sn2-post to pad
+truncate -s $(($(snapshot_table_entry_size "$TEST_DIR/sn2-pre") - 48)) \
+ "$TEST_DIR/sn2-post"
+
+# Construct snapshot table
+cat "$TEST_DIR"/sn0-{pre,extra,post} \
+ "$TEST_DIR"/sn1-{pre,extra,post} \
+ "$TEST_DIR"/sn2-{pre,extra,post} \
+ | dd of="$TEST_IMG.v2.orig" bs=1 seek=$sn_table_ofs conv=notrunc \
+ &> /dev/null
+
+# Done!
+TEST_IMG="$TEST_IMG.v2.orig" _check_test_img
+print_snapshot_table "$TEST_IMG.v2.orig"
+
+echo
+echo '=== Upgrade to v3 ==='
+echo
+
+cp "$TEST_IMG.v2.orig" "$TEST_IMG.v3.orig"
+$QEMU_IMG amend -o compat=1.1 "$TEST_IMG.v3.orig"
+TEST_IMG="$TEST_IMG.v3.orig" _check_test_img
+print_snapshot_table "$TEST_IMG.v3.orig"
+
+echo
+echo '=== Repair botched v3 ==='
+echo
+
+# Force the v2 file to be v3. v3 requires each snapshot table entry
+# to have at least 16 bytes of extra data, so it will not comply to
+# the qcow2 v3 specification; but we can fix that.
+cp "$TEST_IMG.v2.orig" "$TEST_IMG"
+
+# Set version
+poke_file "$TEST_IMG" 4 '\x00\x00\x00\x03'
+# Increase header length (necessary for v3)
+poke_file "$TEST_IMG" 100 '\x00\x00\x00\x68'
+# Set refcount order (necessary for v3)
+poke_file "$TEST_IMG" 96 '\x00\x00\x00\x04'
+
+_check_test_img -r all
+print_snapshot_table "$TEST_IMG"
+
+
+# From now on, just test the qcow2 version we are supposed to test.
+# (v3 by default, v2 by choice through $IMGOPTS.)
+# That works because we always write all known extra data when
+# updating the snapshot table, independent of the version.
+
+if echo "$IMGOPTS" | grep -q 'compat=\(0\.10\|v2\)' 2> /dev/null; then
+ subver=v2
+else
+ subver=v3
+fi
+
+echo
+echo '=== Add new snapshot ==='
+echo
+
+cp "$TEST_IMG.$subver.orig" "$TEST_IMG"
+$QEMU_IMG snapshot -c sn3 "$TEST_IMG"
+_check_test_img
+print_snapshot_table "$TEST_IMG"
+
+echo
+echo '=== Remove different snapshots ==='
+
+for sn in sn0 sn1 sn2; do
+ echo
+ echo "--- $sn ---"
+
+ cp "$TEST_IMG.$subver.orig" "$TEST_IMG"
+ $QEMU_IMG snapshot -d $sn "$TEST_IMG"
+ _check_test_img
+ print_snapshot_table "$TEST_IMG"
+done
+
+echo
+echo '=== Reject too much unknown extra data ==='
+echo
+
+cp "$TEST_IMG.$subver.orig" "$TEST_IMG"
+$QEMU_IMG snapshot -c sn3 "$TEST_IMG"
+
+sn_table_ofs=$(peek_file_be "$TEST_IMG" 64 8)
+sn0_ofs=$sn_table_ofs
+sn1_ofs=$((sn0_ofs + $(snapshot_table_entry_size "$TEST_IMG" $sn0_ofs)))
+sn2_ofs=$((sn1_ofs + $(snapshot_table_entry_size "$TEST_IMG" $sn1_ofs)))
+sn3_ofs=$((sn2_ofs + $(snapshot_table_entry_size "$TEST_IMG" $sn2_ofs)))
+
+# 64 kB of extra data should be rejected
+# (Note that this also induces a refcount error, because it spills
+# over to the next cluster. That's a good way to test that we can
+# handle simultaneous snapshot table and refcount errors.)
+poke_file "$TEST_IMG" $((sn3_ofs + 36)) '\x00\x01\x00\x00'
+
+# Print error
+_img_info
+echo
+_check_test_img
+echo
+
+# Should be repairable
+_check_test_img -r all
+
+echo
+echo '=== Snapshot table too big ==='
+echo
+
+sn_table_ofs=$(peek_file_be "$TEST_IMG.v3.orig" 64 8)
+
+# Fill a snapshot with 1 kB of extra data, a 65535-char ID, and a
+# 65535-char name, and repeat it as many times as necessary to fill
+# 64 MB (the maximum supported by qemu)
+
+touch "$TEST_DIR/sn0"
+
+# Full size (fixed + extra + ID + name + padding)
+sn_size=$((40 + 1024 + 65535 + 65535 + 2))
+
+# We only need the fixed part, though.
+truncate -s 40 "$TEST_DIR/sn0"
+
+# 65535-char ID string
+poke_file "$TEST_DIR/sn0" 12 '\xff\xff'
+# 65535-char name
+poke_file "$TEST_DIR/sn0" 14 '\xff\xff'
+# 1 kB of extra data
+poke_file "$TEST_DIR/sn0" 36 '\x00\x00\x04\x00'
+
+# Create test image
+_make_test_img 64M
+
+# Hook up snapshot table somewhere safe (at 1 MB)
+poke_file "$TEST_IMG" 64 '\x00\x00\x00\x00\x00\x10\x00\x00'
+
+offset=1048576
+size_written=0
+sn_count=0
+while [ $size_written -le $((64 * 1048576)) ]; do
+ dd if="$TEST_DIR/sn0" of="$TEST_IMG" bs=1 seek=$offset conv=notrunc \
+ &> /dev/null
+ offset=$((offset + sn_size))
+ size_written=$((size_written + sn_size))
+ sn_count=$((sn_count + 1))
+done
+truncate -s "$offset" "$TEST_IMG"
+
+# Give the last snapshot (the one to be removed) an L1 table so we can
+# see how that is handled when repairing the image
+# (Put it two clusters before 1 MB, and one L2 table one cluster
+# before 1 MB)
+poke_file "$TEST_IMG" $((offset - sn_size + 0)) \
+ '\x00\x00\x00\x00\x00\x0e\x00\x00'
+poke_file "$TEST_IMG" $((offset - sn_size + 8)) \
+ '\x00\x00\x00\x01'
+
+# Hook up the L2 table
+poke_file "$TEST_IMG" $((1048576 - 2 * 65536)) \
+ '\x80\x00\x00\x00\x00\x0f\x00\x00'
+
+# Make sure all of the clusters we just hooked up are allocated:
+# - The snapshot table
+# - The last snapshot's L1 and L2 table
+refblock0_allocate $((1048576 - 2 * 65536)) $offset
+
+poke_file "$TEST_IMG" 60 \
+ "$(printf '%08x' $sn_count | sed -e 's/\(..\)/\\x\1/g')"
+
+# Print error
+_img_info
+echo
+_check_test_img
+echo
+
+# Should be repairable
+_check_test_img -r all
+
+echo
+echo "$((sn_count - 1)) snapshots should remain:"
+echo " qemu-img info reports $(_img_info | grep -c '^ \{34\}') snapshots"
+echo " Image header reports $(peek_file_be "$TEST_IMG" 60 4) snapshots"
+
+echo
+echo '=== Snapshot table too big with one entry with too much extra data ==='
+echo
+
+# For this test, we reuse the image from the previous case, which has
+# a snapshot table that is right at the limit.
+# Our layout looks like this:
+# - (a number of snapshot table entries)
+# - One snapshot with $extra_data_size extra data
+# - One normal snapshot that breaks the 64 MB boundary
+# - One normal snapshot beyond the 64 MB boundary
+#
+# $extra_data_size is calculated so that simply by virtue of it
+# decreasing to 1 kB, the penultimate snapshot will fit into 64 MB
+# limit again. The final snapshot will always be beyond the limit, so
+# that we can see that the repair algorithm does still determine the
+# limit to be somewhere, even when truncating one snapshot's extra
+# data.
+
+# The last case has removed the last snapshot, so calculate
+# $old_offset to get the current image's real length
+old_offset=$((offset - sn_size))
+
+# The layout from the previous test had one snapshot beyond the 64 MB
+# limit; we want the same (after the oversized extra data has been
+# truncated to 1 kB), so we drop the last three snapshots and
+# construct them from scratch.
+offset=$((offset - 3 * sn_size))
+sn_count=$((sn_count - 3))
+
+# Assuming we had already written one of the three snapshots
+# (necessary so we can calculate $extra_data_size next).
+size_written=$((size_written - 2 * sn_size))
+
+# Increase the extra data size so we go past the limit
+# (The -1024 comes from the 1 kB of extra data we already have)
+extra_data_size=$((64 * 1048576 + 8 - sn_size - (size_written - 1024)))
+
+poke_file "$TEST_IMG" $((offset + 36)) \
+ "$(printf '%08x' $extra_data_size | sed -e 's/\(..\)/\\x\1/g')"
+
+offset=$((offset + sn_size - 1024 + extra_data_size))
+size_written=$((size_written - 1024 + extra_data_size))
+sn_count=$((sn_count + 1))
+
+# Write the two normal snapshots
+for ((i = 0; i < 2; i++)); do
+ dd if="$TEST_DIR/sn0" of="$TEST_IMG" bs=1 seek=$offset conv=notrunc \
+ &> /dev/null
+ offset=$((offset + sn_size))
+ size_written=$((size_written + sn_size))
+ sn_count=$((sn_count + 1))
+
+ if [ $i = 0 ]; then
+ # Check that the penultimate snapshot is beyond the 64 MB limit
+ echo "Snapshot table size should equal $((64 * 1048576 + 8)):" \
+ $size_written
+ echo
+ fi
+done
+
+truncate -s $offset "$TEST_IMG"
+refblock0_allocate $old_offset $offset
+
+poke_file "$TEST_IMG" 60 \
+ "$(printf '%08x' $sn_count | sed -e 's/\(..\)/\\x\1/g')"
+
+# Print error
+_img_info
+echo
+_check_test_img
+echo
+
+# Just truncating the extra data should be sufficient to shorten the
+# snapshot table so only one snapshot exceeds the extra size
+_check_test_img -r all
+
+echo
+echo '=== Too many snapshots ==='
+echo
+
+# Create a v2 image, for speeds' sake: All-zero snapshot table entries
+# are only valid in v2.
+IMGOPTS='compat=0.10' _make_test_img 64M
+
+# Hook up snapshot table somewhere safe (at 1 MB)
+poke_file "$TEST_IMG" 64 '\x00\x00\x00\x00\x00\x10\x00\x00'
+# "Create" more than 65536 snapshots (twice that many here)
+poke_file "$TEST_IMG" 60 '\x00\x02\x00\x00'
+
+# 40-byte all-zero snapshot table entries are valid snapshots, but
+# only in v2 (v3 needs 16 bytes of extra data, so we would have to
+# write 131072x '\x10').
+truncate -s $((1048576 + 40 * 131072)) "$TEST_IMG"
+
+# But let us give one of the snapshots to be removed an L1 table so
+# we can see how that is handled when repairing the image.
+# (Put it two clusters before 1 MB, and one L2 table one cluster
+# before 1 MB)
+poke_file "$TEST_IMG" $((1048576 + 40 * 65536 + 0)) \
+ '\x00\x00\x00\x00\x00\x0e\x00\x00'
+poke_file "$TEST_IMG" $((1048576 + 40 * 65536 + 8)) \
+ '\x00\x00\x00\x01'
+
+# Hook up the L2 table
+poke_file "$TEST_IMG" $((1048576 - 2 * 65536)) \
+ '\x80\x00\x00\x00\x00\x0f\x00\x00'
+
+# Make sure all of the clusters we just hooked up are allocated:
+# - The snapshot table
+# - The last snapshot's L1 and L2 table
+refblock0_allocate $((1048576 - 2 * 65536)) $((1048576 + 40 * 131072))
+
+# Print error
+_img_info
+echo
+_check_test_img
+echo
+
+# Should be repairable
+_check_test_img -r all
+
+echo
+echo '65536 snapshots should remain:'
+echo " qemu-img info reports $(_img_info | grep -c '^ \{34\}') snapshots"
+echo " Image header reports $(peek_file_be "$TEST_IMG" 60 4) snapshots"
+
+# success, all done
+echo "*** done"
+status=0
diff --git a/tests/qemu-iotests/261.out b/tests/qemu-iotests/261.out
new file mode 100644
index 0000000000..2600354566
--- /dev/null
+++ b/tests/qemu-iotests/261.out
@@ -0,0 +1,346 @@
+QA output created by 261
+
+=== Create v2 template ===
+
+Formatting 'TEST_DIR/t.IMGFMT.v2.orig', fmt=IMGFMT size=67108864
+No errors were found on the image.
+Snapshots in TEST_DIR/t.IMGFMT.v2.orig:
+ [0]
+ ID: 1
+ Name: sn0
+ Extra data size: 0
+ [1]
+ ID: 2
+ Name: sn1
+ Extra data size: 42
+ VM state size: 0
+ Disk size: 67108864
+ Unknown extra data: very important data
+ [2]
+ ID: 3
+ Name: sn2
+ Extra data size: 8
+ VM state size: 0
+
+=== Upgrade to v3 ===
+
+No errors were found on the image.
+Snapshots in TEST_DIR/t.IMGFMT.v3.orig:
+ [0]
+ ID: 1
+ Name: sn0
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+ [1]
+ ID: 2
+ Name: sn1
+ Extra data size: 42
+ VM state size: 0
+ Disk size: 67108864
+ Unknown extra data: very important data
+ [2]
+ ID: 3
+ Name: sn2
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+
+=== Repair botched v3 ===
+
+Repairing snapshot table entry 0 is incomplete
+Repairing snapshot table entry 2 is incomplete
+The following inconsistencies were found and repaired:
+
+ 0 leaked clusters
+ 2 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+Snapshots in TEST_DIR/t.IMGFMT:
+ [0]
+ ID: 1
+ Name: sn0
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+ [1]
+ ID: 2
+ Name: sn1
+ Extra data size: 42
+ VM state size: 0
+ Disk size: 67108864
+ Unknown extra data: very important data
+ [2]
+ ID: 3
+ Name: sn2
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+
+=== Add new snapshot ===
+
+No errors were found on the image.
+Snapshots in TEST_DIR/t.IMGFMT:
+ [0]
+ ID: 1
+ Name: sn0
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+ [1]
+ ID: 2
+ Name: sn1
+ Extra data size: 42
+ VM state size: 0
+ Disk size: 67108864
+ Unknown extra data: very important data
+ [2]
+ ID: 3
+ Name: sn2
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+ [3]
+ ID: 4
+ Name: sn3
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+
+=== Remove different snapshots ===
+
+--- sn0 ---
+No errors were found on the image.
+Snapshots in TEST_DIR/t.IMGFMT:
+ [0]
+ ID: 2
+ Name: sn1
+ Extra data size: 42
+ VM state size: 0
+ Disk size: 67108864
+ Unknown extra data: very important data
+ [1]
+ ID: 3
+ Name: sn2
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+
+--- sn1 ---
+No errors were found on the image.
+Snapshots in TEST_DIR/t.IMGFMT:
+ [0]
+ ID: 1
+ Name: sn0
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+ [1]
+ ID: 3
+ Name: sn2
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+
+--- sn2 ---
+No errors were found on the image.
+Snapshots in TEST_DIR/t.IMGFMT:
+ [0]
+ ID: 1
+ Name: sn0
+ Extra data size: 16
+ VM state size: 0
+ Disk size: 67108864
+ [1]
+ ID: 2
+ Name: sn1
+ Extra data size: 42
+ VM state size: 0
+ Disk size: 67108864
+ Unknown extra data: very important data
+
+=== Reject too much unknown extra data ===
+
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Too much extra metadata in snapshot table entry 3
+You can force-remove this extra metadata with qemu-img check -r all
+
+qemu-img: ERROR failed to read the snapshot table: Too much extra metadata in snapshot table entry 3
+You can force-remove this extra metadata with qemu-img check -r all
+qemu-img: Check failed: File too large
+
+Discarding too much extra metadata in snapshot table entry 3 (65536 > 1024)
+ERROR cluster 10 refcount=0 reference=1
+Rebuilding refcount structure
+Repairing cluster 1 refcount=1 reference=0
+Repairing cluster 2 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+ 0 leaked clusters
+ 2 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+
+=== Snapshot table too big ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Snapshot table is too big
+You can force-remove all 1 overhanging snapshots with qemu-img check -r all
+
+qemu-img: ERROR failed to read the snapshot table: Snapshot table is too big
+You can force-remove all 1 overhanging snapshots with qemu-img check -r all
+qemu-img: Check failed: File too large
+
+Discarding 1 overhanging snapshots (snapshot table is too big)
+Leaked cluster 14 refcount=1 reference=0
+Leaked cluster 15 refcount=1 reference=0
+Leaked cluster 1039 refcount=1 reference=0
+Leaked cluster 1040 refcount=1 reference=0
+Repairing cluster 14 refcount=1 reference=0
+Repairing cluster 15 refcount=1 reference=0
+Repairing cluster 1039 refcount=1 reference=0
+Repairing cluster 1040 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+ 4 leaked clusters
+ 1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+
+507 snapshots should remain:
+ qemu-img info reports 507 snapshots
+ Image header reports 507 snapshots
+
+=== Snapshot table too big with one entry with too much extra data ===
+
+Snapshot table size should equal 67108872: 67108872
+
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Too much extra metadata in snapshot table entry 505
+You can force-remove this extra metadata with qemu-img check -r all
+
+qemu-img: ERROR failed to read the snapshot table: Too much extra metadata in snapshot table entry 505
+You can force-remove this extra metadata with qemu-img check -r all
+qemu-img: Check failed: File too large
+
+Discarding too much extra metadata in snapshot table entry 505 (116944 > 1024)
+Discarding 1 overhanging snapshots (snapshot table is too big)
+Leaked cluster 1041 refcount=1 reference=0
+Leaked cluster 1042 refcount=1 reference=0
+Repairing cluster 1041 refcount=1 reference=0
+Repairing cluster 1042 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+ 2 leaked clusters
+ 2 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+
+=== Too many snapshots ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Snapshot table too large
+
+qemu-img: ERROR snapshot table too large
+You can force-remove all 65536 overhanging snapshots with qemu-img check -r all
+qemu-img: Check failed: File too large
+
+Discarding 65536 overhanging snapshots
+Leaked cluster 14 refcount=1 reference=0
+Leaked cluster 15 refcount=1 reference=0
+Leaked cluster 56 refcount=1 reference=0
+Leaked cluster 57 refcount=1 reference=0
+Leaked cluster 58 refcount=1 reference=0
+Leaked cluster 59 refcount=1 reference=0
+Leaked cluster 60 refcount=1 reference=0
+Leaked cluster 61 refcount=1 reference=0
+Leaked cluster 62 refcount=1 reference=0
+Leaked cluster 63 refcount=1 reference=0
+Leaked cluster 64 refcount=1 reference=0
+Leaked cluster 65 refcount=1 reference=0
+Leaked cluster 66 refcount=1 reference=0
+Leaked cluster 67 refcount=1 reference=0
+Leaked cluster 68 refcount=1 reference=0
+Leaked cluster 69 refcount=1 reference=0
+Leaked cluster 70 refcount=1 reference=0
+Leaked cluster 71 refcount=1 reference=0
+Leaked cluster 72 refcount=1 reference=0
+Leaked cluster 73 refcount=1 reference=0
+Leaked cluster 74 refcount=1 reference=0
+Leaked cluster 75 refcount=1 reference=0
+Leaked cluster 76 refcount=1 reference=0
+Leaked cluster 77 refcount=1 reference=0
+Leaked cluster 78 refcount=1 reference=0
+Leaked cluster 79 refcount=1 reference=0
+Leaked cluster 80 refcount=1 reference=0
+Leaked cluster 81 refcount=1 reference=0
+Leaked cluster 82 refcount=1 reference=0
+Leaked cluster 83 refcount=1 reference=0
+Leaked cluster 84 refcount=1 reference=0
+Leaked cluster 85 refcount=1 reference=0
+Leaked cluster 86 refcount=1 reference=0
+Leaked cluster 87 refcount=1 reference=0
+Leaked cluster 88 refcount=1 reference=0
+Leaked cluster 89 refcount=1 reference=0
+Leaked cluster 90 refcount=1 reference=0
+Leaked cluster 91 refcount=1 reference=0
+Leaked cluster 92 refcount=1 reference=0
+Leaked cluster 93 refcount=1 reference=0
+Leaked cluster 94 refcount=1 reference=0
+Leaked cluster 95 refcount=1 reference=0
+Repairing cluster 14 refcount=1 reference=0
+Repairing cluster 15 refcount=1 reference=0
+Repairing cluster 56 refcount=1 reference=0
+Repairing cluster 57 refcount=1 reference=0
+Repairing cluster 58 refcount=1 reference=0
+Repairing cluster 59 refcount=1 reference=0
+Repairing cluster 60 refcount=1 reference=0
+Repairing cluster 61 refcount=1 reference=0
+Repairing cluster 62 refcount=1 reference=0
+Repairing cluster 63 refcount=1 reference=0
+Repairing cluster 64 refcount=1 reference=0
+Repairing cluster 65 refcount=1 reference=0
+Repairing cluster 66 refcount=1 reference=0
+Repairing cluster 67 refcount=1 reference=0
+Repairing cluster 68 refcount=1 reference=0
+Repairing cluster 69 refcount=1 reference=0
+Repairing cluster 70 refcount=1 reference=0
+Repairing cluster 71 refcount=1 reference=0
+Repairing cluster 72 refcount=1 reference=0
+Repairing cluster 73 refcount=1 reference=0
+Repairing cluster 74 refcount=1 reference=0
+Repairing cluster 75 refcount=1 reference=0
+Repairing cluster 76 refcount=1 reference=0
+Repairing cluster 77 refcount=1 reference=0
+Repairing cluster 78 refcount=1 reference=0
+Repairing cluster 79 refcount=1 reference=0
+Repairing cluster 80 refcount=1 reference=0
+Repairing cluster 81 refcount=1 reference=0
+Repairing cluster 82 refcount=1 reference=0
+Repairing cluster 83 refcount=1 reference=0
+Repairing cluster 84 refcount=1 reference=0
+Repairing cluster 85 refcount=1 reference=0
+Repairing cluster 86 refcount=1 reference=0
+Repairing cluster 87 refcount=1 reference=0
+Repairing cluster 88 refcount=1 reference=0
+Repairing cluster 89 refcount=1 reference=0
+Repairing cluster 90 refcount=1 reference=0
+Repairing cluster 91 refcount=1 reference=0
+Repairing cluster 92 refcount=1 reference=0
+Repairing cluster 93 refcount=1 reference=0
+Repairing cluster 94 refcount=1 reference=0
+Repairing cluster 95 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+ 42 leaked clusters
+ 65536 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+
+65536 snapshots should remain:
+ qemu-img info reports 65536 snapshots
+ Image header reports 65536 snapshots
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d95d556414..b11b2e808c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -273,4 +273,5 @@
256 rw quick
257 rw
258 rw quick
+261 rw
262 rw quick migration
--
2.21.0
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h Max Reitz
@ 2019-08-19 19:06 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:06 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1166 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> endof() is a useful macro, we can make use of it outside of virtio.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> include/hw/virtio/virtio.h | 7 -------
> include/qemu/compiler.h | 7 +++++++
> hw/block/virtio-blk.c | 4 ++--
> hw/net/virtio-net.c | 10 +++++-----
> 4 files changed, 14 insertions(+), 14 deletions(-)
>
> +++ b/include/qemu/compiler.h
> @@ -60,6 +60,13 @@
>
> #define sizeof_field(type, field) sizeof(((type *)0)->field)
>
> +/*
> + * Calculate the number of bytes up to and including the given 'field' of
> + * 'container'.
> + */
> +#define endof(container, field) \
> + (offsetof(container, field) + sizeof_field(container, field))
Bike-shedding: I might have done s/container/type/ as part of the
motion, to match the above definition of sizeof_field (and in C, we tend
to refer to 'type's, not 'container's). But doesn't affect correctness
of the patch.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 02/16] qcow2: Use endof()
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 02/16] qcow2: Use endof() Max Reitz
@ 2019-08-19 19:09 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:09 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1230 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2-snapshot.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
What, the file got larger in spite of using a helper macro for ease of
readability? Good thing that 'lines of code' is not the only metric of
goodness ;)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index d0e7fa9311..752883e5c3 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -92,11 +92,12 @@ int qcow2_read_snapshots(BlockDriverState *bs)
> }
> offset += extra_data_size;
>
> - if (extra_data_size >= 8) {
> + if (extra_data_size >= endof(QCowSnapshotExtraData,
> + vm_state_size_large)) {
> sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
> }
>
> - if (extra_data_size >= 16) {
> + if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
Yes, that is nicer.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data Max Reitz
@ 2019-08-19 19:23 ` Eric Blake
2019-08-20 11:42 ` Max Reitz
2019-08-19 19:34 ` [Qemu-devel] " Eric Blake
1 sibling, 1 reply; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:23 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1354 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> The qcow2 specification says to ignore unknown extra data fields in
> snapshot table entries. Currently, we discard it whenever we update the
> image, which is a bit different from "ignore".
>
> This patch makes the qcow2 driver keep all unknown extra data fields
> when updating an image's snapshot table.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2.h | 5 ++++
> block/qcow2-snapshot.c | 61 +++++++++++++++++++++++++++++++++++-------
> 2 files changed, 56 insertions(+), 10 deletions(-)
>
> @@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
> sn = s->snapshots + i;
> offset = ROUND_UP(offset, 8);
> offset += sizeof(h);
> - offset += sizeof(extra);
> + offset += MAX(sizeof(extra), sn->extra_data_size);
Why would we ever have less than sizeof(extra) bytes to write on output,
since we always produce the fields on creation and synthesize the
missing fields of extra on read? Can't you rewrite this as:
assert(sn->extra_data_size >= sizeof(extra));
offset += sn->extra_data_size;
Otherwise,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade Max Reitz
@ 2019-08-19 19:25 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:25 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1906 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> qcow2 v3 requires every snapshot table entry to have two extra data
> fields: The 64-bit VM state size, and the virtual disk size. Both are
> optional for v2 images, so they may not be present.
>
> qcow2_upgrade() therefore should update the snapshot table to ensure all
> entries have these extra data fields.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> @@ -4768,7 +4770,33 @@ static int qcow2_upgrade(BlockDriverState *bs, int target_version,
> /* There are no other versions (yet) that you can upgrade to */
> assert(target_version == 3);
>
> - status_cb(bs, 0, 1, cb_opaque);
> + status_cb(bs, 0, 2, cb_opaque);
> +
> + /*
> + * In v2, snapshots do not need to have extra data. v3 requires
> + * the 64-bit VM state size and the virtual disk size to be
> + * present.
> + * qcow2_write_snapshots() will always write the list in the
> + * v3-compliant format.
> + */
> + need_snapshot_update = false;
> + for (i = 0; i < s->nb_snapshots; i++) {
> + if (s->snapshots[i].extra_data_size <
> + sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
> + sizeof_field(QCowSnapshotExtraData, disk_size))
sizeof(extra) would be more concise than two sizeof_field() added
together, but might cause problems if we later expand the size of extra
for other reasons, but don't revisit this code. So I actually like what
you did here.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data Max Reitz
2019-08-19 19:23 ` Eric Blake
@ 2019-08-19 19:34 ` Eric Blake
2019-08-20 11:43 ` Max Reitz
1 sibling, 1 reply; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:34 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 2201 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> The qcow2 specification says to ignore unknown extra data fields in
> snapshot table entries. Currently, we discard it whenever we update the
> image, which is a bit different from "ignore".
>
> This patch makes the qcow2 driver keep all unknown extra data fields
> when updating an image's snapshot table.
>
> @@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
> sn->date_sec = be32_to_cpu(h.date_sec);
> sn->date_nsec = be32_to_cpu(h.date_nsec);
> sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec);
> - extra_data_size = be32_to_cpu(h.extra_data_size);
> + sn->extra_data_size = be32_to_cpu(h.extra_data_size);
>
> id_str_size = be16_to_cpu(h.id_str_size);
> name_size = be16_to_cpu(h.name_size);
>
> - /* Read extra data */
> + if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
> + ret = -EFBIG;
> + error_setg(errp, "Too much extra metadata in snapshot table "
> + "entry %i", i);
> + goto fail;
We fail if extra_data_size is > 1024...
> + if (sn->extra_data_size > sizeof(extra)) {
> + /* Store unknown extra data */
> + size_t unknown_extra_data_size =
> + sn->extra_data_size - sizeof(extra);
> +
But read at most 1008 bytes into sn->unknown_extra_data.
> @@ -234,6 +257,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
> }
> offset += sizeof(extra);
>
> + if (sn->extra_data_size > sizeof(extra)) {
> + size_t unknown_extra_data_size =
> + sn->extra_data_size - sizeof(extra);
> +
> + /* qcow2_read_snapshots() ensures no unbounded allocation */
> + assert(unknown_extra_data_size <= BDRV_REQUEST_MAX_BYTES);
So this assertion is quite loose in what it permits; tighter would be
assert(unknown_extra_data_size <= QCOW_MAX_SNAPSHOT_EXTRA_DATA -
sizeof(extra))
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries Max Reitz
@ 2019-08-19 19:37 ` Eric Blake
2019-08-20 11:46 ` Max Reitz
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:37 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 996 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> The only case where we currently reject snapshot table entries is when
> they have too much extra data. Fix them with qemu-img check -r all by
> counting it as a corruption, reducing their extra_data_size, and then
> letting qcow2_check_fix_snapshot_table() do the rest.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2-snapshot.c | 67 +++++++++++++++++++++++++++++++++++-------
> 1 file changed, 56 insertions(+), 11 deletions(-)
>
> @@ -64,6 +80,8 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
> s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
>
> for(i = 0; i < s->nb_snapshots; i++) {
> + bool truncate_unknown_extra_data = false;
Worth adding space after 'for' while in the vicinity?
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length Max Reitz
@ 2019-08-19 19:40 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:40 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 555 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> When repairing the snapshot table, we truncate entries that have too
> much extra data. This frees up space that we do not have to count
> towards the snapshot table size.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2-snapshot.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables Max Reitz
@ 2019-08-19 19:43 ` Eric Blake
2019-08-20 12:09 ` Max Reitz
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:43 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1028 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> We currently refuse to open qcow2 images with overly long snapshot
> tables. This patch makes qemu-img check -r all drop all offending
> entries past what we deem acceptable.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2-snapshot.c | 88 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 78 insertions(+), 10 deletions(-)
I know I was reluctant in v1, but you also managed to convince me that
it really takes a LOT of effort to get a table with that many entries.
And a user has to opt-in to 'qemu-img -r' (it may discard a snapshot
they value, but that beats not being able to use the image under qemu at
all, and we don't erase it for plain 'qemu-img check'). So I'm okay
with this going in. Maybe the commit message can state this sort of
reasoning.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries Max Reitz
@ 2019-08-19 19:45 ` Eric Blake
2019-08-20 12:12 ` Max Reitz
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:45 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 603 bytes --]
On 8/19/19 1:55 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
Short on the reasoning why this isn't a problem in practice. (Again,
because we only do it via opt-in qemu-img -r; you can already learn if
qemu-img will have problem with your file created externally without
destroying the image, and elect to not have qemu-img clean it if you
don't like the algorithm qemu-img will use).
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy Max Reitz
@ 2019-08-19 19:46 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2019-08-19 19:46 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 517 bytes --]
On 8/19/19 1:56 PM, Max Reitz wrote:
> qcow2 v3 images require every snapshot table entry to have at least 16
> bytes of extra data. If they do not, let qemu-img check -r all fix it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/qcow2-snapshot.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling Max Reitz
@ 2019-08-19 20:25 ` Eric Blake
2019-08-20 11:51 ` Max Reitz
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2019-08-19 20:25 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1600 bytes --]
On 8/19/19 1:56 PM, Max Reitz wrote:
> Add a test how our qcow2 driver handles extra data in snapshot table
> entries, and how it repairs overly long snapshot tables.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> +++ b/tests/qemu-iotests/261.out
> @@ -0,0 +1,346 @@
> +QA output created by 261
> +
> +=== Create v2 template ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.v2.orig', fmt=IMGFMT size=67108864
> +No errors were found on the image.
> +Snapshots in TEST_DIR/t.IMGFMT.v2.orig:
> + [0]
> + ID: 1
> + Name: sn0
> + Extra data size: 0
> + [1]
> + ID: 2
> + Name: sn1
> + Extra data size: 42
> + VM state size: 0
> + Disk size: 67108864
> + Unknown extra data: very important data
Hmm - possibly one more patch to write - but when checking snapshots for
accuracy, do we want to insist that the 32-bit truncated VM state size
is either 0 or matches the low 32-bits of the 64-bit VM state size
field? Any mismatch between those fields (other than the 32-bit field
being left 0 because we knew to use the 64-bit field) might be a hint of
a possible corruption. But there's no good way to correct it other than
wiping the 32-bit field to 0; and for a v2 image, any change we make to
the 32-bit field might actually make the snapshot unusable for an older
client that doesn't know how to use the 64-bit field. So maybe we just
overlook that.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
2019-08-19 19:23 ` Eric Blake
@ 2019-08-20 11:42 ` Max Reitz
2019-10-11 14:57 ` Max Reitz
0 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-20 11:42 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1499 bytes --]
On 19.08.19 21:23, Eric Blake wrote:
> On 8/19/19 1:55 PM, Max Reitz wrote:
>> The qcow2 specification says to ignore unknown extra data fields in
>> snapshot table entries. Currently, we discard it whenever we update the
>> image, which is a bit different from "ignore".
>>
>> This patch makes the qcow2 driver keep all unknown extra data fields
>> when updating an image's snapshot table.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/qcow2.h | 5 ++++
>> block/qcow2-snapshot.c | 61 +++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 56 insertions(+), 10 deletions(-)
>>
>
>> @@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>> sn = s->snapshots + i;
>> offset = ROUND_UP(offset, 8);
>> offset += sizeof(h);
>> - offset += sizeof(extra);
>> + offset += MAX(sizeof(extra), sn->extra_data_size);
>
> Why would we ever have less than sizeof(extra) bytes to write on output,
> since we always produce the fields on creation and synthesize the
> missing fields of extra on read? Can't you rewrite this as:
>
> assert(sn->extra_data_size >= sizeof(extra));
> offset += sn->extra_data_size;
Hm, but I don’t prop up extra_data_size to be at least sizeof(extra). I
can do that, but it would add a few extra lines here and there.
> Otherwise,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
In any case, thanks for reviewing again :-)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
2019-08-19 19:34 ` [Qemu-devel] " Eric Blake
@ 2019-08-20 11:43 ` Max Reitz
0 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-20 11:43 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 2389 bytes --]
On 19.08.19 21:34, Eric Blake wrote:
> On 8/19/19 1:55 PM, Max Reitz wrote:
>> The qcow2 specification says to ignore unknown extra data fields in
>> snapshot table entries. Currently, we discard it whenever we update the
>> image, which is a bit different from "ignore".
>>
>> This patch makes the qcow2 driver keep all unknown extra data fields
>> when updating an image's snapshot table.
>>
>
>> @@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
>> sn->date_sec = be32_to_cpu(h.date_sec);
>> sn->date_nsec = be32_to_cpu(h.date_nsec);
>> sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec);
>> - extra_data_size = be32_to_cpu(h.extra_data_size);
>> + sn->extra_data_size = be32_to_cpu(h.extra_data_size);
>>
>> id_str_size = be16_to_cpu(h.id_str_size);
>> name_size = be16_to_cpu(h.name_size);
>>
>> - /* Read extra data */
>> + if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
>> + ret = -EFBIG;
>> + error_setg(errp, "Too much extra metadata in snapshot table "
>> + "entry %i", i);
>> + goto fail;
>
> We fail if extra_data_size is > 1024...
>
>
>> + if (sn->extra_data_size > sizeof(extra)) {
>> + /* Store unknown extra data */
>> + size_t unknown_extra_data_size =
>> + sn->extra_data_size - sizeof(extra);
>> +
>
> But read at most 1008 bytes into sn->unknown_extra_data.
>
>> @@ -234,6 +257,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>> }
>> offset += sizeof(extra);
>>
>> + if (sn->extra_data_size > sizeof(extra)) {
>> + size_t unknown_extra_data_size =
>> + sn->extra_data_size - sizeof(extra);
>> +
>> + /* qcow2_read_snapshots() ensures no unbounded allocation */
>> + assert(unknown_extra_data_size <= BDRV_REQUEST_MAX_BYTES);
>
> So this assertion is quite loose in what it permits; tighter would be
>
> assert(unknown_extra_data_size <= QCOW_MAX_SNAPSHOT_EXTRA_DATA -
> sizeof(extra))
As I said in the last version, I have this assertion here just because
of the following bdrv_pwrite(); so all we need to assert is that it fits
BDRV_REQUEST_MAX_BYTES (which it clearly does, as you say).
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries
2019-08-19 19:37 ` Eric Blake
@ 2019-08-20 11:46 ` Max Reitz
0 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-20 11:46 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1114 bytes --]
On 19.08.19 21:37, Eric Blake wrote:
> On 8/19/19 1:55 PM, Max Reitz wrote:
>> The only case where we currently reject snapshot table entries is when
>> they have too much extra data. Fix them with qemu-img check -r all by
>> counting it as a corruption, reducing their extra_data_size, and then
>> letting qcow2_check_fix_snapshot_table() do the rest.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/qcow2-snapshot.c | 67 +++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 56 insertions(+), 11 deletions(-)
>>
>
>> @@ -64,6 +80,8 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
>> s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
>>
>> for(i = 0; i < s->nb_snapshots; i++) {
>> + bool truncate_unknown_extra_data = false;
>
> Worth adding space after 'for' while in the vicinity?
Hm, it doesn’t bother me enough, at least. It’d probably be better to
just fix all occurrences of that in block/qcow2* in one patch (and there
are a few indeed). That is, if someone is sufficiently bothered by it. ;-)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling
2019-08-19 20:25 ` Eric Blake
@ 2019-08-20 11:51 ` Max Reitz
0 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-20 11:51 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1684 bytes --]
On 19.08.19 22:25, Eric Blake wrote:
> On 8/19/19 1:56 PM, Max Reitz wrote:
>> Add a test how our qcow2 driver handles extra data in snapshot table
>> entries, and how it repairs overly long snapshot tables.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>
>> +++ b/tests/qemu-iotests/261.out
>> @@ -0,0 +1,346 @@
>> +QA output created by 261
>> +
>> +=== Create v2 template ===
>> +
>> +Formatting 'TEST_DIR/t.IMGFMT.v2.orig', fmt=IMGFMT size=67108864
>> +No errors were found on the image.
>> +Snapshots in TEST_DIR/t.IMGFMT.v2.orig:
>> + [0]
>> + ID: 1
>> + Name: sn0
>> + Extra data size: 0
>> + [1]
>> + ID: 2
>> + Name: sn1
>> + Extra data size: 42
>> + VM state size: 0
>> + Disk size: 67108864
>> + Unknown extra data: very important data
>
> Hmm - possibly one more patch to write - but when checking snapshots for
> accuracy, do we want to insist that the 32-bit truncated VM state size
> is either 0 or matches the low 32-bits of the 64-bit VM state size
> field? Any mismatch between those fields (other than the 32-bit field
> being left 0 because we knew to use the 64-bit field) might be a hint of
> a possible corruption. But there's no good way to correct it other than
> wiping the 32-bit field to 0; and for a v2 image, any change we make to
> the 32-bit field might actually make the snapshot unusable for an older
> client that doesn't know how to use the 64-bit field. So maybe we just
> overlook that.
The spec clearly says that when the 64-bit field is present, the 32-bit
field is to be ignored. So there’s at least no standard-compliant way
of checking it.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables
2019-08-19 19:43 ` Eric Blake
@ 2019-08-20 12:09 ` Max Reitz
2019-08-20 13:04 ` Eric Blake
0 siblings, 1 reply; 36+ messages in thread
From: Max Reitz @ 2019-08-20 12:09 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1480 bytes --]
On 19.08.19 21:43, Eric Blake wrote:
> On 8/19/19 1:55 PM, Max Reitz wrote:
>> We currently refuse to open qcow2 images with overly long snapshot
>> tables. This patch makes qemu-img check -r all drop all offending
>> entries past what we deem acceptable.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/qcow2-snapshot.c | 88 +++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 78 insertions(+), 10 deletions(-)
>
> I know I was reluctant in v1, but you also managed to convince me that
> it really takes a LOT of effort to get a table with that many entries.
> And a user has to opt-in to 'qemu-img -r' (it may discard a snapshot
> they value, but that beats not being able to use the image under qemu at
> all, and we don't erase it for plain 'qemu-img check'). So I'm okay
> with this going in. Maybe the commit message can state this sort of
> reasoning.
So maybe:
The user cannot choose which snapshots are removed. This is fine
because we have chosen the maximum snapshot table size to be so large
(64 MB) that it cannot be reasonably reached. If the snapshot table
exceeds this size, the image has probably been corrupted in some way; in
this case, it is most important to just make the image usable such that
the user can copy off at least the active layer.
(Also note that the snapshots will be removed only with "-r all", so a
plain "check" or "check -r leaks" will not delete any data.)
?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 484 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries
2019-08-19 19:45 ` Eric Blake
@ 2019-08-20 12:12 ` Max Reitz
0 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-08-20 12:12 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 604 bytes --]
On 19.08.19 21:45, Eric Blake wrote:
> On 8/19/19 1:55 PM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>
> Short on the reasoning why this isn't a problem in practice. (Again,
> because we only do it via opt-in qemu-img -r; you can already learn if
> qemu-img will have problem with your file created externally without
> destroying the image, and elect to not have qemu-img clean it if you
> don't like the algorithm qemu-img will use).
OK. I’ll add the same message as for patch 12, just with s/64 MB/65536
snapshots/, if that seems good to you.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables
2019-08-20 12:09 ` Max Reitz
@ 2019-08-20 13:04 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2019-08-20 13:04 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1690 bytes --]
On 8/20/19 7:09 AM, Max Reitz wrote:
> On 19.08.19 21:43, Eric Blake wrote:
>> On 8/19/19 1:55 PM, Max Reitz wrote:
>>> We currently refuse to open qcow2 images with overly long snapshot
>>> tables. This patch makes qemu-img check -r all drop all offending
>>> entries past what we deem acceptable.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>> block/qcow2-snapshot.c | 88 +++++++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 78 insertions(+), 10 deletions(-)
>>
>> I know I was reluctant in v1, but you also managed to convince me that
>> it really takes a LOT of effort to get a table with that many entries.
>> And a user has to opt-in to 'qemu-img -r' (it may discard a snapshot
>> they value, but that beats not being able to use the image under qemu at
>> all, and we don't erase it for plain 'qemu-img check'). So I'm okay
>> with this going in. Maybe the commit message can state this sort of
>> reasoning.
>
> So maybe:
>
> The user cannot choose which snapshots are removed. This is fine
> because we have chosen the maximum snapshot table size to be so large
> (64 MB) that it cannot be reasonably reached. If the snapshot table
> exceeds this size, the image has probably been corrupted in some way; in
> this case, it is most important to just make the image usable such that
> the user can copy off at least the active layer.
> (Also note that the snapshots will be removed only with "-r all", so a
> plain "check" or "check -r leaks" will not delete any data.)
I like it.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data
2019-08-20 11:42 ` Max Reitz
@ 2019-10-11 14:57 ` Max Reitz
0 siblings, 0 replies; 36+ messages in thread
From: Max Reitz @ 2019-10-11 14:57 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1587 bytes --]
On 20.08.19 13:42, Max Reitz wrote:
> On 19.08.19 21:23, Eric Blake wrote:
>> On 8/19/19 1:55 PM, Max Reitz wrote:
>>> The qcow2 specification says to ignore unknown extra data fields in
>>> snapshot table entries. Currently, we discard it whenever we update the
>>> image, which is a bit different from "ignore".
>>>
>>> This patch makes the qcow2 driver keep all unknown extra data fields
>>> when updating an image's snapshot table.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>> block/qcow2.h | 5 ++++
>>> block/qcow2-snapshot.c | 61 +++++++++++++++++++++++++++++++++++-------
>>> 2 files changed, 56 insertions(+), 10 deletions(-)
>>>
>>
>>> @@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>>> sn = s->snapshots + i;
>>> offset = ROUND_UP(offset, 8);
>>> offset += sizeof(h);
>>> - offset += sizeof(extra);
>>> + offset += MAX(sizeof(extra), sn->extra_data_size);
>>
>> Why would we ever have less than sizeof(extra) bytes to write on output,
>> since we always produce the fields on creation and synthesize the
>> missing fields of extra on read? Can't you rewrite this as:
>>
>> assert(sn->extra_data_size >= sizeof(extra));
>> offset += sn->extra_data_size;
>
> Hm, but I don’t prop up extra_data_size to be at least sizeof(extra). I
> can do that, but it would add a few extra lines here and there.
On second thought, it doesn’t work at all because then there is no way
to later detect whether the image is compliant or not.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2019-10-11 14:58 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19 18:55 [Qemu-devel] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h Max Reitz
2019-08-19 19:06 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 02/16] qcow2: Use endof() Max Reitz
2019-08-19 19:09 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 03/16] qcow2: Add Error ** to qcow2_read_snapshots() Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data Max Reitz
2019-08-19 19:23 ` Eric Blake
2019-08-20 11:42 ` Max Reitz
2019-10-11 14:57 ` Max Reitz
2019-08-19 19:34 ` [Qemu-devel] " Eric Blake
2019-08-20 11:43 ` Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 05/16] qcow2: Make qcow2_write_snapshots() public Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 06/16] qcow2: Put qcow2_upgrade() into its own function Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade Max Reitz
2019-08-19 19:25 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 08/16] qcow2: Separate qcow2_check_read_snapshot_table() Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 09/16] qcow2: Add qcow2_check_fix_snapshot_table() Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries Max Reitz
2019-08-19 19:37 ` Eric Blake
2019-08-20 11:46 ` Max Reitz
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length Max Reitz
2019-08-19 19:40 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables Max Reitz
2019-08-19 19:43 ` Eric Blake
2019-08-20 12:09 ` Max Reitz
2019-08-20 13:04 ` Eric Blake
2019-08-19 18:55 ` [Qemu-devel] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries Max Reitz
2019-08-19 19:45 ` Eric Blake
2019-08-20 12:12 ` Max Reitz
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy Max Reitz
2019-08-19 19:46 ` Eric Blake
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 15/16] iotests: Add peek_file* functions Max Reitz
2019-08-19 18:56 ` [Qemu-devel] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling Max Reitz
2019-08-19 20:25 ` Eric Blake
2019-08-20 11:51 ` Max Reitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).