RE: [PATCH v6 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Shaopeng Tan (Fujitsu)]

Hi Ilpo,

> On Tue, 7 Feb 2023, Shaopeng Tan (Fujitsu) wrote:
> 
> > > On Tue, 31 Jan 2023, Shaopeng Tan wrote:
> > >
> > > > After creating a child process with fork() in CAT test, if an
> > > > error occurs or a signal such as SIGINT is received, the parent
> > > > process will be terminated immediately, and therefor the child
> > > > process will not be killed and also resctrlfs is not unmounted.
> > > >
> > > > There is a signal handler registered in CMT/MBM/MBA tests, which
> > > > kills child process, unmount resctrlfs, cleanups result files,
> > > > etc., if a signal such as SIGINT is received.
> > > >
> > > > Commonize the signal handler registered for CMT/MBM/MBA tests and
> > > > reuse it in CAT too.
> > > >
> > > > To reuse the signal handler, make the child process in CAT wait to
> > > > be killed by parent process in any case (an error occurred or a
> > > > signal was received), and when killing child process use global
> > > > bm_pid instead of local bm_pid.
> > > >
> > > > Also, since the MBA/MBA/CMT/CAT are run in order, unregister the
> > > > signal handler at the end of each test so that the signal handler
> > > > cannot be inherited by other tests.
> > > >
> > > > Signed-off-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
> > > > ---
> 
> > > >  	ret = cat_val(&param);
> > > > -	if (ret)
> > > > -		return ret;
> > > > -
> > > > -	ret = check_results(&param);
> > > > -	if (ret)
> > > > -		return ret;
> > > > +	if (ret == 0)
> > > > +		ret = check_results(&param);
> > >
> > > It would be take this program flow fix out of the signal handler
> > > change into a separate change.
> >
> > Do you mean this fix should be separated into two patches?
> 
> Yes.
> 
> Currently, I see your patch doing (mainly) two things:
> 1) cleaning up the messy signal handler logic
> 2) fixing the early return in case of error from cat_val() or
>    check_results()
> 
> Both are good changes and both are needed to fully fix things. But (IMHO)
> those are indepedent enough that it would warrant to split this change into two.

Thanks for your advice, I will split it in next version


Best regards,
Shaopeng TAN