Re: [Qemu-devel] [PATCH v2 2/4] qemu-iotests: s390x: fix test 051

[Max Reitz]

On 24.11.2015 22:17, Sascha Silbe wrote:
> Dear Max,

Hi! :-)

> Max Reitz <address@hidden> writes:
> 
>> OK, so it is expected for s390x; however, this is strictly speaking not
>> the output file for s390x but for any platform but PC. That's why I'd
>> rather not have it in this “generic” reference output.
>>
>> This patch already assumes that the iotests only support s390 and PC
>> (hunk @@ -251,28 +273,37 @@ in 051 adds a case statement which knows
>> only these two cases). Therefore, I would be fine with renaming this
>> reference output file to "051.s390.out" with a copy
>> "051.s390-ccw-virtio.out" and just removing the generic "051.out". Then
>> you can keep the warnings in.
>>
>> (Or you filter the warnings out and keep it as "051.out".)
> 
> This PC/s390x-only hunk looks like an oversight to me.

Not really, see
http://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg01906.html
and
http://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg02851.html

I noticed, but I am fine with it since the tests probably won't run on
anything but x86/pc and s390 anyway (without modifications; most of the
changes this series is making to make the iotests work on s390 are
necessary for other non-pc platforms as well, and that shows to me that
apparently nobody tried to run the iotests on non-pc platforms before
s390, or didn't care enough about them to fix them).

>                                                        We should make
> one of the options the default. I'd prefer defaulting to virtio (see
> below), but since the test previously hard-coded IDE that would be fine,
> too.

In my first reply above, I noted that virtio0 may not be available on
all platforms either. Therefore, I'd rather have an explicit list of
platforms there than an asterisk where it does not belong.

However, my second reply above spawned a bit of a discussion, where
Kevin simply proposed to change the ID of the drive to something known,
i.e. just set the ID by adding an id=drive0 or something to the -drive
parameter.

Thanks for reminding me of the above, I had already forgotten. Indeed,
we should just add id=drive0 to the -drive parameter and use drive0. A
similar solution may be possible in most other places as well where PC
and s390 differ due to the names of the default devices available.

> For the other cases, IIRC it's really PC that's the odd one out, that's
> why I suggested adding a PC-specific output file and patching the
> generic output file to look like the output on most other architectures.

Actually, I remember it was me:
http://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg00862.html ;-)

(Not that it really matters, of course)

> But I'm fine with anything that gets the job done on both PC and s390x
> today.

As am I.

>        I'll have a closer look again once things settle down a
> bit. While the support for one output file per architecture is a very
> useful generic solution, we should make use of it only sparingly. The
> git log already shows that people forget to update test output
> files. This will get worse with multiple output files.

Well, maintaining the iotests for s390 may be a difficult task in
itself. Most people (including myself) don't have an s390 machine, so we
can't test whether modifications or additions we make to the iotests
work there.

> One of the things I'm considering is splitting off the IDE tests to a
> separate test case and skipping it entirely on machines that do not
> support IDE. Another is using virtio-blk on all architectures whenever
> the test case doesn't care about the device type. (I doubt the tests
> currently work on architectures that don't support virtio, but will of
> course check this).

The question is whether we really do have IDE test cases in the iotests.
I don't think so, the actual IDE tests should be part of qtest. The
iotests only test the block layer itself, so as I noted above we should
most of the time get around the issues addressed in this series by
simply manually setting the ID of the drive we are creating (instead of
relying on a certain default name like ide0-hd0 or virtio0).

Max

signature.asc
Description: OpenPGP digital signature