[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V8 0/8] qcow2: rollback the modification on fail
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH V8 0/8] qcow2: rollback the modification on fail in snapshot creation |
Date: |
Mon, 6 Jan 2014 13:11:36 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- [Qemu-devel] [PATCH V8 8/8] qemu-iotests: add test for qcow2 snapshot, (continued)
- [Qemu-devel] [PATCH V8 8/8] qemu-iotests: add test for qcow2 snapshot, Wenchao Xia, 2014/01/03
- [Qemu-devel] [PATCH V8 3/8] util: add error_append(), Wenchao Xia, 2014/01/03
- [Qemu-devel] [PATCH V8 4/8] qcow2: return int for qcow2_free_clusters(), Wenchao Xia, 2014/01/03
- [Qemu-devel] [PATCH V8 2/8] qcow2: add error message in qcow2_write_snapshots(), Wenchao Xia, 2014/01/03
- [Qemu-devel] [PATCH V8 6/8] qcow2: rollback on fail in qcow2_snapshot_create(), Wenchao Xia, 2014/01/03
- [Qemu-devel] [PATCH V8 1/8] snapshot: add parameter *errp in snapshot create, Wenchao Xia, 2014/01/03
- [Qemu-devel] [PATCH V8 5/8] qcow2: full rollback on fail in qcow2_write_snapshots(), Wenchao Xia, 2014/01/03
- [Qemu-devel] [PATCH V8 7/8] blkdebug: add debug events for snapshot, Wenchao Xia, 2014/01/03
- Re: [Qemu-devel] [PATCH V8 0/8] qcow2: rollback the modification on fail in snapshot creation,
Stefan Hajnoczi <=