[PATCH] iotests: fix usage -machine accel= together with -accel option

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH] iotests: fix usage -machine accel= together with -accel option
@ 2019-12-23  7:43 Vladimir Sementsov-Ogievskiy
  2019-12-23  8:39 ` Paolo Bonzini
  0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-12-23  7:43 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, vsementsov, qemu-devel, mreitz

Commit 6f6e1698a68ceb made these options incompatible, but it breaks
iotests:
  -accel qtest comes from QEMUQtestMachine
  and
  -machine accel=qtest comes from QEMU_OPTIONS

After this patch, -accel may still be duplicated, but this is less
invasive.

Also, fix extra comma in comment, added by the same 6f6e1698a68ceb

Fixes: 6f6e1698a68ceb
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 vl.c                     | 2 +-
 tests/qemu-iotests/check | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 86474a55c9..9fb859969c 100644
--- a/vl.c
+++ b/vl.c
@@ -2779,7 +2779,7 @@ static void configure_accelerators(const char *progname)
         for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
             /*
              * Filter invalid accelerators here, to prevent obscenities
-             * such as "-machine accel=tcg,,thread=single".
+             * such as "-machine accel=tcg,thread=single".
              */
             if (accel_find(*tmp)) {
                 qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 90970b0549..2890785a10 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -587,13 +587,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
 
 case "$QEMU_PROG" in
     *qemu-system-arm|*qemu-system-aarch64)
-        export QEMU_OPTIONS="-nodefaults -display none -machine virt,accel=qtest"
+        export QEMU_OPTIONS="-nodefaults -display none -machine virt -accel qtest"
         ;;
     *qemu-system-tricore)
-        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard,accel=qtest"
+        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard -accel qtest"
         ;;
     *)
-        export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
+        export QEMU_OPTIONS="-nodefaults -display none -accel qtest"
         ;;
 esac
 
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] iotests: fix usage -machine accel= together with -accel option
  2019-12-23  7:43 [PATCH] iotests: fix usage -machine accel= together with -accel option Vladimir Sementsov-Ogievskiy
@ 2019-12-23  8:39 ` Paolo Bonzini
  2019-12-23 12:00   ` Vladimir Sementsov-Ogievskiy
  2020-01-07 12:46   ` Thomas Huth
  0 siblings, 2 replies; 4+ messages in thread
From: Paolo Bonzini @ 2019-12-23  8:39 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, qemu-devel, mreitz

On 23/12/19 08:43, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/vl.c b/vl.c
> index 86474a55c9..9fb859969c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2779,7 +2779,7 @@ static void configure_accelerators(const char *progname)
>          for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
>              /*
>               * Filter invalid accelerators here, to prevent obscenities
> -             * such as "-machine accel=tcg,,thread=single".
> +             * such as "-machine accel=tcg,thread=single".

The double comma is intentional.  Without the "if" below, the comma
would be escaped and parsed as "-accel tcg,thread=single".

>               */
>              if (accel_find(*tmp)) {
>                  qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 90970b0549..2890785a10 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -587,13 +587,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
>  
>  case "$QEMU_PROG" in
>      *qemu-system-arm|*qemu-system-aarch64)
> -        export QEMU_OPTIONS="-nodefaults -display none -machine virt,accel=qtest"
> +        export QEMU_OPTIONS="-nodefaults -display none -machine virt -accel qtest"
>          ;;
>      *qemu-system-tricore)
> -        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard,accel=qtest"
> +        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard -accel qtest"
>          ;;
>      *)
> -        export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
> +        export QEMU_OPTIONS="-nodefaults -display none -accel qtest"
>          ;;
>  esac
>  
> 

This part is good, but what is the reproducer?

