[Qemu-devel] [PATCH 0/4] Add common QEMU control functionality to qemu-iotests

[Qemu-devel] [PATCH 0/4] Add common QEMU control functionality to qemu-iotests

[Jeff Cody]

This adds some common functionality to control QEMU for qemu-iotests.

Additionally, test 085 is updated to use this new functionality.

Some minor fixups along the way, to clear up spaced pathname issues, 
for common.rc, test 019, and test 086.


Jeff Cody (4):
  block: qemu-iotests - add common.qemu, for bash-controlled qemu tests
  block: qemu-iotests - update 085 to use common.qemu
  block: qemu-iotests - fix image cleanup when using spaced pathnames
  block: qemu-iotests: make test 019 and 086 work with spaced pathnames

 tests/qemu-iotests/019         |   2 +-
 tests/qemu-iotests/085         |  73 +++---------------
 tests/qemu-iotests/086         |   8 +-
 tests/qemu-iotests/common.qemu | 164 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.rc   |   4 +-
 5 files changed, 183 insertions(+), 68 deletions(-)
 create mode 100644 tests/qemu-iotests/common.qemu

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Jeff Cody]

This creates some common functions for bash language qemu-iotests
to control, and communicate with, a running QEMU process.

4 functions are introduced:

    1. _launch_qemu()
        This launches the QEMU process(es), and sets up the file
        descriptors and fifos for communication.  You can choose to
        launch each QEMU process listening for either QMP or HMP
        monitor.  You can call this function multiple times, and
        save the handle returned from each.

Commands 2 and 3 use the handle received from _launch_qemu(), to talk
to the appropriate process.

    2. _send_qemu_cmd()
        Sends a command string, specified by $2, to QEMU.  If $2 is
        non-NULL, will wait for it as the required resulting.  Failure
        to receive $3 will cause the test to fail.

    3. _timed_wait_for()
        Waits for a response, for up to a default of 10 seconds.  If
        $2 is not seen in that time (anywhere in the response), then
        the test fails.  Primarily used by _send_qemu_cmd, but could
        be useful standalone, as well.

    4. _cleanup_qemu()
        Kills the running QEMU processes, and removes the fifos.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/common.qemu | 164 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)
 create mode 100644 tests/qemu-iotests/common.qemu

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
new file mode 100644
index 0000000..8068395
--- /dev/null
+++ b/tests/qemu-iotests/common.qemu
@@ -0,0 +1,164 @@
+#!/bin/bash
+#
+# This allows for launching of multiple QEMU instances, with independent
+# communication possible to each instance.
+#
+# Each instance can choose, at launch, to use either the QMP or the
+# HMP (monitor) interface.
+#
+# All instances are cleaned up via _cleanup_qemu, including killing the
+# running qemu instance.
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+QEMU_COMM_TIMEOUT=10
+
+QEMU_FIFO_IN="${TEST_DIR}/qmp-in-$$"
+QEMU_FIFO_OUT="${TEST_DIR}/qmp-out-$$"
+
+QEMU_PID=
+_QEMU_HANDLE=0
+QEMU_HANDLE=0
+
+# If bash version is >= 4.1, these will be overwritten and dynamic
+# file descriptor values assigned.
+_out_fd=3
+_in_fd=4
+
+# Wait for expected QMP response from QEMU.  Will time out
+# after 10 seconds, which counts as failure.
+#
+# Override QEMU_COMM_TIMEOUT for a timeout different than the
+# default 10 seconds
+#
+# $1: The handle to use
+# $2+ All remaining arguments comprise the string to search for
+#    in the response.
+#
+# If $silent is set to anything but an empty string, then
+# response is not echoed out.
+function _timed_wait_for()
+{
+    local h=${1}
+    shift
+    while read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+    do
+        if [ -z "${silent}" ]; then
+            echo "${resp}" | _filter_testdir | _filter_qemu
+        fi
+        grep -q "${*}" < <(echo ${resp})
+        if [ $? -eq 0 ]; then
+            return
+        fi
+    done
+    echo "Timeout waiting for ${*} on handle ${h}"
+    exit 1  # Timeout means the test failed
+}
+
+
+# Sends QMP or HMP command to QEMU, and waits for the expected response
+#
+# $1:       QEMU handle to use
+# $2:       String of the QMP command to send
+# ${@: -1}  (Last string passed)
+#             String that the QEMU response should contain. If $2 is a null
+#             string, do not wait for a response
+function _send_qemu_cmd()
+{
+    local h=${1}
+    shift
+    # This array element extraction is done to accomodate pathnames with spaces
+    echo "${@: 1:${#@}-1}" >&${QEMU_IN[${h}]}
+    shift
+
+    if [ -n "${1}" ]
+    then
+        _timed_wait_for ${h} "${@: -1}"
+    fi
+}
+
+
+# Launch a QEMU process.
+#
+# Input parameters:
+# $qemu_comm_method: set this variable to 'monitor' (case insensitive)
+#                    to use the QEMU HMP monitor for communication.
+#                    Otherwise, the default of QMP is used.
+# Returns:
+# $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
+#
+function _launch_qemu()
+{
+    local comm=
+    local fifo_out=
+    local fifo_in=
+
+    if (shopt -s nocasematch; [[ "${qemu_comm_method}" == "monitor" ]])
+    then
+        comm="-monitor stdio -qmp none"
+    else
+        local qemu_comm_method="qmp"
+        comm="-monitor none -qmp stdio"
+    fi
+
+    fifo_out=${QEMU_FIFO_OUT}_${_QEMU_HANDLE}
+    fifo_in=${QEMU_FIFO_IN}_${_QEMU_HANDLE}
+    mkfifo "${fifo_out}"
+    mkfifo "${fifo_in}"
+
+    "${QEMU}" -nographic -serial none ${comm} "${@}" 2>&1 \
+                                                     >"${fifo_out}" \
+                                                     <"${fifo_in}" &
+    QEMU_PID[${_QEMU_HANDLE}]=$!
+
+    if [ "${BASH_VERSINFO[0]}" -ge "4" ] && [ "${BASH_VERSINFO[1]}" -ge "1" ]
+    then
+        # bash >= 4.1 required for automatic fd
+        exec {_out_fd}<"${fifo_out}"
+        exec {_in_fd}>"${fifo_in}"
+    else
+        let _out_fd++
+        let _in_fd++
+        eval "exec ${_out_fd}<'${fifo_out}'"
+        eval "exec ${_in_fd}>'${fifo_in}'"
+    fi
+
+    QEMU_OUT[${_QEMU_HANDLE}]=${_out_fd}
+    QEMU_IN[${_QEMU_HANDLE}]=${_in_fd}
+
+    if [ "${qemu_comm_method}" == "qmp" ]
+    then
+        # Don't print response, since it has version information in it
+        silent=yes _timed_wait_for ${_QEMU_HANDLE} "capabilities"
+    fi
+    QEMU_HANDLE=${_QEMU_HANDLE}
+    let _QEMU_HANDLE++
+}
+
+
+# Silenty kills the QEMU process
+function _cleanup_qemu()
+{
+    # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
+    for i in "${!QEMU_OUT[@]}"
+    do
+        kill -KILL ${QEMU_PID[$i]}
+        wait ${QEMU_PID[$i]} 2>/dev/null # silent kill
+        rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
+    done
+}
+
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] block: qemu-iotests - update 085 to use common.qemu

