7:06 p.m.
Derived from the nwfilter test program, this one works with libvirt's
networks. It creates networks from provided xml files and checks for
expected configuration in iptables, running processes and virsh output
using provided data files with commands to execute and the expected
results for after creating the network and after tearing it down
(*.post.dat). 3 tests are currently not passing due to a bug in
libvirt...
Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com>
---
Build.PL | 2
scripts/networks/100-apply-verify-host.t | 10
scripts/networks/networkApplyTest.sh | 343 +++++++++++++
scripts/networks/networkxml2hostout/tck-testnet-1.dat | 11
scripts/networks/networkxml2hostout/tck-testnet-1.post.dat | 4
scripts/networks/networkxml2hostout/tck-testnet-2.dat | 8
scripts/networks/networkxml2hostout/tck-testnet-2.post.dat | 4
scripts/networks/networkxml2xmlin/tck-testnet-1.xml | 12
scripts/networks/networkxml2xmlin/tck-testnet-2.xml | 12
9 files changed, 405 insertions(+), 1 deletion(-)
Index: libvirt-tck/scripts/networks/networkApplyTest.sh
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkApplyTest.sh
@@ -0,0 +1,343 @@
+#!/bin/bash
+
+VIRSH=virsh
+
+uri=
+if [ "x${LIBVIRT_TCK_CONFIG}x" != "xx" ]; then
+ uri_exp=`cat ${LIBVIRT_TCK_CONFIG} | grep "^uri\s*=" | sed -e
's/uri\s*=\s*//' | tail -n 1`
+ if [ "x${uri_exp}x" != "xx" ]; then
+ eval "uri=${uri_exp}"
+ fi
+ if [ "x${uri}x" == "xx" ]; then
+ uri="qemu:///system"
+ fi
+else
+ uri="qemu:///system"
+fi
+LIBVIRT_URI=${uri}
+
+
+FLAG_WAIT="$((1<<0))"
+FLAG_VERBOSE="$((1<<2))"
+FLAG_LIBVIRT_TEST="$((1<<3))"
+FLAG_TAP_TEST="$((1<<4))"
+FLAG_FORCE_CLEAN="$((1<<5))"
+
+failctr=0
+passctr=0
+attachfailctr=0
+attachctr=0
+
+TAP_FAIL_LIST=""
+TAP_FAIL_CTR=0
+TAP_TOT_CTR=0
+
+function usage() {
+ local cmd="$0"
+cat <<EOF
+Usage: ${cmd} [--help|-h|-?] [--noattach] [--wait] [--verbose]
+ [--libvirt-test] [--tap-test]
+
+Options:
+ --help,-h,-? : Display this help screen.
+ --wait : Wait for the user to press the enter key once an error
+ was detected
+ --verbose : Verbose output
+ --libvirt-test : Use the libvirt test output format
+ --tap-test : TAP format output
+ --force : Allow the automatic cleaning of VMs and networks
+ previously created by the TCK test suite
+
+This test creates libvirt 'networks' and checks for expected results
+(iptables, running processes (dnsmasq)) using provided xml and data
+file respectively.
+EOF
+}
+
+
+function tap_fail() {
+ echo "not ok $1 - ${2:0:66}"
+ TAP_FAIL_LIST+="$1 "
+ ((TAP_FAIL_CTR++))
+ ((TAP_TOT_CTR++))
+}
+
+function tap_pass() {
+ echo "ok $1 - ${2:0:70}"
+ ((TAP_TOT_CTR++))
+}
+
+function tap_final() {
+ local okay
+
+ [ -n "${TAP_FAIL_LIST}" ] && echo "FAILED tests
${TAP_FAIL_LIST}"
+
+ okay=`echo "($TAP_TOT_CTR-$TAP_FAIL_CTR)*100/$TAP_TOT_CTR" | bc -l`
+ echo "Failed ${TAP_FAIL_CTR}/${TAP_TOT_CTR} tests, ${okay:0:5}% okay"
+}
+
+# A wrapper for mktemp in case it does not exist
+# Echos the name of a temporary file.
+function mktmpfile() {
+ local tmp
+ type -P mktemp > /dev/null
+ if [ $? -eq 0 ]; then
+ tmp=$(mktemp -t nwfvmtest.XXXXXX)
+ echo ${tmp}
+ else
+ while :; do
+ tmp="/tmp/nwfvmtest.${RANDOM}"
+ if [ ! -f ${tmp} ]; then
+ touch ${tmp}
+ chmod 666 ${tmp}
+ echo ${tmp}
+ break
+ fi
+ done
+ fi
+ return 0
+}
+
+
+function checkExpectedOutput() {
+ local xmlfile="$1"
+ local datafile="$2"
+ local flags="$3"
+ local skipregex="$4"
+ local cmd line tmpfile tmpfile2 skip
+
+ tmpfile=`mktmpfile`
+ tmpfile2=`mktmpfile`
+
+ exec 4<${datafile}
+
+ read <&4
+ line="${REPLY}"
+
+ while [ "x${line}x" != "xx" ]; do
+ cmd=`echo ${line##\#}`
+
+ skip=0
+ if [ "x${skipregex}x" != "xx" ]; then
+ skip=`echo ${cmd} | grep -c -E ${skipregex}`
+ fi
+
+ eval ${cmd} 2>&1 | tee ${tmpfile} 1>/dev/null
+
+ rm ${tmpfile2} 2>/dev/null
+ touch ${tmpfile2}
+
+ while [ 1 ]; do
+ read <&4
+ line="${REPLY}"
+
+ if [ "${line:0:1}" == "#" ] || [ "x${line}x" ==
"xx" ]; then
+
+ if [ ${skip} -ne 0 ]; then
+ break
+ fi
+
+ diff ${tmpfile} ${tmpfile2} >/dev/null
+
+ if [ $? -ne 0 ]; then
+ if [ $((flags & FLAG_VERBOSE)) -ne 0 ]; then
+ echo "FAIL ${xmlfile} : ${cmd}"
+ diff ${tmpfile} ${tmpfile2}
+ fi
+ ((failctr++))
+ if [ $((flags & FLAG_WAIT)) -ne 0 ]; then
+ echo "tmp files: $tmpfile, $tmpfile2"
+ echo "Press enter"
+ read
+ fi
+ [ $((flags & FLAG_LIBVIRT_TEST)) -ne 0 ] && \
+ test_result $((passctr+failctr)) "" 1
+ [ $((flags & FLAG_TAP_TEST)) -ne 0 ] && \
+ tap_fail $((passctr+failctr)) "${xmlfile} : ${cmd}"
+ else
+ ((passctr++))
+ [ $((flags & FLAG_VERBOSE)) -ne 0 ] && \
+ echo "PASS ${xmlfile} : ${cmd}"
+ [ $((flags & FLAG_LIBVIRT_TEST)) -ne 0 ] && \
+ test_result $((passctr+failctr)) "" 0
+ [ $((flags & FLAG_TAP_TEST)) -ne 0 ] && \
+ tap_pass $((passctr+failctr)) "${xmlfile} : ${cmd}"
+ fi
+
+ break
+
+ fi
+ echo "${line}" >> ${tmpfile2}
+ done
+ done
+
+ exec 4>&-
+
+ rm -rf "${tmpfile}" "${tmpfile2}" 2>/dev/null
+}
+
+
+function doTest() {
+ local xmlfile="$1"
+ local datafile="$2"
+ local postdatafile="$3"
+ local flags="$4"
+ local netname
+
+ if [ ! -r "${xmlfile}" ]; then
+ echo "FAIL : Cannot access filter XML file ${xmlfile}."
+ return 1
+ fi
+
+ netname=`cat "${xmlfile}" | sed -n
's/.*<name>\([[:print:]]*\)<.*/\1/p'`
+
+ ${VIRSH} net-create "${xmlfile}" > /dev/null
+
+ checkExpectedOutput "${xmlfile}" "${datafile}" "${flags}"
""
+
+ ${VIRSH} net-destroy "${netname}" > /dev/null
+
+ if [ -r ${postdatafile} ]; then
+ checkExpectedOutput "${xmlfile}" "${postdatafile}"
"${flags}" ""
+ fi
+
+ return 0
+}
+
+
+function runTests() {
+ local xmldir="$1"
+ local hostoutdir="$2"
+ local flags="$3"
+ local datafiles f c
+ local tap_total=0 ctr=0
+
+ pushd ${PWD} > /dev/null
+ cd ${hostoutdir}
+ datafiles=`ls *.dat`
+ popd > /dev/null
+
+ if [ $((flags & FLAG_TAP_TEST)) -ne 0 ]; then
+ # Need to count the number of total tests
+ for fil in ${datafiles}; do
+ c=$(grep -c "^#" ${hostoutdir}/${fil})
+ ((tap_total+=c))
+ ((ctr++))
+ done
+ echo "1..${tap_total}"
+ fi
+
+ for fil in `echo "${datafiles}" | grep -v "\.post\.dat$"`; do
+ f=${fil%%.dat}
+ doTest "${xmldir}/${f}.xml" "${hostoutdir}/${fil}" \
+ "${hostoutdir}/${f}.post.dat" "${flags}"
+ done
+
+ if [ $((flags & FLAG_LIBVIRT_TEST)) -ne 0 ]; then
+ test_final $((passctr+failctr)) $failctr
+ elif [ $((flags & FLAG_TAP_TEST)) -ne 0 ]; then
+ tap_final
+ else
+ echo ""
+ echo "Summary: ${failctr} failures, ${passctr} passes,"
+ if [ ${attachctr} -ne 0 ]; then
+ echo " ${attachfailctr} interface attachment failures with
${attachctr} attempts"
+ fi
+ fi
+}
+
+
+function main() {
+ local prgname="$0"
+ local vm1 vm2
+ local xmldir="networkxml2xmlin"
+ local hostoutdir="networkxml2hostout"
+ local res rc
+ local flags
+
+ while [ $# -ne 0 ]; do
+ case "$1" in
+ --help|-h|-\?) usage ${prgname}; exit 0;;
+ --wait) ((flags |= FLAG_WAIT ));;
+ --verbose) ((flags |= FLAG_VERBOSE ));;
+ --libvirt-test) ((flags |= FLAG_LIBVIRT_TEST ));;
+ --tap-test) ((flags |= FLAG_TAP_TEST ));;
+ --force) ((flags |= FLAG_FORCE_CLEAN ));;
+ *) usage ${prgname}; exit 1;;
+ esac
+ shift 1
+ done
+
+ if [ `uname` != "Linux" ]; then
+ if [ $((flags & FLAG_TAP_TEST)) -ne 0 ]; then
+ echo "1..0 # Skipped: Only valid on Linux hosts"
+ else
+ echo "This script will only run on Linux."
+ fi
+ exit 1;
+ fi
+
+ if [ $((flags & FLAG_TAP_TEST)) -ne 0 ]; then
+ if [ "${LIBVIRT_URI}" != "qemu:///system" ]; then
+ echo "1..0 # Skipped: Only valid for Qemu system driver"
+ exit 0
+ fi
+
+ for name in `virsh list | awk '{print $2}'`
+ do
+ case ${name} in
+ tck*)
+ if [ "x${LIBVIRT_TCK_AUTOCLEAN}" == "x1" -o \
+ $((flags & FLAG_FORCE_CLEAN)) -ne 0 ]; then
+ res=$(virsh destroy ${name} 2>&1)
+ res=$(virsh undefine ${name} 2>&1)
+ if [ $? -ne 0 ]; then
+ echo "Bail out! Could not undefine VM ${name}: ${res}"
+ exit 0
+ fi
+ else
+ echo "Bail out! TCK VMs from previous tests still exist, use --force to
clean"
+ exit 1
+ fi
+ esac
+ done
+
+ for name in `virsh net-list | sed -n '3,$p'`
+ do
+ case ${name} in
+ tck*)
+ if [ "x${LIBVIRT_TCK_AUTOCLEAN}" == "x1" -o \
+ $((flags & FLAG_FORCE_CLEAN)) -ne 0 ]; then
+ res=$(virsh net-destroy ${name} 2>&1)
+ rc=$?
+ res=$(virsh net-undefine ${name} 2>&1)
+ if [ $rc -ne 0 -a $? -ne 0 ]; then
+ echo "Bail out! Could not destroy/undefine network ${name}:
${res}"
+ exit 1
+ fi
+ else
+ echo "Bail out! Network ${name} already exists, use --force to
clean"
+ exit 1
+ fi
+ esac
+ done
+ fi
+
+ if [ $((flags & FLAG_LIBVIRT_TEST)) -ne 0 ]; then
+ pushd ${PWD} > /dev/null
+ . test-lib.sh
+ if [ $? -ne 0 ]; then
+ exit 1
+ fi
+ test_intro $this_test
+ popd > /dev/null
+ fi
+
+ res=$(${VIRSH} capabilities 2>&1)
+
+ runTests "${xmldir}" "${hostoutdir}" "${flags}"
+
+ return 0
+}
+
+main "$@"
Index: libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.dat
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.dat
@@ -0,0 +1,11 @@
+#iptables -t nat -L -n | grep 10.1.2
+MASQUERADE tcp -- 10.1.2.0/24 !10.1.2.0/24 masq ports: 1024-65535
+MASQUERADE udp -- 10.1.2.0/24 !10.1.2.0/24 masq ports: 1024-65535
+MASQUERADE all -- 10.1.2.0/24 !10.1.2.0/24
+#iptables -n -L FORWARD | grep 10.1.2
+ACCEPT all -- anywhere 10.1.2.0/24 state RELATED,ESTABLISHED
+ACCEPT all -- 10.1.2.0/24 anywhere
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
+dnsmasq --strict-order --bind-interfaces
--pid-file=/var/run/libvirt/network/tck-testnet.pid --conf-file= --listen-address
10.1.2.1 --except-interface lo --dhcp-range 10.1.2.2,10.1.2.254 --dhcp-lease-max=253
--dhcp-no-override
+#virsh net-list | grep tck-testnet
+tck-testnet active no
Index: libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-1.xml
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-1.xml
@@ -0,0 +1,12 @@
+<network>
+ <name>tck-testnet</name>
+ <uuid>aadc8920-502a-4774-ac2b-cd382a204d06</uuid>
+ <forward mode='nat'/>
+ <bridge name='testbr' stp='on' delay='0' />
+ <ip address='10.1.2.1' netmask='255.255.255.0'>
+ <dhcp>
+ <range start='10.1.2.2' end='10.1.2.254' />
+ </dhcp>
+ </ip>
+</network>
+
Index: libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.dat
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.dat
@@ -0,0 +1,8 @@
+#iptables -L FORWARD | grep 10.1.2
+ACCEPT all -- anywhere 10.1.2.0/24
+ACCEPT all -- 10.1.2.0/24 anywhere
+#iptables -t nat -L | grep 10.1.2
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
+dnsmasq --strict-order --bind-interfaces
--pid-file=/var/run/libvirt/network/tck-testnet.pid --conf-file= --listen-address
10.1.2.1 --except-interface lo --dhcp-range 10.1.2.2,10.1.2.254 --dhcp-lease-max=253
--dhcp-no-override
+#virsh net-list | grep tck-testnet
+tck-testnet active no
Index: libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-2.xml
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-2.xml
@@ -0,0 +1,12 @@
+<network>
+ <name>tck-testnet</name>
+ <uuid>aadc8920-502a-4774-ac2b-cd382a204d06</uuid>
+ <forward mode='route'/>
+ <bridge name='testbr' stp='on' delay='0' />
+ <ip address='10.1.2.1' netmask='255.255.255.0'>
+ <dhcp>
+ <range start='10.1.2.2' end='10.1.2.254' />
+ </dhcp>
+ </ip>
+</network>
+
Index: libvirt-tck/scripts/networks/100-apply-verify-host.t
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/100-apply-verify-host.t
@@ -0,0 +1,10 @@
+#!/bin/bash
+
+pwd=$(dirname $0)
+
+pushd ${PWD} > /dev/null
+
+cd ${pwd}
+bash ./networkApplyTest.sh --tap-test
+
+popd > /dev/null
Index: libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.post.dat
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.post.dat
@@ -0,0 +1,4 @@
+#iptables -t nat -L -n | grep 10.1.2
+#iptables -n -L FORWARD | grep 10.1.2
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
+#virsh net-list | grep tck-testnet
Index: libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.post.dat
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.post.dat
@@ -0,0 +1,4 @@
+#iptables -t nat -L -n | grep 10.1.2
+#iptables -n -L FORWARD | grep 10.1.2
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
+#virsh net-list | grep tck-testnet
Index: libvirt-tck/Build.PL
===================================================================
--- libvirt-tck.orig/Build.PL
+++ libvirt-tck/Build.PL
@@ -29,7 +29,7 @@ sub process_pkgdata_files {
my $name = $File::Find::name;
if (-d) {
$tck_dirs{$name} = [];
- } elsif (-f && (/\.t$/ || /\.sh$/ || /\.fwall$/ || /\.xml$/)) {
+ } elsif (-f && /\.(t|sh|fwall|xml|dat)$/) {
push @{$tck_dirs{$dir}}, $name;
}
};
10:10 a.m.
On Fri, Oct 22, 2010 at 08:06:36PM -0400, Stefan Berger wrote:
Derived from the nwfilter test program, this one works with
libvirt's
networks. It creates networks from provided xml files and checks for
expected configuration in iptables, running processes and virsh output
using provided data files with commands to execute and the expected
results for after creating the network and after tearing it down
(*.post.dat). 3 tests are currently not passing due to a bug in
libvirt...
I've just posted a patch that should hopefully fix the problems
seen here
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:30 a.m.
On 10/25/2010 11:10 AM, Daniel P. Berrange wrote:
On Fri, Oct 22, 2010 at 08:06:36PM -0400, Stefan Berger wrote:
> Derived from the nwfilter test program, this one works with libvirt's
> networks. It creates networks from provided xml files and checks for
> expected configuration in iptables, running processes and virsh output
> using provided data files with commands to execute and the expected
> results for after creating the network and after tearing it down
> (*.post.dat). 3 tests are currently not passing due to a bug in
> libvirt...
I've just posted a patch that should hopefully fix the problems
seen here
Daniel
Locally applied and passes v2 of the test. ACK.
Stefan
6:08 p.m.
subject line: s/exected/expected/
On 10/22/2010 06:06 PM, Stefan Berger wrote:
Derived from the nwfilter test program, this one works with
libvirt's
networks. It creates networks from provided xml files and checks for
expected configuration in iptables, running processes and virsh output
using provided data files with commands to execute and the expected
results for after creating the network and after tearing it down
(*.post.dat). 3 tests are currently not passing due to a bug in
libvirt...
Now that the libvirt bug is squashed,
Signed-off-by: Stefan Berger<stefanb(a)us.ibm.com>
---
Build.PL | 2
scripts/networks/100-apply-verify-host.t | 10
scripts/networks/networkApplyTest.sh | 343 +++++++++++++
scripts/networks/networkxml2hostout/tck-testnet-1.dat | 11
scripts/networks/networkxml2hostout/tck-testnet-1.post.dat | 4
scripts/networks/networkxml2hostout/tck-testnet-2.dat | 8
scripts/networks/networkxml2hostout/tck-testnet-2.post.dat | 4
scripts/networks/networkxml2xmlin/tck-testnet-1.xml | 12
scripts/networks/networkxml2xmlin/tck-testnet-2.xml | 12
9 files changed, 405 insertions(+), 1 deletion(-)
Index: libvirt-tck/scripts/networks/networkApplyTest.sh
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkApplyTest.sh
@@ -0,0 +1,343 @@
+#!/bin/bash
Is this really bash-specific, or can we shoot for /bin/sh?
+
+VIRSH=virsh
+
+uri=
+if [ "x${LIBVIRT_TCK_CONFIG}x" != "xx" ]; then
+ uri_exp=`cat ${LIBVIRT_TCK_CONFIG} | grep "^uri\s*=" | sed -e
's/uri\s*=\s*//' | tail -n 1`
If it's bash-specific, I'd prefer to see $() rather than ``. But until
I see a definite bash-ism, I'd shoot for /bin/sh instead.
Missing "" around ${LIBVIRT_TCK_CONFIG}.
\s is non-portable; you can't expect either grep or sed to understand it.
cat | grep | sed | tail wastes 3 processes; you can do it all with a
single sed:
uri_exp=`sed -n '/^uri[ ]*=[ ]*/ {
s///
h
}
$ {
x
p
}' < "$LIBVIRT_TCK_CONFIG"`
[Sed deciphered: for lines that match ^uri\s*=\s*, delete the matched
prefix, and put the line in hold space, overriding anything already
there. Then, at end of input, exchange the hold space back out and print
it, which will thus be the last uri= line found.]
+ if [ "x${uri_exp}x" != "xx" ]; then
+ eval "uri=${uri_exp}"
This is unsafe - it can end up executing arbitrary user input from the
config file. And why do you need the eval in the first place? Isn't it
just sufficient to do:
uri=${uri_exp}
Also, this is not robust if $uri_exp is inherited from the environment,
because you failed to initialize uri_exp when LIBVIRT_TCK_CONFIG is not
specified.
+ fi
+ if [ "x${uri}x" == "xx" ]; then
+ uri="qemu:///system"
+ fi
+else
+ uri="qemu:///system"
+fi
+LIBVIRT_URI=${uri}
+
+
+FLAG_WAIT="$((1<<0))"
$(()) assumes POSIX, but is not portable to Solaris. It's easy to work
around:
FLAG_WAIT=1
+FLAG_VERBOSE="$((1<<2))"
FLAG_VERBOSE=2
+FLAG_LIBVIRT_TEST="$((1<<3))"
FLAG_LIBVIRT_TEST=4
+FLAG_TAP_TEST="$((1<<4))"
FLAG_TAP_TEST=8
+FLAG_FORCE_CLEAN="$((1<<5))"
FLAG_FORCE_CLEAN=16
+
+failctr=0
+passctr=0
+attachfailctr=0
+attachctr=0
+
+TAP_FAIL_LIST=""
+TAP_FAIL_CTR=0
+TAP_TOT_CTR=0
+
+function usage() {
+ local cmd="$0"
local is a non-POSIX bash-ism. Also, $0 is non-portable to some
versions of zsh; better would be:
cmd="$0"
function_usage() {
... $cmd ...
+cat<<EOF
+Usage: ${cmd} [--help|-h|-?] [--noattach] [--wait] [--verbose]
+ [--libvirt-test] [--tap-test]
+
+Options:
+ --help,-h,-? : Display this help screen.
+ --wait : Wait for the user to press the enter key once an error
+ was detected
+ --verbose : Verbose output
+ --libvirt-test : Use the libvirt test output format
+ --tap-test : TAP format output
+ --force : Allow the automatic cleaning of VMs and networks
+ previously created by the TCK test suite
+
+This test creates libvirt 'networks' and checks for expected results
+(iptables, running processes (dnsmasq)) using provided xml and data
+file respectively.
+EOF
+}
+
+
+function tap_fail() {
+ echo "not ok $1 - ${2:0:66}"
Bash-ism; do you really have to truncate to just the first 66 bytes, or
can you use the portable:
echo "not ok $1 - $2"
assuming that $2 does not contain any \ (if it does, you'd have to use
'printf %s\\n' instead of 'echo').
+ TAP_FAIL_LIST+="$1 "
Bash-ism; you can use the portable:
TAP_FAIL_LIST="$TAP_FAIL_LIST$1 "
+ ((TAP_FAIL_CTR++))
Bash-ism; you can use the portable:
TAP_FAIL_CTR=`expr $TAP_FAIL_CTR + 1`
+ ((TAP_TOT_CTR++))
Likewise.
> +}
> +
> +function tap_pass() {
> + echo "ok $1 - ${2:0:70}"
+ ((TAP_TOT_CTR++))
Similar comments.
+}
+
+function tap_final() {
+ local okay
Another use of local.
+
+ [ -n "${TAP_FAIL_LIST}" ]&& echo "FAILED tests
${TAP_FAIL_LIST}"
+
+ okay=`echo "($TAP_TOT_CTR-$TAP_FAIL_CTR)*100/$TAP_TOT_CTR" | bc -l`
+ echo "Failed ${TAP_FAIL_CTR}/${TAP_TOT_CTR} tests, ${okay:0:5}% okay"
Bash-ism; do we really need to output percentage passing, or can we just
get rid of $okay?
+}
+
+# A wrapper for mktemp in case it does not exist
+# Echos the name of a temporary file.
+function mktmpfile() {
+ local tmp
Another local.
+ type -P mktemp> /dev/null
Bash-ism.
+ if [ $? -eq 0 ]; then
+ tmp=$(mktemp -t nwfvmtest.XXXXXX)
+ echo ${tmp}
+ else
+ while :; do
+ tmp="/tmp/nwfvmtest.${RANDOM}"
Bash-ism, with a VERY predictable filename on dash; two runs are
guaranteed to collide.
+ if [ ! -f ${tmp} ]; then
+ touch ${tmp}
+ chmod 666 ${tmp}
Ouch - that's multiple security holes. Someone could inject a symlink
at $tmp in between your [ ! f ] and touch, and your chmod makes the file
world-writeable. All told, that means you may have just granted an
arbitrary user write access to a file they shouldn't know about, just
because they won the race at creating the symlink.
+ echo ${tmp}
+ break
+ fi
+ done
+ fi
+ return 0
+}
Rather than trying to securely create a temporary file in shell (which
is nigh impossible if mktemp doesn't exist), it is MUCH better to create
a secure temporary directory, then create arbitrary files within that
directory. Here's how autoconf creates secure directories:
# Create a (secure) tmp directory for tmp files.
{
tmp=`(umask 077 && mktemp -d "./confXXXXXX") 2>/dev/null`
&&
test -n "$tmp" && test -d "$tmp"
} ||
{
tmp=./conf$$-$RANDOM
(umask 077 && mkdir "$tmp")
} || { echo failed... >&2; exit 1; }
And while that use of $RANDOM will still be worthless on dash, the
addition of $$ adds a bit of collision avoidance, and the atomicity of
mkdir is the final piece of the puzzle to guarantee that you did not
lose a race.
+
+
+function checkExpectedOutput() {
+ local xmlfile="$1"
+ local datafile="$2"
+ local flags="$3"
+ local skipregex="$4"
+ local cmd line tmpfile tmpfile2 skip
More instances of local.
+
+ tmpfile=`mktmpfile`
+ tmpfile2=`mktmpfile`
+
+ exec 4<${datafile}
+
+ read<&4
+ line="${REPLY}"
+
+ while [ "x${line}x" != "xx" ]; do
+ cmd=`echo ${line##\#}`
Bash-ism, and underquoted if $line contains spaces. You could use:
case $line in
#*) cmd=`echo "$line" | sed 's/^#//'` ;;
*) cmd=$line ;;
esac
+
+ skip=0
+ if [ "x${skipregex}x" != "xx" ]; then
+ skip=`echo ${cmd} | grep -c -E ${skipregex}`
On some systems, you still have to use egrep instead of grep -E.
Usually, this involves populating $EGREP to one of the two variants up
front.
+ fi
+
+ eval ${cmd} 2>&1 | tee ${tmpfile} 1>/dev/null
+
+ rm ${tmpfile2} 2>/dev/null
This is usually written as rm -f $tmpfile2, rather than redirecting stderr.
+ touch ${tmpfile2}
Removing and then recreating a temporary file is unsafe, if the
temporary file did not already reside in a secure temporary directory.
It's faster to do:
: >"$tmpfile2"
to do the truncation in one step, rather than the two-step rm/touch.
+
+ while [ 1 ]; do
This is generally written 'while :; do' in shell.
+ read<&4
+ line="${REPLY}"
+
+ if [ "${line:0:1}" == "#" ] || [ "x${line}x" ==
"xx" ]; then
Bash-ism. Portable alternative:
case $line in
'' | #*) continue ;;
esac
+
+ if [ ${skip} -ne 0 ]; then
TAB indentation; libvirt-TCK is probably a bit more relaxed, but I'm
rather used to space indentation.
+ break
+ fi
+
+ diff ${tmpfile} ${tmpfile2}>/dev/null
+
+ if [ $? -ne 0 ]; then
+ if [ $((flags& FLAG_VERBOSE)) -ne 0 ]; then
Bash-ism. What's setting $flags in the first place? Can we re-write
this to instead use 5 flag variables, which either contain : or false,
so that you can do:
if $flag_verbose; then
+ echo "FAIL ${xmlfile} : ${cmd}"
+ diff ${tmpfile} ${tmpfile2}
+ fi
+ ((failctr++))
bash-ism
+ if [ $((flags& FLAG_WAIT)) -ne 0 ]; then
+ echo "tmp files: $tmpfile, $tmpfile2"
+ echo "Press enter"
+ read
+ fi
+ [ $((flags& FLAG_LIBVIRT_TEST)) -ne 0 ]&& \
+ test_result $((passctr+failctr)) "" 1
+ [ $((flags& FLAG_TAP_TEST)) -ne 0 ]&& \
+ tap_fail $((passctr+failctr)) "${xmlfile} : ${cmd}"
+ else
+ ((passctr++))
+ [ $((flags& FLAG_VERBOSE)) -ne 0 ]&& \
+ echo "PASS ${xmlfile} : ${cmd}"
+ [ $((flags& FLAG_LIBVIRT_TEST)) -ne 0 ]&& \
+ test_result $((passctr+failctr)) "" 0
+ [ $((flags& FLAG_TAP_TEST)) -ne 0 ]&& \
+ tap_pass $((passctr+failctr)) "${xmlfile} : ${cmd}"
Etc.
+ fi
+
+ break
+
+ fi
+ echo "${line}">> ${tmpfile2}
+ done
+ done
+
+ exec 4>&-
+
+ rm -rf "${tmpfile}" "${tmpfile2}" 2>/dev/null
Don't mix rm -f and redirecting stderr; do only one or the other (I
prefer -f).
+}
+
+
+function doTest() {
+ local xmlfile="$1"
+ local datafile="$2"
+ local postdatafile="$3"
+ local flags="$4"
+ local netname
More use of local.
+
+ if [ ! -r "${xmlfile}" ]; then
+ echo "FAIL : Cannot access filter XML file ${xmlfile}."
+ return 1
+ fi
+
+ netname=`cat "${xmlfile}" | sed -n
's/.*<name>\([[:print:]]*\)<.*/\1/p'`
Useless use of cat.
Unfortunately, it is not yet portable to use [[:print:]] in sed. The
alternative is:
netname=`sed -n 's/.*<name>\([^<]*\).*/\1p' < "$xmlfile"`
+
+ ${VIRSH} net-create "${xmlfile}"> /dev/null
+
+ checkExpectedOutput "${xmlfile}" "${datafile}"
"${flags}" ""
+
+ ${VIRSH} net-destroy "${netname}"> /dev/null
+
+ if [ -r ${postdatafile} ]; then
+ checkExpectedOutput "${xmlfile}" "${postdatafile}"
"${flags}" ""
+ fi
+
+ return 0
+}
+
+
+function runTests() {
+ local xmldir="$1"
+ local hostoutdir="$2"
+ local flags="$3"
+ local datafiles f c
+ local tap_total=0 ctr=0
More use of local.
+
+ pushd ${PWD}> /dev/null
bash-ism.
+ cd ${hostoutdir}
+ datafiles=`ls *.dat`
+ popd> /dev/null
Rather than rely on pushd/popd, you could do:
datafiles=`cd "$hostoutdir" && echo *.dat`
+
+ if [ $((flags& FLAG_TAP_TEST)) -ne 0 ]; then
again wondering if we can use a /bin/sh portable means of tracking flags.
+ # Need to count the number of total tests
+ for fil in ${datafiles}; do
+ c=$(grep -c "^#" ${hostoutdir}/${fil})
+ ((tap_total+=c))
+ ((ctr++))
bash-isms
+ done
+ echo "1..${tap_total}"
+ fi
+
+ for fil in `echo "${datafiles}" | grep -v "\.post\.dat$"`; do
Wasted subprocesses. Why not:
for fil in $datafiles; do
case $fil in
*.post.dat) continue;;
esac
+ f=${fil%%.dat}
POSIX-ism that won't work on Solaris. But the alternative requires
forking a sed process, even on bash.
+ doTest "${xmldir}/${f}.xml"
"${hostoutdir}/${fil}" \
+ "${hostoutdir}/${f}.post.dat" "${flags}"
+ done
+
+ if [ $((flags& FLAG_LIBVIRT_TEST)) -ne 0 ]; then
+ test_final $((passctr+failctr)) $failctr
bash-isms
+ elif [ $((flags& FLAG_TAP_TEST)) -ne 0 ]; then
+ tap_final
+ else
+ echo ""
+ echo "Summary: ${failctr} failures, ${passctr} passes,"
+ if [ ${attachctr} -ne 0 ]; then
+ echo " ${attachfailctr} interface attachment failures with
${attachctr} attempts"
+ fi
+ fi
+}
+
+
+function main() {
+ local prgname="$0"
+ local vm1 vm2
+ local xmldir="networkxml2xmlin"
+ local hostoutdir="networkxml2hostout"
+ local res rc
+ local flags
More uses of local
+
+ while [ $# -ne 0 ]; do
+ case "$1" in
+ --help|-h|-\?) usage ${prgname}; exit 0;;
+ --wait) ((flags |= FLAG_WAIT ));;
+ --verbose) ((flags |= FLAG_VERBOSE ));;
+ --libvirt-test) ((flags |= FLAG_LIBVIRT_TEST ));;
+ --tap-test) ((flags |= FLAG_TAP_TEST ));;
+ --force) ((flags |= FLAG_FORCE_CLEAN ));;
Ah; here's where you were setting $flags. Like I said, it would be more
portable to set five different variables; pre-set them to false, and
then convert them to : if the command line option is supplied. It makes
testing avoid arithmetic in the first place:
flag_wait=false
case "$1" in
--wait) flag_wait=:;;
...
esac
if $flag_wait; then
...
fi
> + *) usage ${prgname}; exit 1;;
> + esac
> + shift 1
> + done
> +
> + if [ `uname` != "Linux" ]; then
> + if [ $((flags& FLAG_TAP_TEST)) -ne 0 ]; then
> + echo "1..0 # Skipped: Only valid on Linux hosts"
> + else
> + echo "This script will only run on Linux."
> + fi
> + exit 1;
> + fi
+
+ if [ $((flags& FLAG_TAP_TEST)) -ne 0 ]; then
> + if [
"${LIBVIRT_URI}" != "qemu:///system" ]; then
> + echo "1..0 # Skipped: Only valid for Qemu system driver"
> + exit 0
> + fi
> +
> + for name in `virsh list | awk '{print $2}'`
> + do
> + case ${name} in
> + tck*)
> + if [ "x${LIBVIRT_TCK_AUTOCLEAN}" == "x1" -o \
> + $((flags& FLAG_FORCE_CLEAN)) -ne 0 ]; then
> + res=$(virsh destroy ${name} 2>&1)
> + res=$(virsh undefine ${name} 2>&1)
> + if [ $? -ne 0 ]; then
> + echo "Bail out! Could not undefine VM ${name}: ${res}"
> + exit 0
> + fi
> + else
> + echo "Bail out! TCK VMs from previous tests still exist, use --force
to clean"
> + exit 1
> + fi
> + esac
> + done
> +
> + for name in `virsh net-list | sed -n '3,$p'`
> + do
> + case ${name} in
> + tck*)
> + if [ "x${LIBVIRT_TCK_AUTOCLEAN}" == "x1" -o \
> + $((flags& FLAG_FORCE_CLEAN)) -ne 0 ]; then
> + res=$(virsh net-destroy ${name} 2>&1)
> + rc=$?
> + res=$(virsh net-undefine ${name} 2>&1)
> + if [ $rc -ne 0 -a $? -ne 0 ]; then
> + echo "Bail out! Could not destroy/undefine network ${name}:
${res}"
> + exit 1
> + fi
> + else
> + echo "Bail out! Network ${name} already exists, use --force to
clean"
> + exit 1
> + fi
> + esac
> + done
> + fi
> +
> + if [ $((flags& FLAG_LIBVIRT_TEST)) -ne 0 ]; then
> + pushd ${PWD}> /dev/null
Another non-portable use of pushd. If sourcing test-lib.sh leaves us in
a different directory, then we should fix the bug in test-lib.sh.
+ . test-lib.sh
This assumes that test-lib.sh is on $PATH; if you want to explicitly run
the file in the current directory, it needs to be
. ./test-lib.sh
+ if [ $? -ne 0 ]; then
+ exit 1
+ fi
+ test_intro $this_test
+ popd> /dev/null
+ fi
+
+ res=$(${VIRSH} capabilities 2>&1)
+
+ runTests "${xmldir}" "${hostoutdir}" "${flags}"
+
+ return 0
+}
+
+main "$@"
Hopefully you aren't too depressed with me picking apart your shell
scripting :)
Index:
libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.dat
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.dat
@@ -0,0 +1,11 @@
+#iptables -t nat -L -n | grep 10.1.2
+MASQUERADE tcp -- 10.1.2.0/24 !10.1.2.0/24 masq ports: 1024-65535
+MASQUERADE udp -- 10.1.2.0/24 !10.1.2.0/24 masq ports: 1024-65535
+MASQUERADE all -- 10.1.2.0/24 !10.1.2.0/24
+#iptables -n -L FORWARD | grep 10.1.2
Oh, so you really DO want .dat files to specify a combination of shell
commands to eval, followed by expected output of those commands. Hmm, I
probably missed that point in my shell script evaluation above.
+ACCEPT all -- anywhere 10.1.2.0/24 state
RELATED,ESTABLISHED
+ACCEPT all -- 10.1.2.0/24 anywhere
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
Again, [[:print:]] is not portable to all sed; but given that your test
only runs on Linux, I guess we can safely assume GNU (or busybox) sed,
where it does work.
Should you be a little tighter on the first grep, to require '/dnsmasq '?
The second grep would match a10b1c2, which you didn't intend. grep |
sed wastes a process; you could do:
ps aux | sed -n '|/dnsmasq .*10\.1\.2\./
s/.*\\(dnsmasq[[:print:]*]\\)|\\1|p'
+dnsmasq --strict-order --bind-interfaces
--pid-file=/var/run/libvirt/network/tck-testnet.pid --conf-file= --listen-address
10.1.2.1 --except-interface lo --dhcp-range 10.1.2.2,10.1.2.254 --dhcp-lease-max=253
--dhcp-no-override
+#virsh net-list | grep tck-testnet
+tck-testnet active no
Index: libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-1.xml
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-1.xml
@@ -0,0 +1,12 @@
+<network>
+<name>tck-testnet</name>
+<uuid>aadc8920-502a-4774-ac2b-cd382a204d06</uuid>
+<forward mode='nat'/>
+<bridge name='testbr' stp='on' delay='0' />
+<ip address='10.1.2.1' netmask='255.255.255.0'>
+<dhcp>
+<range start='10.1.2.2' end='10.1.2.254' />
+</dhcp>
+</ip>
+</network>
+
Index: libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.dat
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.dat
@@ -0,0 +1,8 @@
+#iptables -L FORWARD | grep 10.1.2
+ACCEPT all -- anywhere 10.1.2.0/24
+ACCEPT all -- 10.1.2.0/24 anywhere
+#iptables -t nat -L | grep 10.1.2
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
same comments on grep|sed
+dnsmasq --strict-order --bind-interfaces
--pid-file=/var/run/libvirt/network/tck-testnet.pid --conf-file= --listen-address
10.1.2.1 --except-interface lo --dhcp-range 10.1.2.2,10.1.2.254 --dhcp-lease-max=253
--dhcp-no-override
+#virsh net-list | grep tck-testnet
+tck-testnet active no
Index: libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-2.xml
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2xmlin/tck-testnet-2.xml
@@ -0,0 +1,12 @@
+<network>
+<name>tck-testnet</name>
+<uuid>aadc8920-502a-4774-ac2b-cd382a204d06</uuid>
+<forward mode='route'/>
+<bridge name='testbr' stp='on' delay='0' />
+<ip address='10.1.2.1' netmask='255.255.255.0'>
+<dhcp>
+<range start='10.1.2.2' end='10.1.2.254' />
+</dhcp>
+</ip>
+</network>
+
Index: libvirt-tck/scripts/networks/100-apply-verify-host.t
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/100-apply-verify-host.t
@@ -0,0 +1,10 @@
+#!/bin/bash
+
+pwd=$(dirname $0)
Underquoted, if $0 contains spaces. Technically, it could also be
problematic if $0 starts with -, although that is less likely, and
'dirname -- "$0"' is unfortunately not portable.
+
+pushd ${PWD}> /dev/null
+
+cd ${pwd}
+bash ./networkApplyTest.sh --tap-test
+
+popd> /dev/null
I'm still not convinced whether we need to require bash for all of
these. What's wrong with:
#!/bin/sh
dir=`dirname "$0"`
(cd "$dir" && ./networkApplyTest.sh --tap-test)
Index:
libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.post.dat
===================================================================
--- /dev/null
+++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-1.post.dat
@@ -0,0 +1,4 @@
+#iptables -t nat -L -n | grep 10.1.2
This grep is too weak. You want:
#iptables -t nat -L -n | grep ' 10\.1\.2'
+#iptables -n -L FORWARD | grep 10.1.2
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
Same grep | sed comments.
> +#virsh net-list | grep tck-testnet
> Index: libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.post.dat
> ===================================================================
> --- /dev/null
> +++ libvirt-tck/scripts/networks/networkxml2hostout/tck-testnet-2.post.dat
> @@ -0,0 +1,4 @@
> +#iptables -t nat -L -n | grep 10.1.2
+#iptables -n -L FORWARD | grep 10.1.2
+#ps aux | grep dnsmasq | grep 10.1.2 | sed -n
's/.*\\(dnsmasq[[:print:]*]\\)/\\1/p'
And again.
+#virsh net-list | grep tck-testnet
Index: libvirt-tck/Build.PL
===================================================================
--- libvirt-tck.orig/Build.PL
+++ libvirt-tck/Build.PL
@@ -29,7 +29,7 @@ sub process_pkgdata_files {
my $name = $File::Find::name;
if (-d) {
$tck_dirs{$name} = [];
- } elsif (-f&& (/\.t$/ || /\.sh$/ || /\.fwall$/ || /\.xml$/)) {
+ } elsif (-f&& /\.(t|sh|fwall|xml|dat)$/) {
push @{$tck_dirs{$dir}}, $name;
}
};
Sorry for sounding so depressing; I'm very pleased to see your efforts
in providing tests for the code you've written. Even though this test
is intended to be skipped on non-Linux, you still have to worry about
merely parsing through the test on other platforms like Solaris, where
/bin/sh won't understand the bash-isms and where /bin/bash is not
guaranteed to exist. But if we decide that requiring the presence of
/bin/bash is acceptable for the TCK, then a lot of my review becomes
irrelevant; but my comments about your mkstemp replacement being
insecure are still applicable even in that case.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:27 p.m.
On 10/26/2010 07:08 PM, Eric Blake wrote:
Sorry for sounding so depressing; I'm very pleased to see your efforts
in providing tests for the code you've written. Even though this test
is intended to be skipped on non-Linux, you still have to worry about
merely parsing through the test on other platforms like Solaris, where
/bin/sh won't understand the bash-isms and where /bin/bash is not
guaranteed to exist. But if we decide that requiring the presence of
/bin/bash is acceptable for the TCK, then a lot of my review becomes
irrelevant; but my comments about your mkstemp replacement being
insecure are still applicable even in that case.
Ok. Well, I hope for bash then...
Stefan
10:33 a.m.
On 10/26/2010 05:27 PM, Stefan Berger wrote:
On 10/26/2010 07:08 PM, Eric Blake wrote:
>
> Sorry for sounding so depressing; I'm very pleased to see your efforts
> in providing tests for the code you've written. Even though this test
> is intended to be skipped on non-Linux, you still have to worry about
> merely parsing through the test on other platforms like Solaris, where
> /bin/sh won't understand the bash-isms and where /bin/bash is not
> guaranteed to exist. But if we decide that requiring the presence of
> /bin/bash is acceptable for the TCK, then a lot of my review becomes
> irrelevant; but my comments about your mkstemp replacement being
> insecure are still applicable even in that case.
>
Ok. Well, I hope for bash then...
IRC verdict - Dan and I are both okay with assuming /bin/bash exists.
So, that just leaves cleaning up the temporary file management. Would
you like me to tackle that as an incremental diff on top of your
original patch?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:56 a.m.
On 10/29/2010 11:33 AM, Eric Blake wrote:
On 10/26/2010 05:27 PM, Stefan Berger wrote:
> On 10/26/2010 07:08 PM, Eric Blake wrote:
>> Sorry for sounding so depressing; I'm very pleased to see your efforts
>> in providing tests for the code you've written. Even though this test
>> is intended to be skipped on non-Linux, you still have to worry about
>> merely parsing through the test on other platforms like Solaris, where
>> /bin/sh won't understand the bash-isms and where /bin/bash is not
>> guaranteed to exist. But if we decide that requiring the presence of
>> /bin/bash is acceptable for the TCK, then a lot of my review becomes
>> irrelevant; but my comments about your mkstemp replacement being
>> insecure are still applicable even in that case.
>>
> Ok. Well, I hope for bash then...
IRC verdict - Dan and I are both okay with assuming /bin/bash exists.
So, that just leaves cleaning up the temporary file management. Would
you like me to tackle that as an incremental diff on top of your
original patch?
Yes, you can put it in on top of it. So is this an ACK now so I can push?
I just currently don't have the time to convert it into a generic shell
script, but put that on a future todo list. Also the original nwfilter
test should probably be converted then as well...
Stefan
5:41 p.m.
On 11/01/2010 04:56 AM, Stefan Berger wrote:
On 10/29/2010 11:33 AM, Eric Blake wrote:
> On 10/26/2010 05:27 PM, Stefan Berger wrote:
>> On 10/26/2010 07:08 PM, Eric Blake wrote:
>>> Sorry for sounding so depressing; I'm very pleased to see your efforts
>>> in providing tests for the code you've written. Even though this test
>>> is intended to be skipped on non-Linux, you still have to worry about
>>> merely parsing through the test on other platforms like Solaris, where
>>> /bin/sh won't understand the bash-isms and where /bin/bash is not
>>> guaranteed to exist. But if we decide that requiring the presence of
>>> /bin/bash is acceptable for the TCK, then a lot of my review becomes
>>> irrelevant; but my comments about your mkstemp replacement being
>>> insecure are still applicable even in that case.
>>>
>> Ok. Well, I hope for bash then...
> IRC verdict - Dan and I are both okay with assuming /bin/bash exists.
> So, that just leaves cleaning up the temporary file management. Would
> you like me to tackle that as an incremental diff on top of your
> original patch?
>
Yes, you can put it in on top of it. So is this an ACK now so I can push?
Actually, since you have already got the test setup ready, would you
mind just squashing this in? If it works for you, then you have my ACK
to push the squashed version, such that libvirt-tck.git only has a
single commit with all the problems already fixed.
This incremental diff to your patch converts `` to $(), fixes a few
useless uses of cat, changes temporary file creation to instead use a
secure temporary directory, ensures grep -E will either work or has a
fallback of egrep, and adds "" around more variables that might be
user-provided.
diff --git i/scripts/networks/networkApplyTest.sh
w/scripts/networks/networkApplyTest.sh
index 547f00f..6627dd3 100644
--- i/scripts/networks/networkApplyTest.sh
+++ w/scripts/networks/networkApplyTest.sh
@@ -2,18 +2,19 @@
VIRSH=virsh
-uri=
-if [ "x${LIBVIRT_TCK_CONFIG}x" != "xx" ]; then
- uri_exp=`cat ${LIBVIRT_TCK_CONFIG} | grep "^uri\s*=" | sed -e
's/uri\s*=\s*//' | tail -n 1`
- if [ "x${uri_exp}x" != "xx" ]; then
- eval "uri=${uri_exp}"
- fi
- if [ "x${uri}x" == "xx" ]; then
- uri="qemu:///system"
- fi
-else
- uri="qemu:///system"
-fi
+# For each line starting with uri=, remove the prefix and set the hold
+# space to the rest of the line. Then at file end, print the hold
+# space, which is effectively the last uri= line encountered.
+uri=$(sed -n '/^uri[ ]*=[ ]*/ {
+ s///
+ h
+}
+$ {
+ x
+ p
+}' < "$LIBVIRT_TCK_CONFIG")
+: "${uri:=qemu:///system}"
+
LIBVIRT_URI=${uri}
@@ -72,29 +73,23 @@ function tap_final() {
[ -n "${TAP_FAIL_LIST}" ] && echo "FAILED tests
${TAP_FAIL_LIST}"
- okay=`echo "($TAP_TOT_CTR-$TAP_FAIL_CTR)*100/$TAP_TOT_CTR" | bc -l`
+ okay=$(echo "($TAP_TOT_CTR-$TAP_FAIL_CTR)*100/$TAP_TOT_CTR" | bc -l)
echo "Failed ${TAP_FAIL_CTR}/${TAP_TOT_CTR} tests, ${okay:0:5}% okay"
}
# A wrapper for mktemp in case it does not exist
-# Echos the name of a temporary file.
-function mktmpfile() {
+# Echos the name of a secure temporary directory.
+function mktmpdir() {
local tmp
- type -P mktemp > /dev/null
- if [ $? -eq 0 ]; then
- tmp=$(mktemp -t nwfvmtest.XXXXXX)
- echo ${tmp}
- else
- while :; do
- tmp="/tmp/nwfvmtest.${RANDOM}"
- if [ ! -f ${tmp} ]; then
- touch ${tmp}
- chmod 666 ${tmp}
- echo ${tmp}
- break
- fi
- done
- fi
+ {
+ tmp=$( (umask 077 && mktemp -d ./nwfvmtest.XXXXXX) 2>/dev/null)
&&
+ test -n "$tmp" && test -d "$tmp"
+ } ||
+ {
+ tmp=./nwfvmtest$$-$RANDOM
+ (umask 077 && mkdir "$tmp")
+ } || { echo "failed to create secure temporary directory" >&2; exit 1;
}
+ echo "${tmp}"
return 0
}
@@ -104,51 +99,56 @@ function checkExpectedOutput() {
local datafile="$2"
local flags="$3"
local skipregex="$4"
- local cmd line tmpfile tmpfile2 skip
+ local cmd line tmpdir tmpfile tmpfile2 skip
- tmpfile=`mktmpfile`
- tmpfile2=`mktmpfile`
+ tmpdir=$(mktmpdir)
+ tmpfile=$tmpdir/file
+ tmpfile2=$tmpdir/file2
+
+ if echo a | grep -E '(a|b)' >/dev/null 2>&1
+ then EGREP='grep -E'
+ else EGREP=egrep
+ fi
- exec 4<${datafile}
+ exec 4<"${datafile}"
read <&4
line="${REPLY}"
while [ "x${line}x" != "xx" ]; do
- cmd=`echo ${line##\#}`
+ cmd=$(echo "${line##\#}")
skip=0
if [ "x${skipregex}x" != "xx" ]; then
- skip=`echo ${cmd} | grep -c -E ${skipregex}`
+ skip=$(echo "${cmd}" | ${EGREP} -c ${skipregex})
fi
- eval ${cmd} 2>&1 | tee ${tmpfile} 1>/dev/null
+ eval "${cmd}" 2>&1 | tee "${tmpfile}" 1>/dev/null
- rm ${tmpfile2} 2>/dev/null
- touch ${tmpfile2}
+ : >"{tmpfile2}"
- while [ 1 ]; do
+ while :; do
read <&4
line="${REPLY}"
if [ "${line:0:1}" == "#" ] || [ "x${line}x" ==
"xx" ]; then
- if [ ${skip} -ne 0 ]; then
- break
- fi
+ if [ ${skip} -ne 0 ]; then
+ break
+ fi
- diff ${tmpfile} ${tmpfile2} >/dev/null
+ diff "${tmpfile}" "${tmpfile2}" >/dev/null
if [ $? -ne 0 ]; then
if [ $((flags & FLAG_VERBOSE)) -ne 0 ]; then
echo "FAIL ${xmlfile} : ${cmd}"
- diff ${tmpfile} ${tmpfile2}
+ diff "${tmpfile}" "${tmpfile2}"
fi
((failctr++))
if [ $((flags & FLAG_WAIT)) -ne 0 ]; then
echo "tmp files: $tmpfile, $tmpfile2"
- echo "Press enter"
- read
+ echo "Press enter"
+ read
fi
[ $((flags & FLAG_LIBVIRT_TEST)) -ne 0 ] && \
test_result $((passctr+failctr)) "" 1
@@ -167,13 +167,13 @@ function checkExpectedOutput() {
break
fi
- echo "${line}" >> ${tmpfile2}
+ echo "${line}" >> "${tmpfile2}"
done
done
exec 4>&-
- rm -rf "${tmpfile}" "${tmpfile2}" 2>/dev/null
+ rm -rf "${tmpdir}"
}
@@ -189,7 +189,7 @@ function doTest() {
return 1
fi
- netname=`cat "${xmlfile}" | sed -n
's/.*<name>\([[:print:]]*\)<.*/\1/p'`
+ netname=$(sed -n 's/.*<name>\([^<]*\)<.*/\1/p'
"${xmlfile}")
${VIRSH} net-create "${xmlfile}" > /dev/null
@@ -197,7 +197,7 @@ function doTest() {
${VIRSH} net-destroy "${netname}" > /dev/null
- if [ -r ${postdatafile} ]; then
+ if [ -r "${postdatafile}" ]; then
checkExpectedOutput "${xmlfile}" "${postdatafile}"
"${flags}" ""
fi
@@ -212,22 +212,25 @@ function runTests() {
local datafiles f c
local tap_total=0 ctr=0
- pushd ${PWD} > /dev/null
- cd ${hostoutdir}
- datafiles=`ls *.dat`
+ pushd "${PWD}" > /dev/null
+ cd "${hostoutdir}"
+ datafiles=$(ls *.dat)
popd > /dev/null
if [ $((flags & FLAG_TAP_TEST)) -ne 0 ]; then
# Need to count the number of total tests
for fil in ${datafiles}; do
- c=$(grep -c "^#" ${hostoutdir}/${fil})
+ c=$(grep -c "^#" "${hostoutdir}/${fil}")
((tap_total+=c))
((ctr++))
done
echo "1..${tap_total}"
fi
- for fil in `echo "${datafiles}" | grep -v "\.post\.dat$"`; do
+ for fil in $datafiles; do
+ case $fil in
+ *.post.dat) continue;;
+ esac
f=${fil%%.dat}
doTest "${xmldir}/${f}.xml" "${hostoutdir}/${fil}" \
"${hostoutdir}/${f}.post.dat" "${flags}"
@@ -268,7 +271,7 @@ function main() {
shift 1
done
- if [ `uname` != "Linux" ]; then
+ if [ $(uname) != "Linux" ]; then
if [ $((flags & FLAG_TAP_TEST)) -ne 0 ]; then
echo "1..0 # Skipped: Only valid on Linux hosts"
else
@@ -283,7 +286,7 @@ function main() {
exit 0
fi
- for name in `virsh list | awk '{print $2}'`
+ for name in $(virsh list | awk '{print $2}')
do
case ${name} in
tck*)
@@ -302,7 +305,7 @@ function main() {
esac
done
- for name in `virsh net-list | sed -n '3,$p'`
+ for name in $(virsh net-list | sed -n '3,$p')
do
case ${name} in
tck*)
@@ -324,8 +327,8 @@ function main() {
fi
if [ $((flags & FLAG_LIBVIRT_TEST)) -ne 0 ]; then
- pushd ${PWD} > /dev/null
- . test-lib.sh
+ pushd "${PWD}" > /dev/null
+ . ./test-lib.sh
if [ $? -ne 0 ]; then
exit 1
fi
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:05 p.m.
On 11/01/2010 06:41 PM, Eric Blake wrote:
On 11/01/2010 04:56 AM, Stefan Berger wrote:
> On 10/29/2010 11:33 AM, Eric Blake wrote:
>> On 10/26/2010 05:27 PM, Stefan Berger wrote:
>>> On 10/26/2010 07:08 PM, Eric Blake wrote:
>>>> Sorry for sounding so depressing; I'm very pleased to see your
efforts
>>>> in providing tests for the code you've written. Even though this
test
>>>> is intended to be skipped on non-Linux, you still have to worry about
>>>> merely parsing through the test on other platforms like Solaris, where
>>>> /bin/sh won't understand the bash-isms and where /bin/bash is not
>>>> guaranteed to exist. But if we decide that requiring the presence of
>>>> /bin/bash is acceptable for the TCK, then a lot of my review becomes
>>>> irrelevant; but my comments about your mkstemp replacement being
>>>> insecure are still applicable even in that case.
>>>>
>>> Ok. Well, I hope for bash then...
>> IRC verdict - Dan and I are both okay with assuming /bin/bash exists.
>> So, that just leaves cleaning up the temporary file management. Would
>> you like me to tackle that as an incremental diff on top of your
>> original patch?
>>
> Yes, you can put it in on top of it. So is this an ACK now so I can push?
Actually, since you have already got the test setup ready, would you
mind just squashing this in? If it works for you, then you have my ACK
to push the squashed version, such that libvirt-tck.git only has a
single commit with all the problems already fixed.
Combined and pushed.
Stefan
5134
days inactive
5144
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Stefan Berger