Re: [libvirt] [PATCH] nwfilters: Test suite for checking created firewall entries
On 04/07/2010 03:02 PM, Stefan Berger wrote:
> If this is a bash test, you need to update the #!.
> Otherwise, you have to give up on local variables.
I'll change this to be a bash script. No point in making the test program
portable across shells.
>> + tmpfile=`mktemp`
>> + tmpfile2=`mktemp`
>
> Not all systems have mktemp; is it worth adding fallback code in that
case?
Hm, two hardcoded files like '/tmp/nwfiltervmtest1' and
'/tmp/nwfiltervmtest2' could actually be a replacement, no?
Yeah, but names that obvious are prone to DoS attacks. If you are
assuming bash, you can at least use $RANDOM to get past the worst of the
security hole in being that blatant. Or, as long as we are assuming
bash and linux, you could just skip the test if mktemp doesn't exist.
>> + while [ ! -z "${line}" ]; do
>
> Depending on the content of line, this is unsafe in some implementations
> of test(1). Better would be:
>
> while [ "x$line" != x ]; do
>
Ok, I'll change that.
No need - if you change this to be #!/bin/bash, then you are guaranteed
that [ ! -z "${line}" ] does the right thing, for all $line.
>> + while [ 1 ]; do
>
> I think that looks better as:
>
> while :; do
hurts my eyes :-)
They are equivalent, so I don't care (I'm just used to writing shell
script in as few characters as possible, such as a=$b rather than
a="${b}", both of which are semantically equivalent but one is 4 bytes
longer to type).
>> + f=`${VIRSH} dumpxml ${vmname} | \
>> + tr -d "\n" | \
>> + sed "s/.*filterref filter='\([a-zA-Z0-9_]\+\)'.*/\1/"`
>
> That would fail on Solaris sed, where sed requires that a trailing
> newline be present in the input. Plus, "\n" is not necessarily portable
> shell (it is unclear whether the \ would be literally preserved). I
> would have used:
>
> { $VIRSH dumpxml $vmname | tr -d '\n'; echo; } | \
> sed ...
Good to know... will change even though the tests would not succeed on
Solaris.
I'll do a check using 'uname' to fail early on anything else than
'Linux'.
Once you've gone so far as to assume Linux, you can get away with a lot
of other assumptions (sed is decent, /bin/bash exists, ...). Then
again, I know that busybox prides itself on minimalistic POSIX
compliance, so it may be that busybox sed behaves like Solaris sed in
requiring text files (I don't know that for sure, though, as I seldom
target busybox).
>
>> + i=`${VIRSH} dumpxml ${vmname} | \
>> + tr -d "\n" | \
>> + sed "s/.*\<interface.*target
dev='\([a-zA-Z0-9_]\+\)'.*<\/
> interface>.*/\1/"`
>> +
>> + if [ "${i}" == "${ifname}" -a "${f}" ==
"${nwfilter}" ]; then
>
> Another abuse of [ -a ] that 'make syntax-check' should have caught.
I doesn't.
Huh. I guess I'll have to look into that.
>> +function main() {
>> + local prgname="$0"
>
> Some shells corrupt $0 in the context of shell functions.
Will change to bash, which will hopefully solve this issue.
Yeah, bash is immune to this particular problem. IIRC, it was zsh and
some proprietary /bin/sh.
> My review stops here - shell is my area of expertise, but my
Linux
> network filtering knowledge is sparse.
Ok. Thanks for the review. Will adapt and re-post.
Well, one of the joys of open source is that you can have multiple
reviewers, each with different angles of expertise, with a cumulative
review better than any one person could give.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 323 bytes)