Re: dd and closed stderr

[Jim Meyering]

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.