07.08.2017 17:29, Eric Blake wrote:
On 08/07/2017 09:16 AM, Vladimir Sementsov-Ogievskiy wrote:
185 iotest is broken.
How to test:
i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \
done; echo N = $i
finished for me like this:
185 2s ... - output mismatch (see 185.out.bad)
--- /work/src/qemu/master/tests/qemu-iotests/185.out 2017-07-14 \
15:14:29.520343805 +0300
+++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
@@ -37,7 +37,7 @@
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, \
"event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, \
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
"len": 4194304, "offset": 4194304, "speed": 65536, "type": \
"mirror"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, \
"event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
"len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}
This diff says both 'len' and 'offset' differ...
=== Start backup job and exit qemu ===
Failures: 185
Failed 1 of 1 tests
N = 8
It doesn't seems logical to expect the constant offset on cancel,
so let filter it out.
s/let/let's/
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
tests/qemu-iotests/185 | 2 +-
tests/qemu-iotests/185.out | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk",
"len": 4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk",
"len": 4194304, "offset": OFFSET, "speed": 65536, "type": "mirror"}}
...while this diff only touched 'offset'. Did you copy-and-paste
incorrectly in the commit message? If so, then with the commit message
fixed, I'm okay with:
Reviewed-by: Eric Blake <ebl...@redhat.com>
Hmm, I can't reproduce with "len = 0", and it looks impossible. But I've
scrolled up in one of my terminals and found this reproduce, really,
'"len": 0', so, that is possible.
And, after looking through the code it looks like it really possible -
s->common.len is set not in the very beginning, but after for ex. call
to block_job_pause_point().
So, the reproduction that I've copied into commit message was very good
match.
But, I'm not sure, that len should be fixed in the same way. May be it
would be better to set earlier and don't update on each iteration (why
is it updated??)
--
Best regards,
Vladimir
