qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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
> 

  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).