xfs
[Top] [All Lists]

Re: [PATCH] Add test 257: Check proper FITRIM argument handling

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH] Add test 257: Check proper FITRIM argument handling
From: Lukas Czerner <lczerner@xxxxxxxxxx>
Date: Mon, 26 Sep 2011 15:52:33 +0200 (CEST)
Cc: Lukas Czerner <lczerner@xxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1317041252.3030.24.camel@doink>
References: <1316787311-23428-2-git-send-email-lczerner@xxxxxxxxxx> <1317021244-7556-1-git-send-email-lczerner@xxxxxxxxxx> <1317041252.3030.24.camel@doink>
User-agent: Alpine 2.00 (LFD 1167 2008-08-23)
On Mon, 26 Sep 2011, Alex Elder wrote:

> On Mon, 2011-09-26 at 09:14 +0200, Lukas Czerner wrote:
> > This test suppose to validate that file systems are using the fitrim
> > arguments right. It checks that the fstrim returns EINVAl in case that
> > the start of the range is beyond the end of the file system, and also
> > that the fstrim works without an error if the length of the range is
> > bigger than the file system (it should be truncated to the file system
> > length automatically within the fitrim implementation).
> > 
> > This test should also catch common problem with overflow of start+len.
> > Some file systems (ext4,xfs) had overflow problems in the past so there
> > is a specific test for it (for ext4 and xfs) as well as generic test for
> > other file systems, but it would be nice if other fs can add their
> > specific checks if this problem does apply to them as well.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> 
> OK, I know I suggested it, and I do like the result, but
> there could be a problem with the use of things like
> "2^32 - 1" being passed to the _math() function.
> 
> The problem lies in the way _math() backs off to try
> to use shell built-in arithmetic, which interprets the
> '^' as a bitwise XOR operator.  (Note, _math() was
> defined in an earlier patch.)
> 
> I think the use of "bc" to do certain math operations
> has some value, and as such I think the right fix is
> just to require "bc" in order for xfstests, or at least
> for any that use the _math() function, and drop the
> fall-back logic out of the definition of _math().
> 
> What do you think?

Yes I had the same concern, but I guess I was just lazy to look at it
:). So if we can require "bc" for xfstests we can simply remove the
fallback. Also maybe adding helper _require_bc, or maybe even better
adding helper _require <whatever> so we can check for <whatever> tool
in any test.

Thanks!
-Lukas


> 
> Assuming we resolve that, this test now looks fine to me.
> 
> Reviewed-by: Alex Elder <aelder@xxxxxxx>
> 
> 
> 

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