From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44145) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W02Te-0003GL-QL for qemu-devel@nongnu.org; Mon, 06 Jan 2014 00:11:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W02TY-0006HW-Py for qemu-devel@nongnu.org; Mon, 06 Jan 2014 00:11:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56205) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W02TY-0006HM-Hp for qemu-devel@nongnu.org; Mon, 06 Jan 2014 00:11:44 -0500 Date: Mon, 6 Jan 2014 13:11:36 +0800 From: Stefan Hajnoczi Message-ID: <20140106051136.GA19239@stefanha-thinkpad.redhat.com> References: <1388718532-18264-1-git-send-email-xiawenc@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1388718532-18264-1-git-send-email-xiawenc@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH V8 0/8] qcow2: rollback the modification on fail in snapshot creation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com On Fri, Jan 03, 2014 at 11:08:44AM +0800, Wenchao Xia wrote: > V2: > 1: all fail case will goto fail section. > 2: add the goto code. > > v3: > Address Stefan's comments: > 2: don't goto fail after allocation failure. > 3: use sn->l1size correctly in qcow2_free_cluster(). > 4-7: add test case to verify the error paths. > Other: > 1: new patch fix a existing bug, which will be exposed in error path test. > > v4: > General change: > rebased on upstream since error path for qcow2_write_snapshots() already > exist in upstream. removed old patch 1 since it is fixed by Max in upstream. > 5: moved the snapshot_l1_update event just before write operation, instead of > before overlap check, since it is more straight. > 6: remove a duplicated error path test about flush after snapshot list > update, add a filter which replace number to X, since now in error in report > detailed message including error cluster number. > Address Stefan's comments: > 1, 2, 4: add *errp to store detailed error message, instead of error_report() > and compile time determined debug printf message. > 3: do not free cluster when fail in header update for safety reason. > Address Eric's comments: > 1, 2, 4: add *errp to store detailed error message, instead of error_report() > and compile time determined debug printf message. > 5: squashed patches that add and use debug events. > 6: added comments about test only on Linux. > > v5: > General change: > 6: rebased on upstream, use case number 070, adjust 070.out due to error > message change in this version. > > Address Max's comments: > 1 use error_setg_errno() when possible, remove "ret =" in functions when > possible since the function does not need to return int value, fix 32bit/ > 64bit issue in printf for "sizeof" and "offse", typo fix. > 2 use error_setg_errno() when possible, fix 32bit/64bit issue in printf > for "sizeof" and "offse", typo fix. > 3 typo fix in comments. > 5 typo fix in commit message. > > Address Eric's comments: > 2 fix 32bit/64bit issue in printf for "sizeof" and "offse". > > v6: > Address Jeff's comments: > 6: add quote for image name in test case. > > v7: > Rebased on Stefan's block tree, since I need to test after Fam's > cache mode series. > 6: change case number to 075 to avoid conflict, add a comments in > case that it covers only default cache mode, qemu-img snapshot would > not be affected by case's cache setting. > > v8: > Address Stefan's comments: > 1/8: typo fix. > 2/8: remove the type case for sizeof and offsetof. > 3/8, 4/8: new patches help appending error message and detect error. > 5/8: old patch that skip cluster free when header update fail, is removed. > Instead, this patch improved qcow2_write_snapshots()'s rollback procedure by > restore header. > 6/8: new variable *err_rollback is introduced to detect sub function's > rollback error. With new function introduced by patch 3/8, message pending > is simplified so old variable Error *err is removed. > Note: patch 5/8 and 6/8 does a full mirrored rollback operation, and > follows the rule that skip following steps when one step fail in rollback > procedure. > 8/8: changed the qcow2 header update fail case correspondly. > > Wenchao Xia (8): > 1 snapshot: add parameter *errp in snapshot create > 2 qcow2: add error message in qcow2_write_snapshots() > 3 util: add error_append() > 4 qcow2: return int for qcow2_free_clusters() > 5 qcow2: full rollback on fail in qcow2_write_snapshots() > 6 qcow2: rollback on fail in qcow2_snapshot_create() > 7 blkdebug: add debug events for snapshot > 8 qemu-iotests: add test for qcow2 snapshot > > block/blkdebug.c | 4 + > block/qcow2-refcount.c | 8 +- > block/qcow2-snapshot.c | 164 ++++++++++++++++++++++++----- > block/qcow2.h | 10 +- > block/rbd.c | 19 ++-- > block/sheepdog.c | 28 +++-- > block/snapshot.c | 19 +++- > blockdev.c | 10 +- > include/block/block.h | 4 + > include/block/block_int.h | 5 +- > include/block/snapshot.h | 5 +- > include/qapi/error.h | 6 + > qemu-img.c | 10 +- > savevm.c | 12 ++- > tests/qemu-iotests/075 | 216 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/075.out | 32 ++++++ > tests/qemu-iotests/common.filter | 7 ++ > tests/qemu-iotests/group | 1 + > util/error.c | 21 ++++ > 19 files changed, 505 insertions(+), 76 deletions(-) > create mode 100755 tests/qemu-iotests/075 > create mode 100644 tests/qemu-iotests/075.out I agree with Peter regarding error_append(). Overall I'm happy with this series but defer to Kevin for a final review. Error handling approach is becoming pretty intricate (there are now 2 error paths "failed and then rolled back successfully" and "failed while rolling back"). Stefan