Willy, Thomas
This is the revision of the v1 syscall helpers [1], just rebased it on 20230606-nolibc-rv32+stkp7a of [2]. It doesn't conflict with the -ENOSYS patchset [3], so, it is ok to simply merge both of them.
This revision mainly applied your suggestions of v1, both of the syscall return and call helpers are simplified or cleaned up.
Changes from v1 -> v2:
* tools/nolibc: sys.h: add __syscall() and __sysret() helpers * Use inline function instead of macro for the syscall return helper (Suggestion from Thomas)
* Rename syscall return helper from __syscall_ret to __sysret (align with __syscall and it is not that long now)
* Make __sysret() be always inline (Suggestion from Willy)
* Simplify the whole __syscall() macro to oneline code (Benefit from the fixed 'long' return type of syscalls)
* tools/nolibc: unistd.h: apply __sysret() helper * Convert the whole _syscall() macro to oneline code
* tools/nolibc: sys.h: apply __sysret() helper * Futher convert both brk() and getpagesize() to oneline code
* tools/nolibc: sys.h: apply __syscall() helper * Keep the same as v1, because the __syscall() usage not changed
Best regards, Zhangjin
--- [1]: https://lore.kernel.org/linux-riscv/cover.1685856497.git.falcon@tinylab.org/ [2]: https://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git [3]: https://lore.kernel.org/linux-riscv/cover.1685780412.git.falcon@tinylab.org/
Zhangjin Wu (4): tools/nolibc: sys.h: add __syscall() and __sysret() helpers tools/nolibc: unistd.h: apply __sysret() helper tools/nolibc: sys.h: apply __sysret() helper tools/nolibc: sys.h: apply __syscall() helper
tools/include/nolibc/sys.h | 366 ++++++---------------------------- tools/include/nolibc/unistd.h | 11 +- 2 files changed, 57 insertions(+), 320 deletions(-)
most of the library routines share the same code model, let's add two helpers to simplify the coding and shrink the code lines too.
One added for syscall return, one added for syscall call.
Thomas suggested to use inline function instead of macro for __sysret(), and he also helped to simplify the __syscall() a lot.
Willy suggested to make __sysret() be always inline.
Suggested-by: Willy Tarreau w@1wt.eu Link: https://lore.kernel.org/linux-riscv/ZH1+hkhiA2+ItSvX@1wt.eu/ Suggested-by: Thomas Weißschuh linux@weissschuh.net Link: https://lore.kernel.org/linux-riscv/ea4e7442-7223-4211-ba29-70821e907888@t-8... Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/include/nolibc/sys.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index 5464f93e863e..c12c14db056e 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -28,6 +28,18 @@ #include "errno.h" #include "types.h"
+/* Syscall return helper, set errno as -ret when ret < 0 */ +static inline __attribute__((always_inline)) long __sysret(long ret) +{ + if (ret < 0) { + SET_ERRNO(-ret); + ret = -1; + } + return ret; +} + +/* Syscall call helper, use syscall name instead of syscall number */ +#define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__))
/* Functions in this file only describe syscalls. They're declared static so * that the compiler usually decides to inline them while still being allowed
most of the library routines share the same code model, let's add two helpers to simplify the coding and shrink the code lines too.
One added for syscall return, one added for syscall call.
Thomas suggested to use inline function instead of macro for __sysret(), and he also helped to simplify the __syscall() a lot.
Willy suggested to make __sysret() be always inline.
Suggested-by: Willy Tarreau w@1wt.eu Link: https://lore.kernel.org/linux-riscv/ZH1+hkhiA2+ItSvX@1wt.eu/ Suggested-by: Thomas Weißschuh linux@weissschuh.net Link: https://lore.kernel.org/linux-riscv/ea4e7442-7223-4211-ba29-70821e907888@t-8... Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/include/nolibc/sys.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index 5464f93e863e..c12c14db056e 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -28,6 +28,18 @@ #include "errno.h" #include "types.h" +/* Syscall return helper, set errno as -ret when ret < 0 */ +static inline __attribute__((always_inline)) long __sysret(long ret)
Sorry, the run-user/run targets in tools/testing/selftests/nolibc/Makefile complains about the above line, seems it doesn't support the 'inline' keyword and requires '__inline__'.
Just checked my own test script and the run-user / run targets, the only difference is it forcely uses -std=c89, do we need to align with the kernel Makefile and use -std=gnu11 instead?
Whatever, I need to change this line to align with the other codes, use __inline__ as we have used in tools/include/nolibc/stdlib.h:
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index 0cfc5157845a..48365288a903 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -29,7 +29,8 @@ #include "types.h"
/* Syscall return helper, set errno as -ret when ret < 0 */ -static inline __attribute__((always_inline)) long __sysret(long ret) +static __inline__ __attribute__((unused, always_inline)) +long __sysret(long ret) { if (ret < 0) { SET_ERRNO(-ret);
Best regards, Zhangjin
+{
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
+}
+/* Syscall call helper, use syscall name instead of syscall number */ +#define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__)) /* Functions in this file only describe syscalls. They're declared static so
- that the compiler usually decides to inline them while still being allowed
-- 2.25.1
From: Zhangjin Wu
Sent: 06 June 2023 09:10
most of the library routines share the same code model, let's add two helpers to simplify the coding and shrink the code lines too.
...
+/* Syscall return helper, set errno as -ret when ret < 0 */ +static inline __attribute__((always_inline)) long __sysret(long ret) +{
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
+}
If that right? I thought that that only the first few (1024?) negative values got used as errno values.
Do all Linux architectures even use negatives for error? I thought at least some used the carry flag. (It is the historic method of indicating a system call failure.)
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David,
On 2023-06-08 14:35:49+0000, David Laight wrote:
From: Zhangjin Wu
Sent: 06 June 2023 09:10
most of the library routines share the same code model, let's add two helpers to simplify the coding and shrink the code lines too.
...
+/* Syscall return helper, set errno as -ret when ret < 0 */ +static inline __attribute__((always_inline)) long __sysret(long ret) +{
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
+}
If that right? I thought that that only the first few (1024?) negative values got used as errno values.
Do all Linux architectures even use negatives for error? I thought at least some used the carry flag. (It is the historic method of indicating a system call failure.)
I guess you are thinking about the architectures native systemcall ABI.
In nolibc these are abstracted away in the architecture-specific assembly wrappers: my_syscall0 to my_syscall6. (A good example would be arch-mips.h)
These normalize the architecture systemcall ABI to negative errornumbers which then are returned from the sys_* wrapper functions.
The sys_* wrapper functions in turn are used by the libc function which translate the negative error number to the libc-style "return -1 and set errno" mechanism. At this point the new __sysret function is used.
Returning negative error numbers in between has the advantage that it can be used without having to set up a global/threadlocal errno variable.
In hope this helped, Thomas
Hi, Thomas, David, Willy
Hi David,
On 2023-06-08 14:35:49+0000, David Laight wrote:
From: Zhangjin Wu
Sent: 06 June 2023 09:10
most of the library routines share the same code model, let's add two helpers to simplify the coding and shrink the code lines too.
...
+/* Syscall return helper, set errno as -ret when ret < 0 */ +static inline __attribute__((always_inline)) long __sysret(long ret) +{
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
+}
If that right? I thought that that only the first few (1024?) negative values got used as errno values.
Thanks David, this question did inspire me to think about the syscalls who returns pointers, we didn't touch them yet:
static __attribute__((unused)) void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset) { void *ret = sys_mmap(addr, length, prot, flags, fd, offset);
if ((unsigned long)ret >= -4095UL) { SET_ERRNO(-(long)ret); ret = MAP_FAILED; } return ret; }
If we convert the return value to 'unsigned long' for the pointers, this compare may be compatible with the old 'long' ret compare 'ret < 0',
/* Syscall return helper, set errno as -ret when ret is in [-4095, -1] */ static __inline__ __attribute__((unused, always_inline)) long __sysret(unsigned long ret) { if (ret >= -4095UL) { SET_ERRNO(-(long)ret); ret = -1; } return ret; }
Or something like musl does:
/* Syscall return helper, set errno as -ret when ret is in [-4095, -1] */ static __inline__ __attribute__((unused, always_inline)) long __sysret(unsigned long ret) { if (ret > -4096UL) { SET_ERRNO(-ret); return -1; } return ret; }
So, it reserves 4095 error values (I'm not sure where documents this, perhaps we need a stanard description in the coming commit message), the others can be used as pointers or the other data.
If this is ok for you, we may need to renew the v3 series [1] or add this as an additional patchset (which may be better for us to learn why we do this) to add the support for the syscalls who return pointers, I did prepare such a series yesterday, welcome more discussions.
[1]: https://lore.kernel.org/linux-riscv/cover.1686135913.git.falcon@tinylab.org/
Do all Linux architectures even use negatives for error? I thought at least some used the carry flag. (It is the historic method of indicating a system call failure.)
I guess you are thinking about the architectures native systemcall ABI.
In nolibc these are abstracted away in the architecture-specific assembly wrappers: my_syscall0 to my_syscall6. (A good example would be arch-mips.h)
Yes, thanks. mips may be the only arch nolibc currently supported who has separated ret and errno.
The manpage of syscall lists more: alpha, ia64, sparc/32, sparc/64, tile.
https://man7.org/linux/man-pages/man2/syscall.2.html
These normalize the architecture systemcall ABI to negative errornumbers which then are returned from the sys_* wrapper functions.
For mips, it is:
#define my_syscall0(num) \ ({ \ register long _num __asm__ ("v0") = (num); \ register long _arg4 __asm__ ("a3"); \ \ __asm__ volatile ( \ "addiu $sp, $sp, -32\n" \ "syscall\n" \ "addiu $sp, $sp, 32\n" \ : "=r"(_num), "=r"(_arg4) \ : "r"(_num) \ : "memory", "cc", "at", "v1", "hi", "lo", \ "t0", "t1", "t2", "t3", "t4", "t5", "t6", "t7", "t8", "t9" \ ); \ _arg4 ? -_num : _num; \ })
I did learn some difference from musl, it did this as following:
static inline long __syscall0(long n) { register long r7 __asm__("$7"); register long r2 __asm__("$2"); __asm__ __volatile__ ( "addu $2,$0,%2 ; syscall" : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2) : SYSCALL_CLOBBERLIST, "$8", "$9", "$10"); return r7 && r2>0 ? -r2 : r2; }
It checks "r2>0" to make sure only convert 'r2' to negated when r2 is positive number, I'm wondering this checking may be about the big pointers, when its first highest bit is 1, then, that may be an issue, if this guess is true, perhaps we should update this together with the revision of __sysret().
Thanks very much.
Best regards, Zhangjin
The sys_* wrapper functions in turn are used by the libc function which translate the negative error number to the libc-style "return -1 and set errno" mechanism. At this point the new __sysret function is used.
Returning negative error numbers in between has the advantage that it can be used without having to set up a global/threadlocal errno variable.
In hope this helped, Thomas
From: Zhangjin Wu
Sent: 09 June 2023 05:43
Hi, Thomas, David, Willy
Hi David,
On 2023-06-08 14:35:49+0000, David Laight wrote:
From: Zhangjin Wu
Sent: 06 June 2023 09:10
most of the library routines share the same code model, let's add two helpers to simplify the coding and shrink the code lines too.
...
+/* Syscall return helper, set errno as -ret when ret < 0 */ +static inline __attribute__((always_inline)) long __sysret(long ret) +{
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
+}
If that right? I thought that that only the first few (1024?) negative values got used as errno values.
Thanks David, this question did inspire me to think about the syscalls who returns pointers, we didn't touch them yet:
I'm also not sure whether lseek() is expected to return values that would be negative.
(I do remember having to patch out some checks (not Linux) in order to use: echo -n xxxx | dd of=/dev/kmem oseek=nnn in order to patch a live kernel!)
Technically read() and write() can do longer transfers, but Linux limits them to MAXINT. IIRC both BSD and SYSV allow drivers return all values (except -1) form ioctl().
The check for -4095UL is probably reasonable.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Use __sysret() to shrink the whole _syscall() to oneline code.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/include/nolibc/unistd.h | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/tools/include/nolibc/unistd.h b/tools/include/nolibc/unistd.h index c20b2fbf065e..7e3c005d28ba 100644 --- a/tools/include/nolibc/unistd.h +++ b/tools/include/nolibc/unistd.h @@ -56,16 +56,7 @@ int tcsetpgrp(int fd, pid_t pid) return ioctl(fd, TIOCSPGRP, &pid); }
-#define _syscall(N, ...) \ -({ \ - long _ret = my_syscall##N(__VA_ARGS__); \ - if (_ret < 0) { \ - SET_ERRNO(-_ret); \ - _ret = -1; \ - } \ - _ret; \ -}) - +#define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__)) #define _sycall_narg(...) __syscall_narg(__VA_ARGS__, 6, 5, 4, 3, 2, 1, 0) #define __syscall_narg(_0, _1, _2, _3, _4, _5, _6, N, ...) N #define _syscall_n(N, ...) _syscall(N, __VA_ARGS__)
Use __sysret() to shrink both brk() and getpagesize() to oneline code.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/include/nolibc/sys.h | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index c12c14db056e..f6e3168b3e50 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -78,13 +78,7 @@ void *sys_brk(void *addr) static __attribute__((unused)) int brk(void *addr) { - void *ret = sys_brk(addr); - - if (!ret) { - SET_ERRNO(ENOMEM); - return -1; - } - return 0; + return __sysret(sys_brk(addr) ? 0 : -ENOMEM); }
static __attribute__((unused)) @@ -547,15 +541,7 @@ static unsigned long getauxval(unsigned long key); static __attribute__((unused)) long getpagesize(void) { - long ret; - - ret = getauxval(AT_PAGESZ); - if (!ret) { - SET_ERRNO(ENOENT); - return -1; - } - - return ret; + return __sysret(getauxval(AT_PAGESZ) ?: -ENOENT); }
Use __syscall() helper to shrink 252 lines of code.
$ git show HEAD^:tools/include/nolibc/sys.h | wc -l 1425 $ git show HEAD:tools/include/nolibc/sys.h | wc -l 1173 $ echo "1425-1173" | bc -l 252
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/include/nolibc/sys.h | 336 +++++-------------------------------- 1 file changed, 42 insertions(+), 294 deletions(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index f6e3168b3e50..0cfc5157845a 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -108,13 +108,7 @@ int sys_chdir(const char *path) static __attribute__((unused)) int chdir(const char *path) { - int ret = sys_chdir(path); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(chdir, path); }
@@ -137,13 +131,7 @@ int sys_chmod(const char *path, mode_t mode) static __attribute__((unused)) int chmod(const char *path, mode_t mode) { - int ret = sys_chmod(path, mode); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(chmod, path, mode); }
@@ -166,13 +154,7 @@ int sys_chown(const char *path, uid_t owner, gid_t group) static __attribute__((unused)) int chown(const char *path, uid_t owner, gid_t group) { - int ret = sys_chown(path, owner, group); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(chown, path, owner, group); }
@@ -189,13 +171,7 @@ int sys_chroot(const char *path) static __attribute__((unused)) int chroot(const char *path) { - int ret = sys_chroot(path); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(chroot, path); }
@@ -212,13 +188,7 @@ int sys_close(int fd) static __attribute__((unused)) int close(int fd) { - int ret = sys_close(fd); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(close, fd); }
@@ -235,13 +205,7 @@ int sys_dup(int fd) static __attribute__((unused)) int dup(int fd) { - int ret = sys_dup(fd); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(dup, fd); }
@@ -264,13 +228,7 @@ int sys_dup2(int old, int new) static __attribute__((unused)) int dup2(int old, int new) { - int ret = sys_dup2(old, new); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(dup2, old, new); }
@@ -288,13 +246,7 @@ int sys_dup3(int old, int new, int flags) static __attribute__((unused)) int dup3(int old, int new, int flags) { - int ret = sys_dup3(old, new, flags); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(dup3, old, new, flags); } #endif
@@ -312,13 +264,7 @@ int sys_execve(const char *filename, char *const argv[], char *const envp[]) static __attribute__((unused)) int execve(const char *filename, char *const argv[], char *const envp[]) { - int ret = sys_execve(filename, argv, envp); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(execve, filename, argv, envp); }
@@ -365,13 +311,7 @@ pid_t sys_fork(void) static __attribute__((unused)) pid_t fork(void) { - pid_t ret = sys_fork(); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(fork); }
@@ -388,13 +328,7 @@ int sys_fsync(int fd) static __attribute__((unused)) int fsync(int fd) { - int ret = sys_fsync(fd); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(fsync, fd); }
@@ -411,13 +345,7 @@ int sys_getdents64(int fd, struct linux_dirent64 *dirp, int count) static __attribute__((unused)) int getdents64(int fd, struct linux_dirent64 *dirp, int count) { - int ret = sys_getdents64(fd, dirp, count); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(getdents64, fd, dirp, count); }
@@ -455,13 +383,7 @@ pid_t sys_getpgid(pid_t pid) static __attribute__((unused)) pid_t getpgid(pid_t pid) { - pid_t ret = sys_getpgid(pid); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(getpgid, pid); }
@@ -562,13 +484,7 @@ int sys_gettimeofday(struct timeval *tv, struct timezone *tz) static __attribute__((unused)) int gettimeofday(struct timeval *tv, struct timezone *tz) { - int ret = sys_gettimeofday(tv, tz); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(gettimeofday, tv, tz); }
@@ -606,13 +522,7 @@ int sys_ioctl(int fd, unsigned long req, void *value) static __attribute__((unused)) int ioctl(int fd, unsigned long req, void *value) { - int ret = sys_ioctl(fd, req, value); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(ioctl, fd, req, value); }
/* @@ -628,13 +538,7 @@ int sys_kill(pid_t pid, int signal) static __attribute__((unused)) int kill(pid_t pid, int signal) { - int ret = sys_kill(pid, signal); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(kill, pid, signal); }
@@ -657,13 +561,7 @@ int sys_link(const char *old, const char *new) static __attribute__((unused)) int link(const char *old, const char *new) { - int ret = sys_link(old, new); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(link, old, new); }
@@ -684,13 +582,7 @@ off_t sys_lseek(int fd, off_t offset, int whence) static __attribute__((unused)) off_t lseek(int fd, off_t offset, int whence) { - off_t ret = sys_lseek(fd, offset, whence); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(lseek, fd, offset, whence); }
@@ -713,13 +605,7 @@ int sys_mkdir(const char *path, mode_t mode) static __attribute__((unused)) int mkdir(const char *path, mode_t mode) { - int ret = sys_mkdir(path, mode); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(mkdir, path, mode); }
@@ -742,13 +628,7 @@ long sys_mknod(const char *path, mode_t mode, dev_t dev) static __attribute__((unused)) int mknod(const char *path, mode_t mode, dev_t dev) { - int ret = sys_mknod(path, mode, dev); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(mknod, path, mode, dev); }
#ifndef MAP_SHARED @@ -806,13 +686,7 @@ int sys_munmap(void *addr, size_t length) static __attribute__((unused)) int munmap(void *addr, size_t length) { - int ret = sys_munmap(addr, length); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(munmap, addr, length); }
/* @@ -832,13 +706,7 @@ int mount(const char *src, const char *tgt, const char *fst, unsigned long flags, const void *data) { - int ret = sys_mount(src, tgt, fst, flags, data); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(mount, src, tgt, fst, flags, data); }
@@ -872,13 +740,7 @@ int open(const char *path, int flags, ...) va_end(args); }
- ret = sys_open(path, flags, mode); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(open, path, flags, mode); }
@@ -898,13 +760,7 @@ static __attribute__((unused)) int prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5) { - int ret = sys_prctl(option, arg2, arg3, arg4, arg5); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(prctl, option, arg2, arg3, arg4, arg5); }
@@ -921,13 +777,7 @@ int sys_pivot_root(const char *new, const char *old) static __attribute__((unused)) int pivot_root(const char *new, const char *old) { - int ret = sys_pivot_root(new, old); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(pivot_root, new, old); }
@@ -956,13 +806,7 @@ int sys_poll(struct pollfd *fds, int nfds, int timeout) static __attribute__((unused)) int poll(struct pollfd *fds, int nfds, int timeout) { - int ret = sys_poll(fds, nfds, timeout); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(poll, fds, nfds, timeout); }
@@ -979,13 +823,7 @@ ssize_t sys_read(int fd, void *buf, size_t count) static __attribute__((unused)) ssize_t read(int fd, void *buf, size_t count) { - ssize_t ret = sys_read(fd, buf, count); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(read, fd, buf, count); }
@@ -1003,13 +841,7 @@ ssize_t sys_reboot(int magic1, int magic2, int cmd, void *arg) static __attribute__((unused)) int reboot(int cmd) { - int ret = sys_reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, cmd, 0); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, cmd, 0); }
@@ -1026,13 +858,7 @@ int sys_sched_yield(void) static __attribute__((unused)) int sched_yield(void) { - int ret = sys_sched_yield(); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(sched_yield); }
@@ -1072,13 +898,7 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva static __attribute__((unused)) int select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeval *timeout) { - int ret = sys_select(nfds, rfds, wfds, efds, timeout); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(select, nfds, rfds, wfds, efds, timeout); }
@@ -1095,13 +915,7 @@ int sys_setpgid(pid_t pid, pid_t pgid) static __attribute__((unused)) int setpgid(pid_t pid, pid_t pgid) { - int ret = sys_setpgid(pid, pgid); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(setpgid, pid, pgid); }
@@ -1118,13 +932,7 @@ pid_t sys_setsid(void) static __attribute__((unused)) pid_t setsid(void) { - pid_t ret = sys_setsid(); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(setsid); }
#if defined(__NR_statx) @@ -1141,13 +949,7 @@ int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct sta static __attribute__((unused)) int statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) { - int ret = sys_statx(fd, path, flags, mask, buf); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(statx, fd, path, flags, mask, buf); } #endif
@@ -1227,13 +1029,7 @@ int sys_stat(const char *path, struct stat *buf) static __attribute__((unused)) int stat(const char *path, struct stat *buf) { - int ret = sys_stat(path, buf); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(stat, path, buf); }
@@ -1256,13 +1052,7 @@ int sys_symlink(const char *old, const char *new) static __attribute__((unused)) int symlink(const char *old, const char *new) { - int ret = sys_symlink(old, new); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(symlink, old, new); }
@@ -1296,13 +1086,7 @@ int sys_umount2(const char *path, int flags) static __attribute__((unused)) int umount2(const char *path, int flags) { - int ret = sys_umount2(path, flags); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(umount2, path, flags); }
@@ -1325,13 +1109,7 @@ int sys_unlink(const char *path) static __attribute__((unused)) int unlink(const char *path) { - int ret = sys_unlink(path); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(unlink, path); }
@@ -1354,38 +1132,20 @@ pid_t sys_wait4(pid_t pid, int *status, int options, struct rusage *rusage) static __attribute__((unused)) pid_t wait(int *status) { - pid_t ret = sys_wait4(-1, status, 0, NULL); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(wait4, -1, status, 0, NULL); }
static __attribute__((unused)) pid_t wait4(pid_t pid, int *status, int options, struct rusage *rusage) { - pid_t ret = sys_wait4(pid, status, options, rusage); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(wait4, pid, status, options, rusage); }
static __attribute__((unused)) pid_t waitpid(pid_t pid, int *status, int options) { - pid_t ret = sys_wait4(pid, status, options, NULL); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(wait4, pid, status, options, NULL); }
@@ -1402,13 +1162,7 @@ ssize_t sys_write(int fd, const void *buf, size_t count) static __attribute__((unused)) ssize_t write(int fd, const void *buf, size_t count) { - ssize_t ret = sys_write(fd, buf, count); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(write, fd, buf, count); }
@@ -1425,13 +1179,7 @@ int sys_memfd_create(const char *name, unsigned int flags) static __attribute__((unused)) int memfd_create(const char *name, unsigned int flags) { - ssize_t ret = sys_memfd_create(name, flags); - - if (ret < 0) { - SET_ERRNO(-ret); - ret = -1; - } - return ret; + return __syscall(memfd_create, name, flags); }
/* make sure to include all global symbols */
Hi Zhangjin,
On 2023-06-06 16:17:38+0800, Zhangjin Wu wrote:
Use __syscall() helper to shrink 252 lines of code.
$ git show HEAD^:tools/include/nolibc/sys.h | wc -l 1425 $ git show HEAD:tools/include/nolibc/sys.h | wc -l 1173 $ echo "1425-1173" | bc -l 252Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/include/nolibc/sys.h | 336 +++++-------------------------------- 1 file changed, 42 insertions(+), 294 deletions(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index f6e3168b3e50..0cfc5157845a 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -108,13 +108,7 @@ int sys_chdir(const char *path) static __attribute__((unused)) int chdir(const char *path) {
- int ret = sys_chdir(path);
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
- return __syscall(chdir, path);
To be honest I'm still not a big fan of the __syscall macro. It's a bit too magic for too little gain.
The commit message argues that the patches make the code shorter.
However doing
__sysret(sys_chdir(path));
instead of
__syscall(chdir, path);
is only three characters longer and the same amout of lines.
Otherwise we would have syscall() _syscall() and __syscall() each doing different things.
And __syscall does not behave like a regular function.
The rest of the patchset looks great.
Maybe Willy can break the tie?
Thomas
Note: If we figure out a way to build syscall() without macros I would like that also :-)
Hi Zhangjin,
On 2023-06-06 16:17:38+0800, Zhangjin Wu wrote:
Use __syscall() helper to shrink 252 lines of code.
$ git show HEAD^:tools/include/nolibc/sys.h | wc -l 1425 $ git show HEAD:tools/include/nolibc/sys.h | wc -l 1173 $ echo "1425-1173" | bc -l 252Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/include/nolibc/sys.h | 336 +++++-------------------------------- 1 file changed, 42 insertions(+), 294 deletions(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index f6e3168b3e50..0cfc5157845a 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -108,13 +108,7 @@ int sys_chdir(const char *path) static __attribute__((unused)) int chdir(const char *path) {
- int ret = sys_chdir(path);
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
- return __syscall(chdir, path);
To be honest I'm still not a big fan of the __syscall macro. It's a bit too magic for too little gain.
The commit message argues that the patches make the code shorter.
However doing
__sysret(sys_chdir(path));
instead of
__syscall(chdir, path);
is only three characters longer and the same amout of lines.
Yeah, I do like your version too, it looks consise too, the only not comfortable part is there are dual calls in one line.
Otherwise we would have syscall() _syscall() and __syscall() each doing different things.
Yes, I'm worried about this too, although the compilers may help a little, but it is too later.
Just brain storming, What about another non-similar name, for example, __syswrap() or __sysin() ?
Or even convert __sysret() to __sysout() and __syscall() to __sysin(), do you like it? or even __sysexit(), __sysentry(), but the __sysexit() may be misused with sys_exit().
/* Syscall return helper, set errno as -ret when ret < 0 */ static __inline__ __attribute__((unused, always_inline)) long __sysout(long ret) { if (ret < 0) { SET_ERRNO(-ret); ret = -1; } return ret; }
/* Syscall call helper, use syscall name instead of syscall number */ #define __sysin(name, ...) __sysout(sys_##name(__VA_ARGS__))
static __attribute__((unused)) int brk(void *addr) { return __sysout(sys_brk(addr) ? 0 : -ENOMEM); }
static __attribute__((unused)) int chdir(const char *path) { return __sysin(chdir, path); }
If we really want something like __syscall()/__sysret(), I do think they should be a pair ;-)
And __syscall does not behave like a regular function.
The rest of the patchset looks great.
Thanks for your nice review.
Maybe Willy can break the tie?
If there is no better solution, I think your version is also a first step to go.
Thomas
Note: If we figure out a way to build syscall() without macros I would like that also :-)
Yes, but it is not easy to cope with the variable number of arguments without a macro.
BTW, do you like to convert the my_syscallN() of sys.h to the syscall() you added? I do worry about it will make the checking of arguments mismatch, exspecially, the checking of number of them hardly.
Best regards, Zhangjin
Hi,
On Wed, Jun 07, 2023 at 08:34:06AM +0800, Zhangjin Wu wrote:
Hi Zhangjin,
On 2023-06-06 16:17:38+0800, Zhangjin Wu wrote:
Use __syscall() helper to shrink 252 lines of code.
$ git show HEAD^:tools/include/nolibc/sys.h | wc -l 1425 $ git show HEAD:tools/include/nolibc/sys.h | wc -l 1173 $ echo "1425-1173" | bc -l 252Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/include/nolibc/sys.h | 336 +++++-------------------------------- 1 file changed, 42 insertions(+), 294 deletions(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index f6e3168b3e50..0cfc5157845a 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -108,13 +108,7 @@ int sys_chdir(const char *path) static __attribute__((unused)) int chdir(const char *path) {
- int ret = sys_chdir(path);
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
- return __syscall(chdir, path);
To be honest I'm still not a big fan of the __syscall macro. It's a bit too magic for too little gain.
The commit message argues that the patches make the code shorter.
However doing
__sysret(sys_chdir(path));
instead of
__syscall(chdir, path);
is only three characters longer and the same amout of lines.
Yeah, I do like your version too, it looks consise too, the only not comfortable part is there are dual calls in one line.
For those who want to debug, having less macros or magic stuff is always better, and in this essence I too find that Thomas' version is more expressive about what is being done. Also, if some syscalls require a specific handling (e.g. mmap() needs to return MAP_FAILED instead), it's much easier to change only the code dealing with the return value and errno setting than having to guess how to reimplement what was magically done in a macro.
Otherwise we would have syscall() _syscall() and __syscall() each doing different things.
Yes, I'm worried about this too, although the compilers may help a little, but it is too later.
The issue is for the person who remembers "I need to use 'syscall'" but never remembering the number of underscores nor the variations.
Just brain storming, What about another non-similar name, for example, __syswrap() or __sysin() ?
Or even convert __sysret() to __sysout() and __syscall() to __sysin(), do you like it? or even __sysexit(), __sysentry(), but the __sysexit() may be misused with sys_exit().
I'd rather use "__set_errno()" to explicitly mention that it's only used to set errno, but sysret would be fine as well IMHO as if we're purist, it also normalizes the return value.
/* Syscall return helper, set errno as -ret when ret < 0 */ static __inline__ __attribute__((unused, always_inline)) long __sysout(long ret) { if (ret < 0) { SET_ERRNO(-ret); ret = -1; } return ret; }/* Syscall call helper, use syscall name instead of syscall number */ #define __sysin(name, ...) __sysout(sys_##name(__VA_ARGS__))
static __attribute__((unused)) int brk(void *addr) { return __sysout(sys_brk(addr) ? 0 : -ENOMEM); } static __attribute__((unused)) int chdir(const char *path) { return __sysin(chdir, path); }
I still don't find this intuitive at all.
If we really want something like __syscall()/__sysret(), I do think they should be a pair ;-)
Then one being called "call" while the other one being "ret" do form a pair, no ?
Thanks, Willy
On Wed, Jun 07, 2023 at 08:34:06AM +0800, Zhangjin Wu wrote:
Hi Zhangjin,
On 2023-06-06 16:17:38+0800, Zhangjin Wu wrote:
Use __syscall() helper to shrink 252 lines of code.
$ git show HEAD^:tools/include/nolibc/sys.h | wc -l 1425 $ git show HEAD:tools/include/nolibc/sys.h | wc -l 1173 $ echo "1425-1173" | bc -l 252Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/include/nolibc/sys.h | 336 +++++-------------------------------- 1 file changed, 42 insertions(+), 294 deletions(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index f6e3168b3e50..0cfc5157845a 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -108,13 +108,7 @@ int sys_chdir(const char *path) static __attribute__((unused)) int chdir(const char *path) {
- int ret = sys_chdir(path);
- if (ret < 0) {
SET_ERRNO(-ret);ret = -1;- }
- return ret;
- return __syscall(chdir, path);
To be honest I'm still not a big fan of the __syscall macro. It's a bit too magic for too little gain.
The commit message argues that the patches make the code shorter.
However doing
__sysret(sys_chdir(path));
instead of
__syscall(chdir, path);
is only three characters longer and the same amout of lines.
Yeah, I do like your version too, it looks consise too, the only not comfortable part is there are dual calls in one line.
For those who want to debug, having less macros or magic stuff is always better, and in this essence I too find that Thomas' version is more expressive about what is being done. Also, if some syscalls require a specific handling (e.g. mmap() needs to return MAP_FAILED instead), it's much easier to change only the code dealing with the return value and errno setting than having to guess how to reimplement what was magically done in a macro.
Ok, so, let's go with Thomas' version ;-)
Otherwise we would have syscall() _syscall() and __syscall() each doing different things.
Yes, I'm worried about this too, although the compilers may help a little, but it is too later.
The issue is for the person who remembers "I need to use 'syscall'" but never remembering the number of underscores nor the variations.
Yeah, it is hard to remember.
Just brain storming, What about another non-similar name, for example, __syswrap() or __sysin() ?
Or even convert __sysret() to __sysout() and __syscall() to __sysin(), do you like it? or even __sysexit(), __sysentry(), but the __sysexit() may be misused with sys_exit().
I'd rather use "__set_errno()" to explicitly mention that it's only used to set errno, but sysret would be fine as well IMHO as if we're purist, it also normalizes the return value.
Ok, let's take the shorter sysret() seems no similar sys_xxx calls.
/* Syscall return helper, set errno as -ret when ret < 0 */ static __inline__ __attribute__((unused, always_inline)) long __sysout(long ret) { if (ret < 0) { SET_ERRNO(-ret); ret = -1; } return ret; }/* Syscall call helper, use syscall name instead of syscall number */ #define __sysin(name, ...) __sysout(sys_##name(__VA_ARGS__))
static __attribute__((unused)) int brk(void *addr) { return __sysout(sys_brk(addr) ? 0 : -ENOMEM); } static __attribute__((unused)) int chdir(const char *path) { return __sysin(chdir, path); }I still don't find this intuitive at all.
If we really want something like __syscall()/__sysret(), I do think they should be a pair ;-)
Then one being called "call" while the other one being "ret" do form a pair, no ?
The 'ret' currently is a part of our old '__syscall', seems not that like a 'pair', it differs from entry/exit ;-)
As a summary, will use 'sysret()' and something like:
static __attribute__((unused)) int chdir(const char *path) { return sysret(chdir(path)); }
to renew the syscall helper patchset, Thanks you very much.
Best regards, Zhangjin
Thanks, Willy
Hi Zhangjin,
On 2023-06-07 13:39:20+0800, Zhangjin Wu wrote:
As a summary, will use 'sysret()' and something like:
static __attribute__((unused)) int chdir(const char *path) { return sysret(chdir(path)); }
to renew the syscall helper patchset, Thanks you very much.
But please to use the "__" prefix. Otherwise it could conflict with user code.
Thomas
Hi Zhangjin,
On 2023-06-07 13:39:20+0800, Zhangjin Wu wrote:
As a summary, will use 'sysret()' and something like:
static __attribute__((unused)) int chdir(const char *path) { return sysret(chdir(path)); }
to renew the syscall helper patchset, Thanks you very much.
But please to use the "__" prefix. Otherwise it could conflict with user code.
Ok, not yet to modify the code, will reserve the '__sysret()' and convert the left parts to this style: '__sysret(chdir(path))', a simple sed script may help to do so.
Thanks a lot. Zhangjin
Thomas
From: Zhangjin Wu
Sent: 07 June 2023 06:39
...
As a summary, will use 'sysret()' and something like:
static __attribute__((unused)) int chdir(const char *path) { return sysret(chdir(path)); }
to renew the syscall helper patchset, Thanks you very much.
While I'm all for using 'cpp-magic' to abstract and (hopefully) simplify things. Token-pasting the sys_ here doesn't seem to gain anything. Anyone grepping the code for 'sys_chdir' is also going to wonder where it is used.
There might be scope for something like: #define syscall_wrapper(func, type) \ static __attribute__((unused)) \ int func(type *arg) \ { \ return sysret(sys_#func(arg)); \ } and then: syscall_wrapper(chdir, const char *) would expand to the code above.
I think you'd need separate defines for each number of arguments.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight
Sent: 10 June 2023 17:34
From: Zhangjin Wu
Sent: 07 June 2023 06:39
...
As a summary, will use 'sysret()' and something like:
static __attribute__((unused)) int chdir(const char *path) { return sysret(chdir(path)); }
to renew the syscall helper patchset, Thanks you very much.
While I'm all for using 'cpp-magic' to abstract and (hopefully) simplify things. Token-pasting the sys_ here doesn't seem to gain anything. Anyone grepping the code for 'sys_chdir' is also going to wonder where it is used.
I think I've spotted a later version of the patch that doesn't paste the sys_
There might be scope for something like: #define syscall_wrapper(func, type) \ static __attribute__((unused)) \ int func(type *arg) \ { \ return sysret(sys_#func(arg)); \ } and then: syscall_wrapper(chdir, const char *) would expand to the code above.
I think you'd need separate defines for each number of arguments.
It is also worth pointing out that once the 'static unused' functions have been defined, all the #define 'goop' used to define them can be summarily #undef'ed. That (mostly) solves the namespace problem.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
linux-kselftest-mirror@lists.linaro.org
-
David Laight -
Thomas Weißschuh -
Willy Tarreau -
Zhangjin Wu