[v2,07/13] tools/nolibc: sys_lseek: add pure 64bit lseek

  use sys_llseek instead of sys_lseek to add 64bit seek even in 32bit
platforms.

This code is based on sysdeps/unix/sysv/linux/lseek.c of glibc and
src/unistd/lseek.c of musl.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 tools/include/nolibc/sys.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
  

On Mon, May 29, 2023, at 21:54, Zhangjin Wu wrote:
> use sys_llseek instead of sys_lseek to add 64bit seek even in 32bit
> platforms.
>
> This code is based on sysdeps/unix/sysv/linux/lseek.c of glibc and
> src/unistd/lseek.c of musl.
>
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> Signed-off-by: Willy Tarreau <w@1wt.eu>
> ---
>  tools/include/nolibc/sys.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> index 98cfa2f6d021..d0720af84b6d 100644
> --- a/tools/include/nolibc/sys.h
> +++ b/tools/include/nolibc/sys.h
> @@ -672,7 +672,17 @@ int link(const char *old, const char *new)
>  static __attribute__((unused))
>  off_t sys_lseek(int fd, off_t offset, int whence)
>  {
> +#if defined(__NR_llseek) || defined(__NR__llseek)
> +#ifndef __NR__llseek
> +#define __NR__llseek __NR_llseek
> +#endif
> +	off_t result;
> +	return my_syscall5(__NR__llseek, fd, offset >> 32, offset, &result, 
> whence) ?: result;
> +#elif defined(__NR_lseek)
>  	return my_syscall3(__NR_lseek, fd, offset, whence);
> +#else
> +#error None of __NR_lseek, __NR_llseek nor __NR__llseek defined, 
> cannot implement sys_lseek()
> +#endif
>  }

This is not technically wrong, but I think a different approach
would be clearer: Instead of having a sys_lseek() that works
differently depending on the macros, why not define the low-level
helpers to match the kernel arguments like

static inline __attribute__((unused))
__kernel_loff_t sys_lseek(int fd, __kernel_loff_t offset, int whence)
{
#ifdef __NR__llseek
	__kernel_loff_t result;
	return my_syscall5(__NR__llseek, fd, offset >> 32, offset, &result,  whence) ?: result;
#else
        
#endif
}

static inline __attribute__((unused))
__kernel_off_t sys_lseek(int fd, __kernel_off_t offset, int whence)
{
#ifdef __NR_lseek
	return my_syscall3(__NR_lseek, fd, offset, whence);
#else
        return -ENOSYS;
#endif
}

And then do the selection inside of the actual lseek,
something like

static __attribute__((unused))
off_t lseek(int fd, off_t offset, int whence)
{
        off_t ret = -ENOSYS;

        if (BITS_PER_LONG == 32)
               ret = sys_llseek(fd, offset, whence);

        if (ret == -ENOSYS)
               ret = sys_lseek(fd, offset, whence);

        if (ret < 0) {
                SET_ERRNO(-ret);
                ret = -1;
        }
        return ret;
       
}

For the loff_t selection, there is no real need to handle the
fallback, so this could just be an if()/else to select 32-bit
or 64-bit, but for the time_t ones the fallback is required
for pre-5.6 kernels.

       Arnd
  

Hi, Arnd, Willy

