Re: [LTP] [PATCH] cgroup: Handle trailing new line in cgroup.controllers

[Petr Vorel <pvorel@suse.cz>]

> Hello,

> Petr Vorel <pvorel@suse.cz> writes:

> > Hi Richie, Martchus,

> > @Richie, please add Fixes: tag when commit

> > I suppose it should be
> > Fixes: 310da3784 ("Add new CGroups APIs")
> > but please check yourself.

> Yes that is right.

+1


> > Why this is useful? It helps to identify which test failures were false
> > positives. Also, you actually not just fix a line character, but also do other
> > validation, it would be worth to mention that.

> The validation is primarily checking my assumptions. We don't want to
> cut the name off at a '_' then get more confusing errors in the future.

+1, looking into the code and description once more, it makes sense.


> >> Am Mittwoch, 25. Oktober 2023, 13:05:33 CEST schrieb Richard Palethorpe via 
> >> ltp:
> >> > +	switch (ctrl_name[l]) {
> >> > +	case '\n': break;
> >> > +	case '\0': break;
> >> > +	default:
> >> > +		tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, 
> >> ctrl_name[l]);

> >> I'm wondering whether that's a bit too restrictive. Or is there any official 
> >> documentation says that you really can only have the letters a-z in cgroup 
> >> names (and not even A-Z). Otherwise it might be better to make this just a 
> >> warning or allow any printable characters.

> Well I assumed there wasn't, but it seems this was actually thought
> about and specified to some extent. I should have scanned the docs.


> > I guess for cgroup v1 [1]

> > 	The name should match [\w.-]+

> Thats the name of the "hierarchy" AFAICT. I don't think that is the
> controller/subsystem name. Those characters would be a pain for naming
> things in C.

Ah, right, I'm sorry.

> > \w Matches a "word" character (alphanumeric plus "_", plus other connector
> > punctuation chars plus Unicode marks). Also '.' and '-' can be used.
> > => [A-Z.-] and others are valid names in v1. Although I'm not sure if
> > cgroup_find_ctrl() is used on systems with cgroups v1.

> None of the existing upstream controllers contain a _ unless we are
> missing one or more. However we should allow it, so I'll add it.

+1

> > Also, shouldn't we check with MAX_CGROUP_TYPE_NAMELEN:

> > 	- name: should be initialized to a unique subsystem name. Should be
> > 	  no longer than MAX_CGROUP_TYPE_NAMELEN.


> It's not exposed to userland or specified in the docs. I suppose I could
> issue a WARN if it is over 32 though. Mostly likely if that happens then
> there is a parsing error.

+1

> The kernel will also issue a WARN if the subsystem name is over 32.

Good. But warning in LTP will be more visible (not everybody parses dmesg).

Kind regards,
Petr

> > For cgroup v2 [2] it looks to be:

> > 	All cgroup core interface files are prefixed with "cgroup." and each
> > 	controller's interface files are prefixed with the controller name and a
> > 	dot. A controller's name is composed of lower case alphabets and '_'s but
> > 	never begins with an '_' so it can be used as the prefix character for
> > 	collision avoidance. Also, interface file names won't start or end with
> > 	terms which are often used in categorizing workloads such as job, service,
> > 	slice, unit or workload.

> > => It matches ^[a-z][a-z_]. At least "_" is missing. Also this validation should
> > specify somewhere if it's for v2 only or for both.

> > Kind regards,
> > Petr

> > [1] https://www.kernel.org/doc/Documentation/cgroup-v1/cgroups.txt
> > [2] https://docs.kernel.org/admin-guide/cgroup-v2.html#avoid-name-collisions

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp