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

Previous Next

Full log

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
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: Tue, 28 Feb 2012 23:33:40 +0000

On 02/28/2012 11:13 PM, Jim Meyering wrote:
> 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.

yes I was too terse

>> -#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.

Yes it's never going to trigger.
Paul suggested MAX(...) so as to doc/enforce the new constraint.
assert() is not used in dd.c yet
I'll leave as is unless Paul comments otherwise (modulo the
extraneous () of course).

cheers,
Pádraig

Previous Next

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