linux-kernel - Re: [PATCH v2 2/3] rtla: fix -C/--cgroup interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <jbf4si2tfwlquzna4sxbemr53ecwo3xwuroi5als5vlk4vla72@trp4p7jxw2ue>
Date: Tue, 2 Sep 2025 14:09:33 -0400
From: Ivan Pravdin <ipravdin.official@...il.com>
To: Crystal Wood <crwood@...hat.com>, rostedt@...dmis.org, corbet@....net, 
	tglozar@...hat.com, linux-trace-kernel@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] rtla: fix -C/--cgroup interface

On Fri, Aug 29, 2025 at 02:12:28PM -0500, Crystal Wood wrote:
> On Tue, 2025-08-12 at 13:21 -0400, Ivan Pravdin wrote:
> > Currently, user can only specify cgroup to the tracer's thread the
> > following ways:
> > 
> >     `-C[cgroup]`
> >     `-C[=cgroup]`
> >     `--cgroup[=cgroup]`
> > 
> > If user tries to specify cgroup as `-C [cgroup]` or `--cgroup [cgroup]`,
> > the parser silently fails and rtla's cgroup is used for the tracer
> > threads.
> > 
> > To make interface more user-friendly, allow user to specify cgroup in
> > the aforementioned way, i.e. `-C [cgroup]` and `--cgroup [cgroup]`
> > 
> > Change documentation to reflect this user interface change.
> 
> I know these are the semantics that --trace implements, but they're
> rather atypical... especially -C=group.

The new semantics allow for -C [group] which is the same as -t
[filename] that has been fixed for -t/--trace.

> 
> > @@ -559,12 +559,17 @@ static struct osnoise_params
> >  			break;
> >  		case 'C':
> >  			params->cgroup = 1;
> > -			if (!optarg) {
> > -				/* will inherit this cgroup */
> > +			if (optarg) {
> > +				if (optarg[0] == '=') {
> > +					/* skip the = */
> > +					params->cgroup_name = &optarg[1];
> > +				} else {
> > +					params->cgroup_name = optarg;
> > +				}
> > +			} else if (optind < argc && argv[optind][0] != '-') {
> > +				params->cgroup_name = argv[optind];
> > +			} else {
> >  				params->cgroup_name = NULL;
> > -			} else if (*optarg == '=') {
> > -				/* skip the = */
> > -				params->cgroup_name = ++optarg;
> 
> If we're going to be consistently using these semantics, we should move
> this logic into a utility function rather than open-coding it
> everywhere.

Sure, I will move it into a utility function in the next version.
 
> Also, theoretically, shouldn't we be advancing optind for the case where
> that's consumed?  Not that it matters much if we don't have positional
> arguments once the options begin, and if we did, then allowing
> "--arg optional-thing" would be ambiguous...

It does seem that we should advance optind when optional argument is
provided. Good catch. I will fix it in the next version.

> -Crystal
> 

	Ivan Pravdin