Paul Eggert <[EMAIL PROTECTED]> wrote: > Pádraig Brady <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 _______________________________________________ Bug-coreutils mailing list [email protected] http://lists.gnu.org/mailman/listinfo/bug-coreutils
