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