[libvirt] [PATCH] nwfilters: Test suite for checking created firewall entries

[Stefan Berger]

Eric Blake <eblake at 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 at redhat.com    +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
> 
> [attachment "signature.asc" deleted by Stefan Berger/Watson/IBM] 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100407/f0b4709c/attachment-0001.htm>