Re: [PATCH 5/6] tools/rv: Parse the mixed enable state of containers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tools/rv: ikm_run_monitor() still treats mixed-state container as enabled
--

commit 46a912d6c838892960e060f3a3369c5f213319f7
Author: Wei Li <liwei391@huawei.com>

tools/rv: Parse the mixed enable state of containers

This commit updates __ikm_read_enable() to parse the "X" mixed enable state
of containers as 2 and displays such containers as [MIX] in rv list.

> diff --git a/tools/verification/rv/src/in_kernel.c b/tools/verification/rv/src/in_kernel.c
> index e6dea4040f8f4..0f2e9f1eca51f 100644
> --- a/tools/verification/rv/src/in_kernel.c
> +++ b/tools/verification/rv/src/in_kernel.c
> @@ -29,21 +29,38 @@ static char *config_reactor;
>   *
>   * __does not log errors.
>   *
> - * Returns the current status, or -1 if the monitor does not exist,
> - * __hence not logging errors.
> + * Returns 0 if disabled, 1 if enabled, 2 if a container with children
> + * in mixed states, or -1 if the monitor does not exist, __hence not
> + * logging errors.
>   */
>  static int __ikm_read_enable(char *monitor_name)
>  {
>  	char path[MAX_PATH];
> -	long long enabled;
> -	int retval;
> +	char *buffer;
> +	int enabled;
>  
>  	snprintf(path, MAX_PATH, "rv/monitors/%s/enable", monitor_name);
>  
> -	retval = tracefs_instance_file_read_number(NULL, path, &enabled);
> -	if (retval < 0)
> +	buffer = tracefs_instance_file_read(NULL, path, NULL);
> +	if (!buffer)
>  		return -1;
>  
> +	switch (buffer[0]) {
> +	case '0':
> +		enabled = 0;
> +		break;
> +	case '1':
> +		enabled = 1;
> +		break;
> +	case 'X':
> +		enabled = 2;
> +		break;
> +	default:
> +		enabled = -1;
> +	}
> +
> +	free(buffer);
> +
>  	return enabled;
>  }

[Severity: Medium]
The commit message notes that rv fails to start a container in a mixed state
because rv mon takes the return value of __ikm_read_enable() as a true value
and reports a spurious "already enabled" message.

However, in tools/verification/rv/src/in_kernel.c, ikm_run_monitor() still
checks for any non-zero return value from __ikm_read_enable():

	retval = __ikm_read_enable(full_name);
	if (retval) {
		err_msg("ikm: monitor %s (in-kernel) is already enabled\n", nested_name);
		return -1;
	}

When a container has children in mixed states, __ikm_read_enable() now
returns 2, which is still non-zero (as is -1 on read errors). Won't
ikm_run_monitor() still treat a mixed-state container as already enabled and
refuse to start it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/CY8PR19MB70098BF493A3B67418C7B9ABB7682@CY8PR19MB7009.namprd19.prod.outlook.com?part=5