Package: coreutils;
Reported by: 積丹尼 Dan Jacobson <jidanni <at> jidanni.org>
Date: Fri, 15 Feb 2019 14:53:01 UTC
Severity: normal
Tags: fixed
Done: Assaf Gordon <assafgordon <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Eric Blake <eblake <at> redhat.com> To: Assaf Gordon <assafgordon <at> gmail.com>, 積丹尼 Dan Jacobson <jidanni <at> jidanni.org>, 34488 <at> debbugs.gnu.org Subject: bug#34488: Add sort --limit, or document workarounds for sort|head error messages Date: Fri, 15 Feb 2019 16:11:10 -0600
[Message part 1 (text/plain, inline)]
On 2/15/19 3:40 PM, Assaf Gordon wrote:
> Helo,
>
> On 2019-02-15 8:20 a.m., Eric Blake wrote:
>> On 2/15/19 8:43 AM, 積丹尼 Dan Jacobson wrote:
>>> sort: write failed: 'standard output': Broken pipe
>>> sort: write error
> [...]
>> Perhaps coreutils should teach 'env' a command-line option to forcefully
>> reset SIGPIPE back to default behavior [...] If we
>> did that, then even if your sh is started with SIGPIPE ignored (so that
>> the shell itself can't restore default behavior), you could do this
>> theoretical invocation:
>>
>> $ seq 9999 | env --default-signal PIPE sort -n | sed 5q | wc -l
>> 5
>
> That is a nice idea, I could've used it myself couple of times.
>
> Attached a suggested patch.
> If this seems like a good direction, I'll complete it with NEWS/docs/etc.
Would we also want an easy way to ignore signals? That's a bit less of
an issue, since you can use 'trap "" $SIG' in the shell; but having the
symmetry in env may be nice (especially since using 'trap' is asymmetric
in that you can't force the shell to un-ignore an inherited ignored signal).
>
> Usage is:
> env --default-signal=PIPE
> env -P ##shortcut to reset SIGPIPE
Nice, since that's probably the most common use case for it.
> env --default-signal=PIPE,INT,FOO
>
>
> This also works nicely with the recent 'env -S' option,
> so a script like so can always start with default SIGPIPE handler:
>
> #!/usr/bin/env -S -P sh
> seq inf | head -n1
Also nice.
>
> -static char const shortopts[] = "+C:iS:u:v0 \t";
> +/* if true, at least one signal handler should be reset. */
> +static bool reset_signals ;
Extra space.
> +
> +/* if element [SIGNUM] is true, the signal handler's should be reset
> + to its defaut. */
default
> +static bool signal_handlers[SIGNUM_BOUND];
> +
> +
> +static char const shortopts[] = "+C:iPS:u:v0 \t";
In the patch subject, you mentioned -D as a synonym to --default-signal,
but it's missing here. (-P as a synonym to --default-signal=PIPE is
fine, though)
>
> static struct option const longopts[] =
> {
> @@ -56,6 +68,7 @@ static struct option const longopts[] =
> {"null", no_argument, NULL, '0'},
> {"unset", required_argument, NULL, 'u'},
> {"chdir", required_argument, NULL, 'C'},
> + {"default-signal", optional_argument, NULL, 'P'},
Wait, you're making -P a synonym to --default-signal, even though -P
takes no options but --default-signal does? And the fact that it is
optional_argument means that you can't have a short option -D (that is,
'--default-signal FOO' is NOT the same as '--default-signal=FOO', but
treats FOO as a program name rather than a signal list). optional
arguments can be odd; I'd consider using required_argument.
> +static void
> +parse_signal_params (const char* optarg)
> +{
> + char signame[SIG2STR_MAX];
> + char *opt_sig;
> + char *optarg_writable = xstrdup (optarg);
> +
> + opt_sig = strtok (optarg_writable, ",");
> + while (opt_sig)
> + {
> + int signum = operand2sig (opt_sig, signame);
> + if (signum < 0)
> + usage (EXIT_FAILURE);
Is blind usage() the best, or should we quote the unrecognized signal
name via error() to call attention to which part of the command line failed?
> +
> + signal_handlers[signum] = true;
> +
> + opt_sig = strtok (NULL, ",");
> + }
> +
> + free (optarg_writable);
> +}
> +
> +static void
> +reset_signal_handlers (void)
> +{
> +
> + if (!reset_signals)
> + return;
> +
> + if (dev_debug)
> + devmsg ("Resetting signal handlers:\n");
> +
> + for (int i=0; i<SIGNUM_BOUND; ++i)
Spaces around =
> + {
> + struct sigaction act;
> +
> + if (!signal_handlers[i])
> + continue;
> +
> + if (dev_debug)
> + {
> + char signame[SIG2STR_MAX];
> + sig2str (i, signame);
> + devmsg (" %s (%d)\n", signame, i);
> + }
> +
> + if (sigaction (i, NULL, &act))
> + die (EXIT_CANCELED, errno, _("sigaction1(sig=%d) failed"), i);
> +
> + act.sa_handler = SIG_DFL;
> + if (sigaction (i, &act, NULL))
> + die (EXIT_CANCELED, errno, _("sigaction2(sig=%d) failed"), i);
Why do you have to call sigaction() twice? Is it to make sure the rest
of &act is set up correctly? But what else in &act needs setup if you
are going to set things to SIG_DFL?
> +
> +
> + }
> +}
Why 2 blank lines?
> +
> int
> main (int argc, char **argv)
> {
> @@ -558,6 +637,13 @@ main (int argc, char **argv)
> case '0':
> opt_nul_terminate_output = true;
> break;
> + case 'P':
> + reset_signals = true;
> + if (optarg)
> + parse_signal_params (optarg);
> + else
> + signal_handlers[SIGPIPE] = true;
> + break;
Hmm - you made it optional so that '--default-signal' and
'--default-signal=PIPE' can be synonyms. If so, the help text in
usage() should definitely call out "--default-signal[=LIST]" to make it
obvious that LIST is an optional argument.
> +++ b/tests/misc/env-signal-handler.sh
> +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
> +print_ver_ seq
> +trap_sigpipe_or_skip_
Hmm - I wonder if trap_sigpipe_or_skip_ could be updated to take
advantage of our just-built env :)
> +
> +# Paraphrasing http://bugs.gnu.org/34488#8:
> +# POSIX requires that sh started with an inherited ignored SIGPIPE must
> +# silently ignore all attempts from within the shell to restore SIGPIPE
> +# handling to child processes of the shell:
> +#
> +# $ (trap '' PIPE; bash -c 'trap - PIPE; seq inf | head -n1')
> +# 1
> +# seq: write error: Broken pipe
> +#
> +# With 'env --default-signal=PIPE', the signal handler can be reset to its
> +# default.
> +
> +# Baseline Test
> +# -------------
> +# Ensure this results in a "broken pipe" error (the first 'trap'
> +# sets SIGPIPE to ignore, and the second 'trap' becomes a no-op instead
> +# of resetting SIGPIPE to its default). Upon a SIGPIPE 'seq' will not be
> +# terminated, instead its write(2) call will return an error.
> +(trap '' PIPE; sh -c 'trap - PIPE; seq 999999 2>err1t | head -n1 > out1')
> +
> +# The exact broken pipe message depends on the operating system, just ensure
> +# there was a 'write error' message in stderr:
> +sed 's/^\(seq: write error:\) .*/\1/' err1t > err1 || framework_failure_
> +
> +printf "1\n" > exp-out || framework_failure_
> +printf "seq: write error:\n" > exp-err1 || framework_failure_
> +
> +compare exp-out out1 || framework_failure_
> +compare exp-err1 err1 || framework_failure_
Nice setup.
> +
> +
> +# env test
> +# --------
> +# With env resetting the signal handler to its defaults, there should be no
> +# error message (because the default SIGPIPE action is to terminate the
> +# 'seq' program):
> +(trap '' PIPE;
> + env --default-signal=PIPE \
> + sh -c 'trap - PIPE; seq 999999 2>err2 | head -n1 > out2')
Perhaps you could also test:
(trap '' PIPE; env --default-signal=PIPE seq 999999 2>err3 | head -n1 >
out3)
But in general I like the patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[signature.asc (application/pgp-signature, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.