[Toybox] chrt.c cleanup

[enh]

On Mon, May 22, 2017 at 7:53 PM, Rob Landley <rob at landley.net> wrote:
> 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?

your time would certainly be better spent making "top --help" work right :-)

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

in the case of seq, i just find the man page's style of

       seq [OPTION]... LAST
       seq [OPTION]... FIRST LAST
       seq [OPTION]... FIRST INCREMENT LAST

grokable, but

  usage: seq [-w|-f fmt_str] [-s sep_str] [first] [increment] last

not. (and shouldn't that be "[first [increment]]"?) even doing that
and including the defaults seems a lot less clear to me than just
listing the three cases

  usage: seq [-w|-f fmt_str] [-s sep_str] [first=1 [increment=1]] last

(the GNU style of just saying "OPTIONS" might help, especially because
it's kind of random at the moment which options get mentioned in the
usage line.)

but your time would certainly be better spent making "top --help" work right :-)

speaking of kind of random, attached is a patch that makes the --help
output more consistent about periods when describing flags (because
the current mostly-never-but-sometimes looks weird).

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

no, i think you're right and this isn't necessary here.

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



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Be-more-consistent-about-periods-in-help-text.patch
Type: text/x-patch
Size: 24086 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20170525/d62dd002/attachment-0002.bin>