Re: [PATCH v2] uapi/fcntl: define RENAME_* and AT_RENAME_* macros

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Sep 3, 2025 at 2:46 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
>
>
> On 9/2/25 2:31 PM, Randy Dunlap wrote:
> > Hi,
> >
> > On 9/1/25 11:58 PM, Amir Goldstein wrote:
> >> On Tue, Sep 2, 2025 at 1:14 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> >>>
> >>> Define the RENAME_* and AT_RENAME_* macros exactly the same as in
> >>> recent glibc <stdio.h> so that duplicate definition build errors in
> >>> both samples/watch_queue/watch_test.c and samples/vfs/test-statx.c
> >>> no longer happen. When they defined in exactly the same way in
> >>> multiple places, the build errors are prevented.
> >>>
> >>> Defining only the AT_RENAME_* macros is not sufficient since they
> >>> depend on the RENAME_* macros, which may not be defined when the
> >>> AT_RENAME_* macros are used.
> >>>
> >>> Build errors being fixed:
> >>>
> >>> for samples/vfs/test-statx.c:
> >>>
> >>> In file included from ../samples/vfs/test-statx.c:23:
> >>> usr/include/linux/fcntl.h:159:9: warning: ‘AT_RENAME_NOREPLACE’ redefined
> >>>   159 | #define AT_RENAME_NOREPLACE     0x0001
> >>> In file included from ../samples/vfs/test-statx.c:13:
> >>> /usr/include/stdio.h:171:10: note: this is the location of the previous definition
> >>>   171 | # define AT_RENAME_NOREPLACE RENAME_NOREPLACE
> >>> usr/include/linux/fcntl.h:160:9: warning: ‘AT_RENAME_EXCHANGE’ redefined
> >>>   160 | #define AT_RENAME_EXCHANGE      0x0002
> >>> /usr/include/stdio.h:173:10: note: this is the location of the previous definition
> >>>   173 | # define AT_RENAME_EXCHANGE RENAME_EXCHANGE
> >>> usr/include/linux/fcntl.h:161:9: warning: ‘AT_RENAME_WHITEOUT’ redefined
> >>>   161 | #define AT_RENAME_WHITEOUT      0x0004
> >>> /usr/include/stdio.h:175:10: note: this is the location of the previous definition
> >>>   175 | # define AT_RENAME_WHITEOUT RENAME_WHITEOUT
> >>>
> >>> for samples/watch_queue/watch_test.c:
> >>>
> >>> In file included from usr/include/linux/watch_queue.h:6,
> >>>                  from ../samples/watch_queue/watch_test.c:19:
> >>> usr/include/linux/fcntl.h:159:9: warning: ‘AT_RENAME_NOREPLACE’ redefined
> >>>   159 | #define AT_RENAME_NOREPLACE     0x0001
> >>> In file included from ../samples/watch_queue/watch_test.c:11:
> >>> /usr/include/stdio.h:171:10: note: this is the location of the previous definition
> >>>   171 | # define AT_RENAME_NOREPLACE RENAME_NOREPLACE
> >>> usr/include/linux/fcntl.h:160:9: warning: ‘AT_RENAME_EXCHANGE’ redefined
> >>>   160 | #define AT_RENAME_EXCHANGE      0x0002
> >>> /usr/include/stdio.h:173:10: note: this is the location of the previous definition
> >>>   173 | # define AT_RENAME_EXCHANGE RENAME_EXCHANGE
> >>> usr/include/linux/fcntl.h:161:9: warning: ‘AT_RENAME_WHITEOUT’ redefined
> >>>   161 | #define AT_RENAME_WHITEOUT      0x0004
> >>> /usr/include/stdio.h:175:10: note: this is the location of the previous definition
> >>>   175 | # define AT_RENAME_WHITEOUT RENAME_WHITEOUT
> >>>
> >>> Fixes: b4fef22c2fb9 ("uapi: explain how per-syscall AT_* flags should be allocated")
> >>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> >>> ---
> >>> Cc: Amir Goldstein <amir73il@gmail.com>
> >>> Cc: Jeff Layton <jlayton@kernel.org>
> >>> Cc: Chuck Lever <chuck.lever@oracle.com>
> >>> Cc: Alexander Aring <alex.aring@gmail.com>
> >>> Cc: Josef Bacik <josef@toxicpanda.com>
> >>> Cc: Aleksa Sarai <cyphar@cyphar.com>
> >>> Cc: Jan Kara <jack@suse.cz>
> >>> Cc: Christian Brauner <brauner@kernel.org>
> >>> Cc: Matthew Wilcox <willy@infradead.org>
> >>> Cc: David Howells <dhowells@redhat.com>
> >>> CC: linux-api@vger.kernel.org
> >>> To: linux-fsdevel@vger.kernel.org
> >>>
> >>>  include/uapi/linux/fcntl.h |    9 ++++++---
> >>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> --- linux-next-20250819.orig/include/uapi/linux/fcntl.h
> >>> +++ linux-next-20250819/include/uapi/linux/fcntl.h
> >>> @@ -156,9 +156,12 @@
> >>>   */
> >>>
> >>>  /* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> >>> -#define AT_RENAME_NOREPLACE    0x0001
> >>> -#define AT_RENAME_EXCHANGE     0x0002
> >>> -#define AT_RENAME_WHITEOUT     0x0004
> >>> +# define RENAME_NOREPLACE (1 << 0)
> >>> +# define AT_RENAME_NOREPLACE RENAME_NOREPLACE
> >>> +# define RENAME_EXCHANGE (1 << 1)
> >>> +# define AT_RENAME_EXCHANGE RENAME_EXCHANGE
> >>> +# define RENAME_WHITEOUT (1 << 2)
> >>> +# define AT_RENAME_WHITEOUT RENAME_WHITEOUT
> >>>
> >>
> >> This solution, apart from being terribly wrong (adjust the source to match
> >> to value of its downstream copy), does not address the issue that Mathew
> >> pointed out on v1 discussion [1]:
> >
> > I didn't forget or ignore this.
> > If the macros have the same values (well, not just values but also the
> > same text), then I don't see why it matters whether they are in some older
> > version of glibc.
> >
> >> $ grep -r AT_RENAME_NOREPLACE /usr/include
> >> /usr/include/linux/fcntl.h:#define AT_RENAME_NOREPLACE  0x0001
> >>
> >> It's not in stdio.h at all.  This is with libc6 2.41-10
> >>
> >> [1] https://lore.kernel.org/linux-fsdevel/aKxfGix_o4glz8-Z@casper.infradead.org/
> >>
> >> I don't know how to resolve the mess that glibc has created.
> >
> > Yeah, I guess I don't either.
> >
> >> Perhaps like this:
> >>
> >> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> >> index f291ab4f94ebc..dde14fa3c2007 100644
> >> --- a/include/uapi/linux/fcntl.h
> >> +++ b/include/uapi/linux/fcntl.h
> >> @@ -155,10 +155,16 @@
> >>   * as possible, so we can use them for generic bits in the future if necessary.
> >>   */
> >>
> >> -/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> >> -#define AT_RENAME_NOREPLACE    0x0001
> >> -#define AT_RENAME_EXCHANGE     0x0002
> >> -#define AT_RENAME_WHITEOUT     0x0004
> >> +/*
> >> + * The legacy renameat2(2) RENAME_* flags are conceptually also
> >> syscall-specific
> >> + * flags, so it could makes sense to create the AT_RENAME_* aliases
> >> for them and
> >> + * maybe later add support for generic AT_* flags to this syscall.
> >> + * However, following a mismatch of definitions in glibc and since no
> >> kernel code
> >> + * currently uses the AT_RENAME_* aliases, we leave them undefined here.
> >> +#define AT_RENAME_NOREPLACE    RENAME_NOREPLACE
> >> +#define AT_RENAME_EXCHANGE     RENAME_EXCHANGE
> >> +#define AT_RENAME_WHITEOUT     RENAME_WHITEOUT
> >> +*/
> >
> > Well, we do have samples/ code that uses fcntl.h (indirectly; maybe
> > that can be fixed).
> > See the build errors in the patch description.
> >
> >
> >>  /* Flag for faccessat(2). */
> >>  #define AT_EACCESS             0x200   /* Test access permitted for
> >
> > With this patch (your suggestion above):
> >
> > IF a userspace program in samples/ uses <uapi/linux/fcntl.h> without
> > using <stdio.h>, [yes, I created one to test this] and without using
> > <uapi/linux/fs.h> then the build fails with similar build errors:
> >
> > ../samples/watch_queue/watch_nostdio.c: In function ‘consumer’:
> > ../samples/watch_queue/watch_nostdio.c:33:32: error: ‘RENAME_NOREPLACE’ undeclared (first use in this function)
> >    33 |                         return RENAME_NOREPLACE;
> > ../samples/watch_queue/watch_nostdio.c:33:32: note: each undeclared identifier is reported only once for each function it appears in
> > ../samples/watch_queue/watch_nostdio.c:37:32: error: ‘RENAME_EXCHANGE’ undeclared (first use in this function)
> >    37 |                         return RENAME_EXCHANGE;
> > ../samples/watch_queue/watch_nostdio.c:41:32: error: ‘RENAME_WHITEOUT’ undeclared (first use in this function)
> >    41 |                         return RENAME_WHITEOUT;
> >
> > This build succeeds with my version 1 patch (full defining of both
> > RENAME_* and AT_RENAME_* macros). It fails with the patch that you suggested
> > above.
> >
> > OK, here's what I propose.
> >
> > a. remove the unused and (sort of) recently added AT_RENAME_* macros
> > in include/uapi/linux/fcntl.h. Nothing in the kernel tree uses them.
> > This is:
> >
> > commit b4fef22c2fb9
> > Author: Aleksa Sarai <cyphar@cyphar.com>
> > Date:   Wed Aug 28 20:19:42 2024 +1000
> >     uapi: explain how per-syscall AT_* flags should be allocated
> >
> > These macros should have never been added here IMO.
> > Just putting them somewhere as examples (in comments) would be OK.
> >

