Re: [PATCH v2] automation: use expect to run QEMU

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 13/08/2024 04:21, Stefano Stabellini wrote:
> 
> 
> Use expect to invoke QEMU so that we can terminate the test as soon as
> we get the right string in the output instead of waiting until the
> final timeout.
> 
> For timeout, instead of an hardcoding the value, use a Gitlab CI
> variable "QEMU_TIMEOUT" that can be changed depending on the latest
> status of the Gitlab CI runners.
> 
> Note that for simplicity in the case of dom0less test, the script only
> checks for the $PASSED string. That is because the dom0 prompt and the
> $PASSED string could happen in any order making it difficult to check
> with expect which checks for strings in a specific order.
I think this comment is no longer true and needs to be removed.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> ---
> Changes in v2:
> - add empty lines for readability
I can only see it being done for one script.

> - changes qemu-key file extension to exp
> - drop xen_msg that is unused
> - use capital letters for exported variables
> - check for both dom0 and domU message on dom0less tests
> 
> https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/1410820286
> ---
>  automation/scripts/qemu-alpine-x86_64.sh      | 16 +++---
>  automation/scripts/qemu-key.exp               | 51 +++++++++++++++++++
>  automation/scripts/qemu-smoke-dom0-arm32.sh   | 15 +++---
>  automation/scripts/qemu-smoke-dom0-arm64.sh   | 15 +++---
>  .../scripts/qemu-smoke-dom0less-arm32.sh      | 17 +++----
>  .../scripts/qemu-smoke-dom0less-arm64.sh      | 15 +++---
>  automation/scripts/qemu-smoke-ppc64le.sh      | 12 ++---
>  automation/scripts/qemu-smoke-riscv64.sh      | 12 ++---
>  automation/scripts/qemu-smoke-x86-64.sh       | 14 ++---
>  automation/scripts/qemu-xtf-dom0less-arm64.sh | 14 +++--
>  10 files changed, 110 insertions(+), 71 deletions(-)
>  create mode 100755 automation/scripts/qemu-key.exp
> 
> diff --git a/automation/scripts/qemu-alpine-x86_64.sh 
> b/automation/scripts/qemu-alpine-x86_64.sh
> index 8e398dcea3..5359e0820b 100755
> --- a/automation/scripts/qemu-alpine-x86_64.sh
> +++ b/automation/scripts/qemu-alpine-x86_64.sh
> @@ -77,18 +77,16 @@ EOF
>  # Run the test
>  rm -f smoke.serial
>  set +e
> -timeout -k 1 720 \
> -qemu-system-x86_64 \
> +export QEMU_CMD="qemu-system-x86_64 \
>      -cpu qemu64,+svm \
>      -m 2G -smp 2 \
>      -monitor none -serial stdio \
>      -nographic \
>      -device virtio-net-pci,netdev=n0 \
> -    -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0 |& \
> -        # Remove carriage returns from the stdout output, as gitlab
> -        # interface chokes on them
> -        tee smoke.serial | sed 's/\r//'
> +    -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0"
> 
> -set -e
> -(grep -q "Domain-0" smoke.serial && grep -q "BusyBox" smoke.serial) || exit 1
> -exit 0
> +export QEMU_LOG="smoke.serial"
> +export LOG_MSG="Domain-0"
> +export PASSED="BusyBox"
> +
> +./automation/scripts/qemu-key.exp
> diff --git a/automation/scripts/qemu-key.exp b/automation/scripts/qemu-key.exp
> new file mode 100755
> index 0000000000..9f3a3364fa
> --- /dev/null
> +++ b/automation/scripts/qemu-key.exp
> @@ -0,0 +1,51 @@
> +#!/usr/bin/expect -f
> +
> +set timeout $env(QEMU_TIMEOUT)
> +
> +log_file -a $env(QEMU_LOG)
> +
> +match_max 10000
> +
> +eval spawn $env(QEMU_CMD)
> +
> +expect_after {
> +    -re "(.*)\r" {
> +        exp_continue
> +    }
> +    timeout {send_user "ERROR-Timeout!\n"; exit 1}
It should be send_error to send the string to stderr

> +    eof {send_user "ERROR-EOF!\n"; exit 1}
> +}
> +
> +if {[info exists env(UBOOT_CMD)]} {
> +    expect "=>"
> +
> +    send "$env(UBOOT_CMD)\r"
> +}
> +
> +if {[info exists env(LOG_MSG)] && [info exists env(PASSED)]} {
> +    expect {
> +        "$env(PASSED)" {
> +            expect "$env(LOG_MSG)"
> +            exit 0
> +        }
> +        "$env(LOG_MSG)" {
> +            expect "$env(PASSED)"
> +            exit 0
> +        }
> +    }
> +}
> +
> +if {[info exists env(LOG_MSG)]} {
> +    expect "$env(LOG_MSG)"
> +}
> +
> +if {[info exists env(PASSED)]} {
> +    expect {
> +        "$env(PASSED)" {
> +            exit 0
> +        }
> +    }
> +}
Given that this script treats both LOG_MSG and PASSED as optional, if a script
does not define PASSED, it will just continue until timeout. Might be worth 
making
PASSED mandatory. Something like:

if {[info exists env(LOG_MSG)]} {
    expect {
        "$env(PASSED)" {
            expect "$env(LOG_MSG)"
            exit 0
        }
        "$env(LOG_MSG)" {
            expect "$env(PASSED)"
            exit 0
        }
    }
}

expect {
    "$env(PASSED)" {
        exit 0
    }
}

In any case, you can do the changes on commit:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal