Re: [Xen-devel] [PATCH 4/6] libxl: Protect fds with CLOEXEC even with forking threads

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 4/6] libxl: Protect fds with 
CLOEXEC even with forking threads"):
> On Fri, 2012-02-17 at 19:15 +0000, Ian Jackson wrote:
> > As yet we do not use this anywherein libxl.  Until all locations in
> 
> "anywherein"

Fixed.

> > libxl which make such fds are converted, libxl__postfork may not work
> > entirely properly.  If these locations do not use O_CLOEXEC (or use
> > calls for which there is no O_CLOEXEC) then multithreaded programs may
> > not work properly.
> > 
> > This introduces a new API call libxl_postfork_child_noexec which must
> > be called by applications which make long-running non-execing
> > children.  Add the appropriate call to xl's postfork function.
> > 
> > On Linux pthread_atfork demands compilation with -pthread, so add
> > PTHREAD_CFLAGS, _LDFLAGS, _LIBS to the corresponding variables in the
> > libxl Makefile.  It is not clear why we got away without these before.
> 
> Since I assume the Makefile bit could safely be split out and applied,
> that bit is:
> Acked-by: Ian Campbell <Ian.Campbell@xxxxxxxxxx>

I'll split it out into a separate patch.

> > +/*
> > + * An application which initialises a libxl_ctx in a parent process
> > + * and then forks a child which does not quickly exec, must
> > + * instead libxl__postfork_child_noexec in the child.  One call
> > + * for all libxl contexts is sufficient.
> > + */
> > +void libxl_postfork_child_noexec(libxl_ctx *ctx);
> 
> Is the ctx passed here required to be one of the existing ctx's or a
> newly allocated one in this context?

I will clarify this comment.

> > +static pthread_mutex_t no_forking = PTHREAD_MUTEX_INITIALIZER;
> 
> We previously had trouble with PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
> not being defined on NetBSD.
> 
> http://www.daemon-systems.org/man/pthread_mutex_init.3.html suggests
> PTHREAD_MUTEX_INITIALIZER is but perhaps we should confirm?

PTHREAD_MUTEX_INITIALIZER is in POSIX and the _NP one isn't.  But I
see Roger has confirmed it's on BSD too, which is good.

> > +static int atfork_registered;
> 
> Does pthread_atfork persist into the children?
> 
> If the parent has set this then it will be set for the child too, which
> if the answer to my question is "No" is a problem.
> 
> I suspect the answer is yes though.

Yes.  (Although I can't find any explicit statement, I think it's
obvious.)

> > +    atfork_lock();
> > +    if (atfork_registered)
> > +        return 0;
> 
> Missing an unlock?

Silly me, I should be using "goto out" as that's what it's for.

> > +void libxl__carefd_begin(void) { atfork_lock(); }
> > +void libxl__carefd_unlock(void) { atfork_unlock(); }
> 
> This naming seems asymmetric.

It's not a symmetric situation.  The complete sequence is one of these
three:

   libxl__carefd_begin
   libxl__carefd_record
   libxl__carefd_unlock
   ...
   libxl__carefd_close

or

   libxl__carefd_begin
   libxl__carefd_opened
   ...
   libxl__carefd_close

or

   libxl__carefd_begin
   libxl__carefd_unlock

I'm open to suggestions for alternative names.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel