Re: [PATCH 3/3] tools/nolibc: simplify mode handling in open() and openat()

[Thomas Weißschuh]

On 2026-04-19 22:16:01+0200, Thomas Weißschuh wrote:
> On 2026-04-19 18:08:07+0200, Willy Tarreau wrote:
> > On Sun, Apr 19, 2026 at 05:29:05PM +0200, Thomas Weißschuh wrote:
> > > The current handling of the optional mode arguments using va_list has
> > > some drawbacks. It is hard for the compiler to optimize away and it
> > > needs specific code to handle the O_ flags that need to pass the mode
> > > parameter. Currently that mode parameter is not respected for O_TMPFILE,
> > > which is a bug.
> > > 
> > > Switch to a macro-based variant which does not generate any additional
> > > code and avoid the explicit handling of 'mode'.
> > > The macros require somewhat recent compiler versions, but users stuck on
> > > old compilers have a trivial workaround by always specifying mode.
> > > 
> > > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > > 
> > > ---
> > > It might also possible to use macros similar to syscall() to not require
> > > __VA_OPT__, but those would be fairly ugly.
> > > ---
> > >  tools/include/nolibc/compiler.h |  2 ++
> > >  tools/include/nolibc/fcntl.h    | 34 +++++++++++++---------------------
> > >  2 files changed, 15 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/tools/include/nolibc/compiler.h b/tools/include/nolibc/compiler.h
> > > index b56570bf9f69..a154aa4e143a 100644
> > > --- a/tools/include/nolibc/compiler.h
> > > +++ b/tools/include/nolibc/compiler.h
> > > @@ -90,4 +90,6 @@
> > >  #  define __nolibc_no_sanitize_undefined
> > >  #endif
> > >  
> > > +#define __nolibc_first_arg(_a1, ...) _a1
> > > +
> > >  #endif /* _NOLIBC_COMPILER_H */
> > > diff --git a/tools/include/nolibc/fcntl.h b/tools/include/nolibc/fcntl.h
> > > index 56650a36f856..89436a72e987 100644
> > > --- a/tools/include/nolibc/fcntl.h
> > > +++ b/tools/include/nolibc/fcntl.h
> > > @@ -25,18 +25,8 @@ int _sys_openat(int dirfd, const char *path, int flags, mode_t mode)
> > >  }
> > >  
> > >  static __attribute__((unused))
> > > -int openat(int dirfd, const char *path, int flags, ...)
> > > +int openat(int dirfd, const char *path, int flags, mode_t mode)
> > >  {
> > > -	mode_t mode = 0;
> > > -
> > > -	if (flags & O_CREAT) {
> > > -		va_list args;
> > > -
> > > -		va_start(args, flags);
> > > -		mode = va_arg(args, mode_t);
> > > -		va_end(args);
> > > -	}
> > > -
> > >  	return __sysret(_sys_openat(dirfd, path, flags, mode));
> > >  }
> > >  
> > > @@ -51,20 +41,22 @@ int _sys_open(const char *path, int flags, mode_t mode)
> > >  }
> > >  
> > >  static __attribute__((unused))
> > > -int open(const char *path, int flags, ...)
> > > +int open(const char *path, int flags, mode_t mode)
> > >  {
> > > -	mode_t mode = 0;
> > > +	return __sysret(_sys_open(path, flags, mode));
> > > +}
> > >  
> > > -	if (flags & O_CREAT) {
> > > -		va_list args;
> > > +#if __nolibc_gnuc_version >= __nolibc_version(8, 0, 0) || \
> > > +	__nolibc_clang_version >= __nolibc_version(12, 0, 0)
> > >  
> > > -		va_start(args, flags);
> > > -		mode = va_arg(args, mode_t);
> > > -		va_end(args);
> > > -	}
> > > +#  define __nolibc_open_mode(...) __nolibc_first_arg(__VA_ARGS__ __VA_OPT__(,) 0)
> > >  
> > > -	return __sysret(_sys_open(path, flags, mode));
> > > -}
> > > +#  define open(path, flags, ...) \
> > > +	  open(path, flags, __nolibc_open_mode(__VA_ARGS__))
> > > +#  define openat(dirfd, path, flags, ...) \
> > > +	  openat(dirfd, path, flags, __nolibc_open_mode(__VA_ARGS__))
> > > +
> > > +#endif
> > >  
> > >  /*
> > >   * int creat(const char *path, mode_t mode);
> > 
> > I'm really confused here. It looks to me like we first define the open()
> > function then define it as a macro that will override it for recent
> > compilers. It doesn't make the code easy to follow, and in addition I'm
> > really seeing this as a step backwards, for having had to systematically
> > modify existing code to append ',0' to all open() calls in order to build
> > with nolibc years ago (hence the leftover that you spotted in the test).
> 
> Why do you see it as a step backwards? The argument is still optional.
> As seen by the usage in the selftest.
> 
> > It's not clear to me what was the problem you faced with O_TMPFILE in the
> > first place. If it's just a matter of making the 3rd arg optional and not
> > depend on a valist, we can instead define open() as a macro that always
> > passes the 3rd arg as zero when not defined. When I need to handle varargs
> > in macros, I do it like this:
> 
> O_TMPFILE is required to respect the mode argument. This did not happen
> in nolibc. With the current structure we would need to add a check for
> flags & O_TMPFILE == O_TMPFILE, as O_TMPFILE is a bitmask, creating even
> more code. Maybe future O_ flags would also require special handling.
> With the optional argument handling we don't have to care about that at
> all. It would be the users responsibility to pass the correct mode.
> 
> >   #define _OPT_ARG(a0, a1, ...) a1
> >   #define open(path, flags, mode...) _open(path, flags, _OPT_ARG(0, ##mode, 0))
> > 
> >   int _open(const char *path, int flags, mode_t mode)
> >   {
> >      ...
> >   }
> > 
> > The macro passes the "mode" argument to _open() when it's present, and when
> > it's absent, it passes the next one in the _OPT_ARG() macro, which is zero.
> > 
> > E.g.:
> > 
> >   #include <stdio.h>
> >   
> >   #define _OPT_ARG(a0, a1, ...) a1
> >   #define open(path, flags, mode...) _open(path, flags, _OPT_ARG(0, ##mode, 0))
> >   
> >   int _open(const char *path, int flags, int mode)
> >   {
> >      return printf("path=%s flags=%d mode=%d\n", path, flags, mode);
> >   }
> 
> This is exactly what I tried to do. But ,## only works for the
> non-first argument and I didn't think of adding a dummy argument, so I
> ended up with __VA_OPT__.
> 
> As open() is defined to be a function, I used the same name for both the
> vararg macro and the underlying function. I slightly prefer that, but if
> you object, let's rename it to _open().
> 
> >   int main(void)
> >   {
> >           open("file1", 12);
> >           open("file2", 34, 56);
> >           return 0;
> >   }
> > 
> >   $ ./a.out
> >   path=file1 flags=12 mode=0
> >   path=file2 flags=34 mode=56
> > 
> > And this one works even on very old compilers (gcc-3.4 successfully tested).
> 
> Let's use that.

Sashiko rightfully complains[0] that '#define open()' will also apply to
struct member functions called open(). So I'm holding off on this patch
for now.

[0] https://sashiko.dev/#/patchset/20260419-nolibc-open-mode-v1-0-8dc5a960daa7%40weissschuh.net