[Toybox] Simplifying (was Re: top -H segfault)

[Rob Landley]

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.

>> 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.

> 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?

>>  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?

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