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

[enh]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-On-Android-ps-default-output-should-match-toolbox.patch
Type: text/x-patch
Size: 820 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20151107/32e41aca/attachment-0005.bin>