Re: [LTP] [PATCH v2 1/2] lib: Add support option for .needs_cmds

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

...
> +++ b/include/tst_cmd.h
> @@ -16,6 +16,12 @@ enum tst_cmd_flags {
>  	TST_CMD_TCONF_ON_MISSING = 2,
>  };

> +struct tst_cmd {
> +	const char *cmd;
> +	unsigned int optional:1;
> +	unsigned int present:1;

Maybe s/present/available/ ?
> +};

Could you please add doc? Whole include/tst_cmd.h is documented, it's obvious
that new functionality should be documented.

And best would be if you use kerneldoc syntax.
https://docs.kernel.org/doc-guide/kernel-doc.html
https://www.sphinx-doc.org/en/master/usage/domains/c.html

> +
>  /*
>   * vfork() + execvp() specified program.
>   *
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 9c21c1728..bef836525 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -617,7 +617,7 @@ struct tst_fs {

>  	const struct tst_tag *tags;

> -	const char *const *needs_cmds;
> +	struct tst_cmd *needs_cmds;

This effectively breaks build, because you haven't update any of these users (24
tests + 2 doc files.

>  	const enum tst_cg_ver needs_cgroup_ver;

> @@ -721,6 +721,12 @@ int tst_creat_unlinked(const char *path, int flags, mode_t mode);
>   */
>  const char *tst_get_tmpdir_root(void);

> +/*
> + * tst_cmd_present would loop over the tst_cmd array and return the supported flag

I'm not a native speaker, but I think:

s/would loop/loops/

> + * value.
> + */
Please use kernel doc syntax (see links above).

> +bool tst_cmd_present(const char *cmd);
> +
>  /*
>   * Validates exit status of child processes
>   */
> diff --git a/lib/tst_cmd.c b/lib/tst_cmd.c
> index 82d60497a..cfd38c31a 100644
> --- a/lib/tst_cmd.c
> +++ b/lib/tst_cmd.c
> @@ -265,7 +265,12 @@ int tst_check_cmd(const char *cmd, const int brk_nosupp)
>  	str = strtok_r(NULL, " ", &next);

>  	if (tst_get_path(cmd_token, path, sizeof(path)))
> -		tst_brkm(TCONF, NULL, "Couldn't find '%s' in $PATH", cmd_token);
> +		if (brk_nosupp) {
> +			tst_brkm(TCONF, NULL, "Couldn't find '%s' in $PATH", cmd_token);
> +		} else {
> +			tst_resm(TCONF, "Couldn't find '%s' in $PATH", cmd_token);
> +			return 1;
> +		}

+1

...
> +++ b/lib/tst_test.c
> @@ -1353,6 +1353,19 @@ static const char *default_fs_type(void)
>  	return tst_dev_fs_type();
>  }

> +bool tst_cmd_present(const char *cmd)
> +{
> +	struct tst_cmd *pcmd = tst_test->needs_cmds;
> +
> +	while (pcmd->cmd) {
> +		if (!strcmp(pcmd->cmd, cmd))
> +			return pcmd->present;
> +
> +		pcmd++;
> +	}
> +	return false;
> +}
> +
>  static void do_setup(int argc, char *argv[])
>  {
>  	char *tdebug_env = getenv("LTP_ENABLE_DEBUG");
> @@ -1422,11 +1435,15 @@ static void do_setup(int argc, char *argv[])
>  		tst_brk(TCONF, "%dbit ABI is not supported", tst_test->needs_abi_bits);

>  	if (tst_test->needs_cmds) {
> -		const char *cmd;
> -		int i;
> +		struct tst_cmd *pcmd = tst_test->needs_cmds;

> -		for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
> -			tst_check_cmd(cmd, 1);
> +		while (pcmd->cmd) {
> +			if (tst_check_cmd(pcmd->cmd, !pcmd->optional))
> +				pcmd->present = 0;
> +			else
> +				pcmd->present = 1;
very nit: use ternary operator?

				pcmd->present = tst_check_cmd(pcmd->cmd, !pcmd->optional) ? 0 : 1;

Kind regards,
Petr

> +			pcmd++;
> +		}

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