I agree.
I did not get this patch from Aleksa,
but I proposed something similar above.

> > This alone fixes all of the build errors in samples/ that I originally
> > reported.
> >
> > b. if a userspace program wants to use the RENAME_* macros, it should
> > #include <linux/fs.h> instead of <linux/fcntl.h>.
> >
> > This fixes the "contrived" build error that I manufactured.
> >
> > Note that some programs in tools/ do use AT_RENAME_* (all 3 macros)
> > but they define those macros locally.
> >
>
> And after more testing, this is what I think works:
>
> a. remove all of the AT_RENAME-* macros from <uapi/linux/fcntl.h>
>    (as above)

ok.

>
> b. put the AT_RENAME_* macros into <uapi/linux/fs.h> like so:
>
> +/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> +# define AT_RENAME_NOREPLACE RENAME_NOREPLACE
> +# define AT_RENAME_EXCHANGE RENAME_EXCHANGE
> +# define AT_RENAME_WHITEOUT RENAME_WHITEOUT
>
> so that they match what is in upstream glibc stdio.h, hence not
> causing duplicate definition errors.

Disagree.
We do not need to define them at all.

The *only* reason we defined them in fcntl.h is so the
definition will be together with the rest of the AT_ flags.
Now we change that to a comment, but there is no reason to
define them at fs.h. Why would we need to do that?

Thanks,
Amir.