On 09/26/2014 09:58 PM, Bernhard Voelker wrote: > On 09/26/2014 05:05 PM, Pádraig Brady wrote: >> Subject: [PATCH] dd: use more robust SIGUSR handling > > s/USR/USR1/ > >> * src/dd.c (ifd_reopen): A new wrapper to ensure we >> don't exit upon receiving a SIGUSR1 in a blocking open() >> on a fifo for example.
I also added an EINTR wrapper for ftruncate(). Note I considered using SA_RESTART instead, but that would mean that uncommitted read()/write() would block on SIGUSR1. I could have undone the SA_RESTART before dd_copy(), though that would be messier I think. >> (iread): Process signals also after a short read. >> (install_signal_handlers): Install SIGINFO/SIGUSR1 handler >> even if set to SIG_IGN, as this is what the parent can easily >> set from a shell script that can send SIGUSR1 without the >> possiblity of inadvertently killing the dd process. >> * doc/corutils.texi (dd invocation): Improve the example to >> show robust usage wrt signal races and short reads. >> * tests/dd/stats.sh: A new test for various signal races. >> * tests/local.mk: Reference the new test. >> * NEWS: Mention the fix. > > Another minor nit: > s/corutils/coreutils/ > > The rest LGTM. > > What about adding "trap '' USR1;" to usage(), too? > You know, many folks are only reading that instead of the > texinfo manual. Yes there should be no inconsistency here. > OTOH, that stats-on-signal feature is such a detail that it may > be worth removing from the manpage at all. I didn't notice it in the man page. I think it's best to leave that detail to the full info page, so I've removed that. Thanks for the review! Latest version is attached. Pádraig.
>From 36240525f010fb1d6d8c52f4d70731c652130f8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Fri, 26 Sep 2014 15:46:28 +0100 Subject: [PATCH] dd: use more robust SIGUSR1 handling * src/dd.c (ifd_reopen): A new wrapper to ensure we don't exit upon receiving a SIGUSR1 in a blocking open() on a fifo for example. (iftruncate): Likewise for ftruncate(). (iread): Process signals also after a short read. (install_signal_handlers): Install SIGINFO/SIGUSR1 handler even if set to SIG_IGN, as this is what the parent can easily set from a shell script that can send SIGUSR1 without the possiblity of inadvertently killing the dd process. * doc/coreutils.texi (dd invocation): Improve the example to show robust usage wrt signal races and short reads. * tests/dd/stats.sh: A new test for various signal races. * tests/local.mk: Reference the new test. * NEWS: Mention the fix. --- NEWS | 3 ++ doc/coreutils.texi | 37 +++++++++++++++++++--------- src/dd.c | 69 +++++++++++++++++++++++++++++++++++++--------------- tests/dd/stats.sh | 57 +++++++++++++++++++++++++++++++++++++++++++ tests/local.mk | 1 + 5 files changed, 135 insertions(+), 32 deletions(-) create mode 100755 tests/dd/stats.sh diff --git a/NEWS b/NEWS index 077c5fc..3e10ac4 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,9 @@ GNU coreutils NEWS -*- outline -*- ** Bug fixes + dd supports more robust SIGINFO/SIGUSR1 handling for outputting statistics. + Previously those signals may have inadvertently terminated the process. + cp no longer issues an incorrect warning about directory hardlinks when a source directory is specified multiple times. Now, consistent with other file types, a warning is issued for source directories with duplicate names, diff --git a/doc/coreutils.texi b/doc/coreutils.texi index 1519fcb..7d32af5 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -9001,23 +9001,36 @@ occur on disk based devices): dd conv=noerror,sync iflag=fullblock </dev/sda1 > /mnt/rescue.img @end example -Sending an @samp{INFO} signal to a running @command{dd} -process makes it print I/O statistics to standard error -and then resume copying. In the example below, -@command{dd} is run in the background to copy 10 million blocks. +Sending an @samp{INFO} signal (or @samp{USR1} signal where that is unavailable) +to a running @command{dd} process makes it print I/O statistics to +standard error and then resume copying. In the example below, +@command{dd} is run in the background to copy 5GB of data. The @command{kill} command makes it output intermediate I/O statistics, and when @command{dd} completes normally or is killed by the @code{SIGINT} signal, it outputs the final statistics. @example -$ dd if=/dev/zero of=/dev/null count=10MB & pid=$! -$ kill -s INFO $pid; wait $pid -3385223+0 records in -3385223+0 records out -1733234176 bytes (1.7 GB) copied, 6.42173 seconds, 270 MB/s -10000000+0 records in -10000000+0 records out -5120000000 bytes (5.1 GB) copied, 18.913 seconds, 271 MB/s +# Ignore the signal so we never inadvertently terminate the dd child. +# Note this is not needed when SIGINFO is available. +trap '' USR1 + +# Run dd with the fullblock iflag to avoid short reads +# which can be triggered by reception of signals. +dd iflag=fullblock if=/dev/zero of=/dev/null count=5000000 bs=1000 & pid=$! + +# Output stats every half second +until ! kill -s USR1 $pid 2>/dev/null; do sleep .5; done +@end example + +The above script will output in the following format + +@example +859+0 records in +859+0 records out +4295000000 bytes (4.3 GB) copied, 0.539934 s, 8.0 GB/s +1000+0 records in +1000+0 records out +5000000000 bytes (5.0 GB) copied, 0.630785 s, 7.9 GB/s @end example @vindex POSIXLY_CORRECT diff --git a/src/dd.c b/src/dd.c index c17f7d8..d22ec59 100644 --- a/src/dd.c +++ b/src/dd.c @@ -627,22 +627,14 @@ Each FLAG symbol may be:\n\ "), stdout); { - char const *siginfo_name = (SIGINFO == SIGUSR1 ? "USR1" : "INFO"); printf (_("\ \n\ Sending a %s signal to a running 'dd' process makes it\n\ print I/O statistics to standard error and then resume copying.\n\ \n\ - $ dd if=/dev/zero of=/dev/null& pid=$!\n\ - $ kill -%s $pid; sleep 1; kill $pid\n\ - 18335302+0 records in\n\ - 18335302+0 records out\n\ - 9387674624 bytes (9.4 GB) copied, 34.6279 seconds, 271 MB/s\n\ -\n\ Options are:\n\ \n\ -"), - siginfo_name, siginfo_name); +"), SIGINFO == SIGUSR1 ? "USR1" : "INFO"); } fputs (HELP_OPTION_DESCRIPTION, stdout); @@ -830,11 +822,7 @@ install_signal_handlers (void) struct sigaction act; sigemptyset (&caught_signals); if (catch_siginfo) - { - sigaction (SIGINFO, NULL, &act); - if (act.sa_handler != SIG_IGN) - sigaddset (&caught_signals, SIGINFO); - } + sigaddset (&caught_signals, SIGINFO); sigaction (SIGINT, NULL, &act); if (act.sa_handler != SIG_IGN) sigaddset (&caught_signals, SIGINT); @@ -843,6 +831,9 @@ install_signal_handlers (void) if (sigismember (&caught_signals, SIGINFO)) { act.sa_handler = siginfo_handler; + /* Note we don't use SA_RESTART here and instead + handle EINTR explicitly in iftruncate() etc. + to avoid blocking on noncommitted read()/write() calls. */ act.sa_flags = 0; sigaction (SIGINFO, &act, NULL); } @@ -856,7 +847,7 @@ install_signal_handlers (void) #else - if (catch_siginfo && signal (SIGINFO, SIG_IGN) != SIG_IGN) + if (catch_siginfo) { signal (SIGINFO, siginfo_handler); siginterrupt (SIGINFO, 1); @@ -1033,6 +1024,10 @@ iread (int fd, char *buf, size_t size) } while (nread < 0 && errno == EINTR); + /* Short read may be due to received signal. */ + if (0 < nread && nread < size) + process_signals (); + if (0 < nread && warn_partial_read) { static ssize_t prev_nread; @@ -1173,6 +1168,40 @@ write_output (void) oc = 0; } +/* Restart on EINTR from fd_reopen(). */ + +static int +ifd_reopen (int desired_fd, char const *file, int flag, mode_t mode) +{ + int ret; + + do + { + process_signals (); + ret = fd_reopen (desired_fd, file, flag, mode); + } + while (ret < 0 && errno == EINTR); + + return ret; +} + +/* Restart on EINTR from ftruncate(). */ + +static int +iftruncate (int fd, off_t length) +{ + int ret; + + do + { + process_signals (); + ret = ftruncate (fd, length); + } + while (ret < 0 && errno == EINTR); + + return ret; +} + /* Return true if STR is of the form "PATTERN" or "PATTERNDELIM...". */ static bool _GL_ATTRIBUTE_PURE @@ -2172,7 +2201,7 @@ dd_copy (void) off_t output_offset = lseek (STDOUT_FILENO, 0, SEEK_CUR); if (output_offset > stdout_stat.st_size) { - if (ftruncate (STDOUT_FILENO, output_offset) != 0) + if (iftruncate (STDOUT_FILENO, output_offset) != 0) { error (0, errno, _("failed to truncate to %" PRIdMAX " bytes" @@ -2248,7 +2277,7 @@ main (int argc, char **argv) } else { - if (fd_reopen (STDIN_FILENO, input_file, O_RDONLY | input_flags, 0) < 0) + if (ifd_reopen (STDIN_FILENO, input_file, O_RDONLY | input_flags, 0) < 0) error (EXIT_FAILURE, errno, _("failed to open %s"), quote (input_file)); } @@ -2275,8 +2304,8 @@ main (int argc, char **argv) need to read to satisfy a 'seek=' request. If we can't read the file, go ahead with write-only access; it might work. */ if ((! seek_records - || fd_reopen (STDOUT_FILENO, output_file, O_RDWR | opts, perms) < 0) - && (fd_reopen (STDOUT_FILENO, output_file, O_WRONLY | opts, perms) + || ifd_reopen (STDOUT_FILENO, output_file, O_RDWR | opts, perms) < 0) + && (ifd_reopen (STDOUT_FILENO, output_file, O_WRONLY | opts, perms) < 0)) error (EXIT_FAILURE, errno, _("failed to open %s"), quote (output_file)); @@ -2293,7 +2322,7 @@ main (int argc, char **argv) " (%lu-byte) blocks"), seek_records, obs); - if (ftruncate (STDOUT_FILENO, size) != 0) + if (iftruncate (STDOUT_FILENO, size) != 0) { /* Complain only when ftruncate fails on a regular file, a directory, or a shared memory object, as POSIX 1003.1-2004 diff --git a/tests/dd/stats.sh b/tests/dd/stats.sh new file mode 100755 index 0000000..386752e --- /dev/null +++ b/tests/dd/stats.sh @@ -0,0 +1,57 @@ +#!/bin/sh +# Check robust handling of SIG{INFO,USR1} + +# Copyright (C) 2014 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src +print_ver_ dd + +env kill -l | grep '^INFO$' && SIGINFO='INFO' || SIGINFO='USR1' + +# This to avoid races in the USR1 case +# as the dd process will terminate by default until +# it has its handler enabled. +trap '' $SIGINFO + +mkfifo_or_skip_ fifo + +for open in '' '1'; do + # Run dd with the fullblock iflag to avoid short reads + # which can be triggered by reception of signals + dd iflag=fullblock if=/dev/zero of=fifo count=100 bs=5000000 2>err & pid=$! + + # Note if we sleep here we give dd a chance to exec and block on open. + # Otherwise we're probably testing SIG_IGN in the forked shell or early dd. + test "$open" && sleep .1 + + # dd will block on open until fifo is opened for reading. + # Timeout in case dd goes away erroneously which we check for below. + timeout 10 sh -c 'wc -c < fifo > nwritten' & + + # Send lots of signals immediately to ensure dd not killed due + # to race setting handler, or blocking on open of fifo. + # Many signals also check that short reads are handled. + until ! kill -s $SIGINFO $pid 2>/dev/null; do + sleep .01 + done + + wait + + # Ensure all data processed and at least last status written + grep '500000000 bytes .* copied' err || { cat err; fail=1; } +done + +Exit $fail diff --git a/tests/local.mk b/tests/local.mk index 97bf5ed..8498acb 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -490,6 +490,7 @@ all_tests = \ tests/dd/stderr.sh \ tests/dd/unblock.pl \ tests/dd/unblock-sync.sh \ + tests/dd/stats.sh \ tests/df/total-verify.sh \ tests/du/2g.sh \ tests/du/8gb.sh \ -- 1.7.7.6
