<div dir="auto">(For those following along at home, a fix was merged yesterday.)</div><div class="gmail_extra"><br><div class="gmail_quote">On Mar 12, 2017 01:04, "Rob Landley" <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 03/11/2017 01:22 PM, enh wrote:<br>
> from the line number addr2line gave above, it's the line marked with !<br>
> below. i've included the block between too, since that modifies<br>
> new->child.<br>
><br>
>   // Recurse down into tasks, retaining thread groups.<br>
>   // Disable show_process at least until we can calculate tcount<br>
>   kcount = TT.kcount;<br>
>   sprintf(toybuf, "/proc/%u/task", pid);<br>
>   new->child = dirtree_flagread(toybuf, DIRTREE_SHUTUP|DIRTREE_PROC, get_ps);<br>
>   TT.threadparent = 0;<br>
>   kcount = TT.kcount-kcount+1;<br>
>   tb = (void *)new->extra;<br>
>   tb->slot[SLOT_tcount] = kcount;<br>
><br>
>   // Fill out tid and thread count for each entry in group<br>
> !  if (new->child) for (dt = new->child->child; dt; dt = dt->next) {<br>
><br>
> so i assume it's that dirtree_flagread that's failing, returning<br>
> DIRTREE_ABORTVAL, and since the code that follows just assumes it's<br>
> either valid or null, we die on the line marked ! (as claimed by the<br>
> stack trace).<br>
><br>
> i don't know is why why dirtree_flagread is failing, though, and can't<br>
> reproduce it.<br>
<br>
Oh that's easy to see from context: /proc/%u exited before /proc/$u/task<br>
could be opened, so dirtree_flagread() went "nope" and we didn't check<br>
for it.<br>
<br>
Asynchronous process exit while we're reading its data is something I<br>
tried to be robust against in the rest of ps, usually by effectively<br>
moving the process exit a milisecond or so earlier. If we couldn't read<br>
the data because process go bye-bye how does that differ from process go<br>
bye-bye an instant before we would have noticed it?<br>
<br>
Looks like I messed up here going back and adding thread support because<br>
the dirtree_flagread() semantics are non-obvious. (The<br>
dirtree_flagread() reopen in thread scanning is a rough edge in the<br>
design. I'm splicing together discontiguous trees manually and the<br>
result doesn't have the checks and review the existing dirtree.c stuff<br>
does.)<br>
<br>
And that's my real concern here: the dirtree_flagread() semantics<br>
_shouldn't_ be non-obvious. Should I change dirtree_flagread() to always<br>
return NULL or valid? Why did I have it NOT do that in the first place<br>
(to the point I commented it not doing that). I vaguely recall there's<br>
some error condition that the caller can only distinguish if it gets the<br>
extra error return to distinguish the case, but I don't remember _what_<br>
the condition is. (Exists but permissions wrong to actually read its<br>
contents, maybe?)<br>
<br>
Alas my toybox work directory's in a bit of a shambles at the moment<br>
because I've got half-finished make.sh changes for getconf.c and<br>
half-finished help text parsing changes, and between the two of 'em it<br>
doesn't _build_ at the moment. (My two weeks in tokyo and the week in<br>
portland before that I was too busy with other stuff to close any tabs.<br>
Happy to be home, but torn between resting and shoveling out...)<br>
<br>
I can do a quick fix here (just add a test for abortval on that line)<br>
but I'd rather change flagread not to return abortval, which means<br>
understanding what I lose by doing that and being comfortable with it.<br>
(And why the _other_ calls to flagread aren't dying more regularly with<br>
abortval returns? How is this NOT screwing up ps with no /proc ?)<br>
<br>
But once again, I'm only getting to this at 4am. (Ok, mostly that's<br>
jetlag royally horking my schedule.) Might not get to this tomorrow if<br>
getconf build infrastructure continues to be stroppy, but it's #3 on my<br>
toybox todo list right now.<br>
<br>
Thanks,<br>
<br>
Rob<br>
</blockquote></div></div>