Re: [LTP] [PATCH v3 1/4] lib: Add support option for .needs_cmds

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> ---
>  include/tst_cmd.h  | 13 +++++++++++++
>  include/tst_test.h | 15 +++++++++++++--
>  lib/tst_cmd.c      |  7 ++++++-
>  lib/tst_test.c     | 22 ++++++++++++++++++----
>  4 files changed, 50 insertions(+), 7 deletions(-)

tst_needs_cmds01.c:15:23: error: initialization of ‘struct tst_cmd *’ from incompatible pointer type ‘const char **’ [-Wincompatible-pointer-types]
   15 |         .needs_cmds = (const char *[]) {
      |                       ^
tst_needs_cmds01.c:15:23: note: (near initialization for ‘test.needs_cmds’)
make[1]: *** [../../include/mk/rules.mk:48: tst_needs_cmds01] Error 1

I asked on v2 not to break build [1], but you didn't reflected that. The reason
for this requirement is that all commits need to built due ability to bisect
(quite obvious requirement).  This mean you need in this commit also modify all
the tests which use .needs_cmds, i.e. 3rd commit "Update test cases use new
needs_cmds" needs to be
squashed into this 1st commit.

[1] https://lore.kernel.org/ltp/20250930133623.GB238065@pevik/

> diff --git a/include/tst_cmd.h b/include/tst_cmd.h
> index 939825646..703589019 100644
> --- a/include/tst_cmd.h
> +++ b/include/tst_cmd.h
> @@ -16,6 +16,19 @@ enum tst_cmd_flags {
>  	TST_CMD_TCONF_ON_MISSING = 2,
>  };

> +/**
> + * struct tst_cmd - This structure provides details about a command needed by LTP test

I would personally avoid "This structure" / "This function".
Missing dot at the end.

> + * @cmd: The name of the command.
> + * @optional: A flag indicating if the command is optional.
> + * @present: A flag indicating if the command was found at runtime, this is an output
Maybe use 2 sentences:
    * @present: A flag indicating if the command was found at runtime. This is an output

> + * parameter, set by the LTP library during the test setup.
> + */
> +struct tst_cmd {
> +	const char *cmd;
> +	unsigned int optional:1;
> +	unsigned int present:1;
Hopefully nobody misuses this to set it in the test.

> +};
> +
>  /*
>   * vfork() + execvp() specified program.
>   *
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 9c21c1728..2730d22c1 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -524,7 +524,7 @@ struct tst_fs {
>   *
>   * @tags: A {} terminated array of test tags. See struct tst_tag for details.
>   *
> - * @needs_cmds: A NULL terminated array of commands required for the test to run.
> + * @needs_cmds: A NULL terminated array of struct tst_cmd required for the test to run.
>   *
>   * @needs_cgroup_ver: If set the test will run only if the specified cgroup
>   *                    version is present on the system.
> @@ -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;

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

> +/**
> + * tst_cmd_present() - Check if a command is present
> + * @cmd: The name of the command to check for.
> + *
> + * This function iterates through the 'needs_cmds' array. It compares the given command name
Please use &tst_test->needs_cmds:

 * This function iterates through the &tst_test->needs_cmds array. It compares
 * the given command name with each entry in the array and returns the 'present'
 * flag for the matching command.

That helps struct to be linkable.

> + * with each entry in the array and returns the 'present' flag for the matching command.
> + *
> + * Return: `true` if the command is present, `false` otherwise.
This needs to be Returns: (missing s)
https://docs.kernel.org/doc-guide/kernel-doc.html

Kind regards,
Petr

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