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 <eespo...@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