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

[Randy Dunlap <rdunlap@infradead.org>]

On 9/3/25 7:14 AM, Amir Goldstein wrote:
> 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?

OK, that works. I'll make a v3 like that.

Thanks.
-- 
~Randy