bug#34488: Add sort --limit, or document workarounds for sort|head error messages
On 2/15/19 7:49 AM, 積丹尼 Dan Jacobson wrote: Well the user thinks "hey I cut short 5 lines, 45 lines, 495 lines, and then one I got a job at a big company and cut short 4995 lines and got this error message just when the boss was looking." I'm afraid I'll have to disagree on this one. This is a generic problem for any program that writes to standard output, and is not peculiar to 'sort'. The 'sort' documentation is not the right place to document this issue, just as it's not the right place to document what happens when you type control-C to interrupt a sort.
bug#34488: Add sort --limit, or document workarounds for sort|head error messages
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 | 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 +{
> + 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, ))
> + die (EXIT_CANCELED, errno, _("sigaction1(sig=%d) failed"), i);
> +
> +act.sa_handler = SIG_DFL;
> +if (sigaction (i, , 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 is set up correctly? But what else in 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.
> +++
bug#34488: Add sort --limit, or document workarounds for sort|head error messages
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 | 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.
Usage is:
env --default-signal=PIPE
env -P ##shortcut to reset SIGPIPE
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
comments welcomed,
- assaf
>From d65ddf38cd5cf60ba6fc4f1bf60f7324a3e6bebd Mon Sep 17 00:00:00 2001
From: Assaf Gordon
Date: Fri, 15 Feb 2019 12:31:48 -0700
Subject: [PATCH] env: new option -D/--default-signal=SIG [FIXME]
See https://bugs.gnu.org/34488#8 .
---
src/env.c| 90 +++-
src/local.mk | 1 +
tests/local.mk | 1 +
tests/misc/env-signal-handler.sh | 68 ++
4 files changed, 159 insertions(+), 1 deletion(-)
create mode 100755 tests/misc/env-signal-handler.sh
diff --git a/src/env.c b/src/env.c
index 3a1a3869e..ebda91589 100644
--- a/src/env.c
+++ b/src/env.c
@@ -21,12 +21,16 @@
#include
#include
#include
+#include
+#include
#include
#include "system.h"
#include "die.h"
#include "error.h"
+#include "operand2sig.h"
#include "quote.h"
+#include "sig2str.h"
/* The official name of this program (e.g., no 'g' prefix). */
#define PROGRAM_NAME "env"
@@ -48,7 +52,15 @@ static bool dev_debug;
static char *varname;
static size_t vnlen;
-static char const shortopts[] = "+C:iS:u:v0 \t";
+/* if true, at least one signal handler should be reset. */
+static bool reset_signals ;
+
+/* if element [SIGNUM] is true, the signal handler's should be reset
+ to its defaut. */
+static bool signal_handlers[SIGNUM_BOUND];
+
+
+static char const shortopts[] = "+C:iPS:u:v0 \t";
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'},
{"debug", no_argument, NULL, 'v'},
{"split-string", required_argument, NULL, 'S'},
{GETOPT_HELP_OPTION_DECL},
@@ -88,8 +101,17 @@ Set each NAME to VALUE in the environment and run COMMAND.\n\
-C, --chdir=DIR change working directory to DIR\n\
"), stdout);
fputs (_("\
+ --default-signal=SIG reset signal SIG to its default signal handler.\n\
+multiple signals can be comma-separated.\n\
+"), stdout);
+ fputs (_("\
-S, --split-string=S process and split S into separate arguments;\n\
used to pass multiple arguments on shebang lines\n\
+"), stdout);
+ fputs (_("\
+ -P same as --default-signal=PIPE\n\
+"), stdout);
+ fputs (_("\
-v, --debug print verbose information for each processing step\n\
"), stdout);
fputs (HELP_OPTION_DESCRIPTION, stdout);
@@ -525,6 +547,63 @@ parse_split_string (const char* str, int /*out*/ *orig_optind,
*orig_optind = 0; /* tell getopt to restart from first 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);
+
+ 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; ihttps://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ seq
+trap_sigpipe_or_skip_
+
+# 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
+# -
+#
bug#33468: A bug with yes and --help
On 2/15/19 12:32 PM, Assaf Gordon wrote: > Attached updated patches, with tests. It's easier to review patches when they are attached inline without compression (then I don't have to decompress and copy from my editor back into my email), but I can cope with your .gz attachments. > > comments welcomed, > - assaf > > P.S. > There is at least one change in behavior, not sure if this is > bad enough to be a regression or doesn't really matter: > > $ yes-OLD me -- --help | head -n1 > me -- --help > > $ yes-NEW me -- --help | head -n1 > me --help I would argue bug-fix. My reference is any other getopt-based utility where -- has observable effects when treated as an explicit option: $ echo pattern > ./-- $ grep pattern -- + if RESET_OPTIND=false, optind is left as-is (suitable for programs > + which do not process further optional parameters (but could still s/optional/option/ > + process parameters directly by examining argv[optind]). */ In the coreutils patch: > For select programs which accept only --help and --version options > (in addition to non-options arguments), process these options before s/non-options/non-option/ > any other options. Also, all coreutils callers pass reset_optind==false; does the gnulib interface still need to provide a reset_optind parameter, given that setting the parameter true forces reliance on the getopt-gnu module as currently coded? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
bug#33468: A bug with yes and --help
Hello Eric and all, Thanks for the quick and detailed review. I've amended all the issues you mentioned. On 2019-02-13 8:20 p.m., Eric Blake wrote: 15 files changed, 46 insertions(+), 141 deletions(-) Nice diffstat. These are of course Bernhard's improvements, I just did the testing (and some minor things). diff --git a/NEWS b/NEWS Is "argument" better than "option" here? Or, maybe: now always process --help and --version options, regardless of any other arguments present before any optinoal -- end-of-options marker. I've used your phrasing, and also separated "nohup" from the rest of the programs, as it does not accept --help/--version anywhere, just as first arguments. Attached updated patches, with tests. comments welcomed, - assaf P.S. There is at least one change in behavior, not sure if this is bad enough to be a regression or doesn't really matter: $ yes-OLD me -- --help | head -n1 me -- --help $ yes-NEW me -- --help | head -n1 me --help gnulib-0001-long-options-add-parse_gnu_standard_options_only.patch.gz Description: application/gzip 0001-all-detect-help-and-version-more-consistently.patch.gz Description: application/gzip
bug#34487:
That's was just a *live image* that was required, The equivalent of the present CD/DVD's ISO was then *debian-live-9.7.0-amd64-gnome.iso*.
bug#34475: Mention even more worries for test -a
By the way https://lists.gnu.org/archive/html/bug-bash/2019-02/msg00052.html says "... contradictory description in the coreutils info document."
bug#34488: Add sort --limit, or document workarounds for sort|head error messages
> "EB" == Eric Blake writes: >> And no fair saying "just save the output" (could be big) "into a file >> first, and do head(1) or sed(1) on that." EB> If you have an app that exits noisily on write failures to an early-exit EB> pipe, your solutions are to quit ignoring SIGPIPE, or to change the EB> pipeline to consume all of the output instead of exiting early and EB> causing write failure. OK, it seems that indeed it would then be best to document: If you want to make sure no such errors occur on large output, instead of $ sort | head use $ sort > file; head file as an _easy to remember_ alternative amongst the many. And OK put this in some general place, and have the sort, etc. pages link to it. Thanks.
bug#34490: console the user that his -n in sort --debug -n was seen
Can you please have assure the user in that message it makes
that it has indeed seen his -n/--numeric-sort.
$ sort --debug
sort: using simple byte comparison
$ sort --debug --numeric-sort
sort: using simple byte comparison
unchanged. User gets nervous.
sort (GNU coreutils) 8.30. Sorry we discussed this before but kinda hard to
find...
bug#34488: Add sort --limit, or document workarounds for sort|head error messages
> "AG" == Assaf Gordon writes: AG> The errors are not "random" - they happen because you explicitly AG> cut short the output of a program. Well the user thinks "hey I cut short 5 lines, 45 lines, 495 lines, and then one I got a job at a big company and cut short 4995 lines and got this error message just when the boss was looking." So be sure to mention it. Thanks.
bug#34488: Add sort --limit, or document workarounds for sort|head error messages
severity 34488 wishlist retitle 34488 doc: sort: expand on "broken pipe" (SIGPIPE) behavior stop Hello, On 2019-02-15 7:43 a.m., 積丹尼 Dan Jacobson wrote: Things start out cheery, but quickly get ugly, $ for i in 9 99 999 9; do seq $i|sort -n|sed 5q|wc -l; done 5 5 5 5 sort: write failed: 'standard output': Broken pipe sort: write error 5 sort: write failed: 'standard output': Broken pipe sort: write error Therefore, kindly add a sort --limit=n, I don't think this is wise, as "head -n5" does exactly that in much more generic way. and/or on (info "(coreutils) sort invocation") admit the problem, and give some workarounds, lest our scripts occasionally spew error messages seemingly randomly, just when the boss is looking. Just to clarify: why do you think this a "problem" ? This is the intended behavior of most proper programs: Upon receiving SIGPIPE they should terminal with an error, unless SIGPIPE is explicitly ignored. The errors are not "random" - they happen because you explicitly cut short the output of a program. It is an important indication about how your pipe works, and sort is not to blame, e.g.: $ seq 10 | head -n1 1 seq: write error: Broken pipe $ seq 100| cat | head -n1 1 cat: write error: Broken pipe seq: write error: Broken pipe This is a good indication that the entire output was not consumed, and is very useful and important in some cases, e.g. when a program crashes before consuming all input. Here's a contrived example: $ seq 100 | sort -S 200 -T /foo/bar sort: cannot create temporary file in '/foo/bar': No such file or directory seq: write error: Broken pipe I force "sort" to fail (limiting it's memory usage and pointing it to non-existing temporarily directory). It is then good to know that seq's output was cut short and not consumed. If you know in advance you will trim the output of a program, either hide the stderr with "2>/dev/null", or use the shell's "trap PIPE" mechanism. And no fair saying "just save the output" (could be big) "into a file first, and do head(1) or sed(1) on that." If you want to consume all input and just print the first 5 lines, you can use "sed -n 1,5p" instead of "sed 5q" - no need for a temporary file. I'm marking this as a documentation "wishlist" item, and patches are always welcomed. regards, - assaf
bug#34488: Add sort --limit, or document workarounds for sort|head error messages
On 2/15/19 8:43 AM, 積丹尼 Dan Jacobson wrote: > Things start out cheery, but quickly get ugly, > > $ for i in 9 99 999 9; do seq $i|sort -n|sed 5q|wc -l; done > 5 > 5 > 5 > 5 > sort: write failed: 'standard output': Broken pipe > sort: write error > 5 > sort: write failed: 'standard output': Broken pipe > sort: write error Your code is demonstrating what happens when sed ends processing without consuming all of sort's output, to the point that sort is producing more output than what stdio can buffer, and thus with enough pending output will trigger a situation of SIGPIPE - but when SIGPIPE is ignored, that instead turns into an EPIPE failure to write(), and sort treats all write() failures as worthy of error reporting. If you want sort to die silently, then don't ignore SIGPIPE, so that sort won't see EPIPE and thus won't be noisy. $ (trap '' PIPE; seq | sort -n | sed 5q | wc -l) 5 sort: write failed: 'standard output': Broken pipe sort: write error $ (trap - PIPE; seq | sort -n | sed 5q | wc -l) 5 Except that POSIX has the nasty requirement 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 | sort -n | sed 5q | wc -l') 5 sort: write failed: 'standard output': Broken pipe sort: write error And unfortunately, there are several common cases of badly-behaved environment setups that leave SIGPIPE ignored in the child (for example, a quick google search found this: https://blog.nelhage.com/2010/02/a-very-subtle-bug/) You HAVE to use some other intermediate program if you want to override an inherited ignored SIGPIPE in sh into an inherited default-behavior SIGPIPE in sort. Perhaps coreutils should teach 'env' a command-line option to forcefully reset SIGPIPE back to default behavior (or add a new coreutil that does the same idea), as a way to work around POSIX' requirement on sh. 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 | env --default-signal PIPE sort -n | sed 5q | wc -l 5 and avoid the EPIPE failures because sort is forced to start with SIGPIPE handling rather than ignored. > > Therefore, kindly add a sort --limit=n, Not scalable. The problem you encountered is NOT the fault of sort, but is common to ALL utilities which dutifully report write() failures on EPIPE errors, which in turn happens when SIGPIPE is ignored. Adding an option to every such utility that produces outtput is not a good use of time. If you want to change things universally, perhaps you should petition POSIX to change the requirements to allow applications the option to silently exit without an error message on EPIPE failures to write(), instead of the current wording that all write() errors must be diagnosed. > and/or on (info "(coreutils) sort invocation") > admit the problem, and give some workarounds, lest > our scripts occasionally spew error messages seemingly randomly, > just when the boss is looking. The problem is not in sort, but in the fact that your environment is ignoring SIGPIPE. Documenting something in sort doesn't scale, but perhaps the documentation could mention SIGPIPE considerations in a more global chapter covering all of the coreutils. > > And no fair saying "just save the output" (could be big) "into a file > first, and do head(1) or sed(1) on that." > If you have an app that exits noisily on write failures to an early-exit pipe, your solutions are to quit ignoring SIGPIPE, or to change the pipeline to consume all of the output instead of exiting early and causing write failure. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
bug#34475: Mention even more worries for test -a
severity 34475 wishlist retitle 34475 doc: test: expand on -a/-o usage stop Hello, On 2019-02-13 6:00 p.m., 積丹尼 Dan Jacobson wrote: First, on the test(1) man page, at [...]> Say instead [...] I'm marking this as "wishlist" item, patches always welcomed. -assaf
bug#34487: dd (coreutils) 8.30 – A written ISO image cannot not be booted from BIOS
tags 34487 notabug close 34487 stop Hello, On 2019-02-15 5:02 a.m., Ricky Tigg wrote: Hi. An ISO image cannot not be booted from BIOS [...] # dd if=debian-9.7.0-amd64-DVD-3.iso of=/dev/sdc A CD/DVD image is not the same as a hard drive. The internal structure differs, and one can not be copied to the other and expected to work. Since this is not a bug in dd, I'm closing this item. For general help regarding operating system installation, please contact the relevant operating system's mailing list. In this case, likely Debian-user mailing list: https://lists.debian.org/debian-user/ See also https://wiki.debian.org/DebianMailingLists . regards, -assaf
bug#34488: Add sort --limit, or document workarounds for sort|head error messages
Things start out cheery, but quickly get ugly, $ for i in 9 99 999 9; do seq $i|sort -n|sed 5q|wc -l; done 5 5 5 5 sort: write failed: 'standard output': Broken pipe sort: write error 5 sort: write failed: 'standard output': Broken pipe sort: write error Therefore, kindly add a sort --limit=n, and/or on (info "(coreutils) sort invocation") admit the problem, and give some workarounds, lest our scripts occasionally spew error messages seemingly randomly, just when the boss is looking. And no fair saying "just save the output" (could be big) "into a file first, and do head(1) or sed(1) on that."
bug#34487: dd (coreutils) 8.30 – A written ISO image cannot not be booted from BIOS
Hi. An ISO image cannot not be booted from BIOS Initial device state: # parted /dev/sdc unit s print free Model: (...) (scsi) Disk /dev/sdc: 15638480s Sector size (logical/physical): 512B/512B Partition Table: msdos Disk Flags: Number Start End SizeType File system Flags 2s15638479s 15638478s Free Space Then commands executed were (Note: a non-live ISO image is involved): # dd if=debian-9.7.0-amd64-DVD-3.iso of=/dev/sdc bs=4M oflag=direct && sync $ df -hT | tail -n1 /dev/sdciso9660 4.4G 4.4G 0 100% /run/media/yk/Debian 9.7.0 amd64 3 # parted /dev/sdc unit s print free Error: /dev/sdc: unrecognised disk label Model: (...) (scsi) Disk /dev/sdc: 15638480s Sector size (logical/physical): 512B/512B Partition Table: unknown Disk Flags: That dd command was also executed without option/parameter 'oflag=direct'. but final result remained identical. I could notice that *dd* erased existent *partition table* and didn't add flag *boot.* Once at last the system was rebooted, then proper device selected from BIOS , the following message was produced: > *Reboot and select proper boot device or insert boot media in selected > boot device and press a key.* >
