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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  include/tst_cmd.h     | 15 +++++++
>  include/tst_private.h |  2 +
>  include/tst_test.h    |  2 +-
>  lib/tst_cmd.c         | 98 +++++++++++++++++++++++++++++++++++++++++++
>  lib/tst_test.c        |  9 ++--
>  5 files changed, 121 insertions(+), 5 deletions(-)
> 
> diff --git a/include/tst_cmd.h b/include/tst_cmd.h
> index 939825646..c7e7e56c1 100644
> --- a/include/tst_cmd.h
> +++ b/include/tst_cmd.h
> @@ -5,6 +5,8 @@
>  #ifndef TST_CMD_H__
>  #define TST_CMD_H__
>  
> +#include <stdbool.h>
> +
>  enum tst_cmd_flags {
>  	/*
>  	 * return the program exit code, otherwise it will call cleanup_fn() if the
> @@ -16,6 +18,19 @@ enum tst_cmd_flags {
>  	TST_CMD_TCONF_ON_MISSING = 2,
>  };
>  
> +struct tst_cmd {
> +	const char *cmd;
> +	unsigned int required:1;

Maybe it would make more sense to flip the logic and add an 'optional'
flag here instead. So that for most of the cases we just need to
initialize the cmd member.

> +	unsigned int support:1;
                     ^
		     This should be named 'present' to match the
		     function name.

> +};
> +
> +
> +/*
> + * tst_cmd_present would loop over the tst_cmd array and return the supported flag
> + * value.
> + */
> +bool tst_cmd_present(struct tst_cmd *pcmd, const char *cmd);
> +
>  /*
>   * vfork() + execvp() specified program.
>   *
> diff --git a/include/tst_private.h b/include/tst_private.h
> index 4c6479f4b..d549cf968 100644
> --- a/include/tst_private.h
> +++ b/include/tst_private.h
> @@ -47,4 +47,6 @@ char tst_kconfig_get(const char *confname);
>   */
>  int tst_check_cmd(const char *cmd, const int brk_nosupp);
>  
> +int tst_check_needs_cmds(struct tst_cmd *cmd, const int brk_nosupp);
> +
>  #endif
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 9c21c1728..8fb7cd86c 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;
>  
>  	const enum tst_cg_ver needs_cgroup_ver;
>  
> diff --git a/lib/tst_cmd.c b/lib/tst_cmd.c
> index 82d60497a..7457d17c4 100644
> --- a/lib/tst_cmd.c
> +++ b/lib/tst_cmd.c
> @@ -330,3 +330,101 @@ error:
>  
>  	return 1;
>  }
> +
> +int tst_check_needs_cmds(struct tst_cmd *cmd, const int brk_nosupp)
> +{
> +	struct version_parser *p;
> +	char *cmd_token, *op_token, *version_token, *next, *str;
> +	char path[PATH_MAX];
> +	char parser_cmd[100];
> +	int ver_parser, ver_get;
> +
> +	strcpy(parser_cmd, cmd->cmd);
> +
> +	cmd_token = strtok_r(parser_cmd, " ", &next);
> +	op_token = strtok_r(NULL, " ", &next);
> +	version_token = strtok_r(NULL, " ", &next);
> +	str = strtok_r(NULL, " ", &next);
> +
> +	if (tst_get_path(cmd_token, path, sizeof(path)))
> +		if (brk_nosupp)
> +			tst_brkm(TCONF, NULL, "Couldn't find '%s' in $PATH", cmd_token);
> +		else
> +			goto error;
> +
> +	if (!op_token)
> +		goto pass;
> +
> +	if (!version_token || str) {
> +		tst_brkm(TCONF, NULL,
> +			"Illegal format(%s), should use format like mkfs.ext4 >= 1.43.0",
> +			cmd->cmd);
> +	}
> +
> +	for (p = &version_parsers[0]; p->cmd; p++) {
> +		if (!strcmp(p->cmd, cmd_token)) {
> +			tst_resm(TINFO, "Parsing %s version", p->cmd);
> +			break;
> +		}
> +	}
> +
> +	if (!p->cmd) {
> +		tst_brkm(TBROK, NULL, "No version parser for %s implemented!",
> +			cmd_token);
> +	}
> +
> +	ver_parser = p->parser();
> +	if (ver_parser < 0)
> +		tst_brkm(TBROK, NULL, "Failed to parse %s version", p->cmd);
> +
> +	ver_get = p->table_get(version_token);
> +	if (ver_get < 0)
> +		tst_brkm(TBROK, NULL, "Failed to get %s version", p->cmd);
> +
> +	if (!strcmp(op_token, ">=")) {
> +		if (ver_parser < ver_get)
> +			goto error;
> +	} else if (!strcmp(op_token, ">")) {
> +		if (ver_parser <= ver_get)
> +			goto error;
> +	} else if (!strcmp(op_token, "<=")) {
> +		if (ver_parser > ver_get)
> +			goto error;
> +	} else if (!strcmp(op_token, "<")) {
> +		if (ver_parser >= ver_get)
> +			goto error;
> +	} else if (!strcmp(op_token, "==")) {
> +		if (ver_parser != ver_get)
> +			goto error;
> +	} else if (!strcmp(op_token, "!=")) {
> +		if (ver_parser == ver_get)
> +			goto error;
> +	} else {
> +		tst_brkm(TCONF, NULL, "Invalid op(%s)", op_token);
> +	}
> +pass:
> +	cmd->support = 1;
> +	return 0;
> +error:
> +	cmd->support = 0;
> +	if (brk_nosupp) {
> +		tst_brkm(TCONF, NULL, "%s requires %s %d, but got %d",
> +			cmd, op_token, ver_get, ver_parser);
> +	} else {
> +		tst_resm(TCONF, "%s requires %s %d, but got %d",
> +			cmd, op_token, ver_get, ver_parser);
> +	}
> +
> +	return 1;
> +}

There is no reason add this function at all, we can just call with the
right arguments and get the return value from tst_check_cmd() in the
tst_test.c instead.

> +bool tst_cmd_present(struct tst_cmd *pcmd, const char *cmd)

No need to pass the struct tst_cmd here, we can access it through the
global tst_test pointer instead. Also this function should go to the tst_test.c.

> +{
> +	while (pcmd->cmd) {
> +		if (!strcmp(pcmd->cmd, cmd))
> +			return pcmd->support;
> +
> +		pcmd++;
> +	}
> +	return false;

If we got here we asked for something that wasn't defined in tst_test
structure so here we should do:

tst_res(TBROK, "Invalid cmd request '%s'", cmd);

> +}


> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index b8894f782..41519d4e1 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1422,11 +1422,12 @@ 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 *cmd = tst_test->needs_cmds;
>  
> -		for (i = 0; (cmd = tst_test->needs_cmds[i]); ++i)
> -			tst_check_cmd(cmd, 1);
> +		while (cmd->cmd) {
> +			tst_check_needs_cmds(cmd,cmd->required);
> +			cmd++;
> +		}
>  	}
>  
>  	if (tst_test->needs_drivers) {
> -- 
> 2.51.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

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