Messages in this thread | | | Date | Wed, 2 Nov 2022 03:41:47 -0600 | Subject | Re: [PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test | From | Shuah Khan <> |
| |
On 11/1/22 03:43, Shaopeng Tan wrote: > After creating a child process with fork() in CAT test, if there is > an error occurs or such as a SIGINT signal is received, the parent > process will be terminated immediately, but the child process will not > be killed and also umount_resctrlfs() will not be called. > > Add a signal handler like other tests to kill child process, umount > resctrlfs, cleanup result files, etc. when an error occurs. > > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > --- > tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c > index 6a8306b0a109..5f81817f4366 100644 > --- a/tools/testing/selftests/resctrl/cat_test.c > +++ b/tools/testing/selftests/resctrl/cat_test.c > @@ -98,12 +98,21 @@ void cat_test_cleanup(void) > remove(RESULT_FILE_NAME2); > } > > +static void ctrl_handler(int signo) > +{ > + kill(bm_pid, SIGKILL); > + umount_resctrlfs(); > + tests_cleanup(); > + ksft_print_msg("Ending\n\n");
Is there a reason to print this message? Remove it unless it serves a purpose.
> + > + exit(EXIT_SUCCESS); > +} > + > int cat_perf_miss_val(int cpu_no, int n, char *cache_type) > { > unsigned long l_mask, l_mask_1; > int ret, pipefd[2], sibling_cpu_no; > char pipe_message; > - pid_t bm_pid;
Odd. bm_pid is used below - why remove it here?
> > cache_size = 0; > > @@ -181,17 +190,19 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) > strcpy(param.filename, RESULT_FILE_NAME1); > param.num_of_runs = 0; > param.cpu_no = sibling_cpu_no; > + } else { > + /* set up ctrl-c handler */ > + if (signal(SIGINT, ctrl_handler) == SIG_ERR || > + signal(SIGHUP, ctrl_handler) == SIG_ERR || > + signal(SIGTERM, ctrl_handler) == SIG_ERR) > + printf("Failed to catch SIGNAL!\n");
Is perror() more appropriate here?
> } > > remove(param.filename); > > ret = cat_val(¶m); > - if (ret) > - return ret; > - > - ret = check_results(¶m); > - if (ret) > - return ret; > + if (ret == 0) > + ret = check_results(¶m);
Why not use a goto in error case to do umount_resctrlfs() instead of changing the conditionals?
> > if (bm_pid == 0) { > /* Tell parent that child is ready */ > @@ -201,7 +212,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) > sizeof(pipe_message)) { > close(pipefd[1]); > perror("# failed signaling parent process"); > - return errno; > } > > close(pipefd[1]); > @@ -226,5 +236,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) > if (bm_pid) > umount_resctrlfs(); > > - return 0; > + return ret; > }
With these changes made:
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
thanks, -- Shuah
| |