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

[enh]

the attached patch fixes the TTY column for me, both on the desktop
and on Android devices.

On Sat, Nov 7, 2015 at 10:33 AM, enh <enh at google.com> wrote:
> On Sat, Nov 7, 2015 at 10:20 AM, enh <enh at google.com> wrote:
>> 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
>
> for completeness, attached is the trivial patch for that.
>
>>>>> 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.
>
>
>
> --
> Elliott Hughes - http://who/enh - http://jessies.org/~enh/
> Android native code/tools questions? Mail me/drop by/add me as a reviewer.



-- 
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-Make-ps-try-harder-to-find-a-name-for-a-tty.patch
Type: text/x-patch
Size: 2955 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20151107/c27cb0ac/attachment-0005.bin>