Discussion:
toybox fdisk
(too old to reply)
enh
2017-05-06 00:58:19 UTC
Permalink
Raw Message
anyone used toybox fdisk? it's in pending, so i've no idea how close
to usable it is. there's a team asking for fdisk and i'm wondering
whether it's "close enough to usable" and i wouldn't be insane to
committing to fixing bugs, or "not even the original author ran it
once"...
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
Rob Landley
2017-05-08 19:20:22 UTC
Permalink
Raw Message
Post by enh
anyone used toybox fdisk? it's in pending, so i've no idea how close
to usable it is. there's a team asking for fdisk and i'm wondering
whether it's "close enough to usable" and i wouldn't be insane to
committing to fixing bugs, or "not even the original author ran it
once"...
The original author is a large corporation that wanted to remain
anonymous after that whole https://lwn.net/Articles/478308/ thing.
They're almost single handledly responsible for the existence of the
"pending" directory because I couldn't keep up with the review on all
their submissions. :)

My big todo items for fdisk (other than finding time to carefully read
through 1300 lines of code) are 1) sfdisk-style API (possibly less
disgusting, but definitely scriptable and well documented), 2) research
how EFI partitions work (to add support for that), 3) come up with 8
gazillion ways to test it which means QEMU in the test suite.

Rob
Rob Landley
2017-05-21 18:15:03 UTC
Permalink
Raw Message
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.

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

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

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

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

