[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]