[PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Vladimir Sementsov-Ogievskiy]

strerror() represents ETIMEDOUT a bit different in Linux and macOS /
FreeBSD. Let's support the latter too.

Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
Here is a fix. Would be great if someone can test it.

I tried to push it by

  git push --force  -o ci.variable="QEMU_CI=1"

to my block branch, I get a blocked pipeline
  https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
but it doesn't have neither freebsd nor macos jobs.. How to get them?

 tests/qemu-iotests/tests/copy-before-write | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
index 16efebbf8f..56937b9dff 100755
--- a/tests/qemu-iotests/tests/copy-before-write
+++ b/tests/qemu-iotests/tests/copy-before-write
@@ -192,6 +192,11 @@ read 1048576/1048576 bytes at offset 0
 
     def test_timeout_break_guest(self):
         log = self.do_cbw_timeout('break-guest-write')
+        # macOS and FreeBSD tend to represent ETIMEDOUT as
+        # "Operation timed out", when Linux prefer
+        # "Connection timed out"
+        log = log.replace('Operation timed out',
+                          'Connection timed out')
         self.assertEqual(log, """\
 wrote 524288/524288 bytes at offset 0
 512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.25.1

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Richard Henderson]

On 7/5/22 21:07, Vladimir Sementsov-Ogievskiy wrote:
> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
> FreeBSD. Let's support the latter too.
> 
> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
> Here is a fix. Would be great if someone can test it.
> 
> I tried to push it by
> 
>    git push --force  -o ci.variable="QEMU_CI=1"
> 
> to my block branch, I get a blocked pipeline
>    https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
> but it doesn't have neither freebsd nor macos jobs.. How to get them?

You'd have to have an attached cirrus token.
Better to just use 'make vm-build-freebsd'.

>       def test_timeout_break_guest(self):
>           log = self.do_cbw_timeout('break-guest-write')
> +        # macOS and FreeBSD tend to represent ETIMEDOUT as
> +        # "Operation timed out", when Linux prefer
> +        # "Connection timed out"
> +        log = log.replace('Operation timed out',
> +                          'Connection timed out')

Um, really?  Matching strerror text?  This is ultra-fragile.
Dare I say broken already.


r~

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Vladimir Sementsov-Ogievskiy]

On 7/5/22 20:22, Richard Henderson wrote:
> On 7/5/22 21:07, Vladimir Sementsov-Ogievskiy wrote:
>> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
>> FreeBSD. Let's support the latter too.
>>
>> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>
>> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
>> Here is a fix. Would be great if someone can test it.
>>
>> I tried to push it by
>>
>>    git push --force  -o ci.variable="QEMU_CI=1"
>>
>> to my block branch, I get a blocked pipeline
>>    https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
>> but it doesn't have neither freebsd nor macos jobs.. How to get them?
> 
> You'd have to have an attached cirrus token.
> Better to just use 'make vm-build-freebsd'.
> 
>>       def test_timeout_break_guest(self):
>>           log = self.do_cbw_timeout('break-guest-write')
>> +        # macOS and FreeBSD tend to represent ETIMEDOUT as
>> +        # "Operation timed out", when Linux prefer
>> +        # "Connection timed out"
>> +        log = log.replace('Operation timed out',
>> +                          'Connection timed out')
> 
> Um, really?  Matching strerror text?  This is ultra-fragile.
> Dare I say broken already.
> 

Unfortunately we lack a good interface to test simple reads and writes in iotests. qemu-io command just print the result to stdout. So, I think, in short-term, this is the best thing to do. And if just `git grep '\(write\|read\) failed:' tests/qemu-iotests/` we'll see that this is the common practice.

-- 
Best regards,
Vladimir

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Thomas Huth]

On 05/07/2022 17.37, Vladimir Sementsov-Ogievskiy wrote:
> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
> FreeBSD. Let's support the latter too.
> 
> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
> Here is a fix. Would be great if someone can test it.

Thanks, seems to work now:

  https://gitlab.com/thuth/qemu/-/jobs/2683487160#L3256
  https://gitlab.com/thuth/qemu/-/jobs/2683487162#L2897

Tested-by: Thomas Huth <thuth@redhat.com>


> I tried to push it by
> 
>    git push --force  -o ci.variable="QEMU_CI=1"
> 
> to my block branch, I get a blocked pipeline
>    https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
> but it doesn't have neither freebsd nor macos jobs.. How to get them?

The instructions are a little bit hidden - you can find them in the 
.gitlab-ci.d/cirrus/README.rst file in your git checkout.

  HTH,
   Thomas

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Thomas Huth]

On 05/07/2022 19.22, Richard Henderson wrote:
> On 7/5/22 21:07, Vladimir Sementsov-Ogievskiy wrote:
>> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
>> FreeBSD. Let's support the latter too.
>>
>> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout 
>> option")
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>
>> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
>> Here is a fix. Would be great if someone can test it.
>>
>> I tried to push it by
>>
>>    git push --force  -o ci.variable="QEMU_CI=1"
>>
>> to my block branch, I get a blocked pipeline
>>    https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
>> but it doesn't have neither freebsd nor macos jobs.. How to get them?
> 
> You'd have to have an attached cirrus token.
> Better to just use 'make vm-build-freebsd'.
> 
>>       def test_timeout_break_guest(self):
>>           log = self.do_cbw_timeout('break-guest-write')
>> +        # macOS and FreeBSD tend to represent ETIMEDOUT as
>> +        # "Operation timed out", when Linux prefer
>> +        # "Connection timed out"
>> +        log = log.replace('Operation timed out',
>> +                          'Connection timed out')
> 
> Um, really?  Matching strerror text?  This is ultra-fragile.
> Dare I say broken already.

Many of the iotests rely on output text matching. It's very fragile, always 
has been and always will be (unless we rewrite the whole test suite to not 
use output text matching anymore). For example, the iotests also do not work 
with the libc from Alpine Linux (where one of the error messages is "I/O 
error" instead of "Input/Output error"), so we only run check-unit and 
check-qtest in the CI there. It's a pity, but that's the way it is 
currently. I think it's still better to tweak some of the text strings here 
instead of not running the tests at all.

  Thomas

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Peter Maydell]

On Wed, 6 Jul 2022 at 08:39, Thomas Huth <thuth@redhat.com> wrote:
> Many of the iotests rely on output text matching. It's very fragile, always
> has been and always will be (unless we rewrite the whole test suite to not
> use output text matching anymore).

Maybe you could have a pre-pass over the "expected results" files
that substituted in the strerror text for the host it was running on.
That's probably a reasonably doable amount of work compared to
a complete testsuite rewrite.

-- PMM

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Hanna Reitz]

On 05.07.22 17:37, Vladimir Sementsov-Ogievskiy wrote:
> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
> FreeBSD. Let's support the latter too.
>
> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>
> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
> Here is a fix. Would be great if someone can test it.
>
> I tried to push it by
>
>    git push --force  -o ci.variable="QEMU_CI=1"
>
> to my block branch, I get a blocked pipeline
>    https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
> but it doesn't have neither freebsd nor macos jobs.. How to get them?
>
>   tests/qemu-iotests/tests/copy-before-write | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
> index 16efebbf8f..56937b9dff 100755
> --- a/tests/qemu-iotests/tests/copy-before-write
> +++ b/tests/qemu-iotests/tests/copy-before-write
> @@ -192,6 +192,11 @@ read 1048576/1048576 bytes at offset 0
>   
>       def test_timeout_break_guest(self):
>           log = self.do_cbw_timeout('break-guest-write')
> +        # macOS and FreeBSD tend to represent ETIMEDOUT as
> +        # "Operation timed out", when Linux prefer
> +        # "Connection timed out"
> +        log = log.replace('Operation timed out',
> +                          'Connection timed out')

If we know for sure that it’s ETIMEDOUT, how about 
os.strerror(errno.ETIMEDOUT)?

Hanna

>           self.assertEqual(log, """\
>   wrote 524288/524288 bytes at offset 0
>   512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Vladimir Sementsov-Ogievskiy]

