On 2018-08-17 21:18, John Snow wrote: > > > On 08/17/2018 03:04 PM, John Snow wrote: >> Change the manual deferment to commit_complete into the implicit >> callback to job_exit, renaming commit_complete to commit_exit. >> >> This conversion does change the timing of when job_completed is >> called to after the bdrv_replace_node and bdrv_unref calls, which >> could have implications for bjob->blk which will now be put down >> after this cleanup. >> >> Kevin highlights that we did not take any permissions for that backend >> at job creation time, so it is safe to reorder these operations. >> >> Signed-off-by: John Snow <js...@redhat.com> >> --- >> block/commit.c | 20 ++++---------------- >> 1 file changed, 4 insertions(+), 16 deletions(-) >> >> diff --git a/block/commit.c b/block/commit.c >> index 4a17bb73ec..93c3b6a39e 100644 >> --- a/block/commit.c >> +++ b/block/commit.c >> @@ -68,19 +68,13 @@ static int coroutine_fn commit_populate(BlockBackend >> *bs, BlockBackend *base, >> return 0; >> } >> >> -typedef struct { >> - int ret; >> -} CommitCompleteData; >> - >> -static void commit_complete(Job *job, void *opaque) >> +static void commit_exit(Job *job) >> { >> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); >> BlockJob *bjob = &s->common; >> - CommitCompleteData *data = opaque; >> BlockDriverState *top = blk_bs(s->top); >> BlockDriverState *base = blk_bs(s->base); >> BlockDriverState *commit_top_bs = s->commit_top_bs; >> - int ret = data->ret; >> bool remove_commit_top_bs = false; >> >> /* Make sure commit_top_bs and top stay around until >> bdrv_replace_node() */ >> @@ -93,8 +87,8 @@ static void commit_complete(Job *job, void *opaque) >> >> if (!job_is_cancelled(job) && ret == 0) { > > forgot to actually squash the change in here that replaces `ret` with > `job->ret`.
A better interface would be one function that is called when .run() was successful, and one that is called when it was not. (They can still resolve into a single function in the job that is just called with a boolean that's either true or false, but accessing *job to find out whether .run() was successful or not seems kind of convoluted to me.) Max
signature.asc
Description: OpenPGP digital signature