From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37962) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W2XIL-0007E2-9H for qemu-devel@nongnu.org; Sun, 12 Jan 2014 21:30:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W2XIC-00060p-19 for qemu-devel@nongnu.org; Sun, 12 Jan 2014 21:30:29 -0500 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:57226) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W2XIB-00060l-8c for qemu-devel@nongnu.org; Sun, 12 Jan 2014 21:30:19 -0500 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 13 Jan 2014 12:30:16 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 7A8863578053 for ; Mon, 13 Jan 2014 13:30:13 +1100 (EST) Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s0D2BC2Y1835496 for ; Mon, 13 Jan 2014 13:11:12 +1100 Received: from d23av02.au.ibm.com (localhost [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s0D2UCMZ013166 for ; Mon, 13 Jan 2014 13:30:12 +1100 Message-ID: <52D34FB0.6000308@linux.vnet.ibm.com> Date: Mon, 13 Jan 2014 10:30:08 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1388950991-30105-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1388950991-30105-7-git-send-email-xiawenc@linux.vnet.ibm.com> <52D1D8E0.8050509@redhat.com> In-Reply-To: <52D1D8E0.8050509@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V9 6/8] qcow2: rollback on fail in qcow2_snapshot_create() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, jcody@redhat.com, peter.crosthwaite@xilinx.com, stefanha@redhat.com 于 2014/1/12 7:50, Max Reitz 写道: > On 05.01.2014 20:43, Wenchao Xia wrote: >> A new variable *err_rollback is added to detect sub function's >> rollback failure. If one step in rollback procedure fails, following >> steps will be skipped, and the error message will be appended >> to errp. >> >> Signed-off-by: Wenchao Xia >> --- >> block/qcow2-snapshot.c | 37 ++++++++++++++++++++++++++++++++----- >> 1 files changed, 32 insertions(+), 5 deletions(-) >> >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 5cac714..29ba534 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -423,6 +423,7 @@ void qcow2_snapshot_create(BlockDriverState *bs, >> int i, ret; >> uint64_t *l1_table = NULL; >> int64_t l1_table_offset; >> + Error *err_rollback = NULL; >> memset(sn, 0, sizeof(*sn)); >> @@ -471,7 +472,7 @@ void qcow2_snapshot_create(BlockDriverState *bs, >> PRIu64 " with size %" PRIu64, >> sn->l1_table_offset, >> (uint64_t)(s->l1_size * sizeof(uint64_t))); >> - goto fail; >> + goto dealloc_l1_table; >> } >> ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table, >> @@ -482,7 +483,7 @@ void qcow2_snapshot_create(BlockDriverState *bs, >> PRIu64 " with size %" PRIu64, >> sn->l1_table_offset, >> (uint64_t)(s->l1_size * sizeof(uint64_t))); >> - goto fail; >> + goto dealloc_l1_table; >> } >> g_free(l1_table); >> @@ -499,7 +500,7 @@ void qcow2_snapshot_create(BlockDriverState *bs, >> "Failed in update of refcount for snapshot >> at %" >> PRIu64 " with size %d", >> s->l1_table_offset, s->l1_size); >> - goto fail; >> + goto dealloc_l1_table; >> } >> /* Append the new snapshot to the snapshot list */ >> @@ -512,12 +513,18 @@ void qcow2_snapshot_create(BlockDriverState *bs, >> s->snapshots = new_snapshot_list; >> s->snapshots[s->nb_snapshots++] = *sn; >> - ret = qcow2_write_snapshots(bs, errp, NULL); >> + ret = qcow2_write_snapshots(bs, errp, &err_rollback); >> if (ret < 0) { >> g_free(s->snapshots); >> s->snapshots = old_snapshot_list; >> s->nb_snapshots--; >> - goto fail; >> + if (error_is_set(&err_rollback)) { >> + error_append(errp, "%s", error_get_pretty(err_rollback)); >> + error_free(err_rollback); >> + goto fail; >> + } else { >> + goto restore_refcount; >> + } >> } >> g_free(old_snapshot_list); >> @@ -537,6 +544,26 @@ void qcow2_snapshot_create(BlockDriverState *bs, >> #endif >> return; >> +restore_refcount: >> + if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, >> s->l1_size, -1) >> + < 0) { >> + /* Nothing can be done now, need image check later, skip >> following >> + rollback action. */ >> + error_append(errp, >> + "In rollback failed to restore refcount in >> snapshot"); > > "Failed to restore the snapshot's refcount during rollback"? Or maybe > you could just leave the "snapshot" out, like "Failed to restore > refcounts during rollback". > >> + goto fail; >> + } >> + >> +dealloc_l1_table: >> + ret = qcow2_free_clusters(bs, sn->l1_table_offset, >> + sn->l1_size * sizeof(uint64_t), >> + QCOW2_DISCARD_ALWAYS); >> + if (ret < 0) { >> + error_append(errp, >> + "In rollback failed to free L1 table: %s\n", > > "Failed to free the L1 table in/during rollback"? > >> + strerror(-ret)); >> + } >> + >> fail: >> g_free(sn->id_str); >> g_free(sn->name); > > Aside from these: > > Reviewed-by: Max Reitz > I'll rework the commit and error message, thanks for review. :)