Eric Blake <eblake@redhat.com> wrote on 04/07/2010 03:59:55 PM:
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.
I'll change this to be a bash script. No point in making the test program portable across shells.
+ 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? Hm, two hardcoded files like '/tmp/nwfiltervmtest1' and '/tmp/nwfiltervmtest2' could actually be a replacement, no?
+ + ${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
Ok, I'll change that.
+ 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.
I'll make it a bash-only script.
+ + exec ${cmd} | grep -v "^Bridge" | grep -v "^$" > ${tmpfile} + + rm ${tmpfile2} + touch ${tmpfile2} + + while [ 1 ]; do
I think that looks better as:
while :; do
hurts my eyes :-)
+ 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 ]
Ok, will fix that anyway.
+ + 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 ...
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'.
+ 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.
+ return 0 + fi + + return 1 +} + + +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.
+ 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.
Ok. Thanks for the review. Will adapt and re-post. Stefan
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
[attachment "signature.asc" deleted by Stefan Berger/Watson/IBM]