Re: [Xen-devel] [PATCH 09/15] libxl: datacopier: provide "prefix data" facilit

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-05-31 at 11:32 +0100, Ian Jackson wrote:
> > On Wed, 2012-05-30 at 17:16 +0100, Ian Jackson wrote:
> > > @@ -1812,6 +1812,12 @@ _hidden void 
> > > libxl__datacopier_init(libxl__datacopier_state *dc);
> > >  _hidden void libxl__datacopier_kill(libxl__datacopier_state *dc);
> > >  _hidden int libxl__datacopier_start(libxl__datacopier_state *dc);
> > >  
> > > +/* Inserts literal data into the output stream.
> > > + * May safely be used only immediately after libxl__datacopier_start.
> > 
> > After datacopier_start the fds are registered, is there not a race
> > between those events firing (perhaps in another thread which has called
> > into libxl) and this thread?
> 
> No, because we have the ctx mutex held all of the time.

Ah!.

> 
> And there isn't a reentrancy hazard either.  As the comments in
> libxl_internal.h have it:
> 
>    *   int libxl__ev_KIND_register(libxl__gc *gc, libxl__ev_KIND *GEN,
>    *                              libxl__ev_KIND_callback *FUNC,
>    *                              DETAILS);
>    *      [...].  FUNC will
>    *      not be called from within the call to _register.  [...]
> 
> I think given your question this warrants a comment:
> 
>   @@ -78,6 +78,13 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, 
> libxl__datacopier_state *dc,
>                                      const void *data, size_t len)
>    {
>        libxl__datacopier_buf *buf;
>   +    /*
>   +     * It is safe for this to be called immediately after _start, as
>   +     * is documented in the public comment.  _start's caller must have
>   +     * the mutex locked, so other threads don't get to mess with the
>   +     * contents, and the fd events cannot happen reentrantly.

Perhaps this could be more explicit about having to hold the mutex from
before _start until after any _prefixdata calls?

Perhaps "...called immediately after _start, while still holding the
mutex, as is documented in the public ..." ? 

"The mutex" here is the CTX lock, right?

>   So we
>   +     * are guaranteed to beat the first data from the read fd.
>   +     */
> 
> > Is the safe place not between datacopier_init and the rest of
> > datacopier_start?
> 
> No, because _start calls _init (just like all the
> libxl__*_make_events_happen functions do).

I spotted that, which is why I said between the _init and the "rest of"
_start, but my original premise was wrong anyway so never mind...

> 
> Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel