On 01/23/2013 10:17 AM, Bernhard Voelker wrote:
On 01/22/2013 02:53 PM, Ondrej Oprala wrote:
The patch is based on this entry
https://bugzilla.redhat.com/show_bug.cgi?id=502026 . It delays
allocation if possible (as suggested in
http://lists.gnu.org/archive/html/bug-coreutils/2009-05/msg00223.html ),
skipping it altogether if dd is not being used for actual copying.
Hi Ondrej,
thanks for working on this.
* src/dd.c (dd_copy): Move and split the code for buffer
allocations to separate functions and call them conditionally.
(alloc_ibuf): Allocate memory for the input buffer.
(alloc_obuf): Allocate memory for the output buffer.
Please describe what you did to alloc_ibuf + sibling:
you added the function, so please tell so, e.g. something like
* src/dd.c (alloc_ibuf, alloc_obuf): Add new function with code
factored out from ...
(dd_copy): ... here. Call the above new functions to allocate
memory for ibuf and obuf at the beginning if needed, or later.
And please add also the new definition of ibuf as a global,
static variable.
diff --git a/src/dd.c b/src/dd.c
[...]
+ /* Delay buffer allocation if possible*/
s/\(possible\)/\1. /
diff --git a/tests/dd/no-allocate.sh b/tests/dd/no-allocate.sh
[...]
+#count and skip is zero, we don't need to allocate memory for input block
+(ulimit -v 10000;dd if=/dev/zero of=x bs=10M count=0) || fail=1
+#non-skippable input, we need to allocate input block size (and we should fail)
+(ulimit -v 10000; echo "abcde" | dd of=x bs=10M seek=1 skip=1 count=0) &&
fail=1
s/#/# /
require_ulimit_
Other than that, the patch looks quite okay - although I needed some time
to find out if there could be a situation where ibuf is not yet allocated
and alloc_obuf lets obuf point to ibuf, but obuf is yet used before ibuf
is allocated later ... and I'm still not 100% convinced.
Can someone confirm this?
(At least, this corner case is not obvious by reading.)
Have a nice day,
Berny
Hi Bernhard,
thanks for the remarks. Also, you were right about the corner case, the
succession of commands
$ touch a
$ dd if=a seek=1 bs=1
$ ^D
actually caused a segfault. Sorry, I didn't notice it before.
I added a few lines to alloc_obuf, which cover the case when the initial
ibuf allocation is skipped but
obuf needs to be allocated and is to be set to ibuf and removed the
newly redundant check before memset.
Thanks,
Ondrej
From 5bcad6426278104f121155e7591b34a18fb912fd Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <[email protected]>
Date: Tue, 22 Jan 2013 14:21:23 +0100
Subject: [PATCH] dd: Postpone buffer allocations if possible.
* src/dd.c: Add new static global variable ibuf.
(alloc_ibuf, alloc_obuf): New functions with code
factored out from dd_copy.
(dd_copy): Call the new functions to allocate memory for
ibuf and obuf when necessary.
* tests/dd/no-allocate.sh: New test.
* tests/local.mk: Add the test.
---
src/dd.c | 91 ++++++++++++++++++++++++++++++-------------------
tests/dd/no-allocate.sh | 29 ++++++++++++++++
tests/local.mk | 1 +
3 files changed, 86 insertions(+), 35 deletions(-)
create mode 100755 tests/dd/no-allocate.sh
diff --git a/src/dd.c b/src/dd.c
index ef5664b..da1d791 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -236,6 +236,9 @@ static uintmax_t r_truncate = 0;
static char newline_character = '\n';
static char space_character = ' ';
+/* Input buffer. */
+static char *ibuf;
+
/* Output buffer. */
static char *obuf;
@@ -1833,16 +1836,52 @@ human_size (size_t n)
return human_readable (n, hbuf, human_opts, 1, 1);
}
+static void
+alloc_ibuf (void)
+{
+ char *real_buf = malloc (input_blocksize + INPUT_BLOCK_SLOP);
+ if (!real_buf)
+ error (EXIT_FAILURE, 0,
+ _("memory exhausted by input buffer of size %zu bytes (%s)"),
+ input_blocksize, human_size (input_blocksize));
+
+ real_buf += SWAB_ALIGN_OFFSET; /* allow space for swab */
+
+ ibuf = ptr_align (real_buf, page_size);
+}
+
+static void
+alloc_obuf (void)
+{
+ if (conversions_mask & C_TWOBUFS)
+ {
+ /* Page-align the output buffer, too. */
+ char *real_obuf = malloc (output_blocksize + OUTPUT_BLOCK_SLOP);
+ if (!real_obuf)
+ error (EXIT_FAILURE, 0,
+ _("memory exhausted by output buffer of size %zu bytes (%s)"),
+ output_blocksize, human_size (output_blocksize));
+ obuf = ptr_align (real_obuf, page_size);
+ }
+ else
+ {
+ if (!ibuf)
+ alloc_ibuf ();
+ obuf = ibuf;
+ }
+
+ /* Write a sentinel to the slop after the buffer,
+ to allow efficient checking for NUL blocks. */
+ assert (sizeof (uintptr_t) <= OUTPUT_BLOCK_SLOP);
+ memset (obuf + output_blocksize, 1, sizeof (uintptr_t));
+}
+
/* The main loop. */
static int
dd_copy (void)
{
- char *ibuf, *bufstart; /* Input buffer. */
- /* These are declared static so that even though we don't free the
- buffers, valgrind will recognize that there is no "real" leak. */
- static char *real_buf; /* real buffer address before alignment */
- static char *real_obuf;
+ char *bufstart; /* Input buffer. */
ssize_t nread; /* Bytes read in the current block. */
/* If nonzero, then the previously read block was partial and
@@ -1869,37 +1908,14 @@ dd_copy (void)
It is necessary when accessing raw (i.e. character special) disk
devices on Unixware or other SVR4-derived system. */
- real_buf = malloc (input_blocksize + INPUT_BLOCK_SLOP);
- if (!real_buf)
- error (EXIT_FAILURE, 0,
- _("memory exhausted by input buffer of size %zu bytes (%s)"),
- input_blocksize, human_size (input_blocksize));
-
- ibuf = real_buf;
- ibuf += SWAB_ALIGN_OFFSET; /* allow space for swab */
-
- ibuf = ptr_align (ibuf, page_size);
-
- if (conversions_mask & C_TWOBUFS)
- {
- /* Page-align the output buffer, too. */
- real_obuf = malloc (output_blocksize + OUTPUT_BLOCK_SLOP);
- if (!real_obuf)
- error (EXIT_FAILURE, 0,
- _("memory exhausted by output buffer of size %zu bytes (%s)"),
- output_blocksize, human_size (output_blocksize));
- obuf = ptr_align (real_obuf, page_size);
- }
- else
- {
- real_obuf = NULL;
- obuf = ibuf;
- }
+ /* Delay buffer allocation if possible. */
+ if ((skip_records > OFF_T_MAX / input_blocksize)
+ || 0 > skip_via_lseek (input_file, STDIN_FILENO, 0, SEEK_CUR))
+ alloc_ibuf ();
- /* Write a sentinel to the slop after the buffer,
- to allow efficient checking for NUL blocks. */
- assert (sizeof (uintptr_t) <= OUTPUT_BLOCK_SLOP);
- memset (obuf + output_blocksize, 1, sizeof (uintptr_t));
+ if ((seek_records > OFF_T_MAX / output_blocksize)
+ || 0 > skip_via_lseek (output_file, STDOUT_FILENO, 0, SEEK_CUR))
+ alloc_obuf ();
if (skip_records != 0 || skip_bytes != 0)
{
@@ -1955,6 +1971,11 @@ dd_copy (void)
if (max_records == 0 && max_bytes == 0)
return exit_status;
+ if (!ibuf)
+ alloc_ibuf ();
+ if (!obuf)
+ alloc_obuf ();
+
while (1)
{
if (r_partial + r_full >= max_records + !!max_bytes)
diff --git a/tests/dd/no-allocate.sh b/tests/dd/no-allocate.sh
new file mode 100755
index 0000000..bf40094
--- /dev/null
+++ b/tests/dd/no-allocate.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+# make sure that dd doesn't allocate memory unnecessarily
+
+# Copyright (C) 2013 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/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ dd
+require_ulimit_
+
+# count and skip is zero, we don't need to allocate memory for input block
+(ulimit -v 10000;dd if=/dev/zero of=x bs=10M count=0) || fail=1
+
+# non-skippable input, we need to allocate input block size (and we should
fail)
+(ulimit -v 10000; echo "abcde" | dd of=x bs=10M seek=1 skip=1 count=0) &&
fail=1
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 82daee5..e011fdf 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -462,6 +462,7 @@ all_tests = \
tests/df/skip-rootfs.sh \
tests/dd/direct.sh \
tests/dd/misc.sh \
+ tests/dd/no-allocate.sh \
tests/dd/nocache.sh \
tests/dd/not-rewound.sh \
tests/dd/reblock.sh \
--
1.7.11.7
