Re: [PATCH v9 10/11] iotests: add support for capturing and matching QMP events

From: Eric Blake <eblake@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-block@nongnu.org, Juan Quintela <quintela@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Max Reitz <mreitz@redhat.com>,
	Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>,
	Paolo Bonzini <pbonzini@redhat.com>, John Snow <jsnow@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v9 10/11] iotests: add support for capturing and matching QMP events
Date: Wed, 20 Jan 2021 12:38:48 -0600	[thread overview]
Message-ID: <ac0d751e-c382-38b4-dd00-8189b1f34e0d@redhat.com> (raw)
In-Reply-To: <20210120104411.3084801-11-berrange@redhat.com>

On 1/20/21 4:44 AM, Daniel P. Berrangé wrote:
> When using the _launch_qemu and _send_qemu_cmd functions from
> common.qemu, any QMP events get mixed in with the output from
> the commands and responses.
> 
> This makes it difficult to write a test case as the ordering
> of events in the output is not stable.
> 
> This introduces a variable 'capture_events' which can be set
> to a list of event names. Any events listed in this variable
> will not be printed, instead collected in the $QEMU_EVENTS
> environment variable.
> 
> A new '_wait_event' function can be invoked to collect events
> at a fixed point in time. The function will first pull events
> cached in $QEMU_EVENTS variable, and if none are found, will
> then read more from QMP.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemu-iotests/common.qemu | 107 ++++++++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> index ef105dfc39..21e4d059f7 100644
> --- a/tests/qemu-iotests/common.qemu
> +++ b/tests/qemu-iotests/common.qemu
> @@ -53,6 +53,15 @@ _in_fd=4
>  # If $mismatch_only is set, only non-matching responses will
>  # be echoed.
>  #
> +# If $capture_events is non-empty, then any QMP event names it lists
> +# will not be echoed out, but instead collected in the $QEMU_EVENTS
> +# variable. The _wait_event function can later be used to received

receive

> +# the cached events.
> +#
> +# If $only_capture_events is set to anything but an empty string,
> +# when an error will be raised if a QMP message is seen which is

s/when/then/

> +# not an event listed in $capture_events.
> +#
>  # If $success_or_failure is set, the meaning of the arguments is
>  # changed as follows:
>  # $2: A string to search for in the response; if found, this indicates
> @@ -78,6 +87,32 @@ _timed_wait_for()
>      QEMU_STATUS[$h]=0
>      while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
>      do
> +        if [ -n "$capture_events" ]; then
> +            capture=0
> +            local evname
> +            for evname in $capture_events
> +            do
> +                grep -q "\"event\": \"${evname}\"" < <(echo "${resp}")

What you have works (thanks to the <() bashism), but could be done in
fewer processes with:

case ${resp} in
  *\"event\":\ \"${evname}\"* ) capture=1 ;;
esac

> +                if [ $? -eq 0 ]; then
> +                    capture=1
> +                fi
> +            done
> +            if [ $capture = 1 ];
> +            then
> +                ev=$(echo "${resp}" | tr -d '\r' | tr % .)
> +                QEMU_EVENTS="${QEMU_EVENTS:+${QEMU_EVENTS}%}${ev}"
> +                if [ -n "$only_capture_events" ]; then
> +                    return
> +                else
> +                    continue
> +                fi
> +            fi
> +        fi
> +        if [ -n "$only_capture_events" ]; then
> +            echo "Only expected $capture_events but got ${resp}"
> +            exit 1
> +        fi
> +
>          if [ -z "${silent}" ] && [ -z "${mismatch_only}" ]; then
>              echo "${resp}" | _filter_testdir | _filter_qemu \
>                             | _filter_qemu_io | _filter_qmp | _filter_hmp
> @@ -172,12 +207,82 @@ _send_qemu_cmd()
>          let count--;
>      done
>      if [ ${QEMU_STATUS[$h]} -ne 0 ] && [ -z "${qemu_error_no_exit}" ]; then
> -        echo "Timeout waiting for ${1} on handle ${h}"
> +        echo "Timeout waiting for command ${1} response on handle ${h}"
>          exit 1 #Timeout means the test failed
>      fi
>  }
>  
>  
> +# Check event cache for a named QMP event
> +#
> +# Input parameters:
> +# $1:       Name of the QMP event to check for
> +#
> +# Checks if the named QMP event that was previously captured
> +# into $QEMU_EVENTS. When matched, the QMP event will be echoed
> +# and the $matched variable set to 1.
> +#
> +# _wait_event is more suitable for test usage in most cases
> +_check_cached_events()
> +{
> +    local evname=${1}
> +
> +    local match="\"event\": \"$evname\""
> +
> +    matched=0
> +    if [ -n "$QEMU_EVENTS" ]; then
> +        CURRENT_QEMU_EVENTS=$QEMU_EVENTS
> +        QEMU_EVENTS=
> +        old_IFS=$IFS
> +        IFS="%"
> +        for ev in $CURRENT_QEMU_EVENTS
> +        do
> +                grep -q "$match" < <(echo "${ev}")
> +            if [ $? -eq 0 -a $matched = 0 ]; then

Odd indentation.  Use of [ ... -a ... ] is not wise (it happens to work
in current bash, but POSIX says it is deprecated); use [ ... ] && [ ...
] instead.

> +                echo "${ev}" | _filter_testdir | _filter_qemu \
> +                           | _filter_qemu_io | _filter_qmp | _filter_hmp
> +                matched=1
> +            else
> +                QEMU_EVENTS="${QEMU_EVENTS:+${QEMU_EVENTS}%}${ev}"
> +            fi
> +        done
> +        IFS=$old_IFS
> +    fi
> +}
> +
> +# Wait for a named QMP event
> +#
> +# Input parameters:
> +# $1:       QEMU handle to use
> +# $2:       Name of the QMP event to wait for
> +#
> +# Checks if the named QMP event that was previously captured

s/that //

> +# into $QEMU_EVENTS. If none are present, then waits for the
> +# event to arrive on the QMP channel. When matched, the QMP
> +# event will be echoed
> +_wait_event()
> +{
> +    local h=${1}
> +    local evname=${2}
> +
> +    while true
> +    do
> +        _check_cached_events $evname
> +
> +        if [ $matched = 1 ];
> +        then
> +            return
> +        fi
> +
> +        only_capture_events=1 qemu_error_no_exit=1 _timed_wait_for ${h}
> +
> +        if [ ${QEMU_STATUS[$h]} -ne 0 ] ; then
> +            echo "Timeout waiting for event ${evname} on handle ${h}"
> +            exit 1 #Timeout means the test failed
> +        fi
> +    done
> +}
> +
>  # Launch a QEMU process.
>  #
>  # Input parameters:
> 

Otherwise makes sense.  Using case instead of 'grep -q < <(echo)' is not
mandatory, and the rest of my comments are trivial, so with them cleaned up,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org