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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 12/20] libxl: Protect fds with 
CLOEXEC even with forking threads"):
> On Fri, 2012-03-16 at 16:26 +0000, Ian Jackson wrote:
> > 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.
> 
> One of the xl callers of postfork does quickly exec. I presume it is
> harmless to call the libxl noexec function anyway?

Yes.  I have clarified the doc comment.

> > + * 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
> 
> One too many underscores after libxl here.

Fixed.

> > +    r = pthread_atfork(atfork_lock, atfork_unlock, atfork_unlock);
> > +    if (r) {
> > +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "pthread_atfork failed");
> > +        rc = ERROR_NOMEM;
> 
> I thought we were calling the magic log+exit function on these now ;-)

Good point.  I have fixed this.  I'll also have to update the alloc
failure handler not to always want to print the allocation size.

> > +            r = close(cf->fd);
> > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_WARNING, "failed to close 
> > fd=%d"
> > +                             " (perhaps of another libxl ctx)", cf->fd);
> 
> Shouldn't there be an if (r < 0) before this logging?

Yes.  I did say I hadn't executed it :-).

> > +int libxl__carefd_close(libxl__carefd *cf)
> > +{
> > +    if (!cf) return 0;
> > +    int r = cf->fd < 0 ? 0 : close(cf->fd);
> > +    int esave = errno;
> > +    LIBXL_LIST_REMOVE(cf, entry);
> 
> Are all the LIBXL_LIST_foo thread safe?

No.

> This function isn't called with the fork lock held.

Good point.

Thanks,
Ian.

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