From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40256) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W28Iy-0006Vq-RK for qemu-devel@nongnu.org; Sat, 11 Jan 2014 18:49:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W28Is-0006ID-Rn for qemu-devel@nongnu.org; Sat, 11 Jan 2014 18:49:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39926) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W28Is-0006Hv-JY for qemu-devel@nongnu.org; Sat, 11 Jan 2014 18:49:22 -0500 Message-ID: <52D1D8E0.8050509@redhat.com> Date: Sun, 12 Jan 2014 00:50:56 +0100 From: Max Reitz 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> In-Reply-To: <1388950991-30105-7-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 6/8] qcow2: rollback on fail in qcow2_snapshot_create() 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: > 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