[PATCH 1/4 V2] chrt: with more than one argument, interpret first argument as priority

[PATCH 1/4 V2] chrt: with more than one argument, interpret first argument as priority

[Benno Schulenberg]

The first argument is a priority not only for `chrt --pid <prio> <pid>`
but also for `chrt <prio> <command> [<argument>...]`.

This fixes an oversight in recent commit e7a2d62434.

Reviewed-by: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
Tested-by: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
Signed-off-by: Benno Schulenberg <bensberg@telfort.nl>
---
 schedutils/chrt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index 550cefe4d..415a9aa77 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -530,7 +530,7 @@ int main(int argc, char **argv)
 
 	errno = 0;
 
-	if (need_prio || argc - optind == 2)
+	if (need_prio || argc - optind > 1)
 		ctl->priority = strtos32_or_err(argv[optind], _("invalid priority argument"));
 	else
 		ctl->priority = 0;
-- 
2.48.1

[PATCH 2/4 V2] chrt: do not try to interpret the --pid option itself as a PID

[Benno Schulenberg]

When not specifying a PID with --pid, `chrt` would report:

  chrt: invalid PID argument: '--pid'

That was silly.  After this change, `chrt --pid` will report:

  chrt: too few arguments

Signed-off-by: Benno Schulenberg <bensberg@telfort.nl>
---
 schedutils/chrt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index 415a9aa77..a72c0de26 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -474,6 +474,8 @@ int main(int argc, char **argv)
 			policy_given = true;
 			break;
 		case 'p':
+			if (argc - optind == 0)
+				errx(EXIT_FAILURE, _("too few arguments"));
 			errno = 0;
 			/* strtopid_or_err() is not suitable here; 0 can be passed.*/
 			ctl->pid = strtos32_or_err(argv[argc - 1], _("invalid PID argument"));
-- 
2.48.1

[PATCH 3/4 V2] chrt: simplify the other check for too few arguments

[Benno Schulenberg]

Without option --pid, always at least two arguments are needed:
the <priority> value and a <command>.  (The 'need_prio' variable
is relevant only for the --pid case.)

Also, make the error message more informative.

CC: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
Signed-off-by: Benno Schulenberg <bensberg@telfort.nl>
---

V2: left in the call errtryhelp()

 schedutils/chrt.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index a72c0de26..731f995bb 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -507,9 +507,8 @@ int main(int argc, char **argv)
 		}
 	}
 
-	if (((ctl->pid > -1) && argc - optind < (need_prio ? 1 : 0)) ||
-	    ((ctl->pid == -1) && argc - optind < (need_prio ? 2 : 1))) {
-		warnx(_("bad usage"));
+	if (argc - optind < (ctl->pid > -1 ? 1 : 2)) {
+		warnx(_("too few arguments"));
 		errtryhelp(EXIT_FAILURE);
 	}
 
@@ -530,11 +529,10 @@ int main(int argc, char **argv)
 	if (ctl->verbose)
 		show_sched_info(ctl);
 
-	errno = 0;
-
-	if (need_prio || argc - optind > 1)
+	if (argc - optind > 1) {
+		errno = 0;
 		ctl->priority = strtos32_or_err(argv[optind], _("invalid priority argument"));
-	else
+	} else
 		ctl->priority = 0;
 
 	if (ctl->runtime && !supports_runtime_param(ctl->policy))
-- 
2.48.1

[PATCH 4/4 V2] chrt: do not try to interpret any other option as a PID either

[Benno Schulenberg]

When doing, for example, `chrt --pid --max`, it would report:

  chrt: invalid PID argument: '--max'

This mistakenly gave the impression that the PID argument has to follow
directly after the --pid option.

Avoid this by delaying the parsing of a PID until after all options have
been parsed.  Temporarily set 'ctl->pid' to zero to indicate that a PID
needs to be read.

After this change, `chrt --pid --max` will simply report the minimum and
maximum valid priorities.  And `chrt --pid -v`:

  chrt: too few arguments

Also, add a missing call of gettext() for the other error message.

CC: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
Signed-off-by: Benno Schulenberg <bensberg@telfort.nl>
---
 schedutils/chrt.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index 731f995bb..10ba7fbf6 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -474,11 +474,7 @@ int main(int argc, char **argv)
 			policy_given = true;
 			break;
 		case 'p':
-			if (argc - optind == 0)
-				errx(EXIT_FAILURE, _("too few arguments"));
-			errno = 0;
-			/* strtopid_or_err() is not suitable here; 0 can be passed.*/
-			ctl->pid = strtos32_or_err(argv[argc - 1], _("invalid PID argument"));
+			ctl->pid = 0;  /* indicate that a PID is expected */
 			break;
 		case 'r':
 			ctl->policy = SCHED_RR;
@@ -507,20 +503,22 @@ int main(int argc, char **argv)
 		}
 	}
 
