xfs
[Top] [All Lists]

Re: [PATCH] xfstests: simplify TRIM_OFF_LEN() in "ltp/fsx.c"

To: <xfs@xxxxxxxxxxx>
Subject: Re: [PATCH] xfstests: simplify TRIM_OFF_LEN() in "ltp/fsx.c"
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 21 Sep 2011 14:11:13 -0500
In-reply-to: <201107202310.p6KNAtQa026739@xxxxxxxxxxxxxxxxxxxxxx>
References: <201107202310.p6KNAtQa026739@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
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:



<Prev in Thread] Current Thread [Next in Thread>