[Toybox] default SIGPIPE handler that calls _exit(0)?

[enh]

On Wed, Apr 29, 2015 at 12:09 PM, Rob Landley <rob at landley.net> wrote:
> On 04/25/2015 02:14 PM, enh wrote:
>> what's the plan wrt SIGPIPE?
>
> Hadn't made any? I have xexit() doing fflush(0) ||
> ferror(stdout) and perror_msg("write") if you redirect
> the output to a disk that fills up and such, but
> getting killed by a signal has a default behavior
> provided by the OS, and I left it alone...
>
>> the desktop is pretty inconsistent.
>> many (but not all) commands install a signal handler that
>> does _exit(0). others (coreutils 8.21's ls, say) do nothing.
>> normally "what you do about SIGPIPE" isn't a problem but on
>> Android that leads to a crash report and people filing "ls
>> crashed" bugs against me. (our default shell PS setup is
>> also noisy about crashes.)
>
> Is exiting due to sigpipe really a crash?

for toybox? probably not. for Angry Birds? definitely. for one of the
command-line tools written in Java? perhaps.

>> in the past, like the desktop, we've fixed this on an ad
>> hoc basis. i just uploaded a change to toolbox
>> (https://android-review.googlesource.com/148331)
>
> How do I get gerrit to show me the darn patch? I clicked
> on the change ID and it opened the same "it touches these
> two files and we'll show you how many lines were changed
> but not _what_ changed" page...

click on the filename in question. i don't think there's a way to see
all files' changes at once...

> If I right click on "gitiles" next to the commit ID it will
> open a tree view of that version, but not a diff view.
>
> Ok, copy the change ID to the clipboard (horrible javascript
> thing to turn it into an editing field as soon as I highlighted
> it to prevent me from doing a normal cut and paste), then
> right click open in new tab the project link in the upper right,
> click on a random commit, paste the commit ID over that commit
> number... nope, it's the same "what's a diff?" web interface...
>
> Ok, clone the darn git project and git show from the command
> line. (Your web interface goes out of its way to be useless.
> It puts _effort_ into being useless.)

...without using one or other of the various "download" options.

> Ok, so it's basically:
>
>   void handler(int i) {_exit(0);}
>   int main(int argc, char *argv[])
>   {
>     signal(SIGPIPE, handler);
>
> Hmmm... ok, leaving aside your crash detector probably being
> overzealous, another problem is toysh. It does some nofork()
> commands and if they got a sigpipe the _shell_ would exit.
>
> So my first instinct (puting this in toy_singleinit()) is
> inappropriate, and... I guess it really _does_ belong in
> main(). Then toysh can set its own signal handlers and they
> should stay set. (toy_init() isn't resetting signal
> handlers. There's a gazillion subtle things about
> toys.recursion that I'll need to audit someday, all of
> which really translate to restrictions on who can call
> toy_exec(). The man 2 execve page lists the things I need
> to worry about.)
>
> Why are we calling _exit() instead of exit()? Does flush(0)
> and calling the atexit() chain cause a problem here? (Modulo
> it being done from the signal stack which might be a bit
> cramped.) There's this whole nofork vague design about
> being able to longjmp() from xexit() instead of exit... which
> is of course where the above "gazillion subtle things" would
> really come in, right now it's not a big deal because you
> only really recurse deeply when you "nice nohup taskset setsid
> chroot xargs time" and so on, and those generally aren't doing
> anything fancy with state _other_ than what they're explicitly
> there to manipulate...

yeah, in toolbox there's nothing complicated to worry about, and _exit
just gets me out of there cleanly and quickly.

>> to do this in the
>> driver instead. [pretend for a moment that toolbox isn't
>> going away...]
>
> It's probably not going away by the Android M deadline, anyway.
>
>> any tool that actually cares about SIGPIPE behavior is
>> unaffected because it can install its own SIGPIPE handler.
>> any tool that doesn't care is unaffected because it's still
>> going to exit; it's just going to exit cleanly.
>
> I'll defer to your definition of "cleanly" for android,
> which means I'm adding it with an if (CFG_TOYBOX_ON_ANDROID)
> as a bug workaround for what _sounds_ like an overzealous
> crash detector. If I'm wrong about that and it should be
> enabled generally, I can take the test out later once
> people explain what I got wrong.

(see the response i'm about to send to one of your later mails on this thread.)

>> does this sound like a good idea for toybox?
>
> It sounds like something you need.
>
> Rob



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.