On 24.11.2015 22:17, Sascha Silbe wrote: > Dear Max, Hi! :-)
> Max Reitz <mre...@redhat.com> 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