Can someone review this please? -Alex
On Wed, 2011-07-20 at 18:10 -0500, Alex Elder wrote:
> A recent commit added a TRIM_OFF_LEN() macro in "ltp/fsx.c":
> 5843147e xfstests: fsx fallocate support is b0rked
> A later commit fixed a problem with that macro:
> c47d7a51 xfstests: fix modulo-by-zero error in fsx
>
> There is an extra flag parameter in that macro that I didn't like
> in either version. When looking at it the second time around I
> concluded that there was no need for the flag after all.
>
> Going back to the first commit, the code that TRIM_OFF_LEN()
> replaced had one of two forms:
> - For OP_READ and OP_MAP_READ:
> if (file_size)
> offset %= file_size;
> else
> offset = 0;
> if (offset + size > file_size)
> size = file_size - offset;
>
> - For all other cases (except OP_TRUNCATE):
> offset %= maxfilelen;
> if (offset + size > maxfilelen)
> size = maxfilelen - offset;
>
> There's no harm in ensuring maxfilelen is non-zero (and doing so
> is safer than what's done above). So both of the above can be
> generalized this way:
> if (SIZE_LIMIT)
> offset %= SIZE_LIMIT;
> else
> offset = 0;
> if (offset + size > SIZE_LIMIT)
> size = SIZE_LIMIT - offset;
>
> In other words, there is no need for the extra flag in the macro.
>
> The following patch just does away with it. It uses the value of
> the "size" parameter directly in avoiding a divide-by-zero, and in
> the process avoids referencing the global "file_size" within the
> macro expansion.
>
> One more thing... It seems like OP_HOLE_PUNCH should be
> limited to the file size rather than to maximum file size
> (but that's a separate discussion).
>
> Signed-off-by: Alex Elder <aelder@xxxxxxx>
>
> ---
> ltp/fsx.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> Index: b/ltp/fsx.c
> ===================================================================
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -987,14 +987,14 @@ docloseopen(void)
> }
> }
>
> -#define TRIM_OFF_LEN(off, len, size, allow_zero_file_size) \
> -do { \
> - if (allow_zero_file_size || file_size) \
> - offset %= size; \
> - else \
> - offset = 0; \
> - if (offset + len > size) \
> - len = size - offset; \
> +#define TRIM_OFF_LEN(off, len, size) \
> +do { \
> + if (size) \
> + offset %= size; \
> + else \
> + offset = 0; \
> + if (offset + len > size) \
> + len = size - offset; \
> } while (0)
>
> void
> @@ -1054,22 +1054,22 @@ test(void)
>
> switch (op) {
> case OP_READ:
> - TRIM_OFF_LEN(offset, size, file_size, 0);
> + TRIM_OFF_LEN(offset, size, file_size);
> doread(offset, size);
> break;
>
> case OP_WRITE:
> - TRIM_OFF_LEN(offset, size, maxfilelen, 1);
> + TRIM_OFF_LEN(offset, size, maxfilelen);
> dowrite(offset, size);
> break;
>
> case OP_MAPREAD:
> - TRIM_OFF_LEN(offset, size, file_size, 0);
> + TRIM_OFF_LEN(offset, size, file_size);
> domapread(offset, size);
> break;
>
> case OP_MAPWRITE:
> - TRIM_OFF_LEN(offset, size, maxfilelen, 1);
> + TRIM_OFF_LEN(offset, size, maxfilelen);
> domapwrite(offset, size);
> break;
>
> @@ -1080,12 +1080,12 @@ test(void)
> break;
>
> case OP_FALLOCATE:
> - TRIM_OFF_LEN(offset, size, maxfilelen, 1);
> + TRIM_OFF_LEN(offset, size, maxfilelen);
> do_preallocate(offset, size);
> break;
>
> case OP_PUNCH_HOLE:
> - TRIM_OFF_LEN(offset, size, maxfilelen, 1);
> + TRIM_OFF_LEN(offset, size, maxfilelen);
> do_punch_hole(offset, size);
> break;
> default:
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
