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@xxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> > > --- > > 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
