Re: [Qemu-devel] [PATCH 1/7] jobs: change start callback to run callback

[Max Reitz]

On 2018-08-17 21:04, John Snow wrote:
> Presently we codify the entry point for a job as the "start" callback,
> but a more apt name would be "run" to clarify the idea that when this
> function returns we consider the job to have "finished," except for
> any cleanup which occurs in separate callbacks later.
> 
> As part of this clarification, change the signature to include an error
> object and a return code. The error ptr is not yet used, and the return
> code while captured, will be overwritten by actions in the job_completed
> function.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/backup.c            |  7 ++++---
>  block/commit.c            |  7 ++++---
>  block/create.c            |  8 +++++---
>  block/mirror.c            | 10 ++++++----
>  block/stream.c            |  7 ++++---
>  include/qemu/job.h        |  2 +-
>  job.c                     |  6 +++---
>  tests/test-bdrv-drain.c   |  7 ++++---
>  tests/test-blockjob-txn.c | 16 ++++++++--------
>  tests/test-blockjob.c     |  7 ++++---
>  10 files changed, 43 insertions(+), 34 deletions(-)

[...]

> diff --git a/job.c b/job.c
> index fa671b431a..898260b2b3 100644
> --- a/job.c
> +++ b/job.c
> @@ -544,16 +544,16 @@ static void coroutine_fn job_co_entry(void *opaque)
>  {
>      Job *job = opaque;
>  
> -    assert(job && job->driver && job->driver->start);
> +    assert(job && job->driver && job->driver->run);
>      job_pause_point(job);
> -    job->driver->start(job);
> +    job->ret = job->driver->run(job, NULL);
>  }

Hmmm, this breaks the iff relationship with job->error.  We might call
job_update_rc() afterwards, but then job_completed() would need to free
it if it overwrites it with the error description from a potential error
object.

Also, I suspect the following patches might fix the relationship anyway?
 (But then an "XXX: This does not hold right now, but will be fixed in a
future patch" in the documentation of Job.error might help.)

Max

signature.asc
Description: OpenPGP digital signature