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
