Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Jun 2023 20:36:53 +0200
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <thomas@t-8ch.de>
To:     Zhangjin Wu <falcon@tinylab.org>, w@1wt.eu
Cc:     arnd@arndb.de, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v2 4/4] tools/nolibc: sys.h: apply __syscall() helper
Message-ID: <7e76f099-4198-421c-8157-430201970c4c@t-8ch.de>
References: <cover.1686036862.git.falcon@tinylab.org>
 <ee86e33d9f0031da5932b0de798f188535308dd7.1686036862.git.falcon@tinylab.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ee86e33d9f0031da5932b0de798f188535308dd7.1686036862.git.falcon@tinylab.org>
Precedence: bulk

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@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 :-)