Hi Ilpo,
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:
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);
}
