Re: [Qemu-devel] [PATCH V5 0/5] block: make qmp_transaction extendable

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-5-8 19:21, Kevin Wolf 写道:
> Am 08.05.2013 um 12:25 hat Wenchao Xia geschrieben:
>>    This serial will package backing chain snapshot code as one case, to make it
>> possible adding more operations later.
>>
>> v2:
>>    Address Kevin's comments:
>>    Use the same prototype prepare, commit, rollback model in original code,
>> commit should never fail.
>>
>> v3:
>>    Address Stefan's comments:
>>    3/5, 4/5: remove *action parameter since later only BlkTransactionStates* is
>> needed.
>>    5/5: embbed BlkTransactionStates in ExternalSnapshotStates, *opaque is
>> removed, related call back function format change for external snapshot.
>>    Address Kevin's comments:
>>    removed all indention in commit message.
>>    1/5: return void for prepare() function, *errp plays the role as error
>> checker.
>>    5/5: mark *commit callback must exist, *rollback callback can be NULL. Align
>> "callback =" in "const BdrvActionOps external_snapshot_ops" to the same colum.
>>    Address Eric's comments:
>>    1/5: better commit message.
>>    5/5: better commit message and comments in code that only one of rollback()
>> or commit() will be called.
>>
>> v4:
>>    5/5: document clean() callback will always be called if it present, declare
>> static for global variable "actions", use array plus .instance_size to remove
>> "switch" checking code according to caller input.
>>
>> v5:
>>    better commit message and comments, switch callback function name "rollback"
>> to "abort".
>>
>> Wenchao Xia (5):
>>    1 block: package preparation code in qmp_transaction()
>>    2 block: move input parsing code in qmp_transaction()
>>    3 block: package committing code in qmp_transaction()
>>    4 block: package rollback code in qmp_transaction()
>>    5 block: make all steps in qmp_transaction() as callback
>>
>>   blockdev.c |  266 ++++++++++++++++++++++++++++++++++++++----------------------
>>   1 files changed, 169 insertions(+), 97 deletions(-)
>
> Thanks, applied all to block-next for 1.6.
>
> Kevin
>
Hi, Kevin
   I am looking at adding internal snapshot into qmp_transaction().
Although it have been discussed before, but there are possible two
ways to do it, want to know you opinion.

Methods:
1) in block.c, break bdrv_snapshot_create() into
bdrv_snapshot_create_prepare() and bdrv_snapshot_create_commit(), add
bdrv_snapshot_create_abort(). In block/qcow2-snapshot.c, do related
changing, adding qcow2_snapshot_create_abort(). Currently the abort()
action can't fail, so just deallocate the cluster, but can't correct
the refs, maybe put a dirty flag to let qemu-img correct it?

2) like external backing chain, on fail qemu don't delete the new
created one, instead it just guarentee qemu works normal, that
means nothing need to be done now. In creation, if it found a snapshot
existing have same name, just fail. User should delete the existing
one himself, and clean up the created one in fail.

1) deployed the commit/abort() conception in the block level,
2) deployed the conception in qemu level similar to external
snapshot chain, I am not sure which is better.

-- 
Best Regards

Wenchao Xia