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

[Petr Vorel <pvorel@suse.cz>]

> Hello,

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

> > Hi Richie,

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> >> V2:
> >> * Add underscore
> >> * Add length check
> >> * Expand commit message
> >> * Use shorter syntax
> > +1

> >>  lib/tst_cgroup.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)

> >> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> >> index 5240aadaa..c5cb20505 100644
> >> --- a/lib/tst_cgroup.c
> >> +++ b/lib/tst_cgroup.c
> >> @@ -433,9 +433,20 @@ __attribute__ ((nonnull, warn_unused_result))
> >>  static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name)
> >>  {
> >>  	struct cgroup_ctrl *ctrl;
> >> +	int l = 0;
> >> +	char c = ctrl_name[l];
> >> +
> >> +	while (c == '_' || (c >= 'a' && c <= 'z'))
> >> +		c = ctrl_name[++l];
> >> +
> >> +	if (l > 32)
> > +1 for this check.
> > I would slightly prefer to add MAX_CGROUP_TYPE_NAMELEN definition into our
> > include/tst_cgroup.h, but it can stay as is.

> > BTW include/linux/cgroup-defs.h contains also MAX_CGROUP_ROOT_NAMELEN, there are
> > also other checks in C files (CGROUP_FILE_NAME_MAX) in case it makes sense to
> > have some checks for these (I have no idea).

> I'm not sure about this. On the one hand it may detect a bug in the
> library or kernel. On the other it's not exposed to userland or defined
> anywhere.

Yep, it's just an idea, I'm not sure myself.

> For me the main thing is to avoid janitorial work like creating and
> merging patches just to increase some internal kernel value that was
> increased or is ignored in some corner case.

+1

> I'll post another patch if there is some obvious way to check the root
> name length that would help the library.

+1

> Thanks for the review, I'll merge this now.

Great.

Kind regards,
Petr

> > Kind regards,
> > Petr

> >> +		tst_res(TWARN, "Subsys name len greater than max known value of MAX_CGROUP_TYPE_NAMELEN: %d > 32", l);

> >> +
> >> +	if (!(c == '\n' || c == '\0'))
> >> +		tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, c);

> >>  	for_each_ctrl(ctrl) {
> >> -		if (!strcmp(ctrl_name, ctrl->ctrl_name))
> >> +		if (!strncmp(ctrl_name, ctrl->ctrl_name, l))
> >>  			return ctrl;
> >>  	}

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