Re: [PATCH XEN v6 25/32] tools/libs/gnttab: Extensive updates to API documentation.

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Dec 09, 2015 at 01:00:34PM +0000, Ian Campbell wrote:
> On Wed, 2015-12-09 at 12:41 +0000, Wei Liu wrote:
> > > + */
> > > +
> > > +/*
> > >   * Grant Table Interface (making use of grants from other domains)
> > >   */
> > >  
> > >  typedef struct xengntdev_handle xengnttab_handle;
> > >  
> > >  /*
> > > - * Note:
> > > - * After fork a child process must not use any opened xc gnttab
> > > - * handle inherited from their parent. They must open a new handle if
> > > - * they want to interact with xc.
> > > + * Returns a handle onto the grant table driver.  Logs errors.
> > > + *
> > > + * Note: After fork a child process must not use any opened gnttab
> > > + * handle inherited from their parent, more access any grant mapped
> > > + * areas associated with that handle.
> > > + *
> > 
> > And this could use the same cloexec trick as you do in other patch for
> > privcmd.
> 
> I think this statement "do not use the handle" still stands regardless of
> the underlying fd being cloexec'd.
> 

Yes.

> However, this did make me think about a comment elsewhere, specifically the
> various instances of:
>  * This is the only function which may be safely called on a
>  * xen<foo>_handle in a child after a fork.
> for several xen<foo>_close() functions.
> 
> This is not really true if the fd is cloexec, since then _close() will
> either fail (EBADF) or, worse, close some other freshly opened file
> descriptor.
> 
> There seems to be two choices here, either require all osdep backends to
> make their fds O_CLOEXEC (which might involve tolerating a racy
> fcntl+FD_CLOEXEC pattern after open on some platforms) or don't set
> O_CLOEXEC ever and declare that the application is responsible for closing
> after fork, and for taking care of the corner cases themselves in
> multithreaded applications.
> 
> The former seems friendlier to me, even if some platforms need to use
> FD_CLOEXEC.
> 
> Hrm, maybe I can extend the atfork trick to cover the open+fcntl bits.
> Hopefully there is no issue with using pthreads from each of the affected
> libraries.
> 
> So, I think the advice in the comment would then be:
> 
>     If you fork and then exec then you must not (and need not) call _close()
>     or any other function on the handle.
> 
>     If you fork but do not exec then it is permissible to call _close(), but
>     it is not permissible to call any other function on the handle.
> 
> Need to think about that wording.
> 

Looks sensible to me FWIW.

> Ian.