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

[Stefan Hajnoczi]

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