On 7/6/22 13:26, Hanna Reitz wrote:
> On 05.07.22 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
>> FreeBSD. Let's support the latter too.
>>
>> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>
>> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
>> Here is a fix. Would be great if someone can test it.
>>
>> I tried to push it by
>>
>>    git push --force  -o ci.variable="QEMU_CI=1"
>>
>> to my block branch, I get a blocked pipeline
>>    https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
>> but it doesn't have neither freebsd nor macos jobs.. How to get them?
>>
>>   tests/qemu-iotests/tests/copy-before-write | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
>> index 16efebbf8f..56937b9dff 100755
>> --- a/tests/qemu-iotests/tests/copy-before-write
>> +++ b/tests/qemu-iotests/tests/copy-before-write
>> @@ -192,6 +192,11 @@ read 1048576/1048576 bytes at offset 0
>>       def test_timeout_break_guest(self):
>>           log = self.do_cbw_timeout('break-guest-write')
>> +        # macOS and FreeBSD tend to represent ETIMEDOUT as
>> +        # "Operation timed out", when Linux prefer
>> +        # "Connection timed out"
>> +        log = log.replace('Operation timed out',
>> +                          'Connection timed out')
> 
> If we know for sure that it’s ETIMEDOUT, how about os.strerror(errno.ETIMEDOUT)?
> 

