[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V7 0/6] qcow2: rollback the modification on fail
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH V7 0/6] qcow2: rollback the modification on fail in snapshot creation |
Date: |
Fri, 20 Dec 2013 15:27:54 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Thu, Dec 05, 2013 at 08:02:46PM +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.
>
> Wenchao Xia (6):
> 1 snapshot: add parameter *errp in snapshot create
> 2 qcow2: add error message in qcow2_write_snapshots()
> 3 qcow2: do not free clusters when fail in header update in
> qcow2_write_snapshots
> 4 qcow2: cancel the modification on fail in qcow2_snapshot_create()
> 5 blkdebug: add debug events for snapshot
> 6 qemu-iotests: add test for qcow2 snapshot
>
> block/blkdebug.c | 4 +
> block/qcow2-snapshot.c | 105 ++++++++++++++++---
> block/qcow2.h | 4 +-
> 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 +-
> qemu-img.c | 10 +-
> savevm.c | 12 ++-
> tests/qemu-iotests/075 | 218
> ++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/075.out | 35 ++++++
> tests/qemu-iotests/common.filter | 7 ++
> tests/qemu-iotests/group | 1 +
> 16 files changed, 429 insertions(+), 57 deletions(-)
> create mode 100755 tests/qemu-iotests/075
> create mode 100644 tests/qemu-iotests/075.out
This looks very close. I pointed out one code path where I feel freeing
clusters is too agressive (better to leak than to corrupt the image).
There are a few minor fixups that would be nice too.
Otherwise, looks good. Good job on extending blkdebug and adding a test
case.
- Re: [Qemu-devel] [PATCH V7 2/6] qcow2: add error message in qcow2_write_snapshots(), (continued)
- [Qemu-devel] [PATCH V7 3/6] qcow2: do not free clusters when fail in header update in qcow2_write_snapshots, Wenchao Xia, 2013/12/05
- [Qemu-devel] [PATCH V7 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create(), Wenchao Xia, 2013/12/05
- [Qemu-devel] [PATCH V7 5/6] blkdebug: add debug events for snapshot, Wenchao Xia, 2013/12/05
- [Qemu-devel] [PATCH V7 6/6] qemu-iotests: add test for qcow2 snapshot, Wenchao Xia, 2013/12/05
- Re: [Qemu-devel] [PATCH V7 0/6] qcow2: rollback the modification on fail in snapshot creation, Wenchao Xia, 2013/12/08
- Re: [Qemu-devel] [PATCH V7 0/6] qcow2: rollback the modification on fail in snapshot creation, Wenchao Xia, 2013/12/12
- Re: [Qemu-devel] [PATCH V7 0/6] qcow2: rollback the modification on fail in snapshot creation, Wenchao Xia, 2013/12/17
- Re: [Qemu-devel] [PATCH V7 0/6] qcow2: rollback the modification on fail in snapshot creation,
Stefan Hajnoczi <=