Re: [PATCH v10 04/25] tools/libxenevtchn: add possibility to not close file descriptor on exec

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/12/2020 16:35, Juergen Gross wrote:
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Wei Liu <wl@xxxxxxx>
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
> V7:
> - new patch
>
> V8:
> - some minor comments by Julien Grall addressed
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Various of your patches still have double SoB.  (Just as a note to be
careful to anyone committing...)

> diff --git a/tools/include/xenevtchn.h b/tools/include/xenevtchn.h
> index 91821ee56d..dadc46ea36 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_open_fd() 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).
>   */

More of the comment block than this needs adjusting in light of the
exec() changes.

> -/* Currently no flags are defined */
> +
> +/* Don't set O_CLOEXEC when opening event channel driver node. */
> +#define XENEVTCHN_NO_CLOEXEC 0x01
> +
>  xenevtchn_handle *xenevtchn_open(struct xentoollog_logger *logger,
>                                   unsigned open_flags);
>  
> +/* Flag XENEVTCHN_NO_CLOEXEC is ignored by xenevtchn_open_fd(). */
> +xenevtchn_handle *xenevtchn_open_fd(struct xentoollog_logger *logger,
> +                                    int fd, unsigned open_flags);
> +

I spotted this before, but didn't have time to reply.

This isn't "open fd".  It is "construct a xenevtchn_handle object around
an already-open fd".  As such, open_flags appears bogus because at no
point are we making an open() call.  (I'd argue that it irrespective of
other things, it wants naming xenevtchn_fdopen() for API familiarity.)

However, the root of the problem is actually the ambiguity in the name. 
These are not flags to the open() system call, but general flags for
xenevtchn.

Therefore, I recommend a prep patch which renames open_flags to just
flags, and while at it, changes to unsigned int rather than a naked
"unsigned" type.  There are no API/ABI implications for this, but it
will help massively with code clarity.

I'd also possibly go as far as to say that plumbing 'flags' down into
osdep ought to split out into a separate patch.  There is also a wild
mix of coding styles even within the hunks here.

Additionally, something in core.c should check for unknown flags and
reject them them with EINVAL.  It was buggy that this wasn't done
before, and really needs to be implemented before we start having cases
where people might plausibly pass something other than 0.

~Andrew

P.S. if you don't fancy doing all of this, my brain could do with a
break from the complicated work, and I can see about organising this
cleanup.