Re: [LTP] [PATCH v8 2/4] Add script to generate arch(s) dependant syscalls

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

the script works for me, great!

Few more comments, I promis the last one, feel free to just ignore them.

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

> --- /dev/null
> +++ b/include/lapi/syscalls/generate_arch.sh
> @@ -0,0 +1,206 @@
> +#!/bin/sh -eu
> +# SPDX-License-Identifier: GPL-2.0-or-later
I would put here your/SUSE/LTP copyright (2024) and the original author
copyright:
# Copyright (c) Marcin Juszkiewicz, 2023-2024

The author does not have it in the script, but it has in LICENSE:
Copyright (c) Marcin Juszkiewicz

But I'm not a lawyer, maybe I'm wrong.

> +#
> +# This is an adaptation of the update-tables.sh script, included in the
> +# syscalls-table project (https://github.com/hrw/syscalls-table) and released
> +# under the MIT license.
> +#
> +# Author: Andrea Cervesato <andrea.cervesato@suse.com>
> +
> +if [ "$#" -eq "0" ]; then
> +	echo "Please provide kernel sources:"
> +	echo ""
> +	echo "$0 path/to/Linux/kernel/sources"
> +	echo ""
> +	exit 1
> +fi

nit: use common syntax ($# is always defined, thus not need to be quoted, 0 as
well, "" is empty string, thus can be omitted):
if [ $# -eq 0 ]; then
	echo "Please provide kernel sources:"
	echo
	echo "$0 path/to/Linux/kernel/sources"
	echo
	exit 1
fi

...
> +generate_list_syscalls_c() {
> +	(
very nit: this first echo is probably not needed.
> +		echo
> +		echo "
> +		#include <stdio.h>
> +		#include <asm/unistd.h>
> +
> +		int main(void)
> +		{
> +		"
> +		for syscall in $(cat ${TEMP}/syscall-names.txt); do
> +			echo "
> +		#ifdef __NR_$syscall
> +			printf(\"$syscall %d\\\n\", __NR_$syscall);
> +		#endif
> +		"
> +		done
> +		echo " return 0;
> +		}"
> +	) >${TEMP}/list-syscalls.c
> +}
...
> +
> +do_all_tables() {
> +	for archdir in ${KERNELSRC}/arch/*; do
> +		arch=$(basename $archdir)
...
> +			arch=x86_64 extraflags=-D__LP64__ generate_table
> +			bits=32
> +			arch=i386 generate_table
> +			arch=x32 extraflags=-D__ILP32__ generate_table
> +			;;
> +		arc | csky | hexagon | m68k | microblaze | nios2 | openrisc | sh | xtensa)
I've never seen spaces between '|' separator in shell case command. It should
not cause anything, but it's just unusual (original code in
https://github.com/hrw/syscalls-table/blob/master/scripts/update-tables.sh
from which we diverged quite a lot now also does not have it).

Kind regards,
Petr

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