| Message ID | 1388950991-30105-6-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <xiawenc@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 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", I think changing the order (and "in" to "during") to "Failed to restore image header at %zu with size %zu during rollback" sounds better. > + 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"); Again, I'd prefer the order "Failed to free snapshot table in/during rollback". > + } > } > + > +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; > } Aside from these: Reviewed-by: Max Reitz <mreitz@redhat.com>
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; }
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 <xiawenc@linux.vnet.ibm.com> --- block/qcow2-snapshot.c | 63 +++++++++++++++++++++++++++++++++++------------ 1 files changed, 47 insertions(+), 16 deletions(-)