On Wed, 30 Aug 2023, Reinette Chatre wrote:
On 8/30/2023 1:59 AM, Ilpo Järvinen wrote:
On Tue, 29 Aug 2023, Reinette Chatre wrote:
This is a tricky one. If I understand correctly this goto target makes
some assumptions about the state (no test plan created yet) and exit
reason (it has to be skipped). A temporary variable is also thrown into
the mix.
So in the end the symmetry proved to be not as simple as was depicted
earlier but "tricky"... I tried to warn about this and it's why I wished
to avoid the allocation entirely. Without allocation, there would have not
been need for the temporary variable nor adjusting the control flow with
that label.
hmmm ... I do not see why an allocation forces the use of a temporary
variable and a change in control (more below).
Can this not be simplified by moving the snippet where
benchmark_cmd[] is initialized to fill_buf to be just before the tests
are run? Perhaps right before ksft_set_plan()?
So I throw a temporary variable into the mix (has_ben) to keep track when
benchmark_cmd needs to be initialized to the default command? It doesn't
play well with what I've in queue after this when user parameters are
collected into a struct which is initialized to default value by a helper
function before any argument processing. That is, initializing the
parameters to defaults needs to be split before and after the parameter
parsing code.
No new temporary variable is needed. Of course, I do not have insight into
what is further down in your queue but based on this work I do think it
can be simplified. Since code is easier to consider, the snippet below
applies on top of this series and shows what I was proposing:
Okay, I'll use the approach you proposed. It'll prevent moving all
initialization of the default values before the argument parsing code
which I would have wanted to do (in a patch that is not part of this
series but it's not an end of the world).
--
i.
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index ae9001ef7b0a..8033eabb9aa8 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -170,11 +170,10 @@ static void run_cat_test(int cpu_no, int no_of_bits)
> int main(int argc, char **argv)
> {
> bool mbm_test = true, mba_test = true, cmt_test = true;
> + const char *benchmark_cmd[BENCHMARK_ARGS] = {};
> int c, cpu_no = 1, i, no_of_bits = 0;
> - const char *benchmark_cmd[BENCHMARK_ARGS];
> char *span_str = NULL;
> bool cat_test = true;
> - char *skip_reason;
> int tests = 0;
> int ret;
>
> @@ -247,17 +246,6 @@ int main(int argc, char **argv)
> }
> }
>
> - /* If no benchmark is given by "-b" argument, use fill_buf. */
> - benchmark_cmd[0] = "fill_buf";
> - ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
> - if (ret < 0)
> - ksft_exit_fail_msg("Out of memory!\n");
> - benchmark_cmd[1] = span_str;
> - benchmark_cmd[2] = "1";
> - benchmark_cmd[3] = "0";
> - benchmark_cmd[4] = "false";
> - benchmark_cmd[5] = NULL;
> -
> last_arg:
>
> ksft_print_header();
> @@ -267,23 +255,30 @@ int main(int argc, char **argv)
> * 1. We write to resctrl FS
> * 2. We execute perf commands
> */
> - if (geteuid() != 0) {
> - skip_reason = "Not running as root. Skipping...\n";
> - goto free_span;
> - }
> + if (geteuid() != 0)
> + return ksft_exit_skip("Not running as root. Skipping...\n");
>
> - if (!check_resctrlfs_support()) {
> - skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n";
> - goto free_span;
> - }
> + if (!check_resctrlfs_support())
> + return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
>
> - if (umount_resctrlfs()) {
> - skip_reason = "resctrl FS unmount failed.\n";
> - goto free_span;
> - }
> + if (umount_resctrlfs())
> + return ksft_exit_skip("resctrl FS unmount failed.\n");
>
> filter_dmesg();
>
> + if (!benchmark_cmd[0]) {
> + /* If no benchmark is given by "-b" argument, use fill_buf. */
> + benchmark_cmd[0] = "fill_buf";
> + ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
> + if (ret < 0)
> + ksft_exit_fail_msg("Out of memory!\n");
> + benchmark_cmd[1] = span_str;
> + benchmark_cmd[2] = "1";
> + benchmark_cmd[3] = "0";
> + benchmark_cmd[4] = "false";
> + benchmark_cmd[5] = NULL;
> + }
> +
> ksft_set_plan(tests ? : 4);
>
> if (mbm_test)
> @@ -300,8 +295,4 @@ int main(int argc, char **argv)
>
> free(span_str);
> ksft_finished();
> -
> -free_span:
> - free(span_str);
> - return ksft_exit_skip(skip_reason);
> }
>
