bug#7362: dd strangeness

[Paul Eggert]

On 02/28/2011 01:41 AM, Pádraig Brady wrote:
> Hmm, it's better to be explicit but I think defaulting to "fullblock"
> is too risky. As an interim step at least, how about just warning
> as per the attached.

Ouch.  This is a pain to think about.  But here are some thoughts anyway:

 * I went back and reread POSIX, and "dd" is allowed to issue
   diagnostics to stderr whenever it likes.  So we don't need to
   worry about POSIXLY_CORRECT if all we want to do is issue diagnostics.
   We can issue them regardless of POSIXLY_CORRECT.

 * I don't understand the business with C_SYNC.  People who use
   conv=sync know what they're doing, or ought to; there's little
   point giving them a warning.

 * For (O_DIRECT | O_CIO), surely this matters only for output_flags.
   If these bits are set in input_flags then O_FULLBLOCK is irrelevant, no?

 * If we care about max_records we should also care about skip_records,
   since short reads matter when skipping in a pipe, too.

 * Since POSIX doesn't specify the direct or cio flags, we're free
   to have them silently enable iflag=fullblock.  But it doesn't sound
   right to do that.  Instead, we should set conversions_mask |= C_TWOBUFS,
   because the input and output blocksizes might differ.

 * If we suggest ibs=whatever rather than iflag=fullblock, our
   suggestions will be portable to other POSIX implementations, which
   is a plus.

 * Rather than warn about potential problems, how about diagnosing the
   problems only when they actually occur?  That would help us avoid
   crying wolf.

Here's a proposed patch that tries to embody all the above, except
that I haven't done the documentation (I figure we should get the
behavior right first....):


>From 85e3716f918e8163695a85d40fe8c561634c9e2e Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Tue, 1 Mar 2011 01:34:57 -0800
Subject: [PATCH] dd: avoid or diagnose some problems with short reads
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/dd.c (iread): Diagnose short reads when they mess up counts.
(scanargs): If oflags=direct or oflags=cio, use
C_TWOBUFS so that the output blocks are typically full.
Derived from a suggestion by Pádraig Brady in:
http://lists.gnu.org/archive/html/bug-coreutils/2011-02/msg00150.html
---
 src/dd.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/src/dd.c b/src/dd.c
index daddc1e..41ad7a3 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -802,7 +802,29 @@ iread (int fd, char *buf, size_t size)
       process_signals ();
       nread = read (fd, buf, size);
       if (! (nread < 0 && errno == EINTR))
-        return nread;
+        {
+          static ssize_t prev_nread;
+          static bool warned;
+
+          if (nread != 0 && 0 < prev_nread && prev_nread < size
+              && iread_fnc == iread
+              && ! (conversions_mask & C_TWOBUFS)
+              && (skip_records
+                  || (0 < max_records && max_records < (uintmax_t) -1))
+              && ! warned)
+            {
+              unsigned long int prev = prev_nread;
+              unsigned long int ibs = input_blocksize;
+              error (0, 0, _("warning: short read (%lu bytes)"), prev);
+              error (0, 0,
+                     _("Perhaps you wanted ibs=%lu rather than bs=%lu?"),
+                     ibs, ibs);
+              warned = true;
+            }
+
+          prev_nread = nread;
+          return nread;
+        }
     }
 }
 
@@ -1075,6 +1097,9 @@ scanargs (int argc, char *const *argv)
       conversions_mask |= C_TWOBUFS;
     }
 
+  if (output_flags & (O_DIRECT | O_CIO))
+    conversions_mask |= C_TWOBUFS;
+
   if (input_blocksize == 0)
     input_blocksize = DEFAULT_BLOCKSIZE;
   if (output_blocksize == 0)
-- 
1.7.4