Eric Blake <ebl...@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 ebl...@redhat.com +1-801-349-2682 > Libvirt virtualization library http://libvirt.org > > [attachment "signature.asc" deleted by Stefan Berger/Watson/IBM]
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list