[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: dd and closed stderr
From: |
Jim Meyering |
Subject: |
Re: dd and closed stderr |
Date: |
Fri, 28 Aug 2009 17:31:19 +0200 |
Eric Blake wrote:
> Jim Meyering <jim <at> meyering.net> writes:
>
>> Regardless of POSIX, what if an application requires that output and
>> attempts to parse it? In that case, a successful exit status is defintely
>> at odds with empty stderr (which could arise also due to ENOSPC).
>>
>> So I'd say it's a bug.
>
> Here's a patch, including a testsuite addition (along with a cleanup to allow
> clean compilation on cygwin). OK to apply?
Thanks!
The second is fine.
Two nits in the first:
>>From 8cea0f3d5628ea3e4f74db0a66486cae5d92122a Mon Sep 17 00:00:00 2001
> From: Eric Blake <address@hidden>
> Date: Fri, 28 Aug 2009 08:40:06 -0600
> Subject: [PATCH 1/2] dd: detect closed stderr
>
> * src/dd.c (maybe_close_stdout): Always flush stderr.
> * tests/dd/stderr: New test, borrowing from misc/close-stdout.
> * tests/Makefile.am (TESTS): Run it.
> * NEWS: Mention this.
> ---
> NEWS | 3 +++
> src/dd.c | 3 +++
> tests/Makefile.am | 1 +
> tests/dd/stderr | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 54 insertions(+), 0 deletions(-)
> create mode 100755 tests/dd/stderr
>
> diff --git a/NEWS b/NEWS
> index c125b31..581620f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,9 @@ GNU coreutils NEWS -*-
> outline -*-
> due to their running on a kernel older than what was implied by headers
> and libraries tested at configure time.
>
> + dd now returns non-zero status if it encountered a write error while
> + printing a summary to stderr.
> +
>
> * Noteworthy changes in release 7.5 (2009-08-20) [stable]
>
> diff --git a/src/dd.c b/src/dd.c
> index dc15cfd..c016d3d 100644
> --- a/src/dd.c
> +++ b/src/dd.c
> @@ -25,6 +25,7 @@
> #include <getopt.h>
>
> #include "system.h"
> +#include "close-stream.h"
> #include "error.h"
> #include "fd-reopen.h"
> #include "gethrxtime.h"
> @@ -450,6 +451,8 @@ maybe_close_stdout (void)
> {
> if (close_stdout_required)
> close_stdout ();
> + else if (close_stream (stderr) != 0)
> + _exit (exit_failure);
Why use exit_failure here? (it works because exitfail.c
initializes that global to the value EXIT_FAILURE)
Everywhere else in dd.c, we use EXIT_FAILURE.
While exit_failure is technically correct, EXIT_FAILURE
is more understandable. Certainly more consistent.
...
> +. $srcdir/test-lib.sh
> +
> +p="$abs_top_builddir"
For consistency with most of the shell code in coreutils,
please elide double quotes in simple shell variable assignments:
p=$abs_top_builddir
Hmm... I see that close-stdout did it the same way.
You're welcome to adjust that one, too.