Re: [RFC PATCH 2/8] transactions: add tran_add_back

[Hanna Reitz]

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