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

[Jim Meyering]

Pádraig Brady <address@hidden> wrote:
> Jim Meyering wrote:
>> Paul Eggert <address@hidden> wrote:
>>>
>>> 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.
>>
>>>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
>
> Paul is correct as usual.
>
> But I'd like to use the attached patch instead of your PATCH 1,
> because I think it simplifies the code and makes it explicit
> in the code and to users reading the docs what's happening in
> this case. It also more closely conforms to POSIX I think
> in the unusual case where both bs= and [io]bs= are specified.

Sure.  I've made the following adjustments:

> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
...
> +In addition, if no data transforming @option{conv} options are

s/opt.*/option is/

> +specified, each input block is copied to the output as a single
> +block without aggregating short reads.
...
> diff --git a/src/dd.c b/src/dd.c
...
>    if (blocksize)
>      input_blocksize = output_blocksize = blocksize;
> +  else
> +    /* POSIX states we aggregate short reads

s/we.../dd aggregates/

> +       into output_blocksize if bs= is not specified.  */
> +    conversions_mask |= C_TWOBUFS;

Put braces around this multi-*line* else clause.
That's my policy: if it takes more than one *line* (not statement),
it deserves braces.

I also added a new test.
If you can think of any others to add, please do.
Here's the latest.  I'll wait to hear back before pushing it.
Yes, I really should use a branch somewhere public...

>From d5ab05998ca068b9a2e217285b15318ad2e5bdb8 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <address@hidden>
Date: Fri, 21 Nov 2008 23:17:44 +0100
Subject: [PATCH 1/2] Revert part of "dd: avoid unnecessary memory copies"

This reverts part of commit fbd87029cfc494a72bb73ade27ef46382c5bc832.
Paul Eggert noticed the problem in
http://lists.gnu.org/archive/html/bug-coreutils/2008-11/msg00153.html
* doc/coreutils.texi (dd invocation): Clarify.
---
 doc/coreutils.texi |    3 +++
 src/dd.c           |   10 +++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 935129f..c9c61ce 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -7573,6 +7573,9 @@ dd invocation
 Set both input and output block sizes to @var{bytes}.
 This makes @command{dd} read and write @var{bytes} per block,
 overriding any @samp{ibs} and @samp{obs} settings.
+In addition, if no data-transforming @option{conv} option is specified,
+each input block is copied to the output as a single block,
+without aggregating short reads.

 @item address@hidden
 @opindex cbs
diff --git a/src/dd.c b/src/dd.c
index e1e38e9..e54cc14 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -455,7 +455,7 @@ Usage: %s [OPERAND]...\n\
       fputs (_("\
 Copy a file, converting and formatting according to the operands.\n\
 \n\
-  bs=BYTES        force ibs=BYTES and obs=BYTES\n\
+  bs=BYTES        read and write BYTES bytes at a time\n\
   cbs=BYTES       convert BYTES bytes at a time\n\
   conv=CONVS      convert the file as per the comma separated symbol list\n\
   count=BLOCKS    copy only BLOCKS input blocks\n\
@@ -1033,13 +1033,17 @@ scanargs (int argc, char *const *argv)

   if (blocksize)
     input_blocksize = output_blocksize = blocksize;
+  else
+    {
+      /* POSIX says dd aggregates short reads into
+        output_blocksize if bs= is not specified.  */
+      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 a5e53ecc2cf8404760094631a772d4ce255bdcac 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 the required behavior

* tests/dd/reblock: New file.  Test for the required functionality.
Based on an example and discussion from this thread:
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  |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 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..542529a
--- /dev/null
+++ b/tests/dd/reblock
@@ -0,0 +1,51 @@
+#!/bin/sh
+# test dd reblocking vs. bs=
+
+# 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
+
+cat <<\EOF > exp-reblock || framework_failure
+0+2 records in
+1+1 records out
+4 bytes (4 B) copied
+EOF
+
+cat <<\EOF > exp-no-reblock || framework_failure
+0+2 records in
+0+2 records out
+4 bytes (4 B) copied
+EOF
+
+fail=0
+
+# ensure that dd reblocks when bs= is not specified
+(echo x; sleep .1; echo y) | dd ibs=3 obs=3 > out 2> err || fail=1
+sed 's/,.*//' err > k && mv k err
+compare err exp-reblock || fail=1
+
+# Demonstrate that bs=N supersedes even following ibs= and obs= settings.
+(echo x; sleep .1; echo y) | dd bs=3 ibs=1 obs=1 > out 2> err || fail=1
+sed 's/,.*//' err > k && mv k err
+compare err exp-no-reblock || fail=1
+
+Exit $fail
--
1.6.0.4.1013.gc6a01