[PATCH 0/3] pull: chrt fixes

[PATCH 0/3] pull: chrt fixes

[Sami Kerola]

Hello,

This pull request comes a little late to be part of the upcoming release,
but perhaps at least the two first changes could be included.  If the
usability improvement is included the new translation string will not going
to have change being translated by v2.28 gets out.  Other than that this is
a trival change set.

----------------------------------------------------------------
The following changes since commit e9cd2e2bd9ec91a0b1050c4aa79555d142985924:
  libmount: Fix possible NULL dereference in get_btrfs_fs_root() (2016-03-31 11:45:55 +0200)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git chrt
for you to fetch changes up to cbeda6d13b98cc63abdd15639ff173b3c2bb8dad:
  chrt: validate priority before trying to use it (2016-04-03 09:26:20 +0100)
----------------------------------------------------------------

Sami Kerola (3):
  chrt: make --sched-* short options to require an argument
  bash-completion: update chrt completion
  chrt: validate priority before trying to use it

 bash-completion/chrt | 40 +++++++++++++++++++++++++++-------------
 schedutils/chrt.c    |  8 ++++++--
 2 files changed, 33 insertions(+), 15 deletions(-)

-- 
2.8.0

[PATCH 1/3] chrt: make --sched-* short options to require an argument

[Sami Kerola]

