Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: drop unnecessary accel=kvm

[Vladimir Sementsov-Ogievskiy]

19.02.2019 16:20, Thomas Huth wrote:
> On 19/02/2019 14.07, Stefan Hajnoczi wrote:
>> On Tue, Feb 19, 2019 at 12:12 PM Vladimir Sementsov-Ogievskiy
>> <address@hidden> wrote:
>>> 19.02.2019 15:02, Thomas Huth wrote:
>>>> On 19/02/2019 12.59, Stefan Hajnoczi wrote:
>>>>> Tests 235 and 238 do not require the kvm accelerator.  TCG works fine.
>>>>>
>>>>> Use the default accelerator instead of requiring kvm.
>>>>>
>>>>> Suggested-by: Thomas Huth <address@hidden>
>>>>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>>>>> ---
>>>>>    tests/qemu-iotests/235 | 1 -
>>>>>    tests/qemu-iotests/238 | 1 -
>>>>>    2 files changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
>>>>> index d6edd97ab4..329da8f0c2 100755
>>>>> --- a/tests/qemu-iotests/235
>>>>> +++ b/tests/qemu-iotests/235
>>>>> @@ -49,7 +49,6 @@ qemu_img_create('-f', iotests.imgfmt, '-o', 
>>>>> 'preallocation=metadata', disk,
>>>>>                    str(size))
>>>>>
>>>>>    vm = QEMUMachine(iotests.qemu_prog)
>>>>> -vm.add_args('-machine', 'accel=kvm')
>>>>
>>>> According to the initial commit log of 235:
>>>>
>>>>    "iotests: simple mirror test with kvm on 1G image"
>>>>
>>>> ... so I assume KVM was used on purpose here?
>>>>
>>>>    Thomas
>>>>
>>>
>>> As I remember kvm is not really necessary, and test have a comment about it:
>>> # And it didn't reproduce if at least one of the following:
>>> ...
>>> # 3. drop kvm and use iotests.VM() (maybe, because of qtest) (however, it 
>>> still
>>> #    reproduces, if just drop kvm, but gdb failed to produce full backtraces
>>> #    for me)
>>>
>>> But the comment should be updated with this patch.
>>
>> Will fix in v2.
> 
> Any chance that you could even make it somehow work with -M accel=qtest
> instead? ... in case someone compiled their QEMU with --disable-tcg, too
> ...?
> 

I didn't investigate why bug not reproduced with qtest.. I think, the simplest
option should be a helper like iotests.needs_tcg(), to skip the test if
it is unsupported.

Or you case is that kvm is OK for you but tcg is not? Stefan, do you have 
strong reason
for removing kvm from iotests? Could it be something like

   if not iotests.supports_tcg():
      vm.add_args('-machine', 'accel=kvm')

?

Hmm, maybe, good option would be topmost option, like we have for format 
-qcow2, -raw, etc. So,
it would be -qtest, -kvm, -tcg.. But in this case nobody will run all test on 
something other
than qtest..

It's not bad to run this test on qtest. The problem is we can miss the bug, if 
not run this
test on kvm or tcg.

-- 
Best regards,
Vladimir