[Toybox] Simplifying (was Re: top -H segfault)
enh
enh at google.com
Mon Jun 6 15:33:24 PDT 2016
On Mon, Jun 6, 2016 at 12:41 PM, Rob Landley <rob at landley.net> wrote:
> Too much chatter, let me simplify. (Patch inlined below.)
>
> On 06/04/2016 04:46 PM, Rob Landley wrote:
>> On 06/04/2016 11:08 AM, enh wrote:
>>> sorry, been busy/out but finally got chance to look at this.
>>
>> I've been a bit distracted recently too, $DAYJOB's is having a Cash Flow
>> Thing and I was a contractor, so I'm job hunting again.
>
> I'm technically unemployed for the moment so should have some time to
> work on backlog stuff, including toybox.
(the old saying about silver linings didn't say that the beneficiary
of the lining would be the same as the recipient of the cloud... :-( )
>>> TNAME seems broken in a couple of ways. it's empty for kernel threads
>>
>> Ok, so:
>>
>> For normal processes, show argv[] skipping initial path.
>> For threads, show thread parent's argv[] -path.
>> For _kernel_ threads, show stat[2].
> ...
>> I checked in a thing,
>
> Let me know if that (in current git) is now displaying what you want.
that gives me enough to replace toolbox top. specifically, "-o
pid,tid,user,pr,ni,%cpu,s,virt,res,pcy,tname,comm" is equivalent to
what we have in automated bugreports. (the interactive behavior is
already fine.)
>> Right now we have ARGS, CMD, CMDLINE, COMM, COMMAND, NAME, and TNAME,
>> all showing slightly different info from three sources (stat[2], args[],
>> and /proc/self/exe)
>
> The reason for so many names is that posix is crazy, but I have to
> build on top of posix instead of coming up with my own fields.
>
> Posix defines -o cmd,comm,args,command but is very vague about their
> meaning. I added -o name,tname. The big problem is all those fields
> put together are just a tiny subset of the data we CAN show, which is:
>
>> 1) /proc/$PID/exe
>> 2) argv[0]
>> 3) stat[2]
>> 4) argv[all]
>> 5) a way of requesting path truncation on any of the above.
>> 6) a way to show thread parent's version of any of the above.
>
> The reason they're different is:
>
>> Everything always has stat[2], although it may be modified for threads.
>> argv[] can be overwritten by the process at will. /proc/self/exe is
>> immutable but only seems to show up for processes (not threads).
>
> Even if I did invent new syntax this would never be entirely generic
> because the thread parent data (#6) has to be copied from the parent
> (due to object lifetime rules), and copying EVERYTHING is expensive.
>
> Plus I'd need a (7) saying "when this is blank, fall back to that",
> since "fall back to stat[2]" isn't universal enough.
>
> So please tell me what behavior you need in terms of the data sources
> and the fallback sequence.
>
>>> all the pieces to replace Android's old top. TNAME gives me what we've
>>> historically called the "process name" and COMM gives me what we've
>>> called the "thread name".
>
> Should I rename "TNAME" to "PNAME"? Or just change the behavior of
> "NAME" to do this instead?
PNAME would certainly make more sense than TNAME (
an example that should be unwrapped enough to be legible:
$ adb shell toybox top -b -n 1 -H -s 6 -o pid,tid,tname,comm | grep adbd
PID TID TNAME [COMM]
190 192 adbd usb ffs open
190 2585 adbd shell srvc 40
190 190 adbd adbd
190 194 adbd <-transport
190 193 adbd ->transport
as you can see, TNAME is the process name and COMM is the thread name.
(this is true of how any Linux libc implements thread names.)
but, yeah, since "NAME" is a non-standard extension added for Android
compatibility anyway, NAME makes sense too :-)
>>> 1939 1939 root 20 0 0.0 S 0 0 fg
>>> kworker/0:3
>>
>> You're showing me what it _is_ doing, not what you want. Let me try to
>> guess what you want.
>>
>> In this case pid==tid, so... you want TNAME to show argv[0] of the
>> current process in that case? Maybe?
>>
>> Something like the attached patch, maybe?
>
> Here is the patch making a second guess at what you want. It's on
> top of current git (the above "is this what you want"), and changes
> the TNAME behavior to "Show parent argv[0]. If that's blank, show
> our argv[0]. If that's blank, show [stat2]."
>
> That way threads show their parents, parents show themselves, and
> kernel threads show the [stat2] name. Is this what you want?
yeah, as far as i can tell this is right.
the only remaining problem is that if we use TNAME in Android's
default ps -o list, we're effectively making -H the default because
the implementation doesn't distinguish between needing to traverse all
the threads and needing to output all the threads. previously there
was no difference, but now for TNAME you need to collect the thread
data, but not output it unless -H. (or unless one of the other
per-thread options is set if you want to keep that behavior. i thought
that was pretty clever, automatically saying "oh, you must have meant
-H" if you're asked to display a thread-specific field.)
so i guess we just need to agree that TNAME is not thread-specific;
almost the opposite :-)
> diff --git a/toys/posix/ps.c b/toys/posix/ps.c
> index 82a8443..879ebfd 100644
> --- a/toys/posix/ps.c
> +++ b/toys/posix/ps.c
> @@ -443,13 +443,24 @@ static char *string_field(struct carveup *tb, struct strawberry *field)
> sl *= -1;
> // First string slot has offset 0, others are offset[-slot-2]
> if (--sl) out += tb->offset[--sl];
> + // If TNAME is blank, show ARGS instead
> + if (which==PS_ARGS && !*out)
> + out = tb->str+tb->offset[-2-typos[which = PS_ARGS].slot];
> if (which==PS_ARGS || which==PS_NAME) {
> - int i;
> + int i, j;
>
> s = out;
> for (i = 0; (which==PS_ARGS) ? i < slot[SLOT_argv0len] : out[i]; i++)
> if (out[i] == '/') s = out+i+1;
> + i = s - out;
> out = s;
> + if (which != field->which) {
> + j = slot[SLOT_argv0len]-i;
> + if (j > 259) j = 259;
> + memcpy(buf, out+i, j);
> + buf[j] = 0;
> + out = buf;
> + }
> }
> if (which>=PS_COMM && !*out) sprintf(out = buf, "[%s]", tb->str);
>
>
> [Deleted explanation of ps object lifetime rules, and explaining why
> your patch didn't do what you thought it did.]
>
> Here is a third option I can implement:
>
>> In a previous message you said a process can write data to argv[0] after
>> forking but before spawning threads, and that this was information you
>> needed to know. So I switched to snapshotting that instead.
>>
>> If I need to snapshot _both_ fields out of the parent, I can do that.
>> But that's adding a slot[7], not redefining slot[6] again.
>
> [Deleted explanation of why layered testing doesn't catch
> code moving between layers, and why testing old assumptions
> doesn't help when the problem is that the design changed.]
>
> [Deleted yet another way of saying writing code is easy and
> figuring out what to do is hard.]
>
> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> 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.
More information about the Toybox
mailing list