Re: [Qemu-devel] [PATCH V7 0/6] qcow2: rollback the modification on fail in snapshot creation

[Stefan Hajnoczi]

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.