[Toybox] [PATCH] Fix the ps -o CMD vs COMM distinction.

[enh]

On Thu, Nov 5, 2015 at 8:39 PM, Rob Landley <rob at landley.net> wrote:
> On 11/02/2015 12:51 PM, enh wrote:
>> On Mon, Nov 2, 2015 at 12:11 AM, Rob Landley <rob at landley.net> wrote:
>>> On 10/31/2015 04:02 PM, enh wrote:
>>>> Better for _me_ would be to change the current code that rewrites
>>>> characters < ' ' to leave '\0' alone if we're not supposed to be
>>>> outputting the arguments, but then toybox would match the Android
>>>> behavior (modulo the fact that we called this field NAME) rather than
>>>> the usual GNU behavior.
>
>
>
>>> Is this another documented deviation from posix in the big comment block
>>> near the top, or did I miss a curve?
>>
>> you mention one wart in this family:
>>
>>  * It also says that -o "args" and "comm" should behave differently but use
>>  * the same title, which is not the same title as the default output. (No.)
>>
>> but NAME is an Android invention. it's basically "imagine if -o comm
>> worked right and didn't truncate regardless of width". (and we've
>> never had -o, so it's the only one of the args/cmd/comm/command family
>> in use on Android.)
>
> Commands have two potential names though.
>
> There's the name the executable was originally called as, which is the
> parenthetical second argument from /proc/$PID/stat, and there's argv[0]
> from /proc/$PID/cmdline. If the process edits its argv[0], the
> /proc/$PID/cmdline stuff changes. (Note: changing the argv[0] pointer
> doesn't edit it, but reaching through that and changing the string
> contents would.)
>
> Also, a process's argv[0] often has a path on it.
>
> #include <stdio.h>
>
> int main(int argc, char *argv[])
> {
>   argv[0][2]='*';
>   sleep(1000);
> }
> gcc blah.c -o blah2
>
> $ ps -o cmd,comm
> CMD                         COMMAND
> ./*lah2                     blah2
> ps -o cmd,comm              ps
> bash                        bash
>
> I'm happy to take a patch adding NAME, but I can't write it myself
> without a little clarification about what you want it to do. :)

attached is what i had in mind:

~/toybox$ ./toybox ps -o CMD,COMM,CMDLINE
CMD                         COMMAND                     CMDLINE
./toybox ps -o CMD,COMM,CMD toybox                      ./toybox
-/bin/bash                  bash                        -/bin/bash

i went with CMDLINE rather than our traditional NAME because it seemed
more intention-revealing (and we've never had -o, so it's not like
anyone's going to notice on our end). it keeps them together in the
help text too, which should make it easier for users to understand
their choices.

>>>> Does anyone actually want the GNU truncating
>>>> behavior? cmd/comm/command seems such a mess that although I wouldn't
>>>> normally suggest adding new stuff, maybe just ignoring all this and
>>>> adding a NAME field isn't such a bad idea after all...
>>>
>>> I have no objection. The old posix default values are kinda nuts these days.
>
> I note that posix says "When the -o option is not specified, the
> standard output format is unspecified." and then goes on to define XSI
> extensions, which is way too much granularity for 2015.
>
> That said, I'm pretty happy to cange the default output and still call
> this ps posix complaint, because _they_ did.
>
>>> How any of that is usable "for scheduling", I couldn't tell you, since
>>> it's all historical usage of long-running processes telling you about
>>> what they did last week...
>>
>> Android has an unrelated non-standard set of scheduling-related
>> fields, but i was worrying about the portable stuff first. (i didn't
>> actually realize that -o comm was broken until working on this. i
>> thought our only deviation there was that we had a different name for
>> the field.)
>
> I'm pretty happy with the portable stuff. What else do you need now?
>
> My remaining big todo items for ps are:
>
> 1) bsd-style options
> 2) --sort
> 3) column autosizing ala ls
>
> (Note that 2 and 3 require the same preread infrastructure...)

(-C might be nice, but i removed the equivalent [which didn't look for
-C took any non-integer argument as a process name] from Android a few
weeks back and no one's noticed yet.)

>>> Anyway, I ripped out the crazy bitmap stuff and redid the default/-f/-l
>>> command line parsing as strings so it would be easy to change 'em. If
>>> you have a better idea what they should be, now's the time to speak up.
>>> (Heck, I've got a CFG_TOYBOX_ANDROID symbol, easy enough to change the
>>> default just for that, if you've got a userbase that'll care.)
>>
>> yeah, as far as i can tell from the internal tree basically 99%
>> existing use of Android's ps is just "ps" and then
>> grepping/sedding/parsing the result.
>
> Which is nice because android ps's other command line arguments are
> kinda nonstandard.
>
> The -nxc are new and don't conflict (partly because posix -n was stupid
> and I yanked it and documented the deviation), but -o is the standard
> way to add fields. (Same goes for -Z but we added that already.)
>
> The -P I added for ppid conflicts with -P policy (although I did ask
> first), and -p SHOW_PRIO conflicts with posix -p SHOW_PIDS (not my fault).

i don't think we should waste flags on any of these (except -n). extra
-o labels is the right way to go. i'm 99% sure no one ever uses them
manually. they're used in the ps output that goes in every bug report
via dumpstate, but it's easy enough for me to supply a custom -o at
that one call site.

interestingly, although the man page says otherwise, the desktop ps
supports -n and interprets it in the same way as Android. so i've
attached a patch for that too.

> You've got thread support, that should go on the todo list above...
>
> exe_abi? *shrug* Ok. (It's not -o abi though, it's --abi.) And it _just_
> says 32 or 64, no elf/binflt/fdpic, or arm/mips/x86? (I note that
> /proc/$PID/exe of shell scripts points to the interpreter executable, so
> that's fun. Of course the really fun code hiding is having your
> executable point to a custom dynamic linker and having that do all the
> work... :)

yeah, i'm not really happy with what we have there at the moment. i
agree it should be a -o label, and did think that even though Android
doesn't have any x32 ABIs, others do, so if we add an equivalent of
--abi to toybox ps, we should try to be more general. this isn't a
priority though. it's only used interactively to answer questions like
"is <x> 64-bit?". (which is another reason why ABI is a terrible name;
on an x86 Android device you might have ARM code running via Houdini.
i don't know how we'd conveniently answer your "is Netflix ARM code or
x86?", but it's disappointing that --abi sounds like it might do so
but is actually unrelated.)

ELFCLASS would have been more appropriate, though not as snappy.

> Android's parsing of unrecognized arguments as pids is basically the
> same as what -p 1,2,3 does. Do you want that extension? (If so, should
> it grep for command names or something if it's not a pid?) You just
> atoi() and error out for zero...

i think we can live without it, so we may as well insist that folks
use the POSIX syntax.

> The android default output is:
>
> [LABEL] USER PID PPID VSIZE RSS [CPU] [PRIO] [POLICY] WCHAN PC [ABI] NAME
>
> Which with no arguments translates to:
>
> USER PID PPID VSIZE RSS WCHAN PC NAME
>
> Where NAME is argv[0] with the path and everything unless that's zero
> length, in which case you get the real name.

can that ever be zero length? that's one thing that's different about
the toolbox implementation and the attached patch, but i can't think
how you'd ever hit that case.

> USER shows username if
> we've got it, PC is eip.
>
> When you say grepping/sedding/parsing, are they chopping out field #3 or
> are they counting spaces? Because counting spaces means we depend on
> (and must preserve) the specific indentation...

we've changed field widths enough that i doubt anyone's [still]
counting spaces, and if they are, we should help them escape that bad
habit.

> A first pass at it seems to be to have android's default output be:
>   ps -o user,pid,ppid,vsize,rss,wchan,addr:10=PC,comm=NAME

our wchan is a little wider for readability, and we have the 's' field:

toybox ps -o user,pid,ppid,vsize,rss,wchan:10,addr:10=PC,s,comm=NAME

if you have the attached patch, this is closer still:

toybox ps -o user,pid,ppid,vsize,rss,wchan:10,addr:10=PC,s,cmdline

>>> Also, on a previous patch: having --ppid like that in the help text is
>>> really ugly. Can we define -P as the short version of this so the help
>>> text doesn't mix longopts with shortopts? (Keeping the long alias for
>>> compatability with existing users, fine. Nothing seems to be using -P
>>> yet...)
>>
>> i did think it odd that GNU didn't have a short option for this
>> (unless they're just wary of stepping on future POSIX's toes), but
>> given that --ppid is the portable option i think that's the better
>> thing to have in the help text.
>>
>> (i'm not as anti-longopt as you in general. especially for rarely-used
>> functionality it's a lot easier to remember the longopt.)
>
> I think I talked about that last time. It's a preference, not a hard
> rule, but it makes help text parsing easier to standardize the format.
> But if we do have --longopts without short options they should go at the
> beginning or end to avoid confusing scripts/config2help.c.
>
>>> I'm trying to cut a release in the next day or two, so getting this
>>> nailed down would be nice...
>>
>> iirc, ps -C is the only standard thing that Android had an equivalent
>> of (with different syntax, of course).
>>
>> i think switching Android's ps over is going to be hard because it's
>> always been very different from regular ps *and* it's very heavily
>> used. i wouldn't block a release on that!
>
> Now that -o exists, users can in theory do a lot less parsing. Of course
> finding the users to convert them is the hard part. :)

yeah, and for better or worse, "seeing what breaks" is often the only
real choice.

it took three or four reverted attempts to get ls switched over, but
hopefully ps won't be any worse than that.

> 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-Add-ps-o-CMDLINE.patch
Type: text/x-patch
Size: 5646 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20151107/8a2f7857/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-ps-n.patch
Type: text/x-patch
Size: 2658 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20151107/8a2f7857/attachment-0011.bin>