[Toybox] [PATCH] killall should kill scripts too.

[enh]

That's my point: there's no way to implement this correctly, so the least
worst choice is to just implement it the same as everyone else. Whatever we
do will be surprising in some instances, but "the same surprises as the
other systems" is at least manageable. I don't see how anyone wins from
toybox having a different but also broken heuristic.

On Sat, Dec 16, 2017, 15:13 Rob Landley <rob at landley.net> wrote:

> Blah, I got deep enough into
> https://github.com/landley/mkroot/commits/master I forgot to check my
> mail for a couple days...
>
> On 12/14/2017 08:01 PM, enh wrote:
> > On Thu, Dec 14, 2017 at 6:32 AM, Rob Landley <rob at landley.net> wrote:
> >> On 12/13/2017 01:48 PM, enh wrote:
> >>> Found running LTP file system tests on Android.
> >>
> >> I have a todo item for this, but I was just going to also check argv[1]
> >> when argv[0] is an absolute path (starts with '/'), and I should adjust
> >> pgrep/pkill to do this too.
> >>
> >> I'd rather not involve comm because it's capped at 15 chars (arbitrarily
> >> truncated), I think the "treat absolute path as a potential interpreter"
> >> logic is simpler and probably less brittle? (The "process could go away
> >> between the two files we read" race probably isn't an issue here but not
> >> having to care about that's a bonus.)
> >
> > i don't like comm truncation either, but it still seems like the
> > better test. it's more intention-revealing given that we don't have a
> > canonical list of all valid interpreters.
>
> An interpreter run from #! will have a path in argv[0], a command run
> from $PATH won't have a path in the name. Given that tinycc's -run mode
> is technically an interpreter, there can't really _be_ a canonical list.
> So it's going to be some sort of heuristic, the question is what.
>
> > suppose you set $EDITOR or $PAGER to an absolute path. that shouldn't
> match.
>
> Hmmm...
>
>   $ /bin/less filename.txt
>   $ killall filename.txt
>
> I can see a case for it. (Especially since killall filename without the
> extension wouldn't match.)
>
> We're not going to get perfect behavior out of this:
>
>   $ time sleep 100 &
>   [1] 19950
>   $ killall time
>   time: no process found
>
> (It's a shell builtin. Yes, you can background a shell builtin. I assume
> there's an automatic subshell in there somewhere. On a related note,
> ctrl-z to suspend "read" turns out to be... problematic.)
>
> > and, sure, comm is truncated, but argv[1] isn't, and comm is only used
> > as a hint for "should i be looking at argv[1] rather than argv[0]?".
>
> Reading two files, more complex parsing (the only way to tell whether )
> is part of the filename or not is to continue to the _last_ ) which
> means strrchr() over the whole string you read...)
>
> > plus checking comm seems to be consistent with other implementations,
> > judging by strace.
>
> Except gnu is often crazy and busybox has a tendency to blindly copy it.
> And whatever we do there are still questionable cases:
>
>   $ echo sleep 100 > thingy.sh
>   $ bash thingy.sh &
>   [1] 19893
>   $ killall thingy.sh
>   thingy.sh: no process found
>   $
>
> What's the _right_ thing to do there? Tying "killall bash" is going to
> leave the system really unhappy. My "path might mean interpreter"
> heuristic wouldn't catch this one either, but it's guaranteed to catch
> anything run from #! because that has to be abspath.
>
> Well, ok, not quite _guaranteed_:
>
>   $ cd ~
>   $ ln -s /bin/bash rutabaga
>   $ echo -e '#!rutabaga\necho ha" > thingy.sh
>   $ chmod +x thingy.sh
>   $ ./thingy.sh
>   ha
>   $
>
> But that's fairly deep voodoo nobody should ever do. :)
>
> >>>  tests/killall.test | 14 +++++++++++++
> >>
> >> Hmmm, I usually use "sleep" for these because it's self cleaning in the
> >> failure case. :)
> >
> > (use of `yes` copied from pgrep tests :-) )
>
> I have not actually met Divya Kothari.
>
> >> It'd be nice to have an executable test as well, but that's slightly
> >> more awkward. (What do I expect to have install that's _not_ going to be
> >> running on the host already? Ah, of course: symlink sleep under a new
> >> name. :)
> >
> > `env python` is the other combination that springs to mind if you're
> > trying for completeness.
>
> Would somebody please explain to me why they trust env to exist at a
> specific absolute path but don't trust python to? I've never understood
> this.
>
> Anyway, I did several hours of work on this back around wednesday, and
> then context switched away and never got back to it. (Unprecedented, I
> know.) Lemme try to finish that up and check it in...
>
> Rob
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20171217/ab4df5c6/attachment-0001.htm>