[v2] Add new CD-ROM related qtests

[Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by Thomas Huth 7 years, 7 months ago

With one of my clean-up patches (see commit 1454509726719e0933c800), I
recently accidentially broke the "-cdrom" parameter (more precisely
"-drive if=scsi") on a couple of boards, since there was no error
detected during the "make check" regression testing. This is clearly an
indication that we are lacking tests in this area.
So this small patch series now introduces some tests for CD-ROM drives:
The first two patches introduce the possibility to check that booting
from CD-ROM drives still works fine for x86 and s390x, and the third
patch adds a test that certain machines can at least still be started
with the "-cdrom" parameter (i.e. that test would have catched the
mistake that I did with my SCSI cleanup patch).

v2:
 - Use g_spawn_sync() instead of execlp() to run genisoimage
 - The "-cdrom" parameter test is now run on all architectures (with
   machine "none" for the machines that are not explicitly checked)
 - Some rewordings and improved comments here and there

Thomas Huth (3):
  tests/boot-sector: Add magic bytes to s390x boot code header
  tests/cdrom-test: Test booting from CD-ROM ISO image file
  tests/cdrom-test: Test that -cdrom parameter is working

 tests/Makefile.include |   2 +
 tests/boot-sector.c    |   9 +-
 tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+), 3 deletions(-)
 create mode 100644 tests/cdrom-test.c

-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by Hervé Poussineau 7 years, 7 months ago

Le 16/03/2018 à 06:39, Thomas Huth a écrit :
> With one of my clean-up patches (see commit 1454509726719e0933c800), I
> recently accidentially broke the "-cdrom" parameter (more precisely
> "-drive if=scsi") on a couple of boards, since there was no error
> detected during the "make check" regression testing. This is clearly an
> indication that we are lacking tests in this area.
> So this small patch series now introduces some tests for CD-ROM drives:
> The first two patches introduce the possibility to check that booting
> from CD-ROM drives still works fine for x86 and s390x, and the third
> patch adds a test that certain machines can at least still be started
> with the "-cdrom" parameter (i.e. that test would have catched the
> mistake that I did with my SCSI cleanup patch).

Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>

> 
> v2:
>   - Use g_spawn_sync() instead of execlp() to run genisoimage
>   - The "-cdrom" parameter test is now run on all architectures (with
>     machine "none" for the machines that are not explicitly checked)
>   - Some rewordings and improved comments here and there
> 
> Thomas Huth (3):
>    tests/boot-sector: Add magic bytes to s390x boot code header
>    tests/cdrom-test: Test booting from CD-ROM ISO image file
>    tests/cdrom-test: Test that -cdrom parameter is working
> 
>   tests/Makefile.include |   2 +
>   tests/boot-sector.c    |   9 +-
>   tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 230 insertions(+), 3 deletions(-)
>   create mode 100644 tests/cdrom-test.c
>