[Jeff Cody]

The new functionality of common.qemu implements the QEMU control
and communication functionality that was originally in test 085.

This removes that now-duplicate functionality, and uses the
common.qemu functions.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/085 | 73 +++++++++-----------------------------------------
 1 file changed, 12 insertions(+), 61 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 33c8dc4..56cd6f8 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -30,10 +30,6 @@ echo "QA output created by $seq"
 
 here=`pwd`
 status=1	# failure is the default!
-qemu_pid=
-
-QMP_IN="${TEST_DIR}/qmp-in-$$"
-QMP_OUT="${TEST_DIR}/qmp-out-$$"
 
 snapshot_virt0="snapshot-v0.qcow2"
 snapshot_virt1="snapshot-v1.qcow2"
@@ -42,10 +38,7 @@ MAX_SNAPSHOTS=10
 
 _cleanup()
 {
-    kill -KILL ${qemu_pid}
-    wait ${qemu_pid} 2>/dev/null  # silent kill
-
-    rm -f "${QMP_IN}" "${QMP_OUT}"
+    _cleanup_qemu
     for i in $(seq 1 ${MAX_SNAPSHOTS})
     do
         rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
@@ -59,43 +52,12 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
+. ./common.qemu
 
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 
-# Wait for expected QMP response from QEMU.  Will time out
-# after 10 seconds, which counts as failure.
-#
-# $1 is the string to expect
-#
-# If $silent is set to anything but an empty string, then
-# response is not echoed out.
-function timed_wait_for()
-{
-    while read -t 10 resp <&5
-    do
-        if [ "${silent}" == "" ]; then
-            echo "${resp}" | _filter_testdir | _filter_qemu
-        fi
-        grep -q "${1}" < <(echo ${resp})
-        if [ $? -eq 0 ]; then
-            return
-        fi
-    done
-    echo "Timeout waiting for ${1}"
-    exit 1  # Timeout means the test failed
-}
-
-# Sends QMP command to QEMU, and waits for the expected response
-#
-# ${1}:  String of the QMP command to send
-# ${2}:  String that the QEMU response should contain
-function send_qmp_cmd()
-{
-    echo "${1}" >&6
-    timed_wait_for "${2}"
-}
 
 # ${1}: unique identifier for the snapshot filename
 function create_single_snapshot()
@@ -104,7 +66,7 @@ function create_single_snapshot()
                       'arguments': { 'device': 'virtio0',
                                      'snapshot-file':'"${TEST_DIR}/${1}-${snapshot_virt0}"',
                                      'format': 'qcow2' } }"
-    send_qmp_cmd "${cmd}" "return"
+    _send_qemu_cmd $h "${cmd}" "return"
 }
 
 # ${1}: unique identifier for the snapshot filename
@@ -120,14 +82,11 @@ function create_group_snapshot()
                        'snapshot-file': '"${TEST_DIR}/${1}-${snapshot_virt1}"' } } ]
              } }"
 
-    send_qmp_cmd "${cmd}" "return"
+    _send_qemu_cmd $h "${cmd}" "return"
 }
 
 size=128M
 
-mkfifo "${QMP_IN}"
-mkfifo "${QMP_OUT}"
-
 _make_test_img $size
 mv "${TEST_IMG}" "${TEST_IMG}.orig"
 _make_test_img $size
@@ -136,23 +95,15 @@ echo
 echo === Running QEMU ===
 echo
 
-"${QEMU}" -nographic -monitor none -serial none -qmp stdio\
-          -drive file="${TEST_IMG}.orig",if=virtio\
-          -drive file="${TEST_IMG}",if=virtio 2>&1 >"${QMP_OUT}" <"${QMP_IN}"&
-qemu_pid=$!
-
-# redirect fifos to file descriptors, to keep from blocking
-exec 5<"${QMP_OUT}"
-exec 6>"${QMP_IN}"
-
-# Don't print response, since it has version information in it
-silent=yes timed_wait_for "capabilities"
+qemu_comm_method="qmp"
+_launch_qemu -drive file="${TEST_IMG}.orig",if=virtio -drive file="${TEST_IMG}",if=virtio
+h=$QEMU_HANDLE
 
 echo
 echo === Sending capabilities ===
 echo
 
-send_qmp_cmd "{ 'execute': 'qmp_capabilities' }" "return"
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
 
 echo
 echo === Create a single snapshot on virtio0 ===
@@ -165,16 +116,16 @@ echo
 echo === Invalid command - missing device and nodename ===
 echo
 
