Re: [PATCH] chrt: when more than one argument, interpret first argument as priority

[Madadi Vineeth Reddy <vineethr@xxxxxxxxxxxxx>]

Hi Benno,

On 26/06/25 16:57, Benno Schulenberg wrote:
> The first argument is a priority not only for `chrt --pid <prio> <pid>`
> but also for `chrt <prio> <command> [<argument>...]`.
> 

Good catch. SCHED_RR is indeed taken by default when no policy is given.

We could address this by initializing `need_prio` to `true` by default,
and then setting it to `false` in the corresponding switch cases for
policies that don't require a priority.  

This change could be added to your patch like below:

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index 0bcdd1a1e..02b202ff4 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -399,7 +399,7 @@ int main(int argc, char **argv)
 {
        struct chrt_ctl _ctl = { .pid = -1, .policy = SCHED_RR }, *ctl = &_ctl;
        int c;
-       bool policy_given = false, need_prio = false;
+       bool policy_given = false, need_prio = true;
 
        static const struct option longopts[] = {
                { "all-tasks",  no_argument, NULL, 'a' },
@@ -437,6 +437,7 @@ int main(int argc, char **argv)
 #ifdef SCHED_BATCH
                        ctl->policy = SCHED_BATCH;
                        policy_given = true;
+                       need_prio = false;
 #endif
                        break;
 
@@ -444,18 +445,19 @@ int main(int argc, char **argv)
 #ifdef SCHED_DEADLINE
                        ctl->policy = SCHED_DEADLINE;
                        policy_given = true;
+                       need_prio = false;
 #endif
                        break;
                case 'e':
 #ifdef SCHED_EXT
                        ctl->policy = SCHED_EXT;
                        policy_given = true;
+                       need_prio = false;
 #endif
                        break;
                case 'f':
                        ctl->policy = SCHED_FIFO;
                        policy_given = true;
-                       need_prio = true;
                        break;
                case 'R':
                        ctl->reset_on_fork = 1;
@@ -464,6 +466,7 @@ int main(int argc, char **argv)
 #ifdef SCHED_IDLE
                        ctl->policy = SCHED_IDLE;
                        policy_given = true;
+                       need_prio = false;
 #endif
                        break;
                case 'm':
@@ -472,6 +475,7 @@ int main(int argc, char **argv)
                case 'o':
                        ctl->policy = SCHED_OTHER;
                        policy_given = true;
+                       need_prio = false;
                        break;
                case 'p':
                        errno = 0;
@@ -481,7 +485,6 @@ int main(int argc, char **argv)
                case 'r':
                        ctl->policy = SCHED_RR;
                        policy_given = true;
-                       need_prio = true;
                        break;
                case 'v':
                        ctl->verbose = 1;
@@ -530,7 +533,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;

Let me know if I am missing anything.

Thanks,
Madadi Vineeth Reddy

> This fixes an oversight in recent commit e7a2d62434.
> 
> CC: Madadi Vineeth Reddy <vineethr@xxxxxxxxxxxxx>
> Signed-off-by: Benno Schulenberg <bensberg@xxxxxxxxxx>
> ---
>  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;