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

Re: [libvirt] [PATCH] [TCK] network: create networks and check for exected results



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 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]