Re: [PATCH v11 05/27] tools/libxenevtchn: add possibility to not close file descriptor on exec

[Jürgen Groß <jgross@xxxxxxxx>]

On 15.01.21 02:01, Andrew Cooper wrote:
> On 14/01/2021 15:37, Juergen Gross wrote:
> > diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h
> > index 3e9b6e7323..b6dd8f3186 100644
> > --- a/tools/include/xenevtchn.h
> > +++ b/tools/include/xenevtchn.h
> > @@ -64,11 +64,25 @@ struct xentoollog_logger;
> >    *
> >    * Calling xenevtchn_close() is the only safe operation on a
> >    * xenevtchn_handle which has been inherited.
> > + *
> > + * Setting XENEVTCHN_NO_CLOEXEC allows to keep the file descriptor used
> > + * for the event channel driver open across exec(2). In order to be able
> > + * to use that file descriptor the new binary activated via exec(2) has
> > + * to call xenevtchn_fdopen() with that file descriptor as parameter in
> > + * order to associate it with a new handle. The file descriptor can be
> > + * obtained via xenevtchn_fd() before calling exec(2).
> >    */
> Earlier commentary in this block is already wrong (refer to gnttab, and
> making what appear to be false claims), and/or made stale by this change.
>
> How about:
>
> /*
>   * Opens the evtchn device node.  Return a handle to the event channel
>   * driver, or NULL on failure, in which case errno will be set
>   * appropriately.
>   *
>   * On fork(2):
>   *
>   *   After fork, a child process must not use any opened evtchn handle
>   *   inherited from their parent.  This includes operations such as
>   *   poll() on the underlying file descriptor.  Calling xenevtchn_close()
>   *   is the only safe operation on a xenevtchn_handle which has been
>   *   inherited.
>   *
>   *   The child must open a new handle if they want to interact with
>   *   evtchn.
>   *
>   * On exec(2):
>   *
>   *   Wherever possible, the device node will be opened with O_CLOEXEC,
>   *   so it is not inherited by the subsequent program.
>   *
>   *   However the XENEVTCHN_NO_CLOEXEC flag may be used to avoid opening
>   *   the device node with O_CLOEXEC.  This is intended for use by
>   *   daemons which support a self-reexec method of updating themselves.
>   *
>   *   In this case, the updated daemon should pass the underlying file
>   *   descriptor it inherited to xenevtchn_fdopen() to reconstruct the
>   *   library handle.
>   */
>
> which I think is somewhat more concise?
Yes, I'll change it.

> > -/* Currently no flags are defined */
> > +
> > +/* Don't set O_CLOEXEC when opening event channel driver node. */
> > +#define XENEVTCHN_NO_CLOEXEC 0x01
> Do we really want an byte-looking constant?  Wouldn't (1 << 0) be a more
> normal way of writing this?
Fine with me.

> > +
> >   xenevtchn_handle *xenevtchn_open(struct xentoollog_logger *logger,
> >                                    unsigned int flags);
> >   
> > +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_fdopen(). */
> > +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger,
> > +                                    int fd, unsigned open_flags);
> True, but see below...
>
> > +
> >   /*
> >    * Close a handle previously allocated with xenevtchn_open().
> xenevtchn_{,fd}open(), now.
Ah, right.

> > diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c
> > index c069d5da71..f2ab27384b 100644
> > --- a/tools/libs/evtchn/core.c
> > +++ b/tools/libs/evtchn/core.c
> >
> > +xenevtchn_handle *xenevtchn_fdopen(struct xentoollog_logger *logger,
> > +                                   int fd, unsigned int flags)
> > +{
> > +    xenevtchn_handle *xce;
> > +
> > +    if ( flags & ~XENEVTCHN_NO_CLOEXEC )
> > +    {
> > +        errno = EINVAL;
> > +        return NULL;
> > +    }
> Do we really want to tolerate XENEVTCHN_NO_CLOEXEC here?  I'd suggest
> rejecting it, because nothing good can come of a caller thinking it has
> avoided setting O_CLOEXEC when in fact it hasn't.
Fine with me.

> > diff --git a/tools/libs/evtchn/freebsd.c b/tools/libs/evtchn/freebsd.c
> > index bb601f350f..ed2baf3c95 100644
> > --- a/tools/libs/evtchn/freebsd.c
> > +++ b/tools/libs/evtchn/freebsd.c
> > @@ -33,8 +33,12 @@
> >   
> >   int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
> >   {
> > -    int fd = open(EVTCHN_DEV, O_RDWR|O_CLOEXEC);
> > +    int open_flags = O_RDWR;
> > +    int fd;
> >   
> > +    if ( !(flags & XENEVTCHN_NO_CLOEXEC) )
> > +        open_flags |= O_CLOEXEC;
> As we're now consistently using hypervisor style, we ought to have an
> extra newline here and in equivalent positions.
Yes.

> If you're happy with the suggestions, I'm happy folding them on commit,
> to avoid yet another posting.
I'll post v12 today.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature