GNU bug report logs - #9157
[PATCH] dd: sparse conv flag

Previous Next

Full log

Message #58 received at 9157 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 9157 <at> debbugs.gnu.org, roman.rybalko <at> romanr.info,
	Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#9157: [PATCH] dd: sparse conv flag
Date: Wed, 29 Feb 2012 00:13:26 +0100

Pádraig Brady wrote:
...

Thanks for working on that.
All I would adjust are a few nits and
add require_sparse_support_ in the test script:

> Subject: [PATCH] dd: add support for the conv=sparse option
>
> Small seeks are not coalesced to larger ones (like is
> done in cache_round() for example, for the moment at least.
>
> conv= is used rather then oflag= for FreeBSD compatibility.
>
> * src/dd.c (last_seek): A new global boolean to flag

"last" is ambiguous.  Does it mean "final" or "preceding"?
From the context below, (not the comment, since it too uses "last")
I see it means "final".  "final_op_was_seek" is longer but ultra clear.
Maybe worth the length.

> whether the last "write" was converted to a seek.
> (usage): Describe the new conf=sparse option.
> (scanargs): Ignore conv=sparse in some combinations.
> (iwrite): Convert a write of a NUL block to a seek if requested.
> (do_copy): Initialize the output buffer to have a sentinel,
> to allow for efficient testing for NUL output blocks.
> If the last block in the file was converted to a seek,
> then convert back to a write so the size ip updated.

s/ip/is/

> * NEWS: Mention the new feature.
> * tests/dd/sparse: A new test for the feature.
> * tests/Makefile.am: Reference the new test.
> ---
>  NEWS               |    3 +
>  doc/coreutils.texi |    7 +++
>  src/dd.c           |  104 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  tests/Makefile.am  |    1 +
>  tests/dd/sparse    |   57 ++++++++++++++++++++++++++++
>  5 files changed, 168 insertions(+), 4 deletions(-)
>  create mode 100755 tests/dd/sparse
>
> diff --git a/NEWS b/NEWS
> index e2e8fc5..8006669 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,9 @@ GNU coreutils NEWS                                    -*- outline -*-
>    dd now accepts the count_bytes, skip_bytes iflags and the seek_bytes
>    oflag, to more easily allow processing portions of a file.
>
> +  dd now accepts the conv=sparse flag to attempt to create sparse
> +  output, by seeking rather than writing to the output file.
> +
>    split now accepts an optional "from" argument to --numeric-suffixes,
>    which changes the start number from the default of 0.
>
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 414626d..f22e7d2 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -8140,6 +8140,13 @@ Change lowercase letters to uppercase.
>
>  The @samp{lcase} and @samp{ucase} conversions are mutually exclusive.
>
> +@item sparse
> +@opindex sparse
> +Try to seek rather than write @sc{nul} output blocks.
> +This will create sparse output when extending.

Maybe add this:
s/\./on a file system that support sparse files./ ?

> +This option is ignored in conjunction with
> +@samp{conv=notrunc} or @samp{oflag=append}.
> +
>  @item swab
>  @opindex swab @r{(byte-swapping)}
>  @cindex byte-swapping
> diff --git a/src/dd.c b/src/dd.c
> index fe44a30..903346f 100644
> --- a/src/dd.c
> +++ b/src/dd.c
> @@ -94,7 +94,7 @@
>     malloc.  See dd_copy for details.  INPUT_BLOCK_SLOP must be no less than
>     OUTPUT_BLOCK_SLOP.  */
>  #define INPUT_BLOCK_SLOP (2 * SWAB_ALIGN_OFFSET + 2 * page_size - 1)
> -#define OUTPUT_BLOCK_SLOP (page_size - 1)
> +#define OUTPUT_BLOCK_SLOP MAX (sizeof (uintptr_t), page_size - 1)

I haven't seen justification for why you're making the above change.
Can sizeof uintptr_t really be larger than page_size-1 (getpagesize()-1)?
This seems so unlikely that it'd deserve an assertion in main where
page_size is set, even though there are only two uses of OUTPUT_BLOCK_SLOP.

If you leave the sizeof, please omit the parentheses.
They are unnecessary here.
Same below.

...
> +  /* Write sentinel to slop after the buffer,
> +     to allow efficient checking for NUL blocks.  */
> +  memset (obuf + output_blocksize, 1, sizeof (uintptr_t));
...
> diff --git a/tests/dd/sparse b/tests/dd/sparse
...
> +
> +. "${srcdir=.}/init.sh"; path_prepend_ ../src
> +print_ver_ dd

require_sparse_support_

> +# Ensure basic sparse generation works
> +truncate -s1M sparse
> +dd bs=32K if=sparse of=sparse.dd conv=sparse
> +test $(stat -c %s sparse) = $(stat -c %s sparse.dd) || fail=1

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.