On 8/6/22 17:32, Vladimir Sementsov-Ogievskiy wrote:
if I understand correctly you suggest:
.prepare = check and then change aiocontext
.abort = revert aiocontext change
.commit = nothing
Yes. And that's is how it used actually now in transactions, for example:
bdrv_attach_child_common (which is .prepare) calls
bdrv_try_set_aio_context() (which is check and then change)
bdrv_attach_child_common_abort (which is .abort) calls
bdrv_try_set_aio_context() to revert .prepare
I see your point, but I think this can be solved just with a doc comment
explaining why the transaction is "local". This is already clear in
bdrv_child_try_change_aio_context (which doesn't take a Transaction*),
it can be documented in bdrv_child_change_aio_context as well.
After all, the point of this series is just to avoid code duplication in
the two visits of the graph, and secondarily to unify the .can_set and
.set callbacks into one. We could do it just as well without
transaction with just a GList of BdrvChild* (keeping the callbcks
separate); what the Transaction API gives is a little more abstraction,
by not having to do linked list manipulation by hand.
To be honest I also don't like very much the placement of the
drained_begin/drained_end in bdrv_change_aio_context. But if the needs
arises to call bdrv_change_aio_context within another transaction, I
think they can be pulled relatively easily (as a subtree drain) in
bdrv_child_try_change_aio_context or in its callers. For now, this
series is already an improvement on several counts.
Paolo
