Re: [PATCH v4 13/16] selftests/resctrl: Convert ctrlgrp & mongrp to pointers

[Reinette Chatre]

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> The struct resctrl_val_param has control and monitor groups as char
> arrays but they are not supposed to be mutated within resctrl_val().
>
> Convert the ctrlgrp and mongrp char array within resctrl_val_param to
> plain const char pointers and adjust the strlen() based checks to
> check NULL instead.
>
> Convert !grp_name check in create_grp() into internal sanity check by
> returning error if the caller asked to create a group but doesn't
> provide a name for the group. The existing code already abides this by
> only calling create_grp() if mongrp is non-NULL so the error should
> never be returned with the current selftests (ctrlgrp is never NULL).
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
>
> v3:
> - Removed wrong comment
> - Changed grp_name check to return -1 on fail (internal sanity check)
> ---
>   tools/testing/selftests/resctrl/resctrl.h   |  4 ++--
>   tools/testing/selftests/resctrl/resctrlfs.c | 15 +++++----------
>   2 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 5967389038d4..a999fbc13fd3 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -91,8 +91,8 @@ struct resctrl_test {
>    */
>   struct resctrl_val_param {
>   	char		*resctrl_val;
> -	char		ctrlgrp[64];
> -	char		mongrp[64];
> +	const char	*ctrlgrp;
> +	const char	*mongrp;
>   	char		filename[64];
>   	unsigned long	mask;
>   	int		num_of_runs;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index a0e84157eb63..6b4448588666 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -464,13 +464,8 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
>   	struct dirent *ep;
>   	DIR *dp;
>   
> -	/*
> -	 * At this point, we are guaranteed to have resctrl FS mounted and if
> -	 * length of grp_name == 0, it means, user wants to use root con_mon
> -	 * grp, so do nothing
> -	 */
> -	if (strlen(grp_name) == 0)
> -		return 0;
> +	if (!grp_name)
> +		return -1;

As I said during review of v2, this should not be an error. I went back to read
your comments and the argument that this is done for benefit of API is unclear since
the default control group does not have a name, exactly what create_grp() supports
when not returning an error in this case.

Reinette