xfs
[Top] [All Lists]

Re: [PATCH] remove bashisms from xfstests

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] remove bashisms from xfstests
From: Christian Kujau <lists@xxxxxxxxxxxxxxx>
Date: Wed, 6 Jan 2010 11:24:10 -0800 (PST)
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20100106164844.GB4209@xxxxxxxxxxxxx>
References: <alpine.DEB.2.01.1001030125040.3483@xxxxxxxxxxxxxxxxxx> <20100106164844.GB4209@xxxxxxxxxxxxx>
User-agent: Alpine 2.01 (DEB 1266 2009-07-14)
On Wed, 6 Jan 2010 at 11:48, Christoph Hellwig wrote:
> > 2) convert "let..." into something (hopefully) more portable
> 
> Dave converted these away from expr due to performance reasons.  I'd
> like too see a prove that performance hasn't regressed due to this
> change.

That's why I hesitated to just change all scripts to /bin/bash, but didn't 
dare to say so, because of the inevitable "but today's computers are fast 
enough" reactions :-)

> > 3) replace 'a == b' with 'a = b' in bourne shell scripts
> 
> This looks fine.  If you want feel free to submit these as a separate
> first patch so that we have the large pile sorted out.

OK, will do.

> I fear this might cause some overhead in the shell.  What about the
> following instead:
> 
> for i in `seq 0 39`; do
>     ln -s $o symlink_$i
>     o=symlink_$i
> done

I didn't go for seq(1), as I feared this wouldn't be available on every 
machine. Given that the xfstests scripts have a lot of "what if we're 
running on IRIX" conditions, I just wasn't sure if seq(1) would be 
available there. But yes, I'd much rather like to do this with seq(1) as 
well, if this is OK to everybody.

> > --- xfstests.orig/032       2010-01-03 00:42:16.601617592 -0800
> > +++ xfstests/032    2010-01-03 00:43:56.321617592 -0800
> > @@ -66,6 +66,7 @@ do
> >     [ $fs = ext3 ] && preargs="-F"
> >     [ $fs = ext4 ] && preargs="-F"
> >     [ $fs = ext4dev ] && preargs="-F"
> > +   [ $fs = nilfs2 ] && preargs="-q"
> 
> This hunk is unrelated.  Please submit it with a trivial one-liner
> description and a signed-off-by line and I'll put it in.

OK, will do.

> > -echo -e -n "\n\r*** XFS QA 044 - done\n\r\n\r" >/dev/console
> > +printf "\n\r*** XFS QA 044 - done\n\r\n\r" >/dev/console
> 
> This looks okay, but should be mentioned in the patch description.

Will do. It's needed, because sh(1)'s echo builtin doesn't know about -e. 

> >  done
> > -popd >/dev/null
> 
> Same here.

Will do. Standard sh(1) doesn't know popd/pushd either.

Thanks Christoph!


Christian.
-- 
BOFH excuse #398:

Data for intranet got routed through the extranet and landed on the internet.

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