Good idea, will try.

-- 
Best regards,
Vladimir

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Vladimir Sementsov-Ogievskiy]

On 7/6/22 13:26, Hanna Reitz wrote:
> On 05.07.22 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
>> FreeBSD. Let's support the latter too.
>>
>> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>
>> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
>> Here is a fix. Would be great if someone can test it.
>>
>> I tried to push it by
>>
>>    git push --force  -o ci.variable="QEMU_CI=1"
>>
>> to my block branch, I get a blocked pipeline
>> https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
>> but it doesn't have neither freebsd nor macos jobs.. How to get them?
>>
>>   tests/qemu-iotests/tests/copy-before-write | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
>> index 16efebbf8f..56937b9dff 100755
>> --- a/tests/qemu-iotests/tests/copy-before-write
>> +++ b/tests/qemu-iotests/tests/copy-before-write
>> @@ -192,6 +192,11 @@ read 1048576/1048576 bytes at offset 0
>>       def test_timeout_break_guest(self):
>>           log = self.do_cbw_timeout('break-guest-write')
>> +        # macOS and FreeBSD tend to represent ETIMEDOUT as
>> +        # "Operation timed out", when Linux prefer
>> +        # "Connection timed out"
>> +        log = log.replace('Operation timed out',
>> +                          'Connection timed out')
> 
> If we know for sure that it’s ETIMEDOUT, how about os.strerror(errno.ETIMEDOUT)?

I've checked this with make vm-build-freebsd, but it doesn't work:

