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