Paolo



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iotests: fix usage -machine accel= together with -accel option
  2019-12-23  8:39 ` Paolo Bonzini
@ 2019-12-23 12:00   ` Vladimir Sementsov-Ogievskiy
  2020-01-07 12:46   ` Thomas Huth
  1 sibling, 0 replies; 4+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-12-23 12:00 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-block@nongnu.org
  Cc: kwolf@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com

23.12.2019 11:39, Paolo Bonzini wrote:
> On 23/12/19 08:43, Vladimir Sementsov-Ogievskiy wrote:
>> diff --git a/vl.c b/vl.c
>> index 86474a55c9..9fb859969c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2779,7 +2779,7 @@ static void configure_accelerators(const char *progname)
>>           for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
>>               /*
>>                * Filter invalid accelerators here, to prevent obscenities
>> -             * such as "-machine accel=tcg,,thread=single".
>> +             * such as "-machine accel=tcg,thread=single".
> 
> The double comma is intentional.  Without the "if" below, the comma
> would be escaped and parsed as "-accel tcg,thread=single".

Ah, OK, than drop this chunk.

> 
>>                */
>>               if (accel_find(*tmp)) {
>>                   qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index 90970b0549..2890785a10 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -587,13 +587,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
>>   
>>   case "$QEMU_PROG" in
>>       *qemu-system-arm|*qemu-system-aarch64)
>> -        export QEMU_OPTIONS="-nodefaults -display none -machine virt,accel=qtest"
>> +        export QEMU_OPTIONS="-nodefaults -display none -machine virt -accel qtest"
>>           ;;
>>       *qemu-system-tricore)
>> -        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard,accel=qtest"
>> +        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard -accel qtest"
>>           ;;
>>       *)
>> -        export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
>> +        export QEMU_OPTIONS="-nodefaults -display none -accel qtest"
>>           ;;
>>   esac
>>   
>>
> 
> This part is good, but what is the reproducer?
> 


For example, 30 iotest fails for me with a lot of
+======================================================================
+ERROR: test_stream (__main__.TestSmallerBackingFile)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "030", line 592, in setUp
+    self.vm.launch()
+  File "/work/src/qemu/master/tests/qemu-iotests/../../python/qemu/machine.py", line 302, in launch
+    self._launch()
+  File "/work/src/qemu/master/tests/qemu-iotests/../../python/qemu/machine.py", line 329, in _launch
+    self._post_launch()
+  File "/work/src/qemu/master/tests/qemu-iotests/../../python/qemu/qtest.py", line 110, in _post_launch
+    super(QEMUQtestMachine, self)._post_launch()
+  File "/work/src/qemu/master/tests/qemu-iotests/../../python/qemu/machine.py", line 274, in _post_launch
+    self._qmp.accept()
+  File "/work/src/qemu/master/tests/qemu-iotests/../../python/qemu/qmp.py", line 157, in accept
+    return self.__negotiate_capabilities()
+  File "/work/src/qemu/master/tests/qemu-iotests/../../python/qemu/qmp.py", line 73, in __negotiate_capabilities
+    raise QMPConnectError
+qemu.qmp.QMPConnectError


and if I add -d, I see

+DEBUG:qemu.machine:Error launching VM
+DEBUG:qemu.machine:Command: '/work/src/qemu/master/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 -display none -vga none -chardev socket,id=mon,path=/tmp/tmp.ANcscZnFct/qemu-23220-monitor.sock -mon chardev=mon,mode=control -qtest unix:path=/tmp/tmp.ANcscZnFct/qemu-23220-qtest.sock -accel qtest -nodefaults -display none -machine accel=qtest -drive if=virtio,id=drive0,file=blkdebug::/ramdisk/x3/test.img,format=qcow2,cache=writeback,backing.node-name=mid,backing.backing.node-name=base'
+DEBUG:qemu.machine:Output: 'qemu-system-x86_64: The -accel and "-machine accel=" options are incompatible\n'


Try just random three python tests: 132 202 203: all fails with same symptoms. (interesting that -d doesn't help for 202 and 203, but still they are fixed after this patch).

I expect that all python tests are broken.. Still I'm lazy to check all, let's just fix.


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iotests: fix usage -machine accel= together with -accel option
  2019-12-23  8:39 ` Paolo Bonzini
  2019-12-23 12:00   ` Vladimir Sementsov-Ogievskiy
@ 2020-01-07 12:46   ` Thomas Huth
  1 sibling, 0 replies; 4+ messages in thread
From: Thomas Huth @ 2020-01-07 12:46 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-block
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, qemu-devel, mreitz

On 23/12/2019 09.39, Paolo Bonzini wrote:
> On 23/12/19 08:43, Vladimir Sementsov-Ogievskiy wrote:
>> diff --git a/vl.c b/vl.c
>> index 86474a55c9..9fb859969c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2779,7 +2779,7 @@ static void configure_accelerators(const char *progname)
>>          for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
>>              /*
>>               * Filter invalid accelerators here, to prevent obscenities
>> -             * such as "-machine accel=tcg,,thread=single".
>> +             * such as "-machine accel=tcg,thread=single".
> 
> The double comma is intentional.  Without the "if" below, the comma
> would be escaped and parsed as "-accel tcg,thread=single".
> 
>>               */
>>              if (accel_find(*tmp)) {
>>                  qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index 90970b0549..2890785a10 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -587,13 +587,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
>>  
>>  case "$QEMU_PROG" in
>>      *qemu-system-arm|*qemu-system-aarch64)
>> -        export QEMU_OPTIONS="-nodefaults -display none -machine virt,accel=qtest"
>> +        export QEMU_OPTIONS="-nodefaults -display none -machine virt -accel qtest"
>>          ;;
>>      *qemu-system-tricore)
>> -        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard,accel=qtest"
>> +        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard -accel qtest"
>>          ;;
>>      *)
>> -        export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
>> +        export QEMU_OPTIONS="-nodefaults -display none -accel qtest"
>>          ;;
>>  esac
>>  
>>
> 
> This part is good, but what is the reproducer?

Make the iotests run with either qemu-system-arm, qemu-system-aarch64 or
qemu-system-tricore, e.g.:

 QEMU_PROG=aarch64-softmmu/qemu-system-aarch64 make check-block

 HTH,
  Thomas



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-01-07 13:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-23  7:43 [PATCH] iotests: fix usage -machine accel= together with -accel option Vladimir Sementsov-Ogievskiy
2019-12-23  8:39 ` Paolo Bonzini
2019-12-23 12:00   ` Vladimir Sementsov-Ogievskiy
2020-01-07 12:46   ` Thomas Huth

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