Subject: Re: [vserver] testfs.sh-0.20 patches
From: Herbert Poetzl <herbert@13thfloor.at>
Date: Fri, 21 Aug 2009 10:03:07 +0200

On Fri, Aug 21, 2009 at 12:07:39AM -0500, Corey Wright wrote:
> On Thu, 20 Aug 2009 15:18:51 +0200
> Herbert Poetzl <herbert@13thfloor.at> wrote:
> 
> > On Wed, Aug 19, 2009 at 01:48:33PM -0500, Corey Wright wrote:
> > > change expected result to fail (0 -> 1) as trying to execute
> > > chattr on a barrier from context 2 is expected to fail.
> > >
> > > this test is problematic because some versions of chattr
> > > (e2fsprogs 1.40.8-2ubuntu2 in ubuntu hardy) return success even
> > > though the barrier is not visible to chattr (ie "chattr: No such
> > > file or directory while trying to stat <barrier>"). so even though
> > > chattr failed to act on the barrier (as it could not see it), it
> > > still reports success (ie exit code 0).
> > >
> > > todo/suggestion: first stat the barrier and only try to chattr the
> > > barrier if stat is successful (because if stat fails, then chattr
> > > will definitely fail).
> > 
> > should probably be the other way round, i.e.
> > chattr && stat 
> > so that the chattr is executed in any case, and
> > the effect on the directory can be checked in
> > a second step ...
> 
> how about exec'ing chattr instead of eval'ing it (as chattr is too
> buggy to test with) and instead test the xattrs (using showattr &
> lsattr as done elsewhere in the code) after the attempted chattr to
> insure it didn't change them?

close, but not perfect, we should at least (explicitly or
implicitly) ensure that chattr _exists_ and was executed,
because otherwise checking for changes doesn't make much
sense (i.e. will give a false positive)

if that is done somehow, we can forget about the chattr
return code completely ...

best,
Herbert

> (see comment/explanation at top of patch.)

> corey
> -- 
> undefined@pobox.com

> testing the return value of chattr is useless:
> 1. sometimes returns success when the barrier couldn't even be stat'ed
> 2. returns success when the barrier couldn't be chattr'ed (permission denied)
> 
> the only meaningful test (besides testing chattr's stderr) is to verify the
> barrier's xattrs didn't change after running chattr.  this is also easiest to
> implement as it is already done elsewhere in the code (cut & paste).
> 
> so, eeval chattr instead of eexec'ing it, and add two tests verifying the
> xattrs (using both showattr & lsattr).  number the tests 105a & 105b, though
> only "105" is printed for both tests.  only one test is probably needed
> (preference to showattr as it dumps all xattrs), but just replicating what was
> done in tests 102 & 103 (showattr & lsattr, respectively).
> 
> --- testfs.sh-0.20b	2009-08-20 19:45:48.000000000 -0500
> +++ testfs.sh-0.20c	2009-08-20 20:36:02.000000000 -0500
> @@ -494,7 +494,9 @@ function do_test () {
>  	eexec 102	0 "setattr --barrier $dpath"
>  	eexec 103	0 "do_xattr_verify $MNT showattr $dpath $attr_B"
>  	eexec 104	0 "do_xattr_verify $MNT lsattr $dpath $lsattr_B"
> -	eexec 105	1 "go_xid 2 chattr =i $dpath"
> +	eeval		  "go_xid 2 chattr =i $dpath"
> +	eexec 105a	0 "do_xattr_verify $MNT showattr $dpath $attr_B"
> +	eexec 105b	0 "do_xattr_verify $MNT lsattr $dpath $lsattr_B"
>  	eexec 106	0 "do_xattr_barrier $MNT $dpath ..^"
>  	eexec 108	0 "setattr --~barrier $dpath"
>  	eexec 109	0 "do_xattr_verify $MNT showattr $dpath $attr_b"