> 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
> > > > 252
> > > >
> > > > Signed-off-by: Zhangjin Wu <falcon@xxxxxxxxxxx>
> > > > ---
> > > > 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
