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

[Max Reitz]

On 2018-08-25 15:33, Max Reitz wrote:

[...]

> Having a central Error object doesn't really make sense.  Whenever the
> block job wants to return an error, it should probably do so as "return"
> values of methods (like .run()).  Same for ret, of course.
> 
> I understand that this is probably really only possible after v2 when
> you've made more use of abort/commit.  But still, I don't think this
> patch improves anything, so I would leave this clean-up until later when
> you can really do something.
> 
> I suppose the idea here is that you want to drop the errp parameter from
> job_completed(), because it is not going to be called by .exit().  But
> the obvious way around this would be to pass an errp to .exit() and then
> pass the result on to job_completed().

You know what, I wrote a really long reply and in the end I realized you
basically did exactly what I wanted.  (Or, rather, I gave two
alternatives, and you did one of them.)

I noticed that having a central error object may still make sense;
.create() shows why, jobs usually fail in .run() and then you need to
keep the error around for a bit.  You can either pass it around, or you
just keep it in Job (those are the two alternatives I gave).

(So I said, either rip out Job.error/Job.err and Job.ret completely, or
keep the iff relationship between Job.err and Job.ret, so together they
give you a meaningful state.)

Now by setting Job.ret and Job.err basically at the same time (both are
results of .run(), and whenever Job.ret is set somewhere else, it is
immediately followed by job_update_rc()), the iff relationship between
Job.err and Job.ret persists.  So that's good!

(In that case I don't know why you removed the "iff" part from the
comment, though.)


The only remaining question I have is why you still pass job->ret to
job_completed().  That seems superfluous to me.

Max


(No, I don't know what's up with me and completely misunderstanding this
series.)

signature.asc
Description: OpenPGP digital signature