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);
> }
>
