From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38471) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W284P-0002YK-N9 for qemu-devel@nongnu.org; Sat, 11 Jan 2014 18:34:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W284J-0002b0-NL for qemu-devel@nongnu.org; Sat, 11 Jan 2014 18:34:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43908) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W284J-0002au-EW for qemu-devel@nongnu.org; Sat, 11 Jan 2014 18:34:19 -0500 Message-ID: <52D1D558.6020008@redhat.com> Date: Sun, 12 Jan 2014 00:35:52 +0100 From: Max Reitz MIME-Version: 1.0 References: <1388950991-30105-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1388950991-30105-6-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1388950991-30105-6-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V9 5/8] qcow2: full rollback on fail in qcow2_write_snapshots() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia , qemu-devel@nongnu.org Cc: kwolf@redhat.com, jcody@redhat.com, peter.crosthwaite@xilinx.com, stefanha@redhat.com 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 > --- > 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