On Wed, 29 May 2024, Reinette Chatre wrote:
> Hi Ilpo,
>
> On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> > The struct resctrl_val_param has control and monitor groups as char
> > arrays but they are not supposed to be mutated within resctrl_val().
> >
> > Convert the ctrlgrp and mongrp char array within resctrl_val_param to
> > plain const char pointers and adjust the strlen() based checks to
> > check NULL instead.
> >
> > Convert !grp_name check in create_grp() into internal sanity check by
> > returning error if the caller asked to create a group but doesn't
> > provide a name for the group. The existing code already abides this by
> > only calling create_grp() if mongrp is non-NULL so the error should
> > never be returned with the current selftests (ctrlgrp is never NULL).
> >
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >
> > v3:
> > - Removed wrong comment
> > - Changed grp_name check to return -1 on fail (internal sanity check)
> > ---
> > tools/testing/selftests/resctrl/resctrl.h | 4 ++--
> > tools/testing/selftests/resctrl/resctrlfs.c | 15 +++++----------
> > 2 files changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/resctrl/resctrl.h
> > b/tools/testing/selftests/resctrl/resctrl.h
> > index 5967389038d4..a999fbc13fd3 100644
> > --- a/tools/testing/selftests/resctrl/resctrl.h
> > +++ b/tools/testing/selftests/resctrl/resctrl.h
> > @@ -91,8 +91,8 @@ struct resctrl_test {
> > */
> > struct resctrl_val_param {
> > char *resctrl_val;
> > - char ctrlgrp[64];
> > - char mongrp[64];
> > + const char *ctrlgrp;
> > + const char *mongrp;
> > char filename[64];
> > unsigned long mask;
> > int num_of_runs;
> > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
> > b/tools/testing/selftests/resctrl/resctrlfs.c
> > index a0e84157eb63..6b4448588666 100644
> > --- a/tools/testing/selftests/resctrl/resctrlfs.c
> > +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> > @@ -464,13 +464,8 @@ static int create_grp(const char *grp_name, char *grp,
> > const char *parent_grp)
> > struct dirent *ep;
> > DIR *dp;
> > - /*
> > - * At this point, we are guaranteed to have resctrl FS mounted and if
> > - * length of grp_name == 0, it means, user wants to use root con_mon
> > - * grp, so do nothing
> > - */
> > - if (strlen(grp_name) == 0)
> > - return 0;
> > + if (!grp_name)
> > + return -1;
>
> As I said during review of v2, this should not be an error. I went back to
> read
> your comments and the argument that this is done for benefit of API is unclear
> since
> the default control group does not have a name, exactly what create_grp()
> supports
> when not returning an error in this case.
Okay, I did not know about this expectation related to the default control
group because it's not used anywhere in the selftest code that always
provides a ctrlgrp name.
I'll change it to return 0.
--
i.
