Re: [LTP] [PATCH v4 1/4] Refactor regen.sh script to generate syscalls

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

nice improvement.

TL;DR typo "kj" + missing SPDX and copyright would be worth to fix before merge.

Below are other minor notes, feel free to ignore.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Rename regen.sh into a more meaningful generate_syscalls.sh name, rename
> order into a more meaningful supported-syscalls.txt name and rewrite
> part of the regen.sh script code in order to execute it from anywhere in
> the filesystem, without need to be in its own folder. The new code is
> also more clear and concise, using native sh features which are
> simplifying the code.

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  configure.ac                                       |   2 +-
>  include/lapi/syscalls/generate_syscalls.sh         | 115 ++++++++++++++++++
>  include/lapi/syscalls/regen.sh                     | 129 ---------------------
>  .../lapi/syscalls/{order => supported-arch.txt}    |   0
>  4 files changed, 116 insertions(+), 130 deletions(-)

> diff --git a/configure.ac b/configure.ac
> index ebbf49e28..45f92477f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -384,7 +384,7 @@ else
>      AC_SUBST([WITH_REALTIME_TESTSUITE],["no"])
>  fi

> -AC_CONFIG_COMMANDS([syscalls.h], [cd ${ac_top_srcdir}/include/lapi/syscalls; ./regen.sh])
> +AC_CONFIG_COMMANDS([syscalls.h], [cd ${ac_top_srcdir}/include/lapi/syscalls; ./generate_syscalls.sh ../syscalls.h])

As Li noted, this will need to be rebased
AC_CONFIG_COMMANDS([syscalls.h], [cd ${ac_top_srcdir}/include/lapi/syscalls; ./generate_syscalls.sh; cd - >/dev/null])

But as you don't do cd, that fix "cd - > /dev/null" can be again removed.

>  # custom functions
>  # NOTE: don't create custom functions for simple checks, put them into this file
> diff --git a/include/lapi/syscalls/generate_syscalls.sh b/include/lapi/syscalls/generate_syscalls.sh
> new file mode 100755
> index 000000000..863f52253
> --- /dev/null
> +++ b/include/lapi/syscalls/generate_syscalls.sh
> @@ -0,0 +1,115 @@
> +#!/bin/sh

The script should have SPDX + some copyright, right?
# SPDX-License-Identifier: GPL-2.0-or-later

> +#
> +# Generate the syscalls.h file, merging all architectures syscalls input file
> +# which are in the current folder and defined inside supported-arch.txt file.
> +
> +SYSCALLS_FILE="${1}"

nit: {} are not needed
SYSCALLS_FILE="$1"

I would personally use ${..} only when needed (IMHO everywhere in the script
just $... can be used (readability).

> +
> +if [ -z "${SYSCALLS_FILE}" ]; then
> +	echo "Please provide the syscalls.h directory:"
> +	echo ""
> +	echo "$0 path/of/syscalls.h"
> +	echo ""
> +	exit 1
> +fi
> +
> +SCRIPT_DIR="$(realpath $(dirname "$0"))"
> +SUPPORTED_ARCH="${SCRIPT_DIR}/supported-arch.txt"
> +
> +merge_syscalls() {
> +	echo '
> +/************************************************
> +* GENERATED FILE: DO NOT EDIT/PATCH THIS FILE  *
> +*  change your arch specific .in file instead  *
> +************************************************/
> +
> +/*
> +kj* Here we stick all the ugly *fallback* logic for linux

typo "kj", can you please remove it?

> +* system call numbers (those __NR_ thingies).
> +*
> +* Licensed under the GPLv2 or later, see the COPYING file.
> +*/
> +
> +#ifndef LAPI_SYSCALLS_H__
> +#define LAPI_SYSCALLS_H__
> +
> +#include <errno.h>
> +#include <sys/syscall.h>
> +#include <asm/unistd.h>
> +
> +#ifdef TST_TEST_H__
> +#define TST_SYSCALL_BRK__(NR, SNR) ({ \
> +tst_brk(TCONF, \
> +	"syscall(%d) " SNR " not supported on your arch", NR); \
> +})
> +#else
> +inline static void dummy_cleanup(void) {}
> +
> +#define TST_SYSCALL_BRK__(NR, SNR) ({ \
> +tst_brkm(TCONF, dummy_cleanup, \
> +	"syscall(%d) " SNR " not supported on your arch", NR); \
> +})
> +#endif
> +
> +#define tst_syscall(NR, ...) ({ \
> +intptr_t tst_ret; \
> +if (NR == __LTP__NR_INVALID_SYSCALL) { \
> +	errno = ENOSYS; \
> +	tst_ret = -1; \
> +} else { \
> +	tst_ret = syscall(NR, ##__VA_ARGS__); \
> +} \
> +if (tst_ret == -1 && errno == ENOSYS) { \
> +	TST_SYSCALL_BRK__(NR, #NR); \
> +} \
> +tst_ret; \
> +})
> +
> +#define __LTP__NR_INVALID_SYSCALL -1' >${SYSCALLS_FILE}
> +
> +	while IFS= read -r arch; do
> +		(
> +			echo
> +			case ${arch} in
> +			sparc64) echo "#if defined(__sparc__) && defined(__arch64__)" ;;
> +			sparc) echo "#if defined(__sparc__) && !defined(__arch64__)" ;;
> +			s390) echo "#if defined(__s390__) && !defined(__s390x__)" ;;
> +			mips_n32) echo "#if defined(__mips__) && defined(_ABIN32)" ;;
> +			mips_n64) echo "#if defined(__mips__) && defined(_ABI64)" ;;
> +			mips_o32) echo "#if defined(__mips__) && defined(_ABIO32) && _MIPS_SZLONG == 32" ;;
> +			*) echo "#ifdef __${arch}__" ;;
> +			esac
> +
> +			while read -r line; do
> +				set -- ${line}
> +				syscall_nr="__NR_$1"
> +				shift
> +
> +				echo "# ifndef ${syscall_nr}"
> +				echo "#  define ${syscall_nr} $*"
> +				echo "# endif"
> +			done <"${SCRIPT_DIR}/${arch}.in"
> +			echo "#endif"
> +			echo
> +		) >>${SYSCALLS_FILE}
> +	done <${SUPPORTED_ARCH}

nit: I would either don't define any function at all (have everything as
inline, thus save 1 indent) or, have this part of the code as a function.

Kind regards,
Petr

> +
> +	(
> +		echo
> +		echo "/* Common stubs */"
> +		while IFS= read -r arch; do
> +			while IFS= read -r line; do
> +				set -- ${line}
> +				syscall_nr="__NR_$1"
> +				shift
> +
> +				echo "# ifndef ${syscall_nr}"
> +				echo "#  define ${syscall_nr} __LTP__NR_INVALID_SYSCALL"
> +				echo "# endif"
> +			done <"${SCRIPT_DIR}/${arch}.in"
> +		done <${SUPPORTED_ARCH}
> +		echo "#endif"
> +	) >>${SYSCALLS_FILE}
> +}
> +
> +merge_syscalls

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp