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

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Petr,

On 10/30/24 15:49, Petr Vorel wrote:
> Hi Andrea,
>
>> +++ b/include/lapi/syscalls/generate_syscalls.sh
>> @@ -0,0 +1,115 @@
>> +#!/bin/sh
>> +# 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"
>> +
>> +if [ -z "${SYSCALLS_FILE}" ]; then
> nit: I would still replace ${...} with $... (e.g. ${SYSCALLS_FILE} =>
> $SYSCALLS_FILE) as I noted in v4 - readability. IMHO there is no need to use
> ${...} in whole script.
>
> The same applies to include/lapi/syscalls/generate_arch.sh in the second commit.

This is a bit debatable. The reason why using ${VAR} over $VAR is the 
readability in the first place (i.e. when you have `${PATH}/bin` instead 
`$PATH/bin` or when you use characters which could mix with PATH var 
name and it might create debug issues), but also the fact it provides 
${VAR:-default} syntax which can always added afterwards.

I usually prefer this method for these reasons.

>
>> +	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"
>> +
>> +echo '
>> +/************************************************
>> +* GENERATED FILE: DO NOT EDIT/PATCH THIS FILE  *
>> +*  change your arch specific .in file instead  *
>> +************************************************/
>> +
>> +/*
>> +* Here we stick all the ugly *fallback* logic for linux
>> +* system call numbers (those __NR_ thingies).
>> +*
>> +* Licensed under the GPLv2 or later, see the COPYING file.
>> +*/
>
> nit: fixing space in /* */ comments (correct in the original regen.sh)
> + replacing old note about COPYING with SPDX in generated header:
Thanks you seen that. Now I know I can't fully trust auto-refactoring.
>
> +++ include/lapi/syscalls/generate_syscalls.sh
> @@ -17,18 +17,16 @@ fi
>   SCRIPT_DIR="$(realpath $(dirname "$0"))"
>   SUPPORTED_ARCH="${SCRIPT_DIR}/supported-arch.txt"
>   
> -echo '
> +echo '// SPDX-License-Identifier: GPL-2.0-or-later
>   /************************************************
> -* GENERATED FILE: DO NOT EDIT/PATCH THIS FILE  *
> -*  change your arch specific .in file instead  *
> -************************************************/
> + * GENERATED FILE: DO NOT EDIT/PATCH THIS FILE  *
> + *  change your arch specific .in file instead  *
> + ************************************************/
>   
>   /*
> -* Here we stick all the ugly *fallback* logic for linux
> -* system call numbers (those __NR_ thingies).
> -*
> -* Licensed under the GPLv2 or later, see the COPYING file.
> -*/
> + * Here we stick all the ugly *fallback* logic for linux
> + * system call numbers (those __NR_ thingies).
> + */
>   
>   #ifndef LAPI_SYSCALLS_H__
>   #define LAPI_SYSCALLS_H__
> ---
>
> Kind regards,
> Petr
Andrea

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