Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim

[Kevin Wolf <kwolf@redhat.com>]

Am 08.08.2018 um 06:02 hat Jeff Cody geschrieben:
> On Tue, Aug 07, 2018 at 12:33:30AM -0400, John Snow wrote:
> > Most jobs do the same thing when they leave their running loop:
> > - Store the return code in a structure
> > - wait to receive this structure in the main thread
> > - signal job completion via job_completed
> > 
> > More seriously, when we utilize job_defer_to_main_loop_bh to call
> > a function that calls job_completed, job_finalize_single will run
> > in a context where it has recursively taken the aio_context lock,
> > which can cause hangs if it puts down a reference that causes a flush.
> > 
> > The job infrastructure is perfectly capable of registering job
> > completion itself when we leave the job's entry point. In this
> > context, we can signal job completion from outside of the aio_context,
> > which should allow for job cleanup code to run with only one lock.
> > 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> 
> I like the simplification, both in SLOC and in exit logic (as seen in
> patches 3-7).

I agree, unifying this seems like a good idea.

Like in the first patch, I'm not convinced of the details, though.
Essentially, this is my objection regarding job->err extended to
job->ret: You rely on jobs setting job->ret and job->err, but the
interfaces don't really show this.

> > @@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
> >      assert(job && job->driver && job->driver->start);
> >      job_pause_point(job);
> >      job->driver->start(job);
> 
> One nit-picky observation here, that is unrelated to this patch: reading
> through, it may not be so obvious that 'start' is really a 'run' or
> 'execute', (linguistically, to me 'start' implies a kick-off rather than
> ongoing execution).

I had exactly the same thought. My proposal is to change the existing...

    CoroutineEntry *start;

...which is just short for...

    void coroutine_fn start(void *opaque);

...into this one:

    int coroutine_fn run(void *opaque, Error **errp);

I see that at the end of the series, you actually introduced an int
return value already. I would have done that from the start, but as long
the final state makes sense, I won't insist.

But can we have the Error **errp addition, too? Pretty please?

Kevin