Re: [Qemu-devel] [PATCH v4 10/14] blockdev: make BlockJobTxn available to qmp 'transaction'

[Fam Zheng <famz@redhat.com>]

On Mon, 08/03 20:13, Max Reitz wrote:
> On 30.07.2015 08:39, Fam Zheng wrote:
> >From: Stefan Hajnoczi <stefanha@redhat.com>
> >
> >Provide a BlockJobTxn to actions executed in a qmp 'transaction'
> >command.  This allows actions to make their block jobs either complete
> >as a group or fail/cancel together.
> >
> >The next patch adds the first user.
> >
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >Reviewed-by: Fam Zheng <famz@redhat.com>
> >Reviewed-by: John Snow <jsnow@redhat.com>
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  blockdev.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index 2eb4f34..aba9d78 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -1279,6 +1279,7 @@ typedef struct BlkActionOps {
> >  struct BlkActionState {
> >      TransactionAction *action;
> >      const BlkActionOps *ops;
> >+    BlockJobTxn *block_job_txn;
> >      QSIMPLEQ_ENTRY(BlkActionState) entry;
> >  };
> >
> >@@ -1883,12 +1884,15 @@ static const BlkActionOps actions[] = {
> >  void qmp_transaction(TransactionActionList *dev_list, Error **errp)
> >  {
> >      TransactionActionList *dev_entry = dev_list;
> >+    BlockJobTxn *block_job_txn;
> >      BlkActionState *state, *next;
> >      Error *local_err = NULL;
> >
> >      QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
> >      QSIMPLEQ_INIT(&snap_bdrv_states);
> >
> >+    block_job_txn = block_job_txn_new();
> >+
> >      /* drain all i/o before any operations */
> >      bdrv_drain_all();
> >
> >@@ -1908,6 +1912,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
> >          state = g_malloc0(ops->instance_size);
> >          state->ops = ops;
> >          state->action = dev_info;
> >+        state->block_job_txn = block_job_txn;
> >          QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
> >
> >          state->ops->prepare(state, &local_err);
> >
> 
> And then state->block_job_txn is never used, thus leaked. I guess this is
> fixed for backup jobs in the next patch (by "The transaction is
> automatically freed when the last job completes or is cancelled"), but for
> any transaction which doesn't contain any block job which is added to the
> transaction, it probably remains leaked.

Good catch! I'll add a reference count to BlockJobTxn.

Fam