Received-SPF: pass (google.com: domain of linux-kernel+bounces-195314-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Thu, 30 May 2024 14:44:57 +0300 (EEST)
To: Reinette Chatre <reinette.chatre@intel.com>
cc: linux-kselftest@vger.kernel.org, Shuah Khan <shuah@kernel.org>, 
    Babu Moger <babu.moger@amd.com>, 
    =?ISO-8859-15?Q?Maciej_Wiecz=F3r-Retman?= <maciej.wieczor-retman@intel.com>, 
    LKML <linux-kernel@vger.kernel.org>, Fenghua Yu <fenghua.yu@intel.com>, 
    Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH v4 13/16] selftests/resctrl: Convert ctrlgrp & mongrp to
 pointers
In-Reply-To: <bafcfe0c-e052-426f-be0d-9bcbb4c16a62@intel.com>
Message-ID: <a14be305-67fc-7c92-3e40-0168084171d6@linux.intel.com>
References: <20240520123020.18938-1-ilpo.jarvinen@linux.intel.com> <20240520123020.18938-14-ilpo.jarvinen@linux.intel.com> <bafcfe0c-e052-426f-be0d-9bcbb4c16a62@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323328-1151364960-1717069497=:1113"

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1151364960-1717069497=:1113
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Wed, 29 May 2024, Reinette Chatre wrote:

> Hi Ilpo,
>=20
> On 5/20/24 5:30 AM, Ilpo J=C3=A4rvinen 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().
> >=20
> > 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.
> >=20
> > 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).
> >=20
> > Signed-off-by: Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >=20
> > 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(-)
> >=20
> > 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 {
> >   =09char=09=09*resctrl_val;
> > -=09char=09=09ctrlgrp[64];
> > -=09char=09=09mongrp[64];
> > +=09const char=09*ctrlgrp;
> > +=09const char=09*mongrp;
> >   =09char=09=09filename[64];
> >   =09unsigned long=09mask;
> >   =09int=09=09num_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)
> >   =09struct dirent *ep;
> >   =09DIR *dp;
> >   -=09/*
> > -=09 * At this point, we are guaranteed to have resctrl FS mounted and =
if
> > -=09 * length of grp_name =3D=3D 0, it means, user wants to use root co=
n_mon
> > -=09 * grp, so do nothing
> > -=09 */
> > -=09if (strlen(grp_name) =3D=3D 0)
> > -=09=09return 0;
> > +=09if (!grp_name)
> > +=09=09return -1;
>=20
> As I said during review of v2, this should not be an error. I went back t=
o
> read
> your comments and the argument that this is done for benefit of API is un=
clear
> 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=
=20
group because it's not used anywhere in the selftest code that always=20
provides a ctrlgrp name.

I'll change it to return 0.

--=20
 i.

--8323328-1151364960-1717069497=:1113--