| Message ID | 2f5c3338898da65210ad3f62d7b7773a96f6d251.1685387484.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nolibc: add part2 of support for rv32 | expand |
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
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 } static __attribute__((unused))