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

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

On 15.12.20 19:09, Andrew Cooper wrote:
> 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...)
Yeah, this is annoying.

They are all after the first "---" mark, so they shouldn't end up in
git AFAICS.

Why git is adding those duplicates I don't know. I had this problem
before, but some git update made it disappear. Now it is back, but
not for all patches. :-(

> > 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.)
Okay.

> 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.
Okay.

> 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.
Fine with me.

> 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.
Are you sure this is safe? I'm not arguing against it, but we considered
to do that and didn't dare to.

> ~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.
I'm fine doing it. I'm sure you'll find some other no-brainer to work
on :-)


Juergen

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature