[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7df771b9-bfd6-465e-b0ba-12d2aab13ec6@nvidia.com>
Date: Thu, 17 Oct 2024 14:45:43 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Christian Brauner
<christian@...uner.io>
CC: 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 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
...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.)
> +
> +#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. :)
thanks,
--
John Hubbard
Powered by blists - more mailing lists