Rob
enh
2017-05-22 16:12:07 UTC
Permalink
Raw Message
Post by Rob Landley
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.
Post by Rob Landley
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.
Post by Rob Landley
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
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.)
Post by Rob Landley
If you can printf("%ld", pid_t) then I can unwrap this bit of stupid
/* 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
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.
Post by Rob Landley
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?
Post by Rob Landley
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...
$ 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
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.)
Post by Rob Landley
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 :-)
Post by Rob Landley
Rob
_______________________________________________
Toybox mailing list
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.
Rob Landley
2017-05-23 02:53:45 UTC
Permalink
Raw Message
Post by enh
Post by Rob Landley
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.)
Post by enh
Post by Rob Landley
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. :)
Post by enh
Post by Rob Landley
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
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.)
Post by enh
Post by Rob Landley
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.)
Post by enh
Post by Rob Landley
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.
Post by enh
Post by Rob Landley
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. :)
Post by enh
Post by Rob Landley
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
Post by enh
you'd have _GNU_SOURCE by default and be
oblivious to this :-)


Rob
enh
2017-05-26 00:59:22 UTC
Permalink
Raw Message
Post by Rob Landley
Post by enh
Post by Rob Landley
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.)
Post by enh
Post by Rob Landley
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
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. :)
Post by enh
Post by Rob Landley
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
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 :-)
Post by Rob Landley
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).
Post by Rob Landley
Post by enh
Post by Rob Landley
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.)
Post by enh
Post by Rob Landley
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.
Post by Rob Landley
Post by enh
Post by Rob Landley
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. :)
Post by enh
Post by Rob Landley
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
Post by enh
you'd have _GNU_SOURCE by default and be
oblivious to this :-)
http://youtu.be/ePcDSutt_Ww
Rob
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
Rob Landley
2017-05-26 07:23:29 UTC
Permalink
Raw Message
Post by enh
Post by Rob Landley
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 :-)
I have a window open on that: the first bug is that my infrastructure
had a design assumption that each block of text slotted into one place,
and the block shared by top/iotop confuses it. Requires a design change
and moving some code around.

(It's been something like 3 days since I looked at said open window
because I got interrupted by $DAYJOB and
https://github.com/landley/mkroot/commits/master and
http://lkml.iu.edu/hypermail/linux/kernel/1705.2/05611.html and so on,
but I intend that to be in the release at the end of the month. I have 6
days left...)

After that I might wind up flying to Tokyo again to do GPS and SDK stuff
for a bit, and there's an automotive linux conference I may or may not
be attending (probably not but can't rule it out)

(I also have a window open replying to Josh Gao, but it's describing the
coding I'm doing to fix it, which I haven't checked in yet. :)

Spinning many plates. I should get my blog up to date again so people
can at least see what I did. :)
Post by enh
Post by Rob Landley
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
That would either repeat the [OPTIONS] or not list them in the usage line.
Post by enh
usage: seq [-w|-f fmt_str] [-s sep_str] [first] [increment] last
not. (and shouldn't that be "[first [increment]]"?)
Probably. And in newer help text I tend to CAPITALIZE things that aren't
keywords. (So the keywords in stuff like "find" and "ifconfig" stand out
as keywords.)

I need to do a help text consistency pass. It's in the "posix
compliance" and "fill out the test suite" heap, after I get proper
mkroot-based tests of ps/modprobe/ifconfig implemented...
Post by enh
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
It took me a moment to work out you're using =1 to specify a default
value here. I haven't seen that before, and given how many things have
name=value assignments on the command line (dd, ps, env...) it seems
potentially misleading.
Post by enh
(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.)
They all should be. Where they're missing its a bug and I want to fix it.

I'm trying to make the --help output be both terse and thorough, which
is HARD. Ubuntu commands' --help text... well it varies wildly:

$ ps --help

Usage:
ps [options]

Try 'ps --help <simple|list|output|threads|misc|all>'
or 'ps --help <s|l|o|t|m|a>'
for additional help text.

For more details see ps(1).

$ ls --help | wc
117 834 7414

But this is _the_ documentation I'm providing for each command. I don't
have man pages, I have --help output. I really want it to fit on one
80x25 screen when possible but just couldn't do it for sed, ps, ls...

Sigh. There's a conflict between "tutorial" and "reference" that comes
up a lot too. Documentation teaching you something you've never seen
before and documentation for when you already have the general idea and
are looking up some detail are different _kinds_ of documentation, and
making one thing do both is really hard.
Post by enh
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).
$ sed --help
--posix
disable all GNU extensions.
-r, --regexp-extended
use extended regular expressions in the script.
-s, --separate
consider files as separate rather than as a single continuous
long stream.
-u, --unbuffered
load minimal amounts of data from the input files and flush
the output buffers more often
-z, --null-data
separate lines by NUL characters
--help display this help and exit
--version output version information and exit

Yes, clearly that can't be allowed to ship. :)

Sigh I agree with this in principle (although I would have gone the
other way and enforced periods), but "git am" refuses to apply to a
modified file even when the changes don't remotely conflict, for reasons
I've asked Junio Hamano about and never got a straight answer.

When there's no actual conflict "git diff > file; git checkout -f; git
am; patch -p1 -i file" works fine. Of course my tree does have conflicts...

Hunk #1 FAILED at 13.
1 out of 3 hunks FAILED -- saving rejects to file toys/lsb/seq.c.rej
Hunk #1 FAILED at 8.
1 out of 2 hunks FAILED -- saving rejects to file
toys/pending/getfattr.c.rej
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file toys/posix/cut.c.rej

Sigh. And the cut.c patch is one big giant hunk for the whole file
because that's the rewrite I did way back when and no longer remember
what I did and need to review my own code as if it's an external
submission when I can find the time...

(I also wind up with open email windows with half-written replies that I
wind up discarding because a later message rendered it moot. Swap
thrashing is inefficient.)

But this one, I can send now. :)

Rob
enh
2017-05-26 15:54:45 UTC
Permalink
Raw Message
Post by Rob Landley
Post by enh
Post by Rob Landley
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 :-)
I have a window open on that: the first bug is that my infrastructure
had a design assumption that each block of text slotted into one place,
and the block shared by top/iotop confuses it. Requires a design change
and moving some code around.
(It's been something like 3 days since I looked at said open window
because I got interrupted by $DAYJOB and
https://github.com/landley/mkroot/commits/master and
http://lkml.iu.edu/hypermail/linux/kernel/1705.2/05611.html and so on,
but I intend that to be in the release at the end of the month. I have 6
days left...)
After that I might wind up flying to Tokyo again to do GPS and SDK stuff
for a bit, and there's an automotive linux conference I may or may not
be attending (probably not but can't rule it out)
(I also have a window open replying to Josh Gao, but it's describing the
coding I'm doing to fix it, which I haven't checked in yet. :)
Spinning many plates. I should get my blog up to date again so people
can at least see what I did. :)
Post by enh
Post by Rob Landley
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
That would either repeat the [OPTIONS] or not list them in the usage line.
Post by enh
usage: seq [-w|-f fmt_str] [-s sep_str] [first] [increment] last
not. (and shouldn't that be "[first [increment]]"?)
Probably. And in newer help text I tend to CAPITALIZE things that aren't
keywords. (So the keywords in stuff like "find" and "ifconfig" stand out
as keywords.)
I need to do a help text consistency pass. It's in the "posix
compliance" and "fill out the test suite" heap, after I get proper
mkroot-based tests of ps/modprobe/ifconfig implemented...
Post by enh
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
It took me a moment to work out you're using =1 to specify a default
value here. I haven't seen that before, and given how many things have
name=value assignments on the command line (dd, ps, env...) it seems
potentially misleading.
Post by enh
(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.)
They all should be. Where they're missing its a bug and I want to fix it.
I'm trying to make the --help output be both terse and thorough, which
$ ps --help
ps [options]
Try 'ps --help <simple|list|output|threads|misc|all>'
or 'ps --help <s|l|o|t|m|a>'
for additional help text.
For more details see ps(1).
$ ls --help | wc
117 834 7414
But this is _the_ documentation I'm providing for each command. I don't
have man pages, I have --help output. I really want it to fit on one
80x25 screen when possible but just couldn't do it for sed, ps, ls...
Sigh. There's a conflict between "tutorial" and "reference" that comes
up a lot too. Documentation teaching you something you've never seen
before and documentation for when you already have the general idea and
are looking up some detail are different _kinds_ of documentation, and
making one thing do both is really hard.
Post by enh
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).
$ sed --help
--posix
disable all GNU extensions.
-r, --regexp-extended
use extended regular expressions in the script.
-s, --separate
consider files as separate rather than as a single continuous
long stream.
-u, --unbuffered
load minimal amounts of data from the input files and flush
the output buffers more often
-z, --null-data
separate lines by NUL characters
--help display this help and exit
--version output version information and exit
Yes, clearly that can't be allowed to ship. :)
Sigh I agree with this in principle (although I would have gone the
other way and enforced periods),
happy to go the other way too; i only removed them because the current
majority was periodless.

(i haven't been able to guess whether it's supposed to be "-a All the
things" or "-a all the things", but if you choose a style i'm sure
i'll have an afternoon where i'm waiting for something for long enough
to fit in a mindless semi-automated search and replace.)
Post by Rob Landley
but "git am" refuses to apply to a
modified file even when the changes don't remotely conflict, for reasons
I've asked Junio Hamano about and never got a straight answer.
When there's no actual conflict "git diff > file; git checkout -f; git
am; patch -p1 -i file" works fine. Of course my tree does have conflicts...
Hunk #1 FAILED at 13.
1 out of 3 hunks FAILED -- saving rejects to file toys/lsb/seq.c.rej
Hunk #1 FAILED at 8.
1 out of 2 hunks FAILED -- saving rejects to file
toys/pending/getfattr.c.rej
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file toys/posix/cut.c.rej
Sigh. And the cut.c patch is one big giant hunk for the whole file
because that's the rewrite I did way back when and no longer remember
what I did and need to review my own code as if it's an external
submission when I can find the time...
(I also wind up with open email windows with half-written replies that I
wind up discarding because a later message rendered it moot. Swap
thrashing is inefficient.)
But this one, I can send now. :)
Rob
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
Rob Landley
2017-05-27 20:53:13 UTC
Permalink
Raw Message
Post by enh
Post by Rob Landley
Sigh I agree with this in principle (although I would have gone the
other way and enforced periods),
happy to go the other way too; i only removed them because the current
majority was periodless.
(i haven't been able to guess whether it's supposed to be "-a All the
things" or "-a all the things",
I capitalize and put periods on my tweets, I'm a bit biased here.
(College english minor and all that.)

I care more about it being consistent than being "right" here. The logic
of having the periods was that there's sometimes more than one sentence
in an option description. But the most common form of that seems to be
"stuff stuff stuff (more stuff)" and removing both periods works there.
Trying to keep 'em to one line more or less means one sentence.

Another thing I need to be consistent about (one way or the other) is
tabs or two spaces after the option in the description lines. That one's
a little weird; sometimes the tab lets you have space for an argument
(df and cpio) and sometimes it doesn't. And then there's things like
"grep" go to two columns, where a tab doesn't make sense. Lots of places
currently get it wrong (cat, bzcat, lsattr, comm, hostname, du...) and
what it looks like in the source and the help text are different because
of tab rounds up to 8 spaces but the menuconfig help text starts
indented by 4 spaces...

Sigh. Part of the reason for the tabs was to keep byte count down in the
help text, and now that I've got gzip in there I should compress the
help text during the build and decompress it when displaying it. (I
remember Busybox did a trick like that back when I worked on it.
Probably still does.) This doesn't save anything during standalone
builds but it's a pretty reasonable savings the rest of the time.
Post by enh
but if you choose a style i'm sure
i'll have an afternoon where i'm waiting for something for long enough
to fit in a mindless semi-automated search and replace.)
https://xkcd.com/303/

For me figuring out the right thing to do is usually the big timesink,
and actually doing a lot more straightforward... except for the part
about plans never surviving contact with the enemy. Clay Shirky said
"the waterfall method amounts to a pledge by all parties not to learn
anything while doing the actual work" in
http://www.shirky.com/weblog/2013/11/healthcare-gov-and-the-gulf-between-planning-and-reality/
which is basically a corollary to http://wiki.c2.com/?PlanToThrowOneAway

Rob

Loading...