-send_qmp_cmd "{ 'execute': 'blockdev-snapshot-sync',
-                      'arguments': { 'snapshot-file':'"${TEST_DIR}"/1-${snapshot_virt0}',
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
+                         'arguments': { 'snapshot-file':'"${TEST_DIR}/1-${snapshot_virt0}"',
                                      'format': 'qcow2' } }" "error"
 
 echo
 echo === Invalid command - missing snapshot-file ===
 echo
 
-send_qmp_cmd "{ 'execute': 'blockdev-snapshot-sync',
-                      'arguments': { 'device': 'virtio0',
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
+                         'arguments': { 'device': 'virtio0',
                                      'format': 'qcow2' } }" "error"
 echo
 echo
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] block: qemu-iotests - fix image cleanup when using spaced pathnames

[Jeff Cody]

The _rm_test_img() function in common.rc did not quote the image
file, which left droppings in the scratch directory (and performed
a potentially unsafe rm -f).

This adds the necessary quotes.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/common.rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 881079b..6b13a45 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -178,10 +178,10 @@ _rm_test_img()
     local img=$1
     if [ "$IMGFMT" = "vmdk" ]; then
         # Remove all the extents for vmdk
-        $QEMU_IMG info $img 2>/dev/null | grep 'filename:' | cut -f 2 -d: \
+        "$QEMU_IMG" info "$img" 2>/dev/null | grep 'filename:' | cut -f 2 -d: \
             | xargs -I {} rm -f "{}"
     fi
-    rm -f $img
+    rm -f "$img"
 }
 
 _cleanup_test_img()
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] block: qemu-iotests: make test 019 and 086 work with spaced pathnames

[Jeff Cody]

Both tests 019 and 086 need proper quotations to work with pathnames
that contain spaces.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/019 | 2 +-
 tests/qemu-iotests/086 | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019
index e67445c..f5ecbf5 100755
--- a/tests/qemu-iotests/019
+++ b/tests/qemu-iotests/019
@@ -96,7 +96,7 @@ mv "$TEST_IMG" "$TEST_IMG.orig"
 for backing_option in "-B " "-o backing_file="; do
 
     echo
-    echo Testing conversion with $backing_option$TEST_IMG.base | _filter_testdir | _filter_imgfmt
+    echo Testing conversion with $backing_option"$TEST_IMG.base" | _filter_testdir | _filter_imgfmt
     echo
     $QEMU_IMG convert -O $IMGFMT $backing_option"$TEST_IMG.base" "$TEST_IMG.orig" "$TEST_IMG"
 
diff --git a/tests/qemu-iotests/086 b/tests/qemu-iotests/086
index 48fe85b..d9a80cf 100755
--- a/tests/qemu-iotests/086
+++ b/tests/qemu-iotests/086
@@ -51,10 +51,10 @@ function run_qemu_img()
 size=128M
 
 _make_test_img $size
-$QEMU_IO -c 'write 0 1M' $TEST_IMG | _filter_qemu_io
-$QEMU_IO -c 'write 2M 1M' $TEST_IMG | _filter_qemu_io
-$QEMU_IO -c 'write 4M 1M' $TEST_IMG | _filter_qemu_io
-$QEMU_IO -c 'write 32M 1M' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'write 0 1M' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'write 2M 1M' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'write 4M 1M' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'write 32M 1M' "$TEST_IMG" | _filter_qemu_io
 
 $QEMU_IMG convert -p -O $IMGFMT -f $IMGFMT "$TEST_IMG" "$TEST_IMG".base  2>&1 |\
     _filter_testdir | sed -e 's/\r/\n/g'
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Benoît Canet]

The Monday 17 Mar 2014 à 21:24:37 (-0400), Jeff Cody wrote :
> This creates some common functions for bash language qemu-iotests
> to control, and communicate with, a running QEMU process.
> 
> 4 functions are introduced:
> 
>     1. _launch_qemu()
>         This launches the QEMU process(es), and sets up the file
>         descriptors and fifos for communication.  You can choose to
>         launch each QEMU process listening for either QMP or HMP
>         monitor.  You can call this function multiple times, and
>         save the handle returned from each.
> 
> Commands 2 and 3 use the handle received from _launch_qemu(), to talk
> to the appropriate process.
> 
>     2. _send_qemu_cmd()
>         Sends a command string, specified by $2, to QEMU.  If $2 is
>         non-NULL, will wait for it as the required resulting.  Failure

"will wait for it as the required resulting" I don't understand this part of the
sentence, probably because I am not a native speaker.

