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

[PATCH 1/4] 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.

CC: 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 0bcdd1a1e..4c45eae80 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;
-- 

Dobjátok a Dunába a kis diktátort.

2.48.1

[PATCH 2/4] 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

CC: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
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 4c45eae80..8ed4d69f3 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"));
-- 

Dobjátok a Dunába a kis diktátort.

2.48.1

[PATCH 3/4] 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, and do not annoyingly
suggest that the user try `chrt --help`.

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

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index 8ed4d69f3..f5ecae6e1 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -507,11 +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"));
-		errtryhelp(EXIT_FAILURE);
-	}
+	if (argc - optind < (ctl->pid > -1 ? 1 : 2))
+		errx(EXIT_FAILURE, _("too few arguments"));
 
 	/* pid exists but priority not given */
 	if (ctl->pid > -1 && argc - optind == 1) {
@@ -530,11 +527,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))
-- 

Dobjátok a Dunába a kis diktátort.

2.48.1

[PATCH 4/4] 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 f5ecae6e1..f358bb273 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,18 +503,20 @@ int main(int argc, char **argv)
 		}
 	}
 
-	if (argc - optind < (ctl->pid > -1 ? 1 : 2))
+	if (argc - optind < (ctl->pid == 0 ? 1 : 2))
 		errx(EXIT_FAILURE, _("too few arguments"));
 
-	/* 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;
 		}
-- 

Dobjátok a Dunába a kis diktátort.

2.48.1

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

[Madadi Vineeth Reddy]

Hi Benno,

On 30/06/25 14:10, Benno Schulenberg wrote:
> 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.
> 
> CC: 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 0bcdd1a1e..4c45eae80 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;

LGTM.

Without this patch:
chrt 12 grep boo README
chrt: unsupported priority value for the policy: 0: see --max for valid range

With this patch:
chrt 12 grep boo README
      See: http://vger.kernel.org/majordomo-info.html#taboo

Reviewed-by: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
Tested-by: Madadi Vineeth Reddy <vineethr@linux.ibm.com>

Thanks,
Madadi Vineeth Reddy

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

[Madadi Vineeth Reddy]

On 30/06/25 14:10, Benno Schulenberg wrote:
> 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

IMO, the current message is already helpful, and I'm not sure
the proposed one is much clearer.

Maybe something like --pid requires an argument would be clearer?

Also, I noticed that currently more than one pid can't be passed
if someone wants to update the custom slice for multiple pids at
once. I can look into adding support for that if it's helpful.

Thanks,
Madadi Vineeth Reddy

> 
> CC: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
> 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 4c45eae80..8ed4d69f3 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"));

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

[Madadi Vineeth Reddy]

On 30/06/25 14:10, 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.)
> 
> Also, make the error message more informative, and do not annoyingly
> suggest that the user try `chrt --help`.

Nit: I think we could still have "Try 'chrt --help' for more information."
along with your "too few arguments" so that user knows exactly how
many arguments are needed.

As is also is fine.

Reviewed-by: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
Tested-by: Madadi Vineeth Reddy <vineethr@linux.ibm.com>

Thanks,
Madadi Vineeth Reddy

> 
> CC: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
> Signed-off-by: Benno Schulenberg <bensberg@telfort.nl>
> ---
>  schedutils/chrt.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/schedutils/chrt.c b/schedutils/chrt.c
> index 8ed4d69f3..f5ecae6e1 100644
> --- a/schedutils/chrt.c
> +++ b/schedutils/chrt.c
> @@ -507,11 +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"));
> -		errtryhelp(EXIT_FAILURE);
> -	}
> +	if (argc - optind < (ctl->pid > -1 ? 1 : 2))
> +		errx(EXIT_FAILURE, _("too few arguments"));
>  
>  	/* pid exists but priority not given */
>  	if (ctl->pid > -1 && argc - optind == 1) {
> @@ -530,11 +527,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] chrt: do not try to interpret any other option as a PID either

[Madadi Vineeth Reddy]

On 30/06/25 14:10, Benno Schulenberg wrote:
> When doing, for example, `chrt --pid --max`, it would report:
> 
>   chrt: invalid PID argument: '--max'
> 

But --max is part of options right?

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

It should come before --pid right?

Thanks,
Madadi Vineeth Reddy