--- /home/qemu/qemu-test.fxwm16/src/tests/qemu-iotests/tests/copy-before-write.out
+++ /usr/home/qemu/qemu-test.fxwm16/build/tests/qemu-iotests/scratch/copy-before-write.out.bad
@@ -1,5 +1,22 @@
-....
+..F.
+======================================================================
+FAIL: test_timeout_break_guest (__main__.TestCbwError)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "/usr/home/qemu/qemu-test.fxwm16/src/tests/qemu-iotests/tests/copy-before-write", line 207, in test_timeout_break_guest
+    """)
+AssertionError: 'wrot[102 chars]led: Connection timed out\nread 1048576/104857[73 chars]c)\n' != 'wrot[102 chars]led: Operation timed out\nread 1048576/1048576[72 chars]c)\n'
+  wrote 524288/524288 bytes at offset 0
+  512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+- write failed: Connection timed out
+?               ^^^^ ^
++ write failed: Operation timed out
+?               ^^ ^^
+  read 1048576/1048576 bytes at offset 0
+  1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
  ----------------------------------------------------------------------
  Ran 4 tests


Probably pythonic os.strerror doesn't correspond to what we have in C..

So, let's just go with my fix.

-- 
Best regards,
Vladimir

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Hanna Reitz]

On 06.07.22 16:46, Vladimir Sementsov-Ogievskiy wrote:
> On 7/6/22 13:26, Hanna Reitz wrote:
>> On 05.07.22 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
>>> FreeBSD. Let's support the latter too.
>>>
>>> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for 
>>> cbw-timeout option")
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>
>>> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
>>> Here is a fix. Would be great if someone can test it.
>>>
>>> I tried to push it by
>>>
>>>    git push --force  -o ci.variable="QEMU_CI=1"
>>>
>>> to my block branch, I get a blocked pipeline
>>> https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
>>> but it doesn't have neither freebsd nor macos jobs.. How to get them?
>>>
>>>   tests/qemu-iotests/tests/copy-before-write | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/tests/copy-before-write 
>>> b/tests/qemu-iotests/tests/copy-before-write
>>> index 16efebbf8f..56937b9dff 100755
>>> --- a/tests/qemu-iotests/tests/copy-before-write
>>> +++ b/tests/qemu-iotests/tests/copy-before-write
>>> @@ -192,6 +192,11 @@ read 1048576/1048576 bytes at offset 0
>>>       def test_timeout_break_guest(self):
>>>           log = self.do_cbw_timeout('break-guest-write')
>>> +        # macOS and FreeBSD tend to represent ETIMEDOUT as
>>> +        # "Operation timed out", when Linux prefer
>>> +        # "Connection timed out"
>>> +        log = log.replace('Operation timed out',
>>> +                          'Connection timed out')
>>
>> If we know for sure that it’s ETIMEDOUT, how about 
>> os.strerror(errno.ETIMEDOUT)?
>
> I've checked this with make vm-build-freebsd, but it doesn't work:
>
> --- 
> /home/qemu/qemu-test.fxwm16/src/tests/qemu-iotests/tests/copy-before-write.out
> +++ 
> /usr/home/qemu/qemu-test.fxwm16/build/tests/qemu-iotests/scratch/copy-before-write.out.bad
> @@ -1,5 +1,22 @@
> -....
> +..F.
> +======================================================================
> +FAIL: test_timeout_break_guest (__main__.TestCbwError)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File 
> "/usr/home/qemu/qemu-test.fxwm16/src/tests/qemu-iotests/tests/copy-before-write", 
> line 207, in test_timeout_break_guest
> +    """)
> +AssertionError: 'wrot[102 chars]led: Connection timed out\nread 
> 1048576/104857[73 chars]c)\n' != 'wrot[102 chars]led: Operation timed 
> out\nread 1048576/1048576[72 chars]c)\n'
> +  wrote 524288/524288 bytes at offset 0
> +  512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +- write failed: Connection timed out
> +?               ^^^^ ^
> ++ write failed: Operation timed out
> +?               ^^ ^^
> +  read 1048576/1048576 bytes at offset 0
> +  1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +
>  ----------------------------------------------------------------------
>  Ran 4 tests
>

:(

> Probably pythonic os.strerror doesn't correspond to what we have in C..

Great...

> So, let's just go with my fix.

Sounds good, then.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH] iotests: fix copy-before-write for macOS and FreeBSD

[Richard Henderson]

On 7/5/22 21:07, Vladimir Sementsov-Ogievskiy wrote:
> strerror() represents ETIMEDOUT a bit different in Linux and macOS /
> FreeBSD. Let's support the latter too.
> 
> Fixes: 9d05a87b77 ("iotests: copy-before-write: add cases for cbw-timeout option")
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> As John and Thomas noted, the new iotests fails for FreeBSD and maxOS.
> Here is a fix. Would be great if someone can test it.
> 
> I tried to push it by
> 
>    git push --force  -o ci.variable="QEMU_CI=1"
> 
> to my block branch, I get a blocked pipeline
>    https://gitlab.com/vsementsov/qemu/-/pipelines/580573238
> but it doesn't have neither freebsd nor macos jobs.. How to get them?
> 
>   tests/qemu-iotests/tests/copy-before-write | 5 +++++
>   1 file changed, 5 insertions(+)

I am going to apply this as a hot fix, trying to re-green the CI.


r~