On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:

On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:

quoted

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:

quoted

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@linuxfoundation.org>
Signed-off-by: Lorenzo Stoakes <redacted>
---
  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.

quoted

+
+#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