| Message ID | 1438238370-16212-11-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Max
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
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);