Re: [Qemu-devel] [PATCH 17/42] job: Move defer_to_main_loop to Job

[Eric Blake]

On 05/16/2018 05:51 AM, Max Reitz wrote:

> > > > -static void commit_complete(BlockJob *job, void *opaque)
> > > > +static void commit_complete(Job *job, void *opaque)
> > > >   {
> > > > -    CommitBlockJob *s = container_of(job, CommitBlockJob, common);
> > > > +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> > >
> > > Now this is just abuse.
> > >
> > > ...but it's not the first time someone packs two container_of() into
> > > one, it appears.  So, whatever, I guess.
> >
> > I don't think it's abuse. Why wouldn't I directly cast to the type that
> > I really want instead of casting to each of the uninteresting parent
> > classes, too?
>
> Because the final parameter is called "member" and not "path". :-)

container_of() is using offsetof(); and in C99 7.17, the parameter is 
named "member-designator" which can indeed jump through multiple layers 
(any valid address constant, as defined in 6.6P9).  I don't see this as 
abuse of the interface.


> > > The best explanation I can come up with is that the original code
> > > acquired the AioContext both of the block device at the time of the BH
> > > (because that needs to be done), and at the time of
> > > block_job_defer_to_main_loop() -- because the latter is probably the
> > > context the block_job_defer_to_main_loop() call came from, so it should
> > > be (b)locked.
> > >
> > > But if that's the case, then the same should be done here.  The context
> > > of the job may change between scheduling the BH and the BH being
> > > executed, so we might lock a different context here than the one
> > > job_defer_to_main_loop() ran in (i.e., job->aio_context at the time of
> > > job_defer_to_main_loop() running).  And maybe we should lock that old
> > > context, too -- just like block_job_defer_to_main_loop_bh() did.
> >
> > Why should we lock the old context? We don't access anything protected
> > by it. Even the data->job->bs access has gone away because we now have
> > job->aio_context.
>
> Because the old code did so and I don't know why. O:-)
>
> Your explanation does make sense to me, though, so:

Then it's best to include that explanation in the commit message itself, 
to save the future reader the hassle of digging up this thread.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org