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; return 0; -fail: +restore_header: + ret0 = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots), + &header_data_old, sizeof(header_data_old)); + if (ret0 < 0) { + error_setg_errno(errp_rollback, -ret0, + "In rollback failed to restore image header at %zu " + "with size %zu", + offsetof(QCowHeader, nb_snapshots), + sizeof(header_data)); + goto fail; + } + +dealloc_sn_table: if (snapshots_offset > 0) { - qcow2_free_clusters(bs, snapshots_offset, snapshots_size, - QCOW2_DISCARD_ALWAYS); + ret0 = qcow2_free_clusters(bs, snapshots_offset, snapshots_size, + QCOW2_DISCARD_ALWAYS); + if (ret0 < 0) { + error_setg_errno(errp_rollback, -ret0, + "In rollback failed to free snapshot table"); + } } + +fail: if (errp) { g_assert(error_is_set(errp)); } @@ -481,7 +512,7 @@ void qcow2_snapshot_create(BlockDriverState *bs, s->snapshots = new_snapshot_list; s->snapshots[s->nb_snapshots++] = *sn; - ret = qcow2_write_snapshots(bs, errp); + ret = qcow2_write_snapshots(bs, errp, NULL); if (ret < 0) { g_free(s->snapshots); s->snapshots = old_snapshot_list; @@ -656,7 +687,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, s->snapshots + snapshot_index + 1, (s->nb_snapshots - snapshot_index - 1) * sizeof(sn)); s->nb_snapshots--; - ret = qcow2_write_snapshots(bs, errp); + ret = qcow2_write_snapshots(bs, errp, NULL); if (ret < 0) { return ret; } -- 1.7.1