Message-ID: <bc07dba0-90c7-4926-ae38-71c3c3e17e16@lucifer.local>
Date: Fri, 18 Oct 2024 07:49:55 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: John Hubbard <jhubbard@...dia.com>
Cc: Christian Brauner <christian@...uner.io>, Shuah Khan <shuah@...nel.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, pedro.falcato@...il.com,
        linux-kselftest@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org, Oliver Sang <oliver.sang@...el.com>
Subject: Re: [PATCH v4 3/4] selftests: pidfd: add pidfd.h UAPI wrapper

On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
> On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
> > Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
> > the linux/pidfd.h UAPI header.
> >
> > Work around this by adding a wrapper for linux/pidfd.h to
> > tools/include/ which sets the linux/fcntl.h header guard ahead of
> > importing the pidfd.h header file.
> >
> > Adjust the pidfd selftests Makefile to reference this include directory and
> > put it at a higher precidence than any make header installed headers to
> > ensure the wrapper is preferred.
>
> ...but we are not actually using the installed headers, now. And we intend
> to continue avoiding them. So the ordering shouldn't matter. More below:
>
> >
> > This way we can directly import the UAPI header file without issue, use the
> > latest system header file without having to duplicate anything.
> >
> > Reviewed-by: Shuah Khan <skhan@...uxfoundation.org>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > ---
> >   tools/include/linux/pidfd.h            | 14 ++++++++++++++
> >   tools/testing/selftests/pidfd/Makefile |  3 +--
> >   2 files changed, 15 insertions(+), 2 deletions(-)
> >   create mode 100644 tools/include/linux/pidfd.h
> >
> > diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
> > new file mode 100644
> > index 000000000000..113c8023072d
> > --- /dev/null
> > +++ b/tools/include/linux/pidfd.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _TOOLS_LINUX_PIDFD_H
> > +#define _TOOLS_LINUX_PIDFD_H
> > +
> > +/*
> > + * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
> > + * work around this by setting the header guard.
> > + */
> > +#define _LINUX_FCNTL_H
> > +#include "../../../include/uapi/linux/pidfd.h"
> > +#undef _LINUX_FCNTL_H
>
> Oh shoot, I think you, Shuah and I were referring to different uapi locations,
> the whole time. And so the basic approach is different after all.
>
> Your include path above actually refers to:
>
>     $(top_srcdir)/include/uapi/linux/fcntl.h
>
> ...but what I was intending was to copy a snapshot of that file (or a
> snapshot from the one generated by "make headers"), to here:
>
>     $(top_srcdir)/tools/include/uapi/linux/fcntl.h

Yeah my first version of this used the uapi one but I thought doing that
might conflict with snapshotting? Also it'd mean you'd absolutely have to
have the $(TOOLS_INCLUDES) earlier in the include priority list and better
maybe to special case in this instance.

>
> ...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk,
> for that reason: to be available to all of the kselftests:
>
>     TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
>
> The reasoning for this directory is further explained here:
>
>     tools/include/uapi/README
>
> (And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's
> progress.)
>
> And now, it's possible to change fcntl.h in place, instead of using a wrapper.
> Although either way seems OK to me. (I'm sort of ignoring the details of
> the actual header file conflict itself, for now.)

The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known
horror show. It's a difficult one to resolve as the UAPI pidfd.h header
needs O_xxx defines but we also need to include this header in kernel code.

An #ifdef __KERNEL__ block might be a solution here but fixing that is out
of scope for these changes.

>
>
> > +
> > +#endif /* _TOOLS_LINUX_PIDFD_H */
> > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > index d731e3e76d5b..f5038c9dae14 100644
> > --- a/tools/testing/selftests/pidfd/Makefile
> > +++ b/tools/testing/selftests/pidfd/Makefile
> > @@ -1,8 +1,7 @@
> >   # SPDX-License-Identifier: GPL-2.0-only
> > -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
> > +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
>
> Instead, it would look like this, which now mostly matches selftests/mm/Makefile,
> which is also helpful, because eventually this can be factored into a common
> piece for all selftests:
>
>     CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
>
> I apologize for just now noticing this! And these kselftests shouldn't require
> so much fussing around, I know. But once we get this just right, it will work
> well and last a long time. :)

Yeah I know, but this won't work due to the header conflict, I was doing
this previously.

Also doing it this way means that uapi snapshot doesn't override the usr/
one if you have that, which I guess you want?

>
> thanks,
> --
> John Hubbard

Powered by blists - more mailing lists