Re: [PATCH v3 1/2] tools/nolibc: fcntl: Add fallocate()

[Thomas Weißschuh]

On 2026-05-02 22:26:07+0100, David Laight wrote:
> On Sat, 2 May 2026 05:00:06 +0200
> Willy Tarreau <w@xxxxxx> wrote:
> 
> > On Fri, May 01, 2026 at 09:18:31AM +0100, David Laight wrote:
> > > On Fri,  1 May 2026 01:41:24 +0900
> > > Daniel Palmer <daniel@xxxxxxxxx> wrote:

(...)

> > > > +/*
> > > > + * int fallocate(int fd, int mode, off_t offset, off_t size);
> > > > + */
> > > > +
> > > > +#if !defined(_sys_fallocate)
> > > > +static __attribute__((unused))
> > > > +int _sys_fallocate(int fd, int mode, off_t offset, off_t size)
> > > > +{
> > > > +	/*
> > > > +	 * For 32 bit machines __kernel_long_t will be 4, off_t will be 8
> > > > +	 * and we need to split offset and size, for 64 machines we can use
> > > > +	 * the values as-is.
> > > > +	 */
> > > > +	const bool offsetsz_two_args = sizeof(__kernel_long_t) != sizeof(off_t);  
> > > 
> > > I don't think you care about the size of off_t.
> > > Were it to be 4 the code would be badly wrong.

I agree here that sizeof(__kernel_long_t) != 8 would be clearer a bit.
Also the intermediate variable could go away.

> > >   
> > > > +
> > > > +	if (offsetsz_two_args)
> > > > +		return __nolibc_syscall6(__NR_fallocate, fd, mode,
> > > > +			__NOLIBC_LLARGPART(offset, 0), __NOLIBC_LLARGPART(offset, 1),
> > > > +			__NOLIBC_LLARGPART(size, 0), __NOLIBC_LLARGPART(size, 1));
> > > > +	else
> > > > +		return __nolibc_syscall4(__NR_fallocate, fd, mode, offset, size);
> > > > +}  
> > > 
> > > The above might be more readable as:
> > > 	if (sizeof(__kernel_long_t) == 8)
> > > 		/* 64 bit, values fit in single arguments */
> > > 		return __nolibc_syscall4(__NR_fallocate, fd, mode, offset, size);
> > > 
> > > 	/* 32 bit, values need splitting, order depends on endianness */
> > > 	/* This test for endianness doesn't rely on any pre-processor defines */
> > > 	if (({union {int x; char c;} u; u.x = 1; u.c;}))
> > > 		/* Little endian */
> > > 		return __nolibc_syscall6(__NR_fallocate, fd, mode,
> > > 			offset, offset >> 32, size, size >> 32);
> > > 	/* Big endian */
> > > 	return __nolibc_syscall6(__NR_fallocate, fd, mode,
> > > 		offset >> 32, offset, size >> 32, size);  
> > 
> > Honestly David, I find Daniel's version way more readable :-) Precisely
> > because the repeated variations are abstracted with this more readable
> > macro. If it was used only once I could possibly agree. Even the
> > endianness test is hard to read, better rely on __BYTE_ORDER__ for
> > this.

Here I agree with Daniel and Willy, the original aproach looks cleaner.

> I did say 'might' :-) and I should probably have used __BYTE_ORDER__.
> 
> Looking again, the code is trying to copy what the compiler generates
> when it passes a 64bit quantity on stack or in 2 registers.
> So f(a, b, c, d) is traditionally 'push d; push c; push b, push a; call f'.
> With the normal 'stack grows down' this gives (in increasing addresses):
> 	return address
> 	a
> 	b
> 	c
> 	d
> If 'b,c' is replaced by a 64bit value then you want the stack memory
> to contain the correct representation of a 64bit value.
> So for LE you need to pass the low part before the high part.

Most architectures should use registers and not the stack.

> But there is one architecture (parisc) where the stack goes the other way.
> It is BE (I believe), but would need the LE argument order.
> 
> I think all the conditionals could be moved out of the fallocate code.
> I'm not sure of the exact pre-processor conditionals but something like:
> 
> #if (__BITS_PER_LONG == 64) || defined(x86_x32) || defined(mips_n32)
> #define __NOLIBC_ARG64(x) (x)
> #else
> /* The on-stack data has to be in the natural order for a 64bit value. */
> #if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN_) ^ defined(STACK_GROWS_UP)
> #define __NOLIBC_ARG64(x) ((int)(x)), ((int)((long long)(x) >> 32))
> #else
> #define __NOLIBC_ARG64(x) ((int)((long long)(x) >> 32)), ((int)(x))
> #endif
> #endif

I highly dislike macros like these which may expand to either a single
or multiple arguments. Also this logic looks much more complicated than
what Daniel proposed.

> Then (if I've got it right):
> static __attribute__((unused))
> int _sys_fallocate(int fd, int mode, off_t offset, off_t size)
> {
> 	return _syscall(__NR_fallocate, fd, mode, __NOLIBC_ARG64(offset),
> 			__NOLIBC_ARG64(size));
> }

We intentionally use __nolibc_syscallX() inside nolibc proper over
_syscall()/syscall() to have the arity of the syscall written out.

> There is the other problem that arm32 (and maybe others) requires the 64bit
> variable be aligned in the stack frame.
> So f(int a, long long b) is effectively f(int a, int pad, long long b).
> (This usually causes grief with lseek().)
> So a pad might be needed for some system calls on some 32bit architectures.
> This could be done with something that expands to '' or '0,'.

I don't see why we have to worry about variable alignment or stack
direction at all. That's the compilers job. We do have both lseek() and
parisc32 nowadays and both work fine. Also with Daniel's series on top.
Am I missing something?


Thomas