On 01/29/2014 11:12 PM, Bernhard Voelker wrote:
> On 01/30/2014 12:02 AM, Bernhard Voelker wrote:
>> On 01/29/2014 07:43 PM, Pádraig Brady wrote:
>>> On 01/29/2014 12:06 PM, Pádraig Brady wrote:
>>>
>>>> --- a/NEWS
>>>> +++ b/NEWS
>>>
>>>> + tail now diagnoses all failures when writing to stdout. Previously
>>>> write
>>>> + errors could have been silently ignored if some data was output.
>>>> + [This bug was present in "the beginning".]
>>>
>>> Actually this is not the case. The atexit handler would have caught
>>> partial write failures, albeit with a less detailed error message.
>>> So I'll remove the NEWS but leave the code changes.
>>
>> Thanks.
>>
>> It seems your patch didn't make it thru the mail systems:
>>
>> error: patch failed: src/head.c:504
>> error: src/head.c: patch does not apply
>> Patch failed at 0001 head,tail: consistently diagnose write errors
>>
>> Can you resend as attachment, please?
>
> Sorry, I missed that this patch was based on the other patch for
> bug http://bugs.gnu.org/16329. After that, it applies cleanly.
>
>> However, if head is exiting earlier on the first write error,
>> then there should be a user visible change.
>>
>> $ seq -f'%1000g' 1000 | { src/head --lines=-1 > /dev/full ; wc -l ; }
>> src/head: write error
>> 0
>>
>> With the patch, I'd expect wc to see more than 0 lines in the above
>> case, right?
>
> Indeed, now with the patch:
>
> $ seq -f'%1000g' 1000 | { src/head --lines=-1 > /dev/full ; wc -l ; }
> src/head: write error: No space left on device
> src/head: write error
> 984
Good point.
I don't think this could cause issues though, as if you're
reliant on position adjustments by head(1), then you should
be doing `head ... && ....` rather than `head ... ; ...`
Or looking at it another way, if head(1) can't do the
output operations asked of it, is there any point proceeding
with the input operations?
So we probably don't need to document this change in behavior.
> Unfortunately, the atexit code now produces a second error diagnostic.
> It doesn't hurt, but it looks a bit ugly.
We discussed that foible previously, and thought it not worth
adding the extra complexity to avoid in general.
http://lists.gnu.org/archive/html/coreutils/2011-05/msg00061.html
Though in this specific case we're using wrapper functions
instead of fwrite() so we can easily add the clearerr() there.
Updated patch attached.
thanks!
Pádraig.
>From c4baa1e21a88e44af8c0592fdb1beccb5c338a7e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Wed, 29 Jan 2014 04:42:56 +0000
Subject: [PATCH] head,tail: consistently diagnose write errors
If we can't output more data, we should immediately
diagnose the issue and exit rather than consuming all
of input (in some cases).
* src/tail.c (xwrite_stdout): Also diagnose the case where
only some data is written. Also clearerr() to avoid the
redundant less specific error from atexit (close_stdout);
* src/head.c (xwrite_stdout): Copy this new function from tail,
and use it to write all output.
* tests/misc/head-write-error.sh: A new test to ensure we
exit immediately on write error.
* tests/local.mk: Reference the new test.
---
src/head.c | 85 +++++++++++++++++-----------------------
src/tail.c | 20 ++++++---
tests/local.mk | 1 +
tests/misc/head-write-error.sh | 47 ++++++++++++++++++++++
4 files changed, 97 insertions(+), 56 deletions(-)
create mode 100755 tests/misc/head-write-error.sh
diff --git a/src/head.c b/src/head.c
index ef368d7..3145fa7 100644
--- a/src/head.c
+++ b/src/head.c
@@ -70,7 +70,6 @@ enum Copy_fd_status
{
COPY_FD_OK = 0,
COPY_FD_READ_ERROR,
- COPY_FD_WRITE_ERROR,
COPY_FD_UNEXPECTED_EOF
};
@@ -147,9 +146,6 @@ diagnose_copy_fd_failure (enum Copy_fd_status err, char const *filename)
case COPY_FD_READ_ERROR:
error (0, errno, _("error reading %s"), quote (filename));
break;
- case COPY_FD_WRITE_ERROR:
- error (0, errno, _("error writing %s"), quote (filename));
- break;
case COPY_FD_UNEXPECTED_EOF:
error (0, errno, _("%s: file has shrunk too much"), quote (filename));
break;
@@ -167,11 +163,24 @@ write_header (const char *filename)
first_file = false;
}
-/* Copy no more than N_BYTES from file descriptor SRC_FD to O_STREAM.
- Return an appropriate indication of success or failure. */
+/* Write N_BYTES from BUFFER to stdout.
+ Exit immediately on error with a single diagnostic. */
+
+static void
+xwrite_stdout (char const *buffer, size_t n_bytes)
+{
+ if (n_bytes > 0 && fwrite (buffer, 1, n_bytes, stdout) < n_bytes)
+ {
+ clearerr (stdout); /* To avoid redundant close_stdout diagnostic. */
+ error (EXIT_FAILURE, errno, _("write error"));
+ }
+}
+
+/* Copy no more than N_BYTES from file descriptor SRC_FD to stdout.
+ Return an appropriate indication of success or read failure. */
static enum Copy_fd_status
-copy_fd (int src_fd, FILE *o_stream, uintmax_t n_bytes)
+copy_fd (int src_fd, uintmax_t n_bytes)
{
char buf[BUFSIZ];
const size_t buf_size = sizeof (buf);
@@ -189,8 +198,7 @@ copy_fd (int src_fd, FILE *o_stream, uintmax_t n_bytes)
if (n_read == 0 && n_bytes != 0)
return COPY_FD_UNEXPECTED_EOF;
- if (fwrite (buf, 1, n_read, o_stream) < n_read)
- return COPY_FD_WRITE_ERROR;
+ xwrite_stdout (buf, n_read);
}
return COPY_FD_OK;
@@ -282,22 +290,12 @@ elide_tail_bytes_pipe (const char *filename, int fd, uintmax_t n_elide_0)
/* Output any (but maybe just part of the) elided data from
the previous round. */
- if ( ! first)
- {
- /* Don't bother checking for errors here.
- If there's a failure, the test of the following
- fwrite or in close_stdout will catch it. */
- fwrite (b[!i] + READ_BUFSIZE, 1, n_elide - delta, stdout);
- }
+ if (! first)
+ xwrite_stdout (b[!i] + READ_BUFSIZE, n_elide - delta);
first = false;
- if (n_elide < n_read
- && fwrite (b[i], 1, n_read - n_elide, stdout) < n_read - n_elide)
- {
- error (0, errno, _("write error"));
- ok = false;
- break;
- }
+ if (n_elide < n_read)
+ xwrite_stdout (b[i], n_read - n_elide);
}
free (b[0]);
@@ -357,14 +355,7 @@ elide_tail_bytes_pipe (const char *filename, int fd, uintmax_t n_elide_0)
buffered_enough = true;
if (buffered_enough)
- {
- if (fwrite (b[i_next], 1, n_read, stdout) < n_read)
- {
- error (0, errno, _("write error"));
- ok = false;
- goto free_mem;
- }
- }
+ xwrite_stdout (b[i_next], n_read);
}
/* Output any remainder: rem bytes from b[i] + n_read. */
@@ -375,12 +366,12 @@ elide_tail_bytes_pipe (const char *filename, int fd, uintmax_t n_elide_0)
size_t n_bytes_left_in_b_i = READ_BUFSIZE - n_read;
if (rem < n_bytes_left_in_b_i)
{
- fwrite (b[i] + n_read, 1, rem, stdout);
+ xwrite_stdout (b[i] + n_read, rem);
}
else
{
- fwrite (b[i] + n_read, 1, n_bytes_left_in_b_i, stdout);
- fwrite (b[i_next], 1, rem - n_bytes_left_in_b_i, stdout);
+ xwrite_stdout (b[i] + n_read, n_bytes_left_in_b_i);
+ xwrite_stdout (b[i_next], rem - n_bytes_left_in_b_i);
}
}
else if (i + 1 == n_bufs)
@@ -399,7 +390,7 @@ elide_tail_bytes_pipe (const char *filename, int fd, uintmax_t n_elide_0)
*/
size_t y = READ_BUFSIZE - rem;
size_t x = n_read - y;
- fwrite (b[i_next], 1, x, stdout);
+ xwrite_stdout (b[i_next], x);
}
}
@@ -458,7 +449,7 @@ elide_tail_bytes_file (const char *filename, int fd, uintmax_t n_elide)
return false;
}
- err = copy_fd (fd, stdout, bytes_remaining - n_elide);
+ err = copy_fd (fd, bytes_remaining - n_elide);
if (err == COPY_FD_OK)
return true;
@@ -504,7 +495,7 @@ elide_tail_lines_pipe (const char *filename, int fd, uintmax_t n_elide)
if (! n_elide)
{
- fwrite (tmp->buffer, 1, n_read, stdout);
+ xwrite_stdout (tmp->buffer, n_read);
continue;
}
@@ -543,7 +534,7 @@ elide_tail_lines_pipe (const char *filename, int fd, uintmax_t n_elide)
last = last->next = tmp;
if (n_elide < total_lines - first->nlines)
{
- fwrite (first->buffer, 1, first->nbytes, stdout);
+ xwrite_stdout (first->buffer, first->nbytes);
tmp = first;
total_lines -= first->nlines;
first = first->next;
@@ -572,7 +563,7 @@ elide_tail_lines_pipe (const char *filename, int fd, uintmax_t n_elide)
for (tmp = first; n_elide < total_lines - tmp->nlines; tmp = tmp->next)
{
- fwrite (tmp->buffer, 1, tmp->nbytes, stdout);
+ xwrite_stdout (tmp->buffer, tmp->nbytes);
total_lines -= tmp->nlines;
}
@@ -588,7 +579,7 @@ elide_tail_lines_pipe (const char *filename, int fd, uintmax_t n_elide)
++tmp->nlines;
--n;
}
- fwrite (tmp->buffer, 1, p - tmp->buffer, stdout);
+ xwrite_stdout (tmp->buffer, p - tmp->buffer);
}
free_lbuffers:
@@ -684,7 +675,7 @@ elide_tail_lines_seekable (const char *pretty_filename, int fd,
return false;
}
- err = copy_fd (fd, stdout, pos - start_pos);
+ err = copy_fd (fd, pos - start_pos);
if (err != COPY_FD_OK)
{
diagnose_copy_fd_failure (err, pretty_filename);
@@ -693,10 +684,8 @@ elide_tail_lines_seekable (const char *pretty_filename, int fd,
}
/* Output the initial portion of the buffer
- in which we found the desired newline byte.
- Don't bother testing for failure for such a small amount.
- Any failure will be detected upon close. */
- fwrite (buffer, 1, n + 1, stdout);
+ in which we found the desired newline byte. */
+ xwrite_stdout (buffer, n + 1);
/* Set file pointer to the byte after what we've output. */
if (lseek (fd, pos + n + 1, SEEK_SET) < 0)
@@ -789,8 +778,7 @@ head_bytes (const char *filename, int fd, uintmax_t bytes_to_write)
}
if (bytes_read == 0)
break;
- if (fwrite (buffer, 1, bytes_read, stdout) < bytes_read)
- error (EXIT_FAILURE, errno, _("write error"));
+ xwrite_stdout (buffer, bytes_read);
bytes_to_write -= bytes_read;
}
return true;
@@ -830,8 +818,7 @@ head_lines (const char *filename, int fd, uintmax_t lines_to_write)
}
break;
}
- if (fwrite (buffer, 1, bytes_to_write, stdout) < bytes_to_write)
- error (EXIT_FAILURE, errno, _("write error"));
+ xwrite_stdout (buffer, bytes_to_write);
}
return true;
}
diff --git a/src/tail.c b/src/tail.c
index a5268c2..c421d81 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -339,13 +339,6 @@ pretty_name (struct File_spec const *f)
return (STREQ (f->name, "-") ? _("standard input") : f->name);
}
-static void
-xwrite_stdout (char const *buffer, size_t n_bytes)
-{
- if (n_bytes > 0 && fwrite (buffer, 1, n_bytes, stdout) == 0)
- error (EXIT_FAILURE, errno, _("write error"));
-}
-
/* Record a file F with descriptor FD, size SIZE, status ST, and
blocking status BLOCKING. */
@@ -385,6 +378,19 @@ write_header (const char *pretty_filename)
first_file = false;
}
+/* Write N_BYTES from BUFFER to stdout.
+ Exit immediately on error with a single diagnostic. */
+
+static void
+xwrite_stdout (char const *buffer, size_t n_bytes)
+{
+ if (n_bytes > 0 && fwrite (buffer, 1, n_bytes, stdout) < n_bytes)
+ {
+ clearerr (stdout); /* To avoid redundant close_stdout diagnostic. */
+ error (EXIT_FAILURE, errno, _("write error"));
+ }
+}
+
/* Read and output N_BYTES of file PRETTY_FILENAME starting at the current
position in FD. If N_BYTES is COPY_TO_EOF, then copy until end of file.
If N_BYTES is COPY_A_BUFFER, then copy at most one buffer's worth.
diff --git a/tests/local.mk b/tests/local.mk
index 9d556f6..4011abd 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -276,6 +276,7 @@ all_tests = \
tests/misc/groups-version.sh \
tests/misc/head-c.sh \
tests/misc/head-pos.sh \
+ tests/misc/head-write-error.sh \
tests/misc/md5sum.pl \
tests/misc/md5sum-bsd.sh \
tests/misc/md5sum-newline.pl \
diff --git a/tests/misc/head-write-error.sh b/tests/misc/head-write-error.sh
new file mode 100755
index 0000000..b749760
--- /dev/null
+++ b/tests/misc/head-write-error.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+# Ensure we diagnose and not continue writing to
+# the output if we get a write error.
+
+# 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_ head
+
+if ! test -w /dev/full || ! test -c /dev/full; then
+ skip_ '/dev/full is required'
+fi
+
+# We can't use /dev/zero as that's bypassed in the --lines case
+# due to lseek() indicating it has a size of zero.
+yes | head -c10M > bigseek || framework_failure_
+
+# Memory is bounded in these cases
+for item in lines bytes; do
+ for N in 0 1; do
+ # pipe case
+ yes | timeout 10s head --$item=-$N > /dev/full 2> err && fail=1
+ test $? = 124 && fail=1
+ test -s err || fail=1
+ rm err
+
+ # seekable case
+ timeout 10s head --$item=-$N bigseek > /dev/full 2> err && fail=1
+ test $? = 124 && fail=1
+ test -s err || fail=1
+ done
+done
+
+Exit $fail
--
1.7.7.6