>         to receive $3 will cause the test to fail.
> 
>     3. _timed_wait_for()
>         Waits for a response, for up to a default of 10 seconds.  If
>         $2 is not seen in that time (anywhere in the response), then
>         the test fails.  Primarily used by _send_qemu_cmd, but could
>         be useful standalone, as well.
> 
>     4. _cleanup_qemu()
>         Kills the running QEMU processes, and removes the fifos.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  tests/qemu-iotests/common.qemu | 164 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 164 insertions(+)
>  create mode 100644 tests/qemu-iotests/common.qemu
> 
> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> new file mode 100644
> index 0000000..8068395
> --- /dev/null
> +++ b/tests/qemu-iotests/common.qemu
> @@ -0,0 +1,164 @@
> +#!/bin/bash
> +#
> +# This allows for launching of multiple QEMU instances, with independent
> +# communication possible to each instance.
> +#
> +# Each instance can choose, at launch, to use either the QMP or the
> +# HMP (monitor) interface.
> +#
> +# All instances are cleaned up via _cleanup_qemu, including killing the
> +# running qemu instance.
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +QEMU_COMM_TIMEOUT=10
> +
> +QEMU_FIFO_IN="${TEST_DIR}/qmp-in-$$"
> +QEMU_FIFO_OUT="${TEST_DIR}/qmp-out-$$"
> +
> +QEMU_PID=
> +_QEMU_HANDLE=0
> +QEMU_HANDLE=0
> +
> +# If bash version is >= 4.1, these will be overwritten and dynamic
> +# file descriptor values assigned.
> +_out_fd=3
> +_in_fd=4
> +
> +# Wait for expected QMP response from QEMU.  Will time out
> +# after 10 seconds, which counts as failure.
> +#
> +# Override QEMU_COMM_TIMEOUT for a timeout different than the
> +# default 10 seconds
> +#
> +# $1: The handle to use
> +# $2+ All remaining arguments comprise the string to search for
> +#    in the response.
> +#
> +# If $silent is set to anything but an empty string, then
> +# response is not echoed out.
> +function _timed_wait_for()
> +{
> +    local h=${1}
> +    shift
> +    while read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
> +    do
> +        if [ -z "${silent}" ]; then
> +            echo "${resp}" | _filter_testdir | _filter_qemu
> +        fi
> +        grep -q "${*}" < <(echo ${resp})
> +        if [ $? -eq 0 ]; then
> +            return
> +        fi
> +    done
> +    echo "Timeout waiting for ${*} on handle ${h}"
> +    exit 1  # Timeout means the test failed
> +}
> +
> +
> +# Sends QMP or HMP command to QEMU, and waits for the expected response
> +#
> +# $1:       QEMU handle to use
> +# $2:       String of the QMP command to send
> +# ${@: -1}  (Last string passed)
> +#             String that the QEMU response should contain. If $2 is a null
> +#             string, do not wait for a response
> +function _send_qemu_cmd()
> +{
> +    local h=${1}
> +    shift
> +    # This array element extraction is done to accomodate pathnames with spaces
> +    echo "${@: 1:${#@}-1}" >&${QEMU_IN[${h}]}
> +    shift
> +
> +    if [ -n "${1}" ]
> +    then
> +        _timed_wait_for ${h} "${@: -1}"

You have done shift before this. Aren't ${*} the remaining strings to wait for ?

> +    fi
> +}
> +
> +
> +# Launch a QEMU process.
> +#
> +# Input parameters:
> +# $qemu_comm_method: set this variable to 'monitor' (case insensitive)
> +#                    to use the QEMU HMP monitor for communication.
> +#                    Otherwise, the default of QMP is used.
> +# Returns:
> +# $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
> +#
> +function _launch_qemu()
> +{
> +    local comm=
> +    local fifo_out=
> +    local fifo_in=
> +
> +    if (shopt -s nocasematch; [[ "${qemu_comm_method}" == "monitor" ]])
> +    then
> +        comm="-monitor stdio -qmp none"
> +    else
> +        local qemu_comm_method="qmp"
> +        comm="-monitor none -qmp stdio"
> +    fi
> +
> +    fifo_out=${QEMU_FIFO_OUT}_${_QEMU_HANDLE}
> +    fifo_in=${QEMU_FIFO_IN}_${_QEMU_HANDLE}
> +    mkfifo "${fifo_out}"
> +    mkfifo "${fifo_in}"
> +
> +    "${QEMU}" -nographic -serial none ${comm} "${@}" 2>&1 \
> +                                                     >"${fifo_out}" \
> +                                                     <"${fifo_in}" &
> +    QEMU_PID[${_QEMU_HANDLE}]=$!
> +
> +    if [ "${BASH_VERSINFO[0]}" -ge "4" ] && [ "${BASH_VERSINFO[1]}" -ge "1" ]
> +    then
> +        # bash >= 4.1 required for automatic fd
> +        exec {_out_fd}<"${fifo_out}"
> +        exec {_in_fd}>"${fifo_in}"

Isn't it ${_out_fd} and ${_in_fd} ?

> +    else
> +        let _out_fd++
> +        let _in_fd++
> +        eval "exec ${_out_fd}<'${fifo_out}'"
> +        eval "exec ${_in_fd}>'${fifo_in}'"
> +    fi
> +
> +    QEMU_OUT[${_QEMU_HANDLE}]=${_out_fd}
> +    QEMU_IN[${_QEMU_HANDLE}]=${_in_fd}
> +
> +    if [ "${qemu_comm_method}" == "qmp" ]
> +    then
> +        # Don't print response, since it has version information in it
> +        silent=yes _timed_wait_for ${_QEMU_HANDLE} "capabilities"
> +    fi
> +    QEMU_HANDLE=${_QEMU_HANDLE}
> +    let _QEMU_HANDLE++
> +}
> +
> +
> +# Silenty kills the QEMU process
> +function _cleanup_qemu()
> +{
> +    # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
> +    for i in "${!QEMU_OUT[@]}"
> +    do
> +        kill -KILL ${QEMU_PID[$i]}
> +        wait ${QEMU_PID[$i]} 2>/dev/null # silent kill
> +        rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
> +    done
> +}
> +
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 2/4] block: qemu-iotests - update 085 to use common.qemu

[Benoît Canet]

