[v8] qemu_iotests: improve debugging options

[PATCH v8 08/16] qemu-iotests: add gdbserver option to script tests too

Posted by Emanuele Giuseppe Esposito 4 years, 7 months ago

Remove read timer in test script when GDB_OPTIONS are set,
so that the bash tests won't timeout while running gdb.

The only limitation here is that running a script with gdbserver
will make the test output mismatch with the expected
results, making the test fail.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/common.qemu | 7 ++++++-
 tests/qemu-iotests/common.rc   | 8 +++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 0fc52d20d7..cbca757b49 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -85,7 +85,12 @@ _timed_wait_for()
     timeout=yes
 
     QEMU_STATUS[$h]=0
-    while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+    read_timeout="-t ${QEMU_COMM_TIMEOUT}"
+    if [ ! -z ${GDB_OPTIONS} ]; then
+        read_timeout=
+    fi
+
+    while IFS= read ${read_timeout} resp <&${QEMU_OUT[$h]}
     do
         if [ -n "$capture_events" ]; then
             capture=0
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index cbbf6d7c7f..a1ef2b5c2f 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -166,8 +166,14 @@ _qemu_wrapper()
         if [ -n "${QEMU_NEED_PID}" ]; then
             echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
         fi
+
+        GDB=""
+        if [ ! -z ${GDB_OPTIONS} ]; then
+            GDB="gdbserver ${GDB_OPTIONS}"
+        fi
+
         VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
-            "$QEMU_PROG" $QEMU_OPTIONS "$@"
+            $GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"
     )
     RETVAL=$?
     _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
-- 
2.31.1

Re: [PATCH v8 08/16] qemu-iotests: add gdbserver option to script tests too

Posted by Max Reitz 4 years, 6 months ago

On 05.07.21 08:57, Emanuele Giuseppe Esposito wrote:
> Remove read timer in test script when GDB_OPTIONS are set,
> so that the bash tests won't timeout while running gdb.
>
> The only limitation here is that running a script with gdbserver
> will make the test output mismatch with the expected
> results, making the test fail.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/common.qemu | 7 ++++++-
>   tests/qemu-iotests/common.rc   | 8 +++++++-
>   2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> index 0fc52d20d7..cbca757b49 100644
> --- a/tests/qemu-iotests/common.qemu
> +++ b/tests/qemu-iotests/common.qemu
> @@ -85,7 +85,12 @@ _timed_wait_for()
>       timeout=yes
>   
>       QEMU_STATUS[$h]=0
> -    while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
> +    read_timeout="-t ${QEMU_COMM_TIMEOUT}"
> +    if [ ! -z ${GDB_OPTIONS} ]; then

Shouldn’t we quote "${GDB_OPTIONS}" so that `test` won’t interpret it as 
its own parameters (if something in there starts with `--`, which I 
don’t think is the intended usage for $GDB_OPTIONS, but, well...)?

(Also, `! -z` is the same as `-n`, but I suppose choosing between the 
two can be a matter of style.)

> +        read_timeout=
> +    fi
> +
> +    while IFS= read ${read_timeout} resp <&${QEMU_OUT[$h]}
>       do
>           if [ -n "$capture_events" ]; then
>               capture=0
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index cbbf6d7c7f..a1ef2b5c2f 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -166,8 +166,14 @@ _qemu_wrapper()
>           if [ -n "${QEMU_NEED_PID}" ]; then
>               echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
>           fi
> +
> +        GDB=""
> +        if [ ! -z ${GDB_OPTIONS} ]; then

Here, too.  (Sorry for not noticing in v3 already...)

Max

> +            GDB="gdbserver ${GDB_OPTIONS}"
> +        fi
> +
>           VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
> -            "$QEMU_PROG" $QEMU_OPTIONS "$@"
> +            $GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"
>       )
>       RETVAL=$?
>       _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL

Re: [PATCH v8 08/16] qemu-iotests: add gdbserver option to script tests too

Posted by Emanuele Giuseppe Esposito 4 years, 6 months ago

>> diff --git a/tests/qemu-iotests/common.qemu 
>> b/tests/qemu-iotests/common.qemu
>> index 0fc52d20d7..cbca757b49 100644
>> --- a/tests/qemu-iotests/common.qemu
>> +++ b/tests/qemu-iotests/common.qemu
>> @@ -85,7 +85,12 @@ _timed_wait_for()
>>       timeout=yes
>>       QEMU_STATUS[$h]=0
>> -    while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
>> +    read_timeout="-t ${QEMU_COMM_TIMEOUT}"
>> +    if [ ! -z ${GDB_OPTIONS} ]; then
> 
> Shouldn’t we quote "${GDB_OPTIONS}" so that `test` won’t interpret it as 
> its own parameters (if something in there starts with `--`, which I 
> don’t think is the intended usage for $GDB_OPTIONS, but, well...)?
> 
> (Also, `! -z` is the same as `-n`, but I suppose choosing between the 
> two can be a matter of style.)
> 

>> +
>> +        GDB=""
>> +        if [ ! -z ${GDB_OPTIONS} ]; then
> 
> Here, too.  (Sorry for not noticing in v3 already...)

Sorry, I forgot to reply to this email. I agree, I will put quotes and 
change `! -z` in `-n`.

I will send v9 because after all this time this serie has some minor 
conflicts with current QEMU version.

Hopefully this will be the last one :)

Thank you,
Emanuele