bug#34488: Add sort --limit, or document workarounds for sort|head error messages

[Eric Blake]

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
Description: OpenPGP digital signature