linux-kernel - Re: [PATCH 7/7] selftests/resctrl: Cleanup benchmark argument parsing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dc2406aa-be6f-e641-786e-cf67f790ce78@intel.com>
Date:   Mon, 14 Aug 2023 10:56:21 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        <linux-kselftest@...r.kernel.org>, Shuah Khan <shuah@...nel.org>,
        Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        <linux-kernel@...r.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH 7/7] selftests/resctrl: Cleanup benchmark argument parsing

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> Benchmark argument is handled by custom argument parsing code which is
> more complicated than it needs to be.
> 
> Process benchmark argument within the normal getopt() handling and drop
> entirely unnecessary ben_ind and has_ben variables. If -b is not given,
> setup the default benchmark command right after the switch statement
> and make -b to goto over it while it terminates the getopt() loop.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
>  .../testing/selftests/resctrl/resctrl_tests.c | 56 +++++++++----------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 81c2ed299e6f..a437aaa69cc5 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -173,25 +173,27 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  
>  int main(int argc, char **argv)
>  {
> -	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
> -	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
> +	bool mbm_test = true, mba_test = true, cmt_test = true;
> +	int c, cpu_no = 1, i, no_of_bits = 0;
>  	const char *benchmark_cmd[BENCHMARK_ARGS];
> -	int ben_ind, tests = 0;
>  	bool cat_test = true;
> +	int tests = 0;
>  
> -	for (i = 0; i < argc; i++) {
> -		if (strcmp(argv[i], "-b") == 0) {
> -			ben_ind = i + 1;
> -			argc_new = ben_ind - 1;
> -			has_ben = true;
> -			break;
> -		}
> -	}
> -
> -	while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
> +	while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) {
>  		char *token;
>  
>  		switch (c) {
> +		case 'b':
> +			optind--;			/* Back to optarg */

The above tab usage is not clear. Also, tail comments in code has been
found to disturb reading to the point that x86 contributors are explicitly
asked to refrain from using it.
Perhaps rather a short summary of algorithm before the code starts?

Apart from this style nitpick it looks good to me. After that is
handled you can add:

Reviewed-by: Reinette Chatre <reinette.chatre@...el.com>

Reinette