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

[Richard Palethorpe <rpalethorpe@suse.de>]

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.

>
> 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.

>
>
>> 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.

>
> \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.

>
> 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.

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

> 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


-- 
Thank you,
Richard.

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