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

[Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>]

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.

-- 
 i.

> To make the child process wait to be killed by parent process
> in any case(an error occurred or a signal was received),
> I fixed it like this.
> 
> This fix was discussed here.
> https://lore.kernel.org/lkml/2ab9ca20-c757-7dd8-b770-2b84d171cbfb@xxxxxxxxx/