-	if (argc - optind < (ctl->pid > -1 ? 1 : 2)) {
+	if (argc - optind < (ctl->pid == 0 ? 1 : 2)) {
 		warnx(_("too few arguments"));
 		errtryhelp(EXIT_FAILURE);
 	}
 
-	/* pid exists but priority not given */
-	if (ctl->pid > -1 && argc - optind == 1) {
-		/* Error if priority is missing for a policy that requires it */
-		if (policy_given && need_prio)
-			errx(EXIT_FAILURE, ("policy %s requires a priority argument"),
+	/* If option --pid was given, parse the very last argument as a PID. */
+	if (ctl->pid == 0) {
+		if (need_prio && argc - optind < 2)
+			errx(EXIT_FAILURE, _("policy %s requires a priority argument"),
 						get_policy_name(ctl->policy));
+		errno = 0;
+		/* strtopid_or_err() is not suitable here, as 0 can be passed. */
+		ctl->pid = strtos32_or_err(argv[argc - 1], _("invalid PID argument"));
 
-		/* If no policy specified, show current settings */
-		if (!policy_given) {
+		/* If no policy nor priority was given, show current settings. */
+		if (!policy_given && argc - optind == 1) {
 			show_sched_info(ctl);
 			return EXIT_SUCCESS;
 		}
-- 
2.48.1

Re: [PATCH 3/4 V2] chrt: simplify the other check for too few arguments

[Madadi Vineeth Reddy]

On 03/07/25 20:17, Benno Schulenberg wrote:
> Without option --pid, always at least two arguments are needed:
> the <priority> value and a <command>.  (The 'need_prio' variable
> is relevant only for the --pid case.)

Yes, need_prio is only relevant for the --pid case as of now.

But priority should also be optional when --pid is not used, for
policies that don’t require it.

With your patch, this fails:
chrt --other ls
chrt: too few arguments
Try 'chrt --help' for more information.

while this works:
chrt --other 0 ls

The first case should work too, to match the behavior introduced in
commit e7a2d62434c2 ("chrt: Make priority optional for policies that don't use it").

Thanks,
Madadi Vineeth Reddy.

> 
> Also, make the error message more informative.
> 
> CC: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
> Signed-off-by: Benno Schulenberg <bensberg@telfort.nl>
> ---
> 
> V2: left in the call errtryhelp()
> 
>  schedutils/chrt.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/schedutils/chrt.c b/schedutils/chrt.c
> index a72c0de26..731f995bb 100644
> --- a/schedutils/chrt.c
> +++ b/schedutils/chrt.c
> @@ -507,9 +507,8 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	if (((ctl->pid > -1) && argc - optind < (need_prio ? 1 : 0)) ||
> -	    ((ctl->pid == -1) && argc - optind < (need_prio ? 2 : 1))) {
> -		warnx(_("bad usage"));
> +	if (argc - optind < (ctl->pid > -1 ? 1 : 2)) {
> +		warnx(_("too few arguments"));
>  		errtryhelp(EXIT_FAILURE);
>  	}
>  
> @@ -530,11 +529,10 @@ int main(int argc, char **argv)
>  	if (ctl->verbose)
>  		show_sched_info(ctl);
>  
> -	errno = 0;
> -
> -	if (need_prio || argc - optind > 1)
> +	if (argc - optind > 1) {
> +		errno = 0;
>  		ctl->priority = strtos32_or_err(argv[optind], _("invalid priority argument"));
> -	else
> +	} else
>  		ctl->priority = 0;
>  
>  	if (ctl->runtime && !supports_runtime_param(ctl->policy))

Re: [PATCH 4/4 V2] chrt: do not try to interpret any other option as a PID either

[Madadi Vineeth Reddy]

On 03/07/25 20:17, Benno Schulenberg wrote:
> When doing, for example, `chrt --pid --max`, it would report:
> 
>   chrt: invalid PID argument: '--max'
> 
> This mistakenly gave the impression that the PID argument has to follow
> directly after the --pid option.
> 
> Avoid this by delaying the parsing of a PID until after all options have
> been parsed.  Temporarily set 'ctl->pid' to zero to indicate that a PID
> needs to be read.
> 
> After this change, `chrt --pid --max` will simply report the minimum and
> maximum valid priorities.  And `chrt --pid -v`:

According to help text,
chrt [options] --pid <priority> <pid>

I’m still not sure about allowing the --max option after --pid.
I’ll leave it to Karel to decide what’s best here.

Thanks,
Madadi Vineeth Reddy

> 
>   chrt: too few arguments
> 
> Also, add a missing call of gettext() for the other error message.
> 
> CC: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
> Signed-off-by: Benno Schulenberg <bensberg@telfort.nl>
> ---
>  schedutils/chrt.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/schedutils/chrt.c b/schedutils/chrt.c
> index 731f995bb..10ba7fbf6 100644
> --- a/schedutils/chrt.c
> +++ b/schedutils/chrt.c
> @@ -474,11 +474,7 @@ int main(int argc, char **argv)
>  			policy_given = true;
>  			break;
>  		case 'p':
> -			if (argc - optind == 0)
> -				errx(EXIT_FAILURE, _("too few arguments"));
> -			errno = 0;
> -			/* strtopid_or_err() is not suitable here; 0 can be passed.*/
> -			ctl->pid = strtos32_or_err(argv[argc - 1], _("invalid PID argument"));
> +			ctl->pid = 0;  /* indicate that a PID is expected */
>  			break;
>  		case 'r':
>  			ctl->policy = SCHED_RR;
> @@ -507,20 +503,22 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	if (argc - optind < (ctl->pid > -1 ? 1 : 2)) {
> +	if (argc - optind < (ctl->pid == 0 ? 1 : 2)) {
>  		warnx(_("too few arguments"));
>  		errtryhelp(EXIT_FAILURE);
>  	}
>  
> -	/* pid exists but priority not given */
> -	if (ctl->pid > -1 && argc - optind == 1) {
> -		/* Error if priority is missing for a policy that requires it */
> -		if (policy_given && need_prio)
> -			errx(EXIT_FAILURE, ("policy %s requires a priority argument"),
> +	/* If option --pid was given, parse the very last argument as a PID. */
> +	if (ctl->pid == 0) {
> +		if (need_prio && argc - optind < 2)
> +			errx(EXIT_FAILURE, _("policy %s requires a priority argument"),
>  						get_policy_name(ctl->policy));
> +		errno = 0;
> +		/* strtopid_or_err() is not suitable here, as 0 can be passed. */
> +		ctl->pid = strtos32_or_err(argv[argc - 1], _("invalid PID argument"));
>  
> -		/* If no policy specified, show current settings */
> -		if (!policy_given) {
> +		/* If no policy nor priority was given, show current settings. */
> +		if (!policy_given && argc - optind == 1) {
>  			show_sched_info(ctl);
>  			return EXIT_SUCCESS;
>  		}

Re: [PATCH 3/4 V2] chrt: simplify the other check for too few arguments

[Benno Schulenberg]

[-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --]


Op 06-07-2025 om 07:58 schreef Madadi Vineeth Reddy:
> But priority should also be optional when --pid is not used, for
> policies that don’t require it.

Oh...  Then the synopses need adjusting, because currently they
indicate the priority value as mandatory when a command is given:

   chrt [options] priority command [argument...]


> With your patch, this fails:
> chrt --other ls
> chrt: too few arguments
> Try 'chrt --help' for more information.
> 
> while this works:
> chrt --other 0 ls
> 
> The first case should work too, to match the behavior introduced in
> commit e7a2d62434c2 ("chrt: Make priority optional for policies that don't use it").

With current git (or after checking out that commit), and then running
`./chrt --other ls`, it says here:

   Segmentatiefout (geheugendump gemaakt)

Running valgrind on it gives:

   ==38370== Invalid read of size 1
   ==38370==    by 0x10C6EB: main (chrt.c:574)

Pointing to: if (strcmp(argv[0], "--") == 0)


Benno


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

chrt from git segfaults

[Benno Schulenberg]

[-- Attachment #1.1: Type: text/plain, Size: 510 bytes --]


Hello Vineeth,

Just in case you missed the bug report buried in the previous message:

> With current git, running `./chrt --other ls` says here:
> 
>    Segmentatiefout (geheugendump gemaakt)
> 
> Running valgrind on it gives:
> 
>    ==38370== Invalid read of size 1
>    ==38370==    by 0x10C6EB: main (chrt.c:574)
> 
> Pointing to: if (strcmp(argv[0], "--") == 0)

My patch will prevent that case from occurring.  But if instead it
should work, something needs fixing.


Benno

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: chrt from git segfaults

[Madadi Vineeth Reddy]

On 09/07/25 12:42, Benno Schulenberg wrote:
> 
> Hello Vineeth,
> 
> Just in case you missed the bug report buried in the previous message:
> 
>> With current git, running `./chrt --other ls` says here:
>>
>>    Segmentatiefout (geheugendump gemaakt)
>>
>> Running valgrind on it gives:
>>
>>    ==38370== Invalid read of size 1
>>    ==38370==    by 0x10C6EB: main (chrt.c:574)
>>
>> Pointing to: if (strcmp(argv[0], "--") == 0)
> 
> My patch will prevent that case from occurring.  But if instead it
> should work, something needs fixing.

Could you spin off a patch to make sure the priority argument is
optional for policies that don’t need it, even when --pid is not used?

Thanks,
Madadi Vineeth Reddy

> 
> 
> Benno

Re: chrt from git segfaults

[Benno Schulenberg]

[-- Attachment #1.1: Type: text/plain, Size: 247 bytes --]


Op 09-07-2025 om 20:45 schreef Madadi Vineeth Reddy:
> Could you spin off a patch to make sure the priority argument is
> optional for policies that don’t need it, even when --pid is not used?

No.  You broke it, you fix it.


Benno


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: chrt from git segfaults

[Madadi Vineeth Reddy]

On 10/07/25 13:41, Benno Schulenberg wrote:
> 
> Op 09-07-2025 om 20:45 schreef Madadi Vineeth Reddy:
>> Could you spin off a patch to make sure the priority argument is
>> optional for policies that don’t need it, even when --pid is not used?
> 
> No.  You broke it, you fix it.
> 

Sure, I’ll send a patch. Thanks.

Thanks,
Madadi Vineeth Reddy

> 
> Benno
> 

Re: [PATCH 4/4 V2] chrt: do not try to interpret any other option as a PID either

[Karel Zak]

On Sun, Jul 06, 2025 at 11:39:44AM +0530, Madadi Vineeth Reddy wrote:
> On 03/07/25 20:17, Benno Schulenberg wrote:
> > When doing, for example, `chrt --pid --max`, it would report:
> > 
> >   chrt: invalid PID argument: '--max'
> > 
> > This mistakenly gave the impression that the PID argument has to follow
> > directly after the --pid option.
> > 
> > Avoid this by delaying the parsing of a PID until after all options have
> > been parsed.  Temporarily set 'ctl->pid' to zero to indicate that a PID
> > needs to be read.
> > 
> > After this change, `chrt --pid --max` will simply report the minimum and
> > maximum valid priorities.  And `chrt --pid -v`:
> 
> According to help text,
> chrt [options] --pid <priority> <pid>
> 
> I’m still not sure about allowing the --max option after --pid.
> I’ll leave it to Karel to decide what’s best here.

My point of view (may be wrong, sched-utils CLI is odd). The core of  
the chrt command line can be described as:

   chrt <options> [<prio>] <process>

where <process> is

    1) new process by exec()
    2) already running process

The default is 1), --pid enables the 2) and in this case  
<process> is PID.

The --pid is just an option to switch between the cases. There is no  
argument for the option; it informs how to interpret the <process>.

The ideal would be to use "--" before <process>.

It means that arbitrary options could be after --pid, including --max.

Does it make sense?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: chrt from git segfaults

[Karel Zak]

On Thu, Jul 10, 2025 at 10:11:58AM +0200, Benno Schulenberg wrote:
> 
> Op 09-07-2025 om 20:45 schreef Madadi Vineeth Reddy:
> > Could you spin off a patch to make sure the priority argument is
> > optional for policies that don’t need it, even when --pid is not used?
> 
> No.  You broke it, you fix it.

Please, please, don't forget to finalize it to avoid releasing it
incomplete ;-)

I have merged Benno's V2 set of patches, so nothing else is in the
queue for now.

Thanks guys!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: chrt from git segfaults

[Madadi Vineeth Reddy]

Hi Karel,

On 14/07/25 17:02, Karel Zak wrote:
> On Thu, Jul 10, 2025 at 10:11:58AM +0200, Benno Schulenberg wrote:
>>
>> Op 09-07-2025 om 20:45 schreef Madadi Vineeth Reddy:
>>> Could you spin off a patch to make sure the priority argument is
>>> optional for policies that don’t need it, even when --pid is not used?
>>
>> No.  You broke it, you fix it.
> 
> Please, please, don't forget to finalize it to avoid releasing it
> incomplete ;-)
> 
> I have merged Benno's V2 set of patches, so nothing else is in the
> queue for now.

I will send the patch to make the priority argument optional when
--pid is not used in the next couple of days. That should complete
the series.

Thanks,
Madadi Vineeth Reddy

> 
> Thanks guys!
> 
>     Karel
> 

Re: [PATCH 4/4 V2] chrt: do not try to interpret any other option as a PID either

[Madadi Vineeth Reddy]

On 14/07/25 16:58, Karel Zak wrote:
> On Sun, Jul 06, 2025 at 11:39:44AM +0530, Madadi Vineeth Reddy wrote:
>> On 03/07/25 20:17, Benno Schulenberg wrote:
>>> When doing, for example, `chrt --pid --max`, it would report:
>>>
>>>   chrt: invalid PID argument: '--max'
>>>
>>> This mistakenly gave the impression that the PID argument has to follow
>>> directly after the --pid option.
>>>
>>> Avoid this by delaying the parsing of a PID until after all options have
>>> been parsed.  Temporarily set 'ctl->pid' to zero to indicate that a PID
>>> needs to be read.
>>>
>>> After this change, `chrt --pid --max` will simply report the minimum and
>>> maximum valid priorities.  And `chrt --pid -v`:
>>
>> According to help text,
>> chrt [options] --pid <priority> <pid>
>>
>> I’m still not sure about allowing the --max option after --pid.
>> I’ll leave it to Karel to decide what’s best here.
> 
> My point of view (may be wrong, sched-utils CLI is odd). The core of  
> the chrt command line can be described as:
> 
>    chrt <options> [<prio>] <process>
> 
> where <process> is
> 
>     1) new process by exec()
>     2) already running process
> 
> The default is 1), --pid enables the 2) and in this case  
> <process> is PID.
> 
> The --pid is just an option to switch between the cases. There is no  
> argument for the option; it informs how to interpret the <process>.
> 
> The ideal would be to use "--" before <process>.
> 
> It means that arbitrary options could be after --pid, including --max.
> 
> Does it make sense?
> 

Yes. That makes sense.

Thanks,
Madadi Vineeth Reddy

>     Karel
> 

Re: chrt from git segfaults

[Benno Schulenberg]

[-- Attachment #1.1: Type: text/plain, Size: 221 bytes --]


Op 14-07-2025 om 13:32 schreef Karel Zak:
> I have merged Benno's V2 set of patches, so nothing else is in the
> queue for now.

I'm not seeing those in git yet.  Did you forget to push to kernel.org?


Benno


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: chrt from git segfaults

[Karel Zak]

On Tue, Jul 15, 2025 at 11:19:46AM +0200, Benno Schulenberg wrote:
> 
> Op 14-07-2025 om 13:32 schreef Karel Zak:
> > I have merged Benno's V2 set of patches, so nothing else is in the
> > queue for now.
> 
> I'm not seeing those in git yet.  Did you forget to push to kernel.org?

All should be on kernel.org and github.com now.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com