[RFC,2/8] transactions: add tran_add_back

First change the transactions from a QLIST to QSIMPLEQ, then
use it to implement tran_add_tail, which allows adding elements
to the end of list transactions.

This is useful if we have some "preparation" transiction callbacks
that we want to run before the others but still only when invoking
finalize/commit/abort.

For example (A and B are lists transaction callbacks):

for (i=0; i < 3; i++) {
	tran_add(A[i]);
	tran_add_tail(B[i]);
}

tran_commit();

Will process transactions in this order: A2 - A1 - A0 - B0 - B1 - B2

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/qemu/transactions.h |  9 +++++++++
 util/transactions.c         | 29 +++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 8 deletions(-)

On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote:
> First change the transactions from a QLIST to QSIMPLEQ, then
> use it to implement tran_add_tail, which allows adding elements
> to the end of list transactions.

The subject still calls it `tran_add_back()` (perhaps from a preliminary 
version?), I think that needs adjustment.

> This is useful if we have some "preparation" transiction callbacks

*transaction

> that we want to run before the others but still only when invoking
> finalize/commit/abort.

I don’t understand this yet (but perhaps it’ll become clearer with the 
following patches); doesn’t the new function do the opposite? I.e., 
basically add some clean-up that’s only used after everything else?

> For example (A and B are lists transaction callbacks):
>
> for (i=0; i < 3; i++) {
> 	tran_add(A[i]);
> 	tran_add_tail(B[i]);
> }
>
> tran_commit();
>
> Will process transactions in this order: A2 - A1 - A0 - B0 - B1 - B2
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   include/qemu/transactions.h |  9 +++++++++
>   util/transactions.c         | 29 +++++++++++++++++++++--------
>   2 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/include/qemu/transactions.h b/include/qemu/transactions.h
> index 2f2060acd9..42783720b9 100644
> --- a/include/qemu/transactions.h
> +++ b/include/qemu/transactions.h
> @@ -50,7 +50,16 @@ typedef struct TransactionActionDrv {
>   typedef struct Transaction Transaction;
>   
>   Transaction *tran_new(void);
> +/*
> + * Add transaction at the beginning of the transaction list.
> + * @tran will be the first transaction to be processed in finalize/commit/abort.

Of course, if you call tran_add() afterwards, this transaction will no 
longer be the first one.  I mean, that’s kind of obvious, but perhaps we 
can still express that here.

Like, perhaps, “finalize/commit/abort process this list in order, so 
after this call, @tran will be the first transaction to be processed”?

> + */
>   void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque);
> +/*
> + * Add transaction at the end of the transaction list.
> + * @tran will be the last transaction to be processed in finalize/commit/abort.

(And then “finalize/commit/abort process this list in order, so after 
this call, @tran will be the last transaction to be processed”)

> + */
> +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque);
>   void tran_abort(Transaction *tran);
>   void tran_commit(Transaction *tran);
>   
> diff --git a/util/transactions.c b/util/transactions.c
> index 2dbdedce95..89e541c4a4 100644
> --- a/util/transactions.c
> +++ b/util/transactions.c

[...]

> @@ -54,20 +54,33 @@ void tran_add(Transaction *tran, TransactionActionDrv *drv, void *opaque)
>           .opaque = opaque
>       };
>   
> -    QSLIST_INSERT_HEAD(&tran->actions, act, entry);
> +    QSIMPLEQ_INSERT_HEAD(&tran->actions, act, entry);
> +}
> +
> +void tran_add_tail(Transaction *tran, TransactionActionDrv *drv, void *opaque)
> +{
> +    TransactionAction *act;
> +
> +    act = g_new(TransactionAction, 1);
> +    *act = (TransactionAction) {
> +        .drv = drv,
> +        .opaque = opaque
> +    };
> +
> +    QSIMPLEQ_INSERT_TAIL(&tran->actions, act, entry);
>   }

Perhaps this could benefit from a function encompassing the common 
functionality, i.e. a tran_do_add(..., bool tail) with

if (tail) {
     QSIMPLEQ_INSERT_TAIL(...);
} else {
     QSIMPLEQ_INSERT_HEAD(...);
}

(Just a light suggestion.)

Hanna

On 7/14/22 17:13, Hanna Reitz wrote:
>> that we want to run before the others but still only when invoking
>> finalize/commit/abort.
> 
> I don’t understand this yet (but perhaps it’ll become clearer with the 
> following patches); doesn’t the new function do the opposite? I.e., 
> basically add some clean-up that’s only used after everything else?

I agree about the commit message being incorrect, but is there any 
reason why transactions work LIFO by default?  Is there anything that 
requires that behavior?

Paolo

On 7/18/22 19:20, Paolo Bonzini wrote:
> On 7/14/22 17:13, Hanna Reitz wrote:
>>> that we want to run before the others but still only when invoking
>>> finalize/commit/abort.
>>
>> I don’t understand this yet (but perhaps it’ll become clearer with the following patches); doesn’t the new function do the opposite? I.e., basically add some clean-up that’s only used after everything else?
> 
> I agree about the commit message being incorrect, but is there any reason why transactions work LIFO by default?  Is there anything that requires that behavior?
> 

Yes. On abort() we want to rollback actions in reverse order. When we create _abort() part of some action we assume that current graph state is exactly the same like it was after _prepare() call, otherwise it just will not work. That means that all actions called _after_ action X, are already rolled-back when we call X_abort().

And keeping that in mind I'm afraid of implementing a possibility to break this order..

Note also, that in block transaction API, most of the action is usually done in _prepare(), so that next action in transaction can rely on result of previous action.