[Toybox] chrt.c cleanup

[enh]

On Sun, May 21, 2017 at 11:15 AM, Rob Landley <rob at landley.net> wrote:
> On 05/09/2017 10:57 AM, enh wrote:
>> On Mon, May 8, 2017 at 4:38 PM, Rob Landley <rob at landley.net> wrote:
>>> On 05/08/2017 03:52 PM, enh wrote:
>>     toys/pending/chrt.c \
>
> Ooh, it's <100 lines! Grab!
>
> Huh, Ubuntu 14.04's headers don't have SCHED_DEADLINE. I wonder why? Hmmm...
>
>   http://retis.sssup.it/~jlelli/talks/rts-like14/SCHED_DEADLINE.pdf
>
> To use it I need two new system calls (sched_setattr() and
> sched_getattr()), so it's not just adding -d for type 6 is it? (I
> remember why this is still in pending!)
>
> Right, skip that for now.
>
> Option string has to start with ^ to stop at the first nonoption
> argument (otherwise if you go "chrt ls -l" it'll try to parse -l for chrt).
>
> 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.

> It's checking !*(toys.optargs+1) before checking *toys.optargs, meaning
> if they go "chrt -p 42" that could be a wild pointer.
>
> I hate the output strings (not unixy, hard to parse via script!) but
> that's what Ubuntu's doing so compatibility trumps sanity...
>
> Don't test !TT.pid, "chrt -p 0" queries the current PID. While we're at
> it, add bounds checking so you can't chrt -p -1.
>
> You're not outputting |SCHED_RESET_ON_FORK when querying via -p, the
> other one does that.
>
> Hmmm, given that other, batch, and idle are 0/0 in -m does it _need_ the
> 0? Yes it does. Can I do "chrt -b0 ls -l"? Nope, has to be separate.
>
> 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.

> 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.)

> If you can printf("%ld", pid_t) then I can unwrap this bit of stupid
> from bits/sched.h:
>
>   /* The official definition.  */
>   struct sched_param
>   {
>     int __sched_priority;
>   };
>
> (P.S. That two space indent is out of glibc!)
>
> Except... glibc does that, bionic does that, the old uClibc I still have
> lying around does that, _klibc_ does that... but for some insane reason
> musl's include/sched.h has:
>
> 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.

> Let's see... the kernel's
> include/uapi/linux/sched/types.h has the "one int" version everybody
> else has (that's where the got it from, I expect) and git annotate says
> it was last touched in commit b7b3c76a0a21 in april 2006. So it's been
> like that unchanged for 11 years.
>
> Meanwhile musl's sched_getparam() is a straight syscall wrapper, so it's
> _only_ setting the first field of this struct, meaning this is a musl
> bug and I can ignore it and just use "int" here.
>
> 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?

> The ubuntu version is accepting "chrt -rp 12345" and ignoring the -r,
> which tells it to _set_ the policy. (Which would require priority.) I
> think I prefer to catch that and error instead...
>
> Hmmm, this failed:
>
>   $ sudo ./chrt -r -p 24532 3
>   $ ./chrt -p 24532
>   pid 24532's current scheduling policy: SCHED_RR
>   pid 24532's current scheduling priority: 3
>   $ sudo ./chrt -rR -p 24532 3
>   chrt: 3 > -1
>
> The report is from:
>
>   atolx_range(*toys.optargs, sched_get_priority_min(policy),
>     sched_get_priority_max(policy));
>
> And it's complaining that sched_get_priority_min(policy) is returning
> -1. Because even with the sudo, once the policy is set to SCHED_RR it
> can't query the min/max range anymore? Ah, no: the problem is that if
> you or in SCHED_RESET_ON_FORK _before_ calling the get_priorities(),
> they return invalid argument.  Need to move the |= afterwards, then it
> works.
>
> 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.)

> 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++ you'd have _GNU_SOURCE by default and be
oblivious to this :-)

> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.