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

[Petr Vorel <pvorel@suse.cz>]

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.

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.


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

I guess for cgroup v1 [1]

	The name should match [\w.-]+

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

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.

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