The Monday 17 Mar 2014 à 21:24:38 (-0400), Jeff Cody wrote :
> The new functionality of common.qemu implements the QEMU control
> and communication functionality that was originally in test 085.
> 
> This removes that now-duplicate functionality, and uses the
> common.qemu functions.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  tests/qemu-iotests/085 | 73 +++++++++-----------------------------------------
>  1 file changed, 12 insertions(+), 61 deletions(-)
> 
> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> index 33c8dc4..56cd6f8 100755
> --- a/tests/qemu-iotests/085
> +++ b/tests/qemu-iotests/085
> @@ -30,10 +30,6 @@ echo "QA output created by $seq"
>  
>  here=`pwd`
>  status=1	# failure is the default!
> -qemu_pid=
> -
> -QMP_IN="${TEST_DIR}/qmp-in-$$"
> -QMP_OUT="${TEST_DIR}/qmp-out-$$"
>  
>  snapshot_virt0="snapshot-v0.qcow2"
>  snapshot_virt1="snapshot-v1.qcow2"
> @@ -42,10 +38,7 @@ MAX_SNAPSHOTS=10
>  
>  _cleanup()
>  {
> -    kill -KILL ${qemu_pid}
> -    wait ${qemu_pid} 2>/dev/null  # silent kill
> -
> -    rm -f "${QMP_IN}" "${QMP_OUT}"
> +    _cleanup_qemu
>      for i in $(seq 1 ${MAX_SNAPSHOTS})
>      do
>          rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
> @@ -59,43 +52,12 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  # get standard environment, filters and checks
>  . ./common.rc
>  . ./common.filter
> +. ./common.qemu
>  
>  _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
>  
> -# Wait for expected QMP response from QEMU.  Will time out
> -# after 10 seconds, which counts as failure.
> -#
> -# $1 is the string to expect
> -#
> -# If $silent is set to anything but an empty string, then
> -# response is not echoed out.
> -function timed_wait_for()
> -{
> -    while read -t 10 resp <&5
> -    do
> -        if [ "${silent}" == "" ]; then
> -            echo "${resp}" | _filter_testdir | _filter_qemu
> -        fi
> -        grep -q "${1}" < <(echo ${resp})
> -        if [ $? -eq 0 ]; then
> -            return
> -        fi
> -    done
> -    echo "Timeout waiting for ${1}"
> -    exit 1  # Timeout means the test failed
> -}
> -
> -# Sends QMP command to QEMU, and waits for the expected response
> -#
> -# ${1}:  String of the QMP command to send
> -# ${2}:  String that the QEMU response should contain
> -function send_qmp_cmd()
> -{
> -    echo "${1}" >&6
> -    timed_wait_for "${2}"
> -}
>  
>  # ${1}: unique identifier for the snapshot filename
>  function create_single_snapshot()
> @@ -104,7 +66,7 @@ function create_single_snapshot()
>                        'arguments': { 'device': 'virtio0',
>                                       'snapshot-file':'"${TEST_DIR}/${1}-${snapshot_virt0}"',
>                                       'format': 'qcow2' } }"
> -    send_qmp_cmd "${cmd}" "return"
> +    _send_qemu_cmd $h "${cmd}" "return"
>  }
>  
>  # ${1}: unique identifier for the snapshot filename
> @@ -120,14 +82,11 @@ function create_group_snapshot()
>                         'snapshot-file': '"${TEST_DIR}/${1}-${snapshot_virt1}"' } } ]
>               } }"
>  
> -    send_qmp_cmd "${cmd}" "return"
> +    _send_qemu_cmd $h "${cmd}" "return"
>  }
>  
>  size=128M
>  
> -mkfifo "${QMP_IN}"
> -mkfifo "${QMP_OUT}"
> -
>  _make_test_img $size
>  mv "${TEST_IMG}" "${TEST_IMG}.orig"
>  _make_test_img $size
> @@ -136,23 +95,15 @@ echo
>  echo === Running QEMU ===
>  echo
>  
> -"${QEMU}" -nographic -monitor none -serial none -qmp stdio\
> -          -drive file="${TEST_IMG}.orig",if=virtio\
> -          -drive file="${TEST_IMG}",if=virtio 2>&1 >"${QMP_OUT}" <"${QMP_IN}"&
> -qemu_pid=$!
> -
> -# redirect fifos to file descriptors, to keep from blocking
> -exec 5<"${QMP_OUT}"
> -exec 6>"${QMP_IN}"
> -
> -# Don't print response, since it has version information in it
> -silent=yes timed_wait_for "capabilities"
> +qemu_comm_method="qmp"
> +_launch_qemu -drive file="${TEST_IMG}.orig",if=virtio -drive file="${TEST_IMG}",if=virtio
> +h=$QEMU_HANDLE
>  
>  echo
>  echo === Sending capabilities ===
>  echo
>  
> -send_qmp_cmd "{ 'execute': 'qmp_capabilities' }" "return"
> +_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
>  
>  echo
>  echo === Create a single snapshot on virtio0 ===
> @@ -165,16 +116,16 @@ echo
>  echo === Invalid command - missing device and nodename ===
>  echo
>  
> -send_qmp_cmd "{ 'execute': 'blockdev-snapshot-sync',
> -                      'arguments': { 'snapshot-file':'"${TEST_DIR}"/1-${snapshot_virt0}',
> +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
> +                         'arguments': { 'snapshot-file':'"${TEST_DIR}/1-${snapshot_virt0}"',
>                                       'format': 'qcow2' } }" "error"
>  
>  echo
>  echo === Invalid command - missing snapshot-file ===
>  echo
>  
> -send_qmp_cmd "{ 'execute': 'blockdev-snapshot-sync',
> -                      'arguments': { 'device': 'virtio0',
> +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
> +                         'arguments': { 'device': 'virtio0',
>                                       'format': 'qcow2' } }" "error"
>  echo
>  echo
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 3/4] block: qemu-iotests - fix image cleanup when using spaced pathnames

[Benoît Canet]

The Monday 17 Mar 2014 à 21:24:39 (-0400), Jeff Cody wrote :
> The _rm_test_img() function in common.rc did not quote the image
> file, which left droppings in the scratch directory (and performed
> a potentially unsafe rm -f).
> 
> This adds the necessary quotes.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  tests/qemu-iotests/common.rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 881079b..6b13a45 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -178,10 +178,10 @@ _rm_test_img()
>      local img=$1
>      if [ "$IMGFMT" = "vmdk" ]; then
>          # Remove all the extents for vmdk
> -        $QEMU_IMG info $img 2>/dev/null | grep 'filename:' | cut -f 2 -d: \
> +        "$QEMU_IMG" info "$img" 2>/dev/null | grep 'filename:' | cut -f 2 -d: \
>              | xargs -I {} rm -f "{}"
>      fi
> -    rm -f $img
> +    rm -f "$img"
>  }
>  
>  _cleanup_test_img()
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 4/4] block: qemu-iotests: make test 019 and 086 work with spaced pathnames

[Benoît Canet]

