[Toybox] chrt.c cleanup

[Rob Landley]

On 05/22/2017 11:12 AM, enh wrote:
>> Do you care if somebody does "chrt -m ls -l"? Right now -m happens and
>> it ignores the rest of the command line.
> 
> seems like a bug.

I added a "chrt -rp 12345" test but left -m overriding. (The ubuntu one
doesn't notice either case.)

>> Except... ARGH! The man page says "-p [prio] pid". Really? REALLY? If I
>> supply -p, then prio becomes optional (without it the thing queries
>> instead of sets) but it's not "-p pid" doing normal #*%(&# option
>> parsing, it's got pid _after_ the optional priority. That's not how
>> option parsing works! (Can I do the sane thing instead of the compatible
>> thing here? Pretty please?)
> 
> looks like ltp is the only thing in the tree using -p, but it seems to
> always provide a priority.

The version you submitted already had this behavior (-p PID together).

The way both implementations are currently written it _has_ to always
provide a priority when setting the policy, even though everything but
-fr require that priority to be 0. I'm strongly tempted to take that out
if we're breaking compatibility anyway, because it's _stupid_.

However if I did that, there would be no common syntax that works for
both, because "stop at first nonoption argument" (which you need if
you're goign to end with COMMAND... that can have who knows what
options) means you can't have the -p PID come after PRIORITY (which is a
nonoption argument), and when it comes before the old one sticks
PRIORITY between -p and the corresponding PID (which is just nuts).

This is a terribly designed command. (So terrible that your first
implementation got the syntax wrong by not being broken _enough_.) The
question is should we follow the original terrible more closely than the
implementation you sent was doing, or should we clean it up to do the
obvious thing and thus break compatibility?

To be honest, I'd like to take out the "priority" argument altogether
and add an (optional) -P to set the priority for -r and -f that actually
_use it, but it'd default to the minimum... darn it, is higher or lower
priority less demanding on the CPU? The chrt man page DOES NOT SAY. It
says to look at sched_setscheduler() which on page 3 finally says:

  Processes scheduled under one of the  real-time  policies
  (SCHED_FIFO, SCHED_RR)  have  a  sched_priority  value  in  the
  range 1 (low) to 99 (high).

Right, my command's help text should explain this. Preferably the -P
description. :)

>> And... how do you indicate that in usage: syntax? Maybe...
>>
>>     usage: chrt [-Rmofrbi] {-p PID [PRIORITY] | [PRIORITY COMMAND...]}
>>
>> Can I do curly brackets to indicate a non-optional grouping? (I think
>> I've seen that. there isn't a spec here, and I don't do multiple usage:
>> lines because the help parser wouldn't know what to do with that.)
> 
> (which i think is a shame. the seq(1) usage is, i think, easier to
> read as three lines than one.)

Eh, I think any usage: line after the first would be treated as literal
text. (The first gets the option merging done to it.) Except it might
care about having exactly one blank line after it, I'm re-reading
through this code in another window and need to properly document it
this time...

I could teach the parser to notice multiple usage lines but then how
would the option merging work? What would that _mean_ to the parser?

One of my original design assumptions is each command should be small
and simple enough that you _can_ have one usage: line. What I intended
toybox to do and what android needs it to do have a certain amount of
tortion going on. (Reality's usually like that.)

>> struct sched_param {
>>         int sched_priority;
>>         int sched_ss_low_priority;
>>         struct timespec sched_ss_repl_period;
>>         struct timespec sched_ss_init_budget;
>>         int sched_ss_max_repl;
>> };
>>
>> Where did they get THAT from?
> 
> POSIX mandates that, iirc. looks like everyone else favored reality over POSIX.

You're right, I missed the second paragraph there. But nothing in Linux
has ever used it and musl's implementation is still just a syscall
wrapper, so I'm sticking with the int for now.

(Sigh. I want good specifications to code to. Posix and LSB aren't, and
I am sad.)

>> Why the toys.stacktop = 0 here? That forces xexec() to exec() rather
>> than running the built-in toybox version, is there a reason to do that here?
> 
> haven't we had bugs in other tools that run commands that were fixed
> by adding this?

We've had cases that needed an actual exec rather than a function call
to run a new command in the same process, but I don't see how this is
one of them?

It's possible android's doing a crazy SELINUX thing I don't understand,
but you're specifying CONFIG_TOYBOX_NORECURSE so it always behaves like
that on android anyway.

>> I switched from linux/sched.h to bits/sched.h
> 
> bits/ is a glibc-ism. (bionic reuses the directory name "bits" for a
> different purpose. seemed better than removing another directory name
> from the global namespace.)

Sorry, bits is where the structure I was looking at is defined. I'm not
actually including anything in chrt.c, the posix sched.h included from
toys.h pulls this in already. :)

>> which provides _almost_
>> all the macros, except SCHED_RESET_ON_FORK. Which isn't there because I
>> _categorically_ refuse  to #define GNU_GNU_ALL_HAIL_STALLMAN for things
>> the Linux kernel developers invented. (Dear glibc developers: up yours,
>> you are wrong.) So that's 1<<30 and a comment. (I could add it to
>> portability.h, but eh.)
> 
> if your files were C++

https://twitter.com/landley/status/638267186948116480

> you'd have _GNU_SOURCE by default and be
> oblivious to this :-)

https://www.youtube.com/watch?v=ePcDSutt_Ww#t=25

Rob