Re: [Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by Mark Cave-Ayland 7 years, 7 months ago

On 16/03/18 05:39, Thomas Huth wrote:

> With one of my clean-up patches (see commit 1454509726719e0933c800), I
> recently accidentially broke the "-cdrom" parameter (more precisely
> "-drive if=scsi") on a couple of boards, since there was no error
> detected during the "make check" regression testing. This is clearly an
> indication that we are lacking tests in this area.
> So this small patch series now introduces some tests for CD-ROM drives:
> The first two patches introduce the possibility to check that booting
> from CD-ROM drives still works fine for x86 and s390x, and the third
> patch adds a test that certain machines can at least still be started
> with the "-cdrom" parameter (i.e. that test would have catched the
> mistake that I did with my SCSI cleanup patch).
> 
> v2:
>   - Use g_spawn_sync() instead of execlp() to run genisoimage
>   - The "-cdrom" parameter test is now run on all architectures (with
>     machine "none" for the machines that are not explicitly checked)
>   - Some rewordings and improved comments here and there
> 
> Thomas Huth (3):
>    tests/boot-sector: Add magic bytes to s390x boot code header
>    tests/cdrom-test: Test booting from CD-ROM ISO image file
>    tests/cdrom-test: Test that -cdrom parameter is working
> 
>   tests/Makefile.include |   2 +
>   tests/boot-sector.c    |   9 +-
>   tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 230 insertions(+), 3 deletions(-)
>   create mode 100644 tests/cdrom-test.c

Hi Thomas,

Nice work - looks like a good, comprehensive test of the -cdrom option.

Acked-By: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by Michael S. Tsirkin 7 years, 7 months ago

On Fri, Mar 16, 2018 at 06:39:52AM +0100, Thomas Huth wrote:
> With one of my clean-up patches (see commit 1454509726719e0933c800), I
> recently accidentially broke the "-cdrom" parameter (more precisely
> "-drive if=scsi") on a couple of boards, since there was no error
> detected during the "make check" regression testing. This is clearly an
> indication that we are lacking tests in this area.
> So this small patch series now introduces some tests for CD-ROM drives:
> The first two patches introduce the possibility to check that booting
> from CD-ROM drives still works fine for x86 and s390x, and the third
> patch adds a test that certain machines can at least still be started
> with the "-cdrom" parameter (i.e. that test would have catched the
> mistake that I did with my SCSI cleanup patch).


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> v2:
>  - Use g_spawn_sync() instead of execlp() to run genisoimage
>  - The "-cdrom" parameter test is now run on all architectures (with
>    machine "none" for the machines that are not explicitly checked)
>  - Some rewordings and improved comments here and there
> 
> Thomas Huth (3):
>   tests/boot-sector: Add magic bytes to s390x boot code header
>   tests/cdrom-test: Test booting from CD-ROM ISO image file
>   tests/cdrom-test: Test that -cdrom parameter is working
> 
>  tests/Makefile.include |   2 +
>  tests/boot-sector.c    |   9 +-
>  tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 230 insertions(+), 3 deletions(-)
>  create mode 100644 tests/cdrom-test.c
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by John Snow 7 years, 6 months ago


On 03/16/2018 01:39 AM, Thomas Huth wrote:
> With one of my clean-up patches (see commit 1454509726719e0933c800), I
> recently accidentially broke the "-cdrom" parameter (more precisely
> "-drive if=scsi") on a couple of boards, since there was no error
> detected during the "make check" regression testing. This is clearly an
> indication that we are lacking tests in this area.
> So this small patch series now introduces some tests for CD-ROM drives:
> The first two patches introduce the possibility to check that booting
> from CD-ROM drives still works fine for x86 and s390x, and the third
> patch adds a test that certain machines can at least still be started
> with the "-cdrom" parameter (i.e. that test would have catched the
> mistake that I did with my SCSI cleanup patch).
> 
> v2:
>  - Use g_spawn_sync() instead of execlp() to run genisoimage
>  - The "-cdrom" parameter test is now run on all architectures (with
>    machine "none" for the machines that are not explicitly checked)
>  - Some rewordings and improved comments here and there
> 
> Thomas Huth (3):
>   tests/boot-sector: Add magic bytes to s390x boot code header
>   tests/cdrom-test: Test booting from CD-ROM ISO image file
>   tests/cdrom-test: Test that -cdrom parameter is working
> 
>  tests/Makefile.include |   2 +
>  tests/boot-sector.c    |   9 +-
>  tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 230 insertions(+), 3 deletions(-)
>  create mode 100644 tests/cdrom-test.c
> 

New file, but no edit to MAINTAINERS.

Re: [Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by Thomas Huth 7 years, 6 months ago

On 29.03.2018 20:28, John Snow wrote:
> 
> 
> On 03/16/2018 01:39 AM, Thomas Huth wrote:
>> With one of my clean-up patches (see commit 1454509726719e0933c800), I
>> recently accidentially broke the "-cdrom" parameter (more precisely
>> "-drive if=scsi") on a couple of boards, since there was no error
>> detected during the "make check" regression testing. This is clearly an
>> indication that we are lacking tests in this area.
>> So this small patch series now introduces some tests for CD-ROM drives:
>> The first two patches introduce the possibility to check that booting
>> from CD-ROM drives still works fine for x86 and s390x, and the third
>> patch adds a test that certain machines can at least still be started
>> with the "-cdrom" parameter (i.e. that test would have catched the
>> mistake that I did with my SCSI cleanup patch).
>>
>> v2:
>>  - Use g_spawn_sync() instead of execlp() to run genisoimage
>>  - The "-cdrom" parameter test is now run on all architectures (with
>>    machine "none" for the machines that are not explicitly checked)
>>  - Some rewordings and improved comments here and there
>>
>> Thomas Huth (3):
>>   tests/boot-sector: Add magic bytes to s390x boot code header
>>   tests/cdrom-test: Test booting from CD-ROM ISO image file
>>   tests/cdrom-test: Test that -cdrom parameter is working
>>
>>  tests/Makefile.include |   2 +
>>  tests/boot-sector.c    |   9 +-
>>  tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 230 insertions(+), 3 deletions(-)
>>  create mode 100644 tests/cdrom-test.c
>>
> 
> New file, but no edit to MAINTAINERS.

Which section do you suggest? It tests IDE CD-ROMs, SCSI CD-ROMs, and
even virtio-block (on s390x) ... so I have a hard time to decide where
this should belong to...

 Thomas

Re: [Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by John Snow 7 years, 6 months ago


On 04/02/2018 02:39 PM, Thomas Huth wrote:
> On 29.03.2018 20:28, John Snow wrote:
>>
>>
>> On 03/16/2018 01:39 AM, Thomas Huth wrote:
>>> With one of my clean-up patches (see commit 1454509726719e0933c800), I
>>> recently accidentially broke the "-cdrom" parameter (more precisely
>>> "-drive if=scsi") on a couple of boards, since there was no error
>>> detected during the "make check" regression testing. This is clearly an
>>> indication that we are lacking tests in this area.
>>> So this small patch series now introduces some tests for CD-ROM drives:
>>> The first two patches introduce the possibility to check that booting
>>> from CD-ROM drives still works fine for x86 and s390x, and the third
>>> patch adds a test that certain machines can at least still be started
>>> with the "-cdrom" parameter (i.e. that test would have catched the
>>> mistake that I did with my SCSI cleanup patch).
>>>
>>> v2:
>>>  - Use g_spawn_sync() instead of execlp() to run genisoimage
>>>  - The "-cdrom" parameter test is now run on all architectures (with
>>>    machine "none" for the machines that are not explicitly checked)
>>>  - Some rewordings and improved comments here and there
>>>
>>> Thomas Huth (3):
>>>   tests/boot-sector: Add magic bytes to s390x boot code header
>>>   tests/cdrom-test: Test booting from CD-ROM ISO image file
>>>   tests/cdrom-test: Test that -cdrom parameter is working
>>>
>>>  tests/Makefile.include |   2 +
>>>  tests/boot-sector.c    |   9 +-
>>>  tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 230 insertions(+), 3 deletions(-)
>>>  create mode 100644 tests/cdrom-test.c
>>>
>>
>> New file, but no edit to MAINTAINERS.
> 
> Which section do you suggest? It tests IDE CD-ROMs, SCSI CD-ROMs, and
> even virtio-block (on s390x) ... so I have a hard time to decide where
> this should belong to...
> 
>  Thomas
> 

I was hoping you'd figure it out, but fine :D

You can stick it under my section if you want, but I'll probably defer
to you if the s390x parts break.

--js

Re: [Qemu-devel] [PATCH v2 0/3] Add new CD-ROM related qtests

Posted by Thomas Huth 7 years, 6 months ago

On 02.04.2018 21:34, John Snow wrote:
> 
> 
> On 04/02/2018 02:39 PM, Thomas Huth wrote:
>> On 29.03.2018 20:28, John Snow wrote:
>>>
>>>
>>> On 03/16/2018 01:39 AM, Thomas Huth wrote:
>>>> With one of my clean-up patches (see commit 1454509726719e0933c800), I
>>>> recently accidentially broke the "-cdrom" parameter (more precisely
>>>> "-drive if=scsi") on a couple of boards, since there was no error
>>>> detected during the "make check" regression testing. This is clearly an
>>>> indication that we are lacking tests in this area.
>>>> So this small patch series now introduces some tests for CD-ROM drives:
>>>> The first two patches introduce the possibility to check that booting
>>>> from CD-ROM drives still works fine for x86 and s390x, and the third
>>>> patch adds a test that certain machines can at least still be started
>>>> with the "-cdrom" parameter (i.e. that test would have catched the
>>>> mistake that I did with my SCSI cleanup patch).
>>>>
>>>> v2:
>>>>  - Use g_spawn_sync() instead of execlp() to run genisoimage
>>>>  - The "-cdrom" parameter test is now run on all architectures (with
>>>>    machine "none" for the machines that are not explicitly checked)
>>>>  - Some rewordings and improved comments here and there
>>>>
>>>> Thomas Huth (3):
>>>>   tests/boot-sector: Add magic bytes to s390x boot code header
>>>>   tests/cdrom-test: Test booting from CD-ROM ISO image file
>>>>   tests/cdrom-test: Test that -cdrom parameter is working
>>>>
>>>>  tests/Makefile.include |   2 +
>>>>  tests/boot-sector.c    |   9 +-
>>>>  tests/cdrom-test.c     | 222 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 230 insertions(+), 3 deletions(-)
>>>>  create mode 100644 tests/cdrom-test.c
>>>>
>>>
>>> New file, but no edit to MAINTAINERS.
>>
>> Which section do you suggest? It tests IDE CD-ROMs, SCSI CD-ROMs, and
>> even virtio-block (on s390x) ... so I have a hard time to decide where
>> this should belong to...
>>
>>  Thomas
>>
> 
> I was hoping you'd figure it out, but fine :D
> 
> You can stick it under my section if you want, but I'll probably defer
> to you if the s390x parts break.

Ok, thanks. But in the long run, we might even need a generic "QTESTS"
section in the MAINTAINERS file, since most of the qtests are currently
not listed there.
And while we're at it, we should maybe also move the qtests to a
separate folder tests/qtests/ or so, so that it is clearer which of the
tests is a qtest and which are something different.
I'll try to come up with some patches once the hard freeze is over...

 Thomas