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 <[email protected]> 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/cy8pr19mb70098bf493a3b67418c7b9abb7...@cy8pr19mb7009.namprd19.prod.outlook.com?part=5