> 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 f5ecae6e1..f358bb273 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,18 +503,20 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	if (argc - optind < (ctl->pid > -1 ? 1 : 2))
> +	if (argc - optind < (ctl->pid == 0 ? 1 : 2))
>  		errx(EXIT_FAILURE, _("too few arguments"));
>  
> -	/* 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 2/4] chrt: do not try to interpret the --pid option itself as a PID

[Benno Schulenberg]

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


Hello Vineeth,

Op 01-07-2025 om 07:05 schreef Madadi Vineeth Reddy:
> On 30/06/25 14:10, Benno Schulenberg wrote:
>> 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
> 
> IMO, the current message is already helpful, and I'm not sure
> the proposed one is much clearer.
> 
> Maybe something like --pid requires an argument would be clearer?

There's no need for that, because when the user specified -p or --pid
without any further argument, then saying "too few arguments" will be
enough.  No need to spell it out.  The advantage of this short message
is that it allows folding the messages for "too few arguments" into a
single one in the overnext commit.  Furthermore, the message is already
used in a few other util-linux tools, so it won't burden translators.

> Also, I noticed that currently more than one pid can't be passed
> if someone wants to update the custom slice for multiple pids at
> once. I can look into adding support for that if it's helpful.

I don't think that is a good idea: the current interface is already
confusing in that _gets_ info when a single argument follows --pid,
and that it _sets_ something when there a two arguments.  Allowing
to set something for multiple PIDs at once while it's not possible
to query multiple PIDs at once... is more confusing.


Benno


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

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

[Benno Schulenberg]

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


Op 01-07-2025 om 07:16 schreef Madadi Vineeth Reddy:
> Nit: I think we could still have "Try 'chrt --help' for more information."
> along with your "too few arguments" so that user knows exactly how
> many arguments are needed.

The reason I don't want to see "Try 'chrt --help' for more information."
when I make a mistake is that it means that I have to read two lines:
the error message plus the "Try...".  It takes me more time, _and_ the
second line doesn't add any information -- it just states the obvious.
It reminds me of Clippy: so eager to help that it becomes obnoxious.


Benno


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

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

[Benno Schulenberg]

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


Op 01-07-2025 om 07:21 schreef Madadi Vineeth Reddy:
> On 30/06/25 14:10, Benno Schulenberg wrote:
>> When doing, for example, `chrt --pid --max`, it would report:
>>
>>    chrt: invalid PID argument: '--max'
>>
> 
> But --max is part of options right?
> 
> According to help text,
> chrt [options] --pid <priority> <pid>
> 
> It should come before --pid right?

Sure.  But the synopses of a command often don't (or even cannot)
fully express all possible arrangements of options and arguments.
So the user ought to be forgiven (when possible) for trying some
unspecified order.

(When I forget the exact invocation of a tool, I normally tack a
--help onto the previous attempt, and this usually prints the help
text, ignoring any other options.  It would be nice if that worked
here too.)

Also, the '--max' is obviously an option, why try to interpret it
as a PID?  Why make the tool obtuser than it needs to be?


Benno


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

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

[Karel Zak]

On Tue, Jul 01, 2025 at 03:34:40PM +0200, Benno Schulenberg wrote:
> I don't think that is a good idea: the current interface is already
> confusing in that _gets_ info when a single argument follows --pid,
> and that it _sets_ something when there a two arguments.  Allowing
> to set something for multiple PIDs at once while it's not possible
> to query multiple PIDs at once... is more confusing.

I agree, the schedutils UI is quite confusing. From time to time, I think  
about a new tool that will combine all the tools into one (setsched,  
lssched?) with a user-friendly interface.

    Karel

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

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

[Karel Zak]

On Tue, Jul 01, 2025 at 03:39:57PM +0200, Benno Schulenberg wrote:
> 
> Op 01-07-2025 om 07:16 schreef Madadi Vineeth Reddy:
> > Nit: I think we could still have "Try 'chrt --help' for more information."
> > along with your "too few arguments" so that user knows exactly how
> > many arguments are needed.
> 
> The reason I don't want to see "Try 'chrt --help' for more information."
> when I make a mistake is that it means that I have to read two lines:

We should be consistent. The `errtryhelp()` function is usually used as the  
default error for an unknown option or when the user is "lost" on the command  
line, for example, if not all required arguments are used.

  warnx(_("too few arguments"));  
  errtryhelp(EXIT_FAILURE);

(or "bad usage") is consistent with the rest of util-linux.

I can imagine a new  function `errclimess(EXIT_FAILURE)` (or a better
name ;-) for exactly this case  to avoid creativity in code. We can
use it everywhere and keep the error message  on one line

    "unexpected number of arguments, try chrt --help"

or so. Would this be a good compromise?

> the error message plus the "Try...".  It takes me more time, _and_ the
> second line doesn't add any information -- it just states the obvious.

It does not have to be obvious to all users, -? -h, -H, --help, sometimes
nothing, etc. 

    Karel


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

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

[Benno Schulenberg]

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


Op 02-07-2025 om 11:01 schreef Karel Zak:
> On Tue, Jul 01, 2025 at 03:39:57PM +0200, Benno Schulenberg wrote:
>> The reason I don't want to see "Try 'chrt --help' for more information."
>> when I make a mistake is that it means that I have to read two lines:
> 
> We should be consistent. The `errtryhelp()` function is usually used as the
> default error for an unknown option or when the user is "lost" on the command
> line, for example, if not all required arguments are used.
> 
>    warnx(_("too few arguments"));
>    errtryhelp(EXIT_FAILURE);
> 
> (or "bad usage") is consistent with the rest of util-linux.

After grepping for -B1 errtryhelp, I now see that this is the style
in the util-linux tools.  I'm sorry to realize that.  :/

>> the
>> second line doesn't add any information -- it just states the obvious.
> 
> It does not have to be obvious to all users, -? -h, -H, --help, sometimes
> nothing, etc.

As far as I can tell, all the util-linux tools understand --help.
So, for util-linux tools, it is rather redundant to tell the user
this.

(There are just three tools that don't understand the short form
-h for help: agettty, col, and setttetm.  fsck does not officially
support -h, but prints a help text anyway, from e2fsprogs.)


Benno


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

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

[Karel Zak]

On Wed, Jul 02, 2025 at 03:39:53PM +0200, Benno Schulenberg wrote:
> As far as I can tell, all the util-linux tools understand --help.
> So, for util-linux tools, it is rather redundant to tell the user
> this.

I mean when users use it on the command line and mix tools from different  
projects and sources. 

    Karel


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