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 <[email protected]> wrote: > On Sat, Nov 7, 2015 at 10:20 AM, enh <[email protected]> wrote: >> On Thu, Nov 5, 2015 at 8:39 PM, Rob Landley <[email protected]> wrote: >>> On 11/02/2015 12:51 PM, enh wrote: >>>> On Mon, Nov 2, 2015 at 12:11 AM, Rob Landley <[email protected]> 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.
From 9d2b77672871d427384e6294db14d7a127988815 Mon Sep 17 00:00:00 2001 From: Elliott Hughes <[email protected]> Date: Sat, 7 Nov 2015 11:29:36 -0800 Subject: [PATCH] Make ps try harder to find a name for a tty. --- toys/posix/ps.c | 68 ++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/toys/posix/ps.c b/toys/posix/ps.c index f3ff39b..bc3060c 100644 --- a/toys/posix/ps.c +++ b/toys/posix/ps.c @@ -181,6 +181,32 @@ static int match_process(long long *slot) return 1; } +static void find_tty_name(char *out, int rdev) { + int major = (rdev>>8)&0xfff, minor = ((rdev>>12)&0xfff00)|(rdev&0xff); + FILE *fp = fopen("/proc/tty/drivers", "r"); + + if (fp) { + int tty_major; + + while (fscanf(fp, "%*s %s %d %*s %*s", out, &tty_major) == 2) { + // TODO: we could parse the minor range too. + if (tty_major == major) { + struct stat st; + + sprintf(out + strlen(out), "%d", minor); + if (!stat(out, &st) && S_ISCHR(st.st_mode) && st.st_rdev == rdev) { + fclose(fp); + return; + } + } + } + fclose(fp); + } + + // Really couldn't find it, so just show major:minor. + sprintf(out, "%d:%d", major, minor); +} + // dirtree callback. // toybuf used as: 1024 /proc/$PID/stat, 1024 slot[], 2048 /proc/$PID/cmdline static int do_ps(struct dirtree *new) @@ -315,28 +341,30 @@ static int do_ps(struct dirtree *new) // TTY } else if (i==12) { - - // Can we readlink() our way to a name? - for (i=0; i<3; i++) { - struct stat st; - - sprintf(scratch, "%lld/fd/%i", *slot, i); - fd = dirtree_parentfd(new); - if (!fstatat(fd, scratch, &st, 0) && S_ISCHR(st.st_mode) - && st.st_rdev == slot[4] - && 0<(len = readlinkat(fd, scratch, out, 2047))) - { - out[len] = 0; - if (!strncmp(out, "/dev/", 5)) out += 5; - - break; + int rdev = slot[4]; + + // The common case is no tty, and we call that "?" rather than "0:0". + if (rdev == 0) strcpy(out, "?"); + else { + // Can we readlink() our way to a name? + for (i=0; i<3; i++) { + struct stat st; + + sprintf(scratch, "%lld/fd/%i", *slot, i); + fd = dirtree_parentfd(new); + if (!fstatat(fd, scratch, &st, 0) && S_ISCHR(st.st_mode) + && st.st_rdev == rdev + && 0<(len = readlinkat(fd, scratch, out, 2047))) + { + out[len] = 0; + break; + } } - } - // Couldn't find it, show major:minor - if (i==3) { - i = slot[4]; - sprintf(out, "%d:%d", (i>>8)&0xfff, ((i>>12)&0xfff00)|(i&0xff)); + // Couldn't find it, try all the tty drivers. + if (i == 3) find_tty_name(out, rdev); + + if (!strncmp(out, "/dev/", 5)) out += 5; } // TIME ELAPSED -- 2.6.0.rc2.230.g3dd15c0
_______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
