From: Jeff Cody <jcody@redhat.com>
To: qemu-devel@nongnu.org
Cc: qemu-block@nongnu.org, jsnow@redhat.com, stefanha@redhat.com,
kwolf@redhat.com, eblake@redhat.com
Subject: Re: [Qemu-devel] [PATCH v5 05/10] qemu-iotests: change qemu pid and fd tracking / cleanup
Date: Wed, 18 Oct 2017 09:59:57 -0400 [thread overview]
Message-ID: <20171018135957.GD17962@localhost.localdomain> (raw)
In-Reply-To: <85b170e6abd3f92bee7449d0970f6ba24e589c72.1508257445.git.jcody@redhat.com>
On Tue, Oct 17, 2017 at 12:31:50PM -0400, Jeff Cody wrote:
> So that later patches can cleanup running qemu processes called from
> different subshells, track resources to cleanup in pid and fd list
> files.
>
> In subsquent patches, ./check will kill all QEMU processes launched with
> the common.qemu framework, and the tests will not need to cleanup
> manually (unless they want to do so as part of the test, e.g. wait for
> a process to end such as migration).
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
> tests/qemu-iotests/common.qemu | 82 ++++++++++++++++++++++++++++++++----------
> tests/qemu-iotests/common.rc | 3 +-
> 2 files changed, 64 insertions(+), 21 deletions(-)
>
> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> index 7b3052d..35a08a6 100644
> --- a/tests/qemu-iotests/common.qemu
> +++ b/tests/qemu-iotests/common.qemu
> @@ -33,6 +33,10 @@ QEMU_FIFO_OUT="${QEMU_TEST_DIR}/qmp-out-$$"
> QEMU_HANDLE=0
> export _QEMU_HANDLE=0
>
> +QEMU_PID_LIST="${QEMU_TEST_DIR}"/qemu-pid.lst
> +QEMU_OUT_FD_LIST="${QEMU_TEST_DIR}"/qemu-out-fd.lst
> +QEMU_IN_FD_LIST="${QEMU_TEST_DIR}"/qemu-in-fd.lst
> +QEMU_FIFO_LIST="${QEMU_TEST_DIR}"/qemu-fifo.lst
> # If bash version is >= 4.1, these will be overwritten and dynamic
> # file descriptor values assigned.
> _out_fd=3
> @@ -193,6 +197,10 @@ function _launch_qemu()
> QEMU_OUT[${_QEMU_HANDLE}]=${_out_fd}
> QEMU_IN[${_QEMU_HANDLE}]=${_in_fd}
> QEMU_STATUS[${_QEMU_HANDLE}]=0
> + echo ${_out_fd} >> "$QEMU_OUT_FD_LIST"
> + echo ${_in_fd} >> "$QEMU_IN_FD_LIST"
> + echo ${fifo_in} >> "$QEMU_FIFO_LIST"
> + echo ${fifo_out} >> "$QEMU_FIFO_LIST"
>
> if [ "${qemu_comm_method}" == "qmp" ]
> then
> @@ -209,35 +217,71 @@ function _launch_qemu()
>
> # Silenty kills the QEMU process
> #
> +# This is able to kill and clean up after QEMU processes without the
> +# need for any subshell-specific variables, so long as the qemu-pidlist
> +# and qemu-out-fd.list and qemu-in-fd.list files are properly maintained.
> +#
> # If $wait is set to anything other than the empty string, the process will not
> # be killed but only waited for, and any output will be forwarded to stdout. If
> # $wait is empty, the process will be killed and all output will be suppressed.
> function _cleanup_qemu()
> {
> - # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
> - for i in "${!QEMU_OUT[@]}"
> + local fifo_path=
> + local testdir_path=
> +
> + if [ ! -e "${QEMU_PID_LIST}" ]; then
> + return
> + fi
> +
> + # get line count, and therefore number of processes to kill
> + numproc=$(wc -l "${QEMU_PID_LIST}" | sed 's/\s.*//')
> +
> + for i in $(seq 1 $numproc)
> do
> local QEMU_PID
> - if [ -f "${QEMU_TEST_DIR}/qemu-${i}.pid" ]; then
> - read QEMU_PID < "${QEMU_TEST_DIR}/qemu-${i}.pid"
> - rm -f "${QEMU_TEST_DIR}/qemu-${i}.pid"
> - if [ -z "${wait}" ] && [ -n "${QEMU_PID}" ]; then
> - kill -KILL ${QEMU_PID} 2>/dev/null
> - fi
> - if [ -n "${QEMU_PID}" ]; then
> - wait ${QEMU_PID} 2>/dev/null # silent kill
> - fi
> + local OUT_FD
> + local IN_FD
> + j=$(expr $i - 1)
> +
> + QEMU_PID=$(tail -n+${i} "${QEMU_PID_LIST}" | head -n1)
> + OUT_FD=$(tail -n+${i} "${QEMU_OUT_FD_LIST}" | head -n1)
> + IN_FD=$(tail -n+${i} "${QEMU_IN_FD_LIST}" | head -n1)
> +
> + if [ -z "${wait}" ] && [ -n "${QEMU_PID}" ]; then
> + kill -KILL ${QEMU_PID} 2>/dev/null
> + fi
> + if [ -n "${QEMU_PID}" ]; then
> + wait ${QEMU_PID} 2>/dev/null # silent kill
> fi
>
> - if [ -n "${wait}" ]; then
> - cat <&${QEMU_OUT[$i]} | _filter_testdir | _filter_qemu \
> - | _filter_qemu_io | _filter_qmp | _filter_hmp
> + if [ -n "${wait}" ] && [ -n "${OUT_FD}" ]; then
> + cat <&${OUT_FD} | _filter_testdir | _filter_qemu \
> + | _filter_qemu_io | _filter_qmp | _filter_hmp
> + fi
> +
> + if [ -n "${IN_FD}" ]; then
> + eval "exec ${IN_FD}<&-" # close file descriptors
> + fi
> + if [ -n "${OUT_FD}" ]; then
> + eval "exec ${OUT_FD}<&-"
> fi
> - rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
> - eval "exec ${QEMU_IN[$i]}<&-" # close file descriptors
> - eval "exec ${QEMU_OUT[$i]}<&-"
>
> - unset QEMU_IN[$i]
> - unset QEMU_OUT[$i]
> + unset QEMU_IN[$j]
> + unset QEMU_OUT[$j]
> done
> +
> + # The FIFOs do not correspond to process entry in the pidlist, so
> + # just clean them up afterwards
> + while read fifo_name
> + do
> + # make sure entries are inside the $TEST_DIR
> + fifo_path=$(dirname -z $(realpath "$fifo_name"))
Pointing out another issue I noticed after testing on a different machine:
in Bash > 4.4, this generates a warning, which breaks diff stats. The fix
is to drop the '-z':
fifo_path=$(dirname $(realpath "$fifo_name"))
> + testdir_path=$(realpath "$QEMU_TEST_DIR")
> + if [ "$fifo_path" == "$testdir_path" ]
> + then
> + rm -f "$fifo_name"
> + fi
> + done < "${QEMU_FIFO_LIST}"
> +
> + rm -f "${QEMU_PID_LIST}" "${QEMU_OUT_FD_LIST}" "${QEMU_IN_FD_LIST}" "$QEMU_FIFO_LIST}"
> }
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index a345ffd..b26b02f 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -40,7 +40,6 @@ poke_file()
> printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
> }
>
> -
> if ! . ./common.config
> then
> echo "$0: failed to source common.config"
> @@ -51,7 +50,7 @@ _qemu_wrapper()
> {
> (
> if [ -n "${QEMU_NEED_PID}" ]; then
> - echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
> + echo $BASHPID >> "${QEMU_TEST_DIR}/qemu-pid.lst"
> fi
> exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
> )
> --
> 2.9.5
>
next prev parent reply other threads:[~2017-10-18 14:00 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-17 16:31 [Qemu-devel] [PATCH v5 00/10] qemu-iotests improvements Jeff Cody
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 01/10] qemu-iotests: refuse to run if TEST_DIR contains spaces Jeff Cody
2017-10-18 1:03 ` Eric Blake
2017-10-18 3:05 ` Jeff Cody
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 02/10] qemu-iotests: set TEST_DIR to a unique dir for each test Jeff Cody
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 03/10] qemu-iotests: automatically clean up bash protocol servers Jeff Cody
2017-10-18 1:15 ` Eric Blake
2017-10-18 14:46 ` Paolo Bonzini
2017-10-18 15:03 ` Jeff Cody
2017-10-18 15:16 ` Paolo Bonzini
2017-10-18 15:34 ` Jeff Cody
2017-10-18 15:39 ` Paolo Bonzini
2017-10-18 15:50 ` Jeff Cody
2017-10-18 15:51 ` Paolo Bonzini
2017-10-18 16:19 ` Jeff Cody
2017-10-18 16:39 ` Paolo Bonzini
2017-10-18 17:27 ` Jeff Cody
2017-10-19 10:23 ` Paolo Bonzini
2017-10-19 14:52 ` Jeff Cody
2017-10-19 21:03 ` Paolo Bonzini
2017-10-18 15:06 ` Eric Blake
2017-10-18 15:43 ` Daniel P. Berrange
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 04/10] qemu-iotests: remove file cleanup from bash tests Jeff Cody
2017-10-18 13:46 ` Eric Blake
2017-10-18 13:56 ` Jeff Cody
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 05/10] qemu-iotests: change qemu pid and fd tracking / cleanup Jeff Cody
2017-10-18 13:59 ` Jeff Cody [this message]
2017-10-18 14:11 ` Eric Blake
2017-10-18 14:22 ` Eric Blake
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 06/10] qemu-iotests: make ./check automatically reap QEMU processes Jeff Cody
2017-10-18 14:24 ` Eric Blake
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 07/10] qemu-iotests: run python tests in own subdirectories Jeff Cody
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 08/10] qemu-iotests: modify python tests to run from subdir Jeff Cody
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 09/10] qemu-iotests: add option to save temp files on error Jeff Cody
2017-10-18 14:33 ` Eric Blake
2017-10-17 16:31 ` [Qemu-devel] [PATCH v5 10/10] qemu-iotests: add support for running multi-threaded iotests Jeff Cody
2017-10-18 3:45 ` Jeff Cody
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171018135957.GD17962@localhost.localdomain \
--to=jcody@redhat.com \
--cc=eblake@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).