These options are expecting an argument, and the long options struct already
required them.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 schedutils/chrt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index 363d6e1..64a5858 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -399,7 +399,7 @@ int main(int argc, char **argv)
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while((c = getopt_long(argc, argv, "+abdDfiphmoPTrRvV", longopts, NULL)) != -1)
+	while((c = getopt_long(argc, argv, "+abdD:fiphmoP:T:rRvV", longopts, NULL)) != -1)
 	{
 		int ret = EXIT_FAILURE;
 
-- 
2.8.0

Re: [PATCH 1/3] chrt: make --sched-* short options to require an argument

[Karel Zak]

On Sun, Apr 03, 2016 at 09:35:27AM +0100, Sami Kerola wrote:
> These options are expecting an argument, and the long options struct already
> required them.

 Good catch, but deadline and runtime should be optional arguments (it
 seems I have merged wrong version of my patch:-(

    Karel


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

Re: [PATCH 1/3] chrt: make --sched-* short options to require an argument

[Karel Zak]

On Mon, Apr 04, 2016 at 02:21:00AM +0200, Karel Zak wrote:
> On Sun, Apr 03, 2016 at 09:35:27AM +0100, Sami Kerola wrote:
> > These options are expecting an argument, and the long options struct already
> > required them.
> 
>  Good catch, but deadline and runtime should be optional arguments (it
>  seems I have merged wrong version of my patch:-(

Sorry, didn't read the patch correctly. You're right. Applied, thanks.

    Karel

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

Re: [PATCH 1/3] chrt: make --sched-* short options to require an argument

[Ruediger Meier]

On Monday 04 April 2016, Karel Zak wrote:
> On Mon, Apr 04, 2016 at 02:21:00AM +0200, Karel Zak wrote:
> > On Sun, Apr 03, 2016 at 09:35:27AM +0100, Sami Kerola wrote:
> > > These options are expecting an argument, and the long options
> > > struct already required them.
> >
> >  Good catch, but deadline and runtime should be optional arguments
> > (it seems I have merged wrong version of my patch:-(
>
> Sorry, didn't read the patch correctly. You're right. Applied,
> thanks.

BTW I like the coreutils convention that new options with optional 
arguments should be long options only. This is less confusing for the 
users.

cu,
Rudi

Re: [PATCH 1/3] chrt: make --sched-* short options to require an argument

[Sami Kerola]

On 4 April 2016 at 10:07, Ruediger Meier <sweet_f_a@gmx.de> wrote:
> On Monday 04 April 2016, Karel Zak wrote:
>> On Mon, Apr 04, 2016 at 02:21:00AM +0200, Karel Zak wrote:
>> > On Sun, Apr 03, 2016 at 09:35:27AM +0100, Sami Kerola wrote:
>> > > These options are expecting an argument, and the long options
>> > > struct already required them.
>> >
>> >  Good catch, but deadline and runtime should be optional arguments
>> > (it seems I have merged wrong version of my patch:-(
>>
>> Sorry, didn't read the patch correctly. You're right. Applied,
>> thanks.
>
> BTW I like the coreutils convention that new options with optional
> arguments should be long options only. This is less confusing for the
> users.

Makes sense, and if Karel agrees Documentation/* files need updating.

BTW the debian bug 791707 that was the reason why I even started to look
chrt is pretty good indication the command is not easy to use or understand.
To me the command is pretty unintuitive, for example

$ chrt --pid --rt 0 foobar $$

is perfectly OK way to change scheduling policy of the running shell, and
foobar is ignored.  I don't quite understand why the --pid makes only the
last argument to be used, while it could use all arguments the getopts() did
eat.  But even so the interface stays quirky as the argument order is

$ chrt <options> <priority> <target>

for example

$ chrt --pid 0 $$

But fixing that cannot be done without breaking ABI, and such change cannot
be approved.  So should the util-linux v2.29 (or perhaps v3.0 as the numbers
are getting a bit large) have new tools:

$ rtctl
$ lsrt

Where the former has expected interface using options as people normally see
them.  For example:

$ rtctl --priority 0 --policy rt --exec 'ls /etc' --pid $$

In above --exec or --pid can be used more than once, but 'settings' type options
should not be allowed more than one instance to avoid confusions.

And 'lsrt' would be for displaying the stuff rtctl is controlling.

Comments?

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 1/3] chrt: make --sched-* short options to require an argument

[Karel Zak]

On Mon, Apr 04, 2016 at 03:43:23PM +0100, Sami Kerola wrote:
> On 4 April 2016 at 10:07, Ruediger Meier <sweet_f_a@gmx.de> wrote:
> > On Monday 04 April 2016, Karel Zak wrote:
> >> On Mon, Apr 04, 2016 at 02:21:00AM +0200, Karel Zak wrote:
> >> > On Sun, Apr 03, 2016 at 09:35:27AM +0100, Sami Kerola wrote:
> >> > > These options are expecting an argument, and the long options
> >> > > struct already required them.
> >> >
> >> >  Good catch, but deadline and runtime should be optional arguments
> >> > (it seems I have merged wrong version of my patch:-(
> >>
> >> Sorry, didn't read the patch correctly. You're right. Applied,
> >> thanks.
> >
> > BTW I like the coreutils convention that new options with optional
> > arguments should be long options only. This is less confusing for the
> > users.
> 
> Makes sense, and if Karel agrees Documentation/* files need updating.

Go ahead.

> BTW the debian bug 791707 that was the reason why I even started to look
> chrt is pretty good indication the command is not easy to use or understand.

All sched-utils has horrible command line, if I good remember we
have already tried to improve things, but unfortunately things are
limited by backward compatibility.

> But fixing that cannot be done without breaking ABI, and such change cannot
> be approved.  So should the util-linux v2.29 (or perhaps v3.0 as the numbers
> are getting a bit large) have new tools:
> 
> $ rtctl
> $ lsrt
> 
> Where the former has expected interface using options as people normally see
> them.  For example:
> 
> $ rtctl --priority 0 --policy rt --exec 'ls /etc' --pid $$
> 
> In above --exec or --pid can be used more than once, but 'settings' type options
> should not be allowed more than one instance to avoid confusions.
> 
> And 'lsrt' would be for displaying the stuff rtctl is controlling.

I have already thought about ls-like util for scheduling stuff and it
seems like a good idea. Not sure if we really need rtctl (I hate the
name, what about 'sched' :-)

    Karel

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

[PATCH 2/3] bash-completion: update chrt completion

[Sami Kerola]

Add couple missing options, and make the completion overall work better.

That said completion is still incomplete, pardon the pun.  After user has
specified policy then giving a hint what priority needs to be specified is
theoretically possible, but such hint is not given.  There does not seem to
be easy way to know when user wants stops specifying options and move to
defining priority in: chrt [options] [prio] [command|pid].

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 bash-completion/chrt | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/bash-completion/chrt b/bash-completion/chrt
index 388d298..ce0b915 100644
--- a/bash-completion/chrt
+++ b/bash-completion/chrt
@@ -8,31 +8,45 @@ _chrt_module()
 		'-h'|'--help'|'-V'|'--version')
 			return 0
 			;;
+		'-T'|'--sched-runtime'|'-P'|'--sched-period'|'-D'|'--sched-deadline')
+			COMPREPLY=( $(compgen -W "nanoseconds" -- $cur) )
+			return 0
+			;;
 	esac
-	# FIXME: -p is ambiguous, it takes either pid or priority as an
-	# argument depending on whether user wanted to get or set the
-	# values.  Perhaps the command interface should be reconsidered.
 	case $cur in
 		-*)
-			OPTS="--batch
-				--fifo
-				--idle
-				--other
-				--rr
-				--reset-on-fork
+			OPTS="
 				--all-tasks
+				--batch
+				--deadline
+				--fifo
 				--help
+				--idle
 				--max
+				--other
 				--pid
+				--reset-on-fork
+				--rr
+				--sched-deadline
+				--sched-period
+				--sched-runtime
 				--verbose
-				--version"
+				--version
+			"
 			COMPREPLY=( $(compgen -W "${OPTS[*]}" -- $cur) )
 			return 0
 			;;
 	esac
-	local PIDS
-	PIDS=$(for I in /proc/[0-9]*; do echo ${I##"/proc/"}; done)
-	COMPREPLY=( $(compgen -W "$PIDS" -- $cur) )
+	local i
+	for i in ${COMP_WORDS[*]}; do
+		case $i in
+		'-p'|'--pid')
+			COMPREPLY=( $(compgen -W "$(cd /proc && echo [0-9]*)" -- $cur) )
+			return 0
+			;;
+		esac
+	done
+	COMPREPLY=( $(compgen -c -- $cur) )
 	return 0
 }
 complete -F _chrt_module chrt
-- 
2.8.0

Re: [PATCH 2/3] bash-completion: update chrt completion

[Karel Zak]

On Sun, Apr 03, 2016 at 09:35:28AM +0100, Sami Kerola wrote:
>  bash-completion/chrt | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)

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

[PATCH 3/3] chrt: validate priority before trying to use it

[Sami Kerola]

Earlier message:

$ chrt -i 1 ls
chrt: failed to set pid 0's policy: Invalid argument

basically told 'something failed', while the new one tries to be more
helpful.

$ chrt -i 1 ls
chrt: unsupported priority value for the policy: 1: see --max for valid range

Addresses: https://bugs.debian.org/791707
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 schedutils/chrt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/schedutils/chrt.c b/schedutils/chrt.c
index 64a5858..202ce25 100644
--- a/schedutils/chrt.c
+++ b/schedutils/chrt.c
@@ -504,7 +504,11 @@ int main(int argc, char **argv)
 #endif
 	if (ctl->pid == -1)
 		ctl->pid = 0;
-
+	if (ctl->priority < sched_get_priority_min(ctl->policy) ||
+	    sched_get_priority_max(ctl->policy) < ctl->priority)
+		errx(EXIT_FAILURE,
+		     _("unsupported priority value for the policy: %d: see --max for valid range"),
+		     ctl->priority);
 	set_sched(ctl);
 
 	if (ctl->verbose)
-- 
2.8.0

Re: [PATCH 3/3] chrt: validate priority before trying to use it

[Karel Zak]

On Sun, Apr 03, 2016 at 09:35:29AM +0100, Sami Kerola wrote:
>  schedutils/chrt.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

 Too late for v2.28, applied to the "next" branch.

    Karel

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