Re: [PATCH] Don't do unneccesary memory copies in dd.

[Jim Meyering]

Paul Eggert <address@hidden> wrote:
> Pádraig Brady <address@hidden> writes:
>> * src/dd.c: If output buffer size would be
>> the same size as the input buffer, just use
>> a single buffer to avoid redundant memory copy.
>
> Unfortunately this patch introduced a bug into 'dd', so that it no
> longer conforms to POSIX.
>
> The C_TWOBUFS option has two effects, one having to do with whether a
> single buffer is used, and the other having to do with whether output
> blocks are dynamically resized to be the same size as input blocks.
> This patch inadvertently caused the latter behavior to change.
>
> Here's an example of the problem:
>
> $ (echo 'x'; sleep 10; echo y) | dd ibs=3 obs=3
>
> POSIX says that in this case the output data must be reblocked into
> blocks of 3 bytes.  With the working 'dd', you'll see a pause of 10
> seconds (because the first 'echo' outputs only 2 bytes), then the x and
> y right away (because we now have all 4 input bytes, and can issue a
> write of 3 bytes and a write of 1 byte).  With a buggy 'dd', you'll see
> the 'x' right away (because we output a 2-byte block), then a pause of
> 10 seconds, then a 'y' (the other 2-byte block).
>
> The simplest fix I see is to revert the patch.  Of course one could up
> with something fancier....

Good catch!  Thank you.
I've done that, and added a test for the required behavior.
I'll  push the following tomorrow:

FYI, with the test below, the unreverted version
would fail like this:

FAIL: dd/reblock.log (exit: 1)
==============================

--- err 2008-11-21 23:21:06.000000000 +0100
+++ exp 2008-11-21 23:21:06.000000000 +0100
@@ -1,3 +1,3 @@
 0+2 records in
-0+2 records out
+1+1 records out
 4 bytes (4 B) copied

>From 80d42899ec71f237b65f5c974e0a2c1b9c121e09 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 21 Nov 2008 23:17:44 +0100
Subject: [PATCH 1/2] Revert "dd: avoid unnecessary memory copies"

This reverts commit fbd87029cfc494a72bb73ade27ef46382c5bc832.
Paul Eggert noticed that the offending was wrong:
http://lists.gnu.org/archive/html/bug-coreutils/2008-11/msg00153.html
---
 src/dd.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/dd.c b/src/dd.c
index e1e38e9..f598e44 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -998,11 +998,13 @@ scanargs (int argc, char *const *argv)
            {
              invalid |= ! (0 < n && n <= MAX_BLOCKSIZE (INPUT_BLOCK_SLOP));
              input_blocksize = n;
+             conversions_mask |= C_TWOBUFS;
            }
          else if (operand_is (name, "obs"))
            {
              invalid |= ! (0 < n && n <= MAX_BLOCKSIZE (OUTPUT_BLOCK_SLOP));
              output_blocksize = n;
+             conversions_mask |= C_TWOBUFS;
            }
          else if (operand_is (name, "bs"))
            {
@@ -1034,12 +1036,15 @@ scanargs (int argc, char *const *argv)
   if (blocksize)
     input_blocksize = output_blocksize = blocksize;

+  /* If bs= was given, both `input_blocksize' and `output_blocksize' will
+     have been set to positive values.  If either has not been set,
+     bs= was not given, so make sure two buffers are used. */
+  if (input_blocksize == 0 || output_blocksize == 0)
+    conversions_mask |= C_TWOBUFS;
   if (input_blocksize == 0)
     input_blocksize = DEFAULT_BLOCKSIZE;
   if (output_blocksize == 0)
     output_blocksize = DEFAULT_BLOCKSIZE;
-  if (input_blocksize != output_blocksize)
-    conversions_mask |= C_TWOBUFS;
   if (conversion_blocksize == 0)
     conversions_mask &= ~(C_BLOCK | C_UNBLOCK);

--
1.6.0.4.1013.gc6a01


>From 7f9388147280a093743f529c91827fbf8b24a7e0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 21 Nov 2008 23:12:17 +0100
Subject: [PATCH 2/2] tests: dd: add a test for today's bug

* tests/dd/reblock: New file.  Test for the required functionality.
Based on an example from Paul Eggert in
http://lists.gnu.org/archive/html/bug-coreutils/2008-11/msg00153.html
* tests/Makefile.am (TESTS): Add dd/reblock.
---
 tests/Makefile.am |    1 +
 tests/dd/reblock  |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)
 create mode 100755 tests/dd/reblock

diff --git a/tests/Makefile.am b/tests/Makefile.am
index f264bd0..8c768bb 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -283,6 +283,7 @@ TESTS =                                             \
   cp/thru-dangling                             \
   dd/misc                                      \
   dd/not-rewound                               \
+  dd/reblock                                   \
   dd/skip-seek                                 \
   dd/skip-seek2                                        \
   dd/unblock-sync                              \
diff --git a/tests/dd/reblock b/tests/dd/reblock
new file mode 100755
index 0000000..3e21d91
--- /dev/null
+++ b/tests/dd/reblock
@@ -0,0 +1,38 @@
+#!/bin/sh
+# ensure that dd reblocks when ibs==obs
+
+# Copyright (C) 2008 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/>.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  dd --version
+fi
+
+. $srcdir/lang-default
+. $srcdir/test-lib.sh
+
+fail=0
+(echo x; sleep .1; echo y) | dd ibs=3 obs=3 > out 2> err || fail=1
+sed 's/,.*//' err > k && mv k err
+cat <<\EOF > exp || fail=1
+0+2 records in
+1+1 records out
+4 bytes (4 B) copied
+EOF
+
+compare err exp || fail=1
+
+Exit $fail
--
1.6.0.4.1013.gc6a01