Re: [Qemu-devel] [PATCH V9 5/8] qcow2: full rollback on fail in qcow2_write_snapshots()
On 05.01.2014 20:43, Wenchao Xia wrote:
Header restore step is added, and old code section fail is
renamed to dealloc_sn_table to tip better, now fail section
does not rollback anything on disk. When one step in rollback
fails, following steps will be skipped, to make sure no dangling
pointer is left.
How about:
A header restore step is added and the old label fail is
renamed to the more verbose dealloc_sn_table, whereas the new fail
section
does not rollback anything on disk. If any step during the rollback
fails, all remaining will be skipped to prevent dangling pointers.
A new parameter *errp_rollback is added to tell caller the result
the caller
of rollback procedure.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
block/qcow2-snapshot.c | 63 +++
1 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5619fc3..5cac714 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -152,7 +152,9 @@ fail:
}
/* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
+static int qcow2_write_snapshots(BlockDriverState *bs,
+ Error **errp,
+ Error **errp_rollback)
{
BDRVQcowState *s = bs-opaque;
QCowSnapshot *sn;
@@ -162,9 +164,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
struct {
uint32_t nb_snapshots;
uint64_t snapshots_offset;
-} QEMU_PACKED header_data;
+} QEMU_PACKED header_data, header_data_old;
int64_t offset, snapshots_offset;
-int ret;
+int ret, ret0;
/* compute the size of the snapshots */
offset = 0;
@@ -192,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
error_setg_errno(errp, -ret,
Failed in flush after snapshot list cluster
allocation);
-goto fail;
+goto dealloc_sn_table;
}
/* The snapshot list position has not yet been updated, so these clusters
@@ -203,7 +205,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
Failed in overlap check for snapshot list cluster
at % PRIi64 with size %d,
offset, snapshots_size);
-goto fail;
+goto dealloc_sn_table;
}
@@ -240,7 +242,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
Failed in write of snapshot header at %
PRIi64 with size %zu,
offset, sizeof(h));
-goto fail;
+goto dealloc_sn_table;
}
offset += sizeof(h);
@@ -250,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
Failed in write of extra snapshot data at %
PRIi64 with size %zu,
offset, sizeof(extra));
-goto fail;
+goto dealloc_sn_table;
}
offset += sizeof(extra);
@@ -260,7 +262,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
Failed in write of snapshot id string at %
PRIi64 with size %d,
offset, id_str_size);
-goto fail;
+goto dealloc_sn_table;
}
offset += id_str_size;
@@ -270,7 +272,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
Failed in write of snapshot name string at %
PRIi64 with size %d,
offset, name_size);
-goto fail;
+goto dealloc_sn_table;
}
offset += name_size;
}
@@ -283,7 +285,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
if (ret 0) {
error_setg_errno(errp, -ret,
Failed in flush after snapshot list update);
-goto fail;
+goto dealloc_sn_table;
+}
+
+/* Start the qcow2 header update */
+ret = bdrv_pread(bs-file, offsetof(QCowHeader, nb_snapshots),
+ header_data_old, sizeof(header_data_old));
+if (ret 0) {
+error_setg_errno(errp, -ret,
+ Failed in read of image header at %zu with size %zu,
I'd prefer Failed to read image header at %zu with size %zu.
+ offsetof(QCowHeader, nb_snapshots),
+ sizeof(header_data_old));
+goto dealloc_sn_table;
}
QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
@@ -299,7 +312,7 @@ static int
[Qemu-devel] [PATCH V9 5/8] qcow2: full rollback on fail in qcow2_write_snapshots()
Header restore step is added, and old code section fail is
renamed to dealloc_sn_table to tip better, now fail section
does not rollback anything on disk. When one step in rollback
fails, following steps will be skipped, to make sure no dangling
pointer is left.
A new parameter *errp_rollback is added to tell caller the result
of rollback procedure.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
block/qcow2-snapshot.c | 63 +++
1 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5619fc3..5cac714 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -152,7 +152,9 @@ fail:
}
/* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
+static int qcow2_write_snapshots(BlockDriverState *bs,
+ Error **errp,
+ Error **errp_rollback)
{
BDRVQcowState *s = bs-opaque;
QCowSnapshot *sn;
@@ -162,9 +164,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
struct {
uint32_t nb_snapshots;
uint64_t snapshots_offset;
-} QEMU_PACKED header_data;
+} QEMU_PACKED header_data, header_data_old;
int64_t offset, snapshots_offset;
-int ret;
+int ret, ret0;
/* compute the size of the snapshots */
offset = 0;
@@ -192,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
error_setg_errno(errp, -ret,
Failed in flush after snapshot list cluster
allocation);
-goto fail;
+goto dealloc_sn_table;
}
/* The snapshot list position has not yet been updated, so these clusters
@@ -203,7 +205,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
Failed in overlap check for snapshot list cluster
at % PRIi64 with size %d,
offset, snapshots_size);
-goto fail;
+goto dealloc_sn_table;
}
@@ -240,7 +242,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
Failed in write of snapshot header at %
PRIi64 with size %zu,
offset, sizeof(h));
-goto fail;
+goto dealloc_sn_table;
}
offset += sizeof(h);
@@ -250,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
Failed in write of extra snapshot data at %
PRIi64 with size %zu,
offset, sizeof(extra));
-goto fail;
+goto dealloc_sn_table;
}
offset += sizeof(extra);
@@ -260,7 +262,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
Failed in write of snapshot id string at %
PRIi64 with size %d,
offset, id_str_size);
-goto fail;
+goto dealloc_sn_table;
}
offset += id_str_size;
@@ -270,7 +272,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
Failed in write of snapshot name string at %
PRIi64 with size %d,
offset, name_size);
-goto fail;
+goto dealloc_sn_table;
}
offset += name_size;
}
@@ -283,7 +285,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
if (ret 0) {
error_setg_errno(errp, -ret,
Failed in flush after snapshot list update);
-goto fail;
+goto dealloc_sn_table;
+}
+
+/* Start the qcow2 header update */
+ret = bdrv_pread(bs-file, offsetof(QCowHeader, nb_snapshots),
+ header_data_old, sizeof(header_data_old));
+if (ret 0) {
+error_setg_errno(errp, -ret,
+ Failed in read of image header at %zu with size %zu,
+ offsetof(QCowHeader, nb_snapshots),
+ sizeof(header_data_old));
+goto dealloc_sn_table;
}
QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
@@ -299,7 +312,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
Failed in update of image header at %zu with size
%zu,
offsetof(QCowHeader, nb_snapshots),
sizeof(header_data));
-goto fail;
+goto restore_header;
}
/* free the old snapshot table */
@@ -309,11 +322,29 @@ static int qcow2_write_snapshots(BlockDriverState *bs,
Error **errp)
s-snapshots_size = snapshots_size;