> On Mon, May 29, 2023, at 21:54, Zhangjin Wu wrote:
> > use sys_llseek instead of sys_lseek to add 64bit seek even in 32bit
> > platforms.
> >
> > This code is based on sysdeps/unix/sysv/linux/lseek.c of glibc and
> > src/unistd/lseek.c of musl.
> >
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > Signed-off-by: Willy Tarreau <w@1wt.eu>
> > ---
> >  tools/include/nolibc/sys.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > index 98cfa2f6d021..d0720af84b6d 100644
> > --- a/tools/include/nolibc/sys.h
> > +++ b/tools/include/nolibc/sys.h
> > @@ -672,7 +672,17 @@ int link(const char *old, const char *new)
> >  static __attribute__((unused))
> >  off_t sys_lseek(int fd, off_t offset, int whence)
> >  {
> > +#if defined(__NR_llseek) || defined(__NR__llseek)
> > +#ifndef __NR__llseek
> > +#define __NR__llseek __NR_llseek
> > +#endif
> > +	off_t result;
> > +	return my_syscall5(__NR__llseek, fd, offset >> 32, offset, &result, 
> > whence) ?: result;
> > +#elif defined(__NR_lseek)
> >  	return my_syscall3(__NR_lseek, fd, offset, whence);
> > +#else
> > +#error None of __NR_lseek, __NR_llseek nor __NR__llseek defined, 
> > cannot implement sys_lseek()
> > +#endif
> >  }
> 
> This is not technically wrong, but I think a different approach
> would be clearer: Instead of having a sys_lseek() that works
> differently depending on the macros, why not define the low-level
> helpers to match the kernel arguments like
> 
> static inline __attribute__((unused))
> __kernel_loff_t sys_lseek(int fd, __kernel_loff_t offset, int whence)
> {
> #ifdef __NR__llseek
> 	__kernel_loff_t result;
> 	return my_syscall5(__NR__llseek, fd, offset >> 32, offset, &result,  whence) ?: result;
> #else
>         
> #endif
> }
> 
> static inline __attribute__((unused))
> __kernel_off_t sys_lseek(int fd, __kernel_off_t offset, int whence)
> {
> #ifdef __NR_lseek
> 	return my_syscall3(__NR_lseek, fd, offset, whence);
> #else
>         return -ENOSYS;
> #endif
> }
> 
> And then do the selection inside of the actual lseek,
> something like
> 
> static __attribute__((unused))
> off_t lseek(int fd, off_t offset, int whence)
> {
>         off_t ret = -ENOSYS;
> 
>         if (BITS_PER_LONG == 32)
>                ret = sys_llseek(fd, offset, whence);
> 
>         if (ret == -ENOSYS)
>                ret = sys_lseek(fd, offset, whence);
> 
>         if (ret < 0) {
>                 SET_ERRNO(-ret);
>                 ret = -1;
>         }
>         return ret;
>        
> }

Yes, It is clearer, thanks. will learn carefully about the kernel types.

> 
> For the loff_t selection, there is no real need to handle the
> fallback, so this could just be an if()/else to select 32-bit
> or 64-bit, but for the time_t ones the fallback is required
> for pre-5.6 kernels.
>

Ok, will test it on the pre-5.6 versions too.

Hi, Willy, what's your suggestion about the oldest kernel versions we plan to support? ;-)

Best regards,
Zhangjin

>        Arnd
  

Hi Zhangjin, Arnd,

On Tue, May 30, 2023 at 09:54:33PM +0800, Zhangjin Wu wrote:
> > And then do the selection inside of the actual lseek,
> > something like
> > 
> > static __attribute__((unused))
> > off_t lseek(int fd, off_t offset, int whence)
> > {
> >         off_t ret = -ENOSYS;
> > 
> >         if (BITS_PER_LONG == 32)
> >                ret = sys_llseek(fd, offset, whence);
> > 
> >         if (ret == -ENOSYS)
> >                ret = sys_lseek(fd, offset, whence);
> > 
> >         if (ret < 0) {
> >                 SET_ERRNO(-ret);
> >                 ret = -1;
> >         }
> >         return ret;
> >        
> > }
> 
> Yes, It is clearer, thanks. will learn carefully about the kernel types.

I, too, like Arnd's proposal here. I tend to use a similar approach in
other projects when possible. Often the limit is the types definition,
which is necessary to define even empty static inline functions. The
only thing is that due to the reliance on -ENOSYS above, the compiler
cannot fully optimize the code away, particularly when both syscalls
are defined, which may result in the compiler emitting the code for
both calls on 32-bit platforms. But the idea is there anyway, and it
may possibly just need a few adjustments based on BITS_PER_LONG after
checking the emitted code.

> > For the loff_t selection, there is no real need to handle the
> > fallback, so this could just be an if()/else to select 32-bit
> > or 64-bit, but for the time_t ones the fallback is required
> > for pre-5.6 kernels.
> >
> 
> Ok, will test it on the pre-5.6 versions too.
> 
> Hi, Willy, what's your suggestion about the oldest kernel versions we plan to
> support? ;-)

Like I said last time, since the code is included in the kernel, we
expect userland developers to use this one to build their code, even
if it's meant to work on older kernels. At the very least I want that
supported kernels continue to work, and then as long as it does not
require particular efforts, it's nice to continue to work on older
ones (think LTS distros, late upgraders of legacy systems etc).

Thanks,
Willy