The Monday 17 Mar 2014 à 21:24:40 (-0400), Jeff Cody wrote :
> Both tests 019 and 086 need proper quotations to work with pathnames
> that contain spaces.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  tests/qemu-iotests/019 | 2 +-
>  tests/qemu-iotests/086 | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019
> index e67445c..f5ecbf5 100755
> --- a/tests/qemu-iotests/019
> +++ b/tests/qemu-iotests/019
> @@ -96,7 +96,7 @@ mv "$TEST_IMG" "$TEST_IMG.orig"
>  for backing_option in "-B " "-o backing_file="; do
>  
>      echo
> -    echo Testing conversion with $backing_option$TEST_IMG.base | _filter_testdir | _filter_imgfmt
> +    echo Testing conversion with $backing_option"$TEST_IMG.base" | _filter_testdir | _filter_imgfmt
>      echo
>      $QEMU_IMG convert -O $IMGFMT $backing_option"$TEST_IMG.base" "$TEST_IMG.orig" "$TEST_IMG"
>  
> diff --git a/tests/qemu-iotests/086 b/tests/qemu-iotests/086
> index 48fe85b..d9a80cf 100755
> --- a/tests/qemu-iotests/086
> +++ b/tests/qemu-iotests/086
> @@ -51,10 +51,10 @@ function run_qemu_img()
>  size=128M
>  
>  _make_test_img $size
> -$QEMU_IO -c 'write 0 1M' $TEST_IMG | _filter_qemu_io
> -$QEMU_IO -c 'write 2M 1M' $TEST_IMG | _filter_qemu_io
> -$QEMU_IO -c 'write 4M 1M' $TEST_IMG | _filter_qemu_io
> -$QEMU_IO -c 'write 32M 1M' $TEST_IMG | _filter_qemu_io
> +$QEMU_IO -c 'write 0 1M' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c 'write 2M 1M' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c 'write 4M 1M' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c 'write 32M 1M' "$TEST_IMG" | _filter_qemu_io
>  
>  $QEMU_IMG convert -p -O $IMGFMT -f $IMGFMT "$TEST_IMG" "$TEST_IMG".base  2>&1 |\
>      _filter_testdir | sed -e 's/\r/\n/g'
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Jeff Cody]

On Wed, Mar 19, 2014 at 02:39:25PM +0100, Benoît Canet wrote:
> The Monday 17 Mar 2014 à 21:24:37 (-0400), Jeff Cody wrote :
> > This creates some common functions for bash language qemu-iotests
> > to control, and communicate with, a running QEMU process.
> > 
> > 4 functions are introduced:
> > 
> >     1. _launch_qemu()
> >         This launches the QEMU process(es), and sets up the file
> >         descriptors and fifos for communication.  You can choose to
> >         launch each QEMU process listening for either QMP or HMP
> >         monitor.  You can call this function multiple times, and
> >         save the handle returned from each.
> > 
> > Commands 2 and 3 use the handle received from _launch_qemu(), to talk
> > to the appropriate process.
> > 
> >     2. _send_qemu_cmd()
> >         Sends a command string, specified by $2, to QEMU.  If $2 is
> >         non-NULL, will wait for it as the required resulting.  Failure
> 
> "will wait for it as the required resulting" I don't understand this part of the
> sentence, probably because I am not a native speaker.
>

I wrote that sentence, and I don't understand it either :)  I think I
merged two sentences inadvertently.

Here is what I meant:

   If $2 is non-NULL, _send_qemu_cmd() will wait to receive $2 as a
   required result string from QEMU.
   

> >         to receive $3 will cause the test to fail.
> > 
> >     3. _timed_wait_for()
> >         Waits for a response, for up to a default of 10 seconds.  If
> >         $2 is not seen in that time (anywhere in the response), then
> >         the test fails.  Primarily used by _send_qemu_cmd, but could
> >         be useful standalone, as well.
> > 
> >     4. _cleanup_qemu()
> >         Kills the running QEMU processes, and removes the fifos.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  tests/qemu-iotests/common.qemu | 164 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 164 insertions(+)
> >  create mode 100644 tests/qemu-iotests/common.qemu
> > 
> > diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> > new file mode 100644
> > index 0000000..8068395
> > --- /dev/null
> > +++ b/tests/qemu-iotests/common.qemu
> > @@ -0,0 +1,164 @@
> > +#!/bin/bash
> > +#
> > +# This allows for launching of multiple QEMU instances, with independent
> > +# communication possible to each instance.
> > +#
> > +# Each instance can choose, at launch, to use either the QMP or the
> > +# HMP (monitor) interface.
> > +#
> > +# All instances are cleaned up via _cleanup_qemu, including killing the
> > +# running qemu instance.
> > +#
> > +# Copyright (C) 2014 Red Hat, Inc.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +QEMU_COMM_TIMEOUT=10
> > +
> > +QEMU_FIFO_IN="${TEST_DIR}/qmp-in-$$"
> > +QEMU_FIFO_OUT="${TEST_DIR}/qmp-out-$$"
> > +
> > +QEMU_PID=
> > +_QEMU_HANDLE=0
> > +QEMU_HANDLE=0
> > +
> > +# If bash version is >= 4.1, these will be overwritten and dynamic
> > +# file descriptor values assigned.
> > +_out_fd=3
> > +_in_fd=4
> > +
> > +# Wait for expected QMP response from QEMU.  Will time out
> > +# after 10 seconds, which counts as failure.
> > +#
> > +# Override QEMU_COMM_TIMEOUT for a timeout different than the
> > +# default 10 seconds
> > +#
> > +# $1: The handle to use
> > +# $2+ All remaining arguments comprise the string to search for
> > +#    in the response.
> > +#
> > +# If $silent is set to anything but an empty string, then
> > +# response is not echoed out.
> > +function _timed_wait_for()
> > +{
> > +    local h=${1}
> > +    shift
> > +    while read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
> > +    do
> > +        if [ -z "${silent}" ]; then
> > +            echo "${resp}" | _filter_testdir | _filter_qemu
> > +        fi
> > +        grep -q "${*}" < <(echo ${resp})
> > +        if [ $? -eq 0 ]; then
> > +            return
> > +        fi
> > +    done
> > +    echo "Timeout waiting for ${*} on handle ${h}"
> > +    exit 1  # Timeout means the test failed
> > +}
> > +
> > +
> > +# Sends QMP or HMP command to QEMU, and waits for the expected response
> > +#
> > +# $1:       QEMU handle to use
> > +# $2:       String of the QMP command to send
> > +# ${@: -1}  (Last string passed)
> > +#             String that the QEMU response should contain. If $2 is a null
                                                                    ^^^
This is a typo, it should say "$3" ----------------------------------|

> > +#             string, do not wait for a response
> > +function _send_qemu_cmd()
> > +{
> > +    local h=${1}
> > +    shift
> > +    # This array element extraction is done to accomodate pathnames with spaces
> > +    echo "${@: 1:${#@}-1}" >&${QEMU_IN[${h}]}
> > +    shift
> > +
> > +    if [ -n "${1}" ]
> > +    then
> > +        _timed_wait_for ${h} "${@: -1}"
> 
> You have done shift before this. Aren't ${*} the remaining strings to wait for ?
> 

I could probably get rid of the 2nd shift, although I would have to
adjust the conditional below.  

I do ${@: -1} because I want the very last whole string to be the item
to wait for - this is only needed to accommodate pathnames with spaces
inside the QMP string.

The value of ${@: -1} should be the same regardless of the shift.

Actually, there is a subtle bug here - the intent was to allow the 3rd
argument to be a NULL string, to not wait for any response.  If we
have spaced pathnames, after the shift ${1} could still be
inadvertently non-NULL.  The easiest fix will probably just be to add
a function _send_qemu_cmd_nowait(), or perhaps a variable to check
(e.g. qemu_nowait)


> > +    fi
> > +}
> > +
> > +
> > +# Launch a QEMU process.
> > +#
> > +# Input parameters:
> > +# $qemu_comm_method: set this variable to 'monitor' (case insensitive)
> > +#                    to use the QEMU HMP monitor for communication.
> > +#                    Otherwise, the default of QMP is used.
> > +# Returns:
> > +# $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
> > +#
> > +function _launch_qemu()
> > +{
> > +    local comm=
> > +    local fifo_out=
> > +    local fifo_in=
> > +
> > +    if (shopt -s nocasematch; [[ "${qemu_comm_method}" == "monitor" ]])
> > +    then
> > +        comm="-monitor stdio -qmp none"
> > +    else
> > +        local qemu_comm_method="qmp"
> > +        comm="-monitor none -qmp stdio"
> > +    fi
> > +
> > +    fifo_out=${QEMU_FIFO_OUT}_${_QEMU_HANDLE}
> > +    fifo_in=${QEMU_FIFO_IN}_${_QEMU_HANDLE}
> > +    mkfifo "${fifo_out}"
> > +    mkfifo "${fifo_in}"
> > +
> > +    "${QEMU}" -nographic -serial none ${comm} "${@}" 2>&1 \
> > +                                                     >"${fifo_out}" \
> > +                                                     <"${fifo_in}" &
> > +    QEMU_PID[${_QEMU_HANDLE}]=$!
> > +
> > +    if [ "${BASH_VERSINFO[0]}" -ge "4" ] && [ "${BASH_VERSINFO[1]}" -ge "1" ]
> > +    then
> > +        # bash >= 4.1 required for automatic fd
> > +        exec {_out_fd}<"${fifo_out}"
> > +        exec {_in_fd}>"${fifo_in}"
> 
> Isn't it ${_out_fd} and ${_in_fd} ?
> 

No, when doing the dynamic file descriptor assignment the '$' is left
off - think of it as assigning a value to a variable (that is
essentially what is happening).

More info: http://www.gnu.org/software/bash/manual/bashref.html#Redirections

> > +    else
> > +        let _out_fd++
> > +        let _in_fd++
> > +        eval "exec ${_out_fd}<'${fifo_out}'"
> > +        eval "exec ${_in_fd}>'${fifo_in}'"
> > +    fi
> > +
> > +    QEMU_OUT[${_QEMU_HANDLE}]=${_out_fd}
> > +    QEMU_IN[${_QEMU_HANDLE}]=${_in_fd}
> > +
> > +    if [ "${qemu_comm_method}" == "qmp" ]
> > +    then
> > +        # Don't print response, since it has version information in it
> > +        silent=yes _timed_wait_for ${_QEMU_HANDLE} "capabilities"
> > +    fi
> > +    QEMU_HANDLE=${_QEMU_HANDLE}
> > +    let _QEMU_HANDLE++
> > +}
> > +
> > +
> > +# Silenty kills the QEMU process
> > +function _cleanup_qemu()
> > +{
> > +    # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
> > +    for i in "${!QEMU_OUT[@]}"
> > +    do
> > +        kill -KILL ${QEMU_PID[$i]}
> > +        wait ${QEMU_PID[$i]} 2>/dev/null # silent kill
> > +        rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
> > +    done
> > +}
> > +
> > -- 
> > 1.8.3.1
> > 

Re: [Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On 03/19/2014 08:19 AM, Jeff Cody wrote:

>>> +    then
>>> +        _timed_wait_for ${h} "${@: -1}"
>>
>> You have done shift before this. Aren't ${*} the remaining strings to wait for ?
>>
> 
> I could probably get rid of the 2nd shift, although I would have to
> adjust the conditional below.  
> 
> I do ${@: -1} because I want the very last whole string to be the item
> to wait for - this is only needed to accommodate pathnames with spaces
> inside the QMP string.

${@: -1} is not portable:

$ bash -c 'set 1 2 3; echo ${@: -1}'
3
$ dash -c 'set 1 2 3; echo ${@: -1}'
dash: 1: Bad substitution

If you want the last argument, you'll have to do something hideous like:

eval \${$#}

Short of using eval, there is no portable way to get at the last
positional argument in dash.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On 03/19/2014 08:28 AM, Eric Blake wrote:
> $ dash -c 'set 1 2 3; echo ${@: -1}'
> dash: 1: Bad substitution
> 
> If you want the last argument, you'll have to do something hideous like:
> 
> eval \${$#}
> 
> Short of using eval, there is no portable way to get at the last
> positional argument in dash.

If you are sure you don't need the other positional arguments, you could
avoid eval with:

shift $(($# - 1)); echo $1

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Jeff Cody]

On Wed, Mar 19, 2014 at 08:28:07AM -0600, Eric Blake wrote:
> On 03/19/2014 08:19 AM, Jeff Cody wrote:
> 
> >>> +    then
> >>> +        _timed_wait_for ${h} "${@: -1}"
> >>
> >> You have done shift before this. Aren't ${*} the remaining strings to wait for ?
> >>
> > 
> > I could probably get rid of the 2nd shift, although I would have to
> > adjust the conditional below.  
> > 
> > I do ${@: -1} because I want the very last whole string to be the item
> > to wait for - this is only needed to accommodate pathnames with spaces
> > inside the QMP string.
> 
> ${@: -1} is not portable:
> 
> $ bash -c 'set 1 2 3; echo ${@: -1}'
> 3
> $ dash -c 'set 1 2 3; echo ${@: -1}'
> dash: 1: Bad substitution
> 
> If you want the last argument, you'll have to do something hideous like:
> 
> eval \${$#}
> 
> Short of using eval, there is no portable way to get at the last
> positional argument in dash.
> 

Yes, and there are likely other bash-isms in some of the shell
scripts in qemu-iotests.  Since #!/bin/bash is explicitly specified,
it seems reasonable that bash-isms would be allowed.  If it was
#!/bin/sh specified as the interpreter, then I would understand
remaining constrained to POSIX-only.

But I think in your next message you have a nice POSIX compatible
method of doing it with shifts, and it is probably best to default to
POSIX when practical.  I'll go ahead and change it to the 
'shift $(($# - 1))' method.

Re: [Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

On 03/19/2014 08:45 AM, Jeff Cody wrote:

>>
>> ${@: -1} is not portable:
>>

> 
> Yes, and there are likely other bash-isms in some of the shell
> scripts in qemu-iotests.  Since #!/bin/bash is explicitly specified,
> it seems reasonable that bash-isms would be allowed.

Ah, indeed, I missed the shebang line.  Where we KNOW we are using bash,
I have no problem using the additional features that bash gives us as a
guarantee.

> But I think in your next message you have a nice POSIX compatible
> method of doing it with shifts, and it is probably best to default to
> POSIX when practical.  I'll go ahead and change it to the 
> 'shift $(($# - 1))' method.

Where the portable way doesn't cost any additional forks, then yes, I
agree that being portable is nice for the sake of anyone
copying-and-pasting from our explicit bash script over to a more generic
/bin/sh script.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/4] block: qemu-iotests - add common.qemu, for bash-controlled qemu tests

[Fam Zheng]

On Mon, 03/17 21:24, Jeff Cody wrote:
> +# Launch a QEMU process.
> +#
> +# Input parameters:
> +# $qemu_comm_method: set this variable to 'monitor' (case insensitive)
> +#                    to use the QEMU HMP monitor for communication.
> +#                    Otherwise, the default of QMP is used.
> +# Returns:
> +# $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance.
> +#
> +function _launch_qemu()
> +{
> +    local comm=
> +    local fifo_out=
> +    local fifo_in=
> +
> +    if (shopt -s nocasematch; [[ "${qemu_comm_method}" == "monitor" ]])
> +    then
> +        comm="-monitor stdio -qmp none"
> +    else
> +        local qemu_comm_method="qmp"
> +        comm="-monitor none -qmp stdio"
> +    fi
> +
> +    fifo_out=${QEMU_FIFO_OUT}_${_QEMU_HANDLE}
> +    fifo_in=${QEMU_FIFO_IN}_${_QEMU_HANDLE}
> +    mkfifo "${fifo_out}"
> +    mkfifo "${fifo_in}"
> +
> +    "${QEMU}" -nographic -serial none ${comm} "${@}" 2>&1 \
> +                                                     >"${fifo_out}" \
> +                                                     <"${fifo_in}" &

Shall we use '-machine accel=qtest' as we do in iotests.py (to run no guest
code)?  Because below patch has a big difference of 067's stability and run
time in my case:

diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
index d025192..a379a3b 100755
--- a/tests/qemu-iotests/067
+++ b/tests/qemu-iotests/067
@@ -39,7 +39,7 @@ _supported_os Linux
 function do_run_qemu()
 {
     echo Testing: "$@"
-    $QEMU -nographic -qmp stdio -serial none "$@"
+    $QEMU -nographic -machine accel=qtest -qmp stdio -serial none "$@"
     echo
 }


Fam

> +    QEMU_PID[${_QEMU_HANDLE}]=$!
> +
> +    if [ "${BASH_VERSINFO[0]}" -ge "4" ] && [ "${BASH_VERSINFO[1]}" -ge "1" ]
> +    then
> +        # bash >= 4.1 required for automatic fd
> +        exec {_out_fd}<"${fifo_out}"
> +        exec {_in_fd}>"${fifo_in}"
> +    else
> +        let _out_fd++
> +        let _in_fd++
> +        eval "exec ${_out_fd}<'${fifo_out}'"
> +        eval "exec ${_in_fd}>'${fifo_in}'"
> +    fi
> +
> +    QEMU_OUT[${_QEMU_HANDLE}]=${_out_fd}
> +    QEMU_IN[${_QEMU_HANDLE}]=${_in_fd}
> +
> +    if [ "${qemu_comm_method}" == "qmp" ]
> +    then
> +        # Don't print response, since it has version information in it
> +        silent=yes _timed_wait_for ${_QEMU_HANDLE} "capabilities"
> +    fi
> +    QEMU_HANDLE=${_QEMU_HANDLE}
> +    let _QEMU_HANDLE++
> +}