On 04/07/2010 12:53 PM, Stefan Berger wrote: > +++ libvirt-acl/tests/nwfiltervmtest.sh > @@ -0,0 +1,186 @@ > +#!/bin/sh
Not all sh understand...
> +function doTest() {
> + local xmlfile="$1"
...local. If this is a bash test, you need to update the #!.
Otherwise, you have to give up on local variables.
> + local fwallfile="$2"
> + local ifname="$3"
> + local cmd line tmpfile tmpfile2
> + local linenums ctr=0
> + local regex="s/${ORIG_IFNAME}/${ifname}/g"
> +
> + if [ ! -r "${xmlfile}" ]; then
> + echo "FAIL : Cannot access filter XML file ${xmlfile}."
> + return 1
> + fi
> +
> + tmpfile=`mktemp`
> + tmpfile2=`mktemp`
Not all systems have mktemp; is it worth adding fallback code in that case?
> +
> + ${VIRSH} nwfilter-define "${xmlfile}" > /dev/null
> +
> + exec 4<&0
> +
> + exec < ${fwallfile}
> +
> + read line
> + 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
> + cmd=`echo ${line##\#} | sed ${regex}`
Another bash-ism, that would need a rewrite if you want to be portable
to generic sh. Or we decide to change the #!, and be done with the issue.
> +
> + exec ${cmd} | grep -v "^Bridge" | grep -v "^$" > ${tmpfile}
> +
> + rm ${tmpfile2}
> + touch ${tmpfile2}
> +
> + while [ 1 ]; do
I think that looks better as:
while :; do
> + read
> +
> + line="${REPLY}"
> +
> + if [ "${line:0:1}" == "#" -o -z "${line}" ]; then
More bash-isms. And [ cond1 -o cond2 ] ought to fail 'make
syntax-check' (if it didn't, then our syntax check regex is a bit
suspect). Of course, if you assume bash, then it is portable, but if
you are portable to any sh, this needs to use [ cond1 ] || [ cond2 ]
> +
> + diff ${tmpfile} ${tmpfile2} > /dev/null
> +
> + if [ $? -ne 0 ]; then
> + echo "FAIL ${xmlfile} : ${cmd}"
> + diff ${tmpfile} ${tmpfile2}
> + else
> + echo "PASS ${xmlfile} : ${cmd}"
> + fi
> +
> + break;
> +
> + fi
> + echo "${line}" | sed ${regex} >> ${tmpfile2}
> + done
> + done
> +
> + exec 0<&4
> + exec 4<&-
> +
> + rm -rf "${tmpfile}" "${tmpfile2}"
> +}
> +
> +
> +function runTests() {
> + local ifname="$1"
> + local xmldir="$2"
> + local fwalldir="$3"
> + local fwallfiles f
> +
> + pushd ${PWD} > /dev/null
> + cd ${fwalldir}
> + fwallfiles=`ls *.fwall`
> + popd > /dev/null
pushd/popd is another bash-ism.
> +
> + for fil in ${fwallfiles}; do
> + f=${fil%%.fwall}
Another bash-ism.
> + doTest "${xmldir}/${f}.xml" "${fwalldir}/${fil}" "${ifname}"
> + done
> +}
> +
> +
> +function checkVM() {
> + local vmname="$1"
> + local ifname="$2"
> + local nwfilter="$3"
> + local f i c
> +
> + c=`virsh dumpxml ${vmname} | grep -c "<interface"`
> + if [ ${c} -ne 1 ]; then
> + echo "VM '${vmname}' has multiple interfaces. I cannot tell for sure "
> + echo "whether this VM has the correct interface name '${ifname}' and "
> + echo "reference the filter '${nwfilter}'. Cowardly skipping this VM..."
> + return 1
> + fi
> +
> + 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 ...
> + 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.
> + return 0
> + fi
> +
> + return 1
> +}
> +
> +
> +function main() {
> + local prgname="$0"
Some shells corrupt $0 in the context of shell functions.
> + local ifname="${ORIG_IFNAME}"
> + local xmldir="nwfilterxml2xmlin"
> + local fwalldir="nwfilterxml2fwallout"
> + local found=0 vms
> + local filtername="testcase"
> +
> + while [ $# -ne 0 ]; do
> + case "$1" in
> + --help|-h|-\?) usage ${prgname}; exit 0;;
> + --ifname|-i) shift 1; ifname="$1";;
> + *) usage ${prgname}; exit 1;;
> + esac
> + shift 1
> + done
> +
> + vms=`${VIRSH} list | grep running | gawk '{print $2}'`
> + if [ -z ${vms} ]; then
> + echo "Error: Need a running VM."
> + exit 1;
> + fi
> +
> + for vm in ${vms}; do
> + checkVM "${vm}" "${ifname}" "${filtername}"
> + if [ $? -eq 0 ]; then
> + found=1;
> + break;
> + fi
> + done
> +
> + if [ ${found} -eq 0 ]; then
> + echo "Error: Suitable VM seems not to be running. Check the help screen";
> + echo "to (--help) to read about requirements to the running VM.";
> + exit 1;
> + fi
> +
> + runTests "${ifname}" "${xmldir}" "${fwalldir}"
> +}
> +
> +main $*
Would that be safer as:
main "$@"
My review stops here - shell is my area of expertise, but my Linux
network filtering knowledge is sparse.
--
Eric Blake ebl...@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
