Re: [Qemu-devel] [PATCH v3 3/3] blkdebug: Add latency injection rule type

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5522 bytes --]

On 12.02.19 22:21, Marc Olson wrote:
> On 1/11/19 7:00 AM, Max Reitz wrote:
>> On 12.11.18 08:06, Marc Olson wrote:

[...]

>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index d4fe710..72f7861 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3057,6 +3057,34 @@
>>>               '*immediately': 'bool' } }
>>>     ##
>>> +# @BlkdebugDelayOptions:
>>> +#
>>> +# Describes a single latency injection for blkdebug.
>>> +#
>>> +# @event:       trigger event
>>> +#
>>> +# @state:       the state identifier blkdebug needs to be in to
>>> +#               actually trigger the event; defaults to "any"
>>> +#
>>> +# @latency:     The delay to add to an I/O, in microseconds.
>>> +#
>>> +# @sector:      specifies the sector index which has to be affected
>>> +#               in order to actually trigger the event; defaults to
>>> "any
>>> +#               sector"
>>> +#
>>> +# @once:        disables further events after this one has been
>>> +#               triggered; defaults to false
>>> +#
>>> +# Since: 3.1
>> Well, 4.0 now, sorry...
> Baking version numbers into code is downright silly. Every single
> version of this patch has made a comment along the lines of, "oops, it
> didn't get reviewed in time for the next version bump, so you have to
> resubmit." With a fast moving project, this is nonsense. If you're
> looking at the code, you should have access to the git history as well
> to determine the version.

True, but these comments are used to generate documentation (e.g.
docs/interopt/qemu-qmp-ref.7 in the build directory).  So they are used
by people who don't have access to the git history.

It might be possible to generate that information from git-blame when
generating the documentation, but how would trivial fixes be handled
that are no functional changes?  For instance, it seems difficult to me
to distinguish between a spelling change for some parameter description
and a change in behavior.

(Then again, we shouldn't have such behavioral changes, hm.)

To me personally, the easiest thing would seem to be some convention to
write ***INSERT VERSION HERE*** into the code and then the maintainer
can just find an replace all instances of that when applying the
patches.  But that sounds a bit silly...

(I don't have an issue with adjusting the version numbers myself as they
are, but it's just as hard for me to find and replace all of them as it
is for you.  And since you're probably going to send a v4 anyway...)

In the end, it's up to the QAPI maintainers.

[...]

>>> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
>>> index 48b4955..976f747 100755
>>> --- a/tests/qemu-iotests/071
>>> +++ b/tests/qemu-iotests/071
>>> @@ -100,6 +100,69 @@ $QEMU_IO -c "open -o
>>> driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event
>>>            -c 'read -P 42 0x38000 512'
>>>     echo
>>> +echo "=== Testing blkdebug latency through filename ==="
>>> +echo
>>> +
>>> +$QEMU_IO -c "open -o
>>> file.driver=blkdebug,file.inject-delay.event=write_aio,file.inject-delay.latency=10000
>>> $TEST_IMG" \
>>> +         -c 'aio_write -P 42 0x28000 512' \
>>> +         -c 'aio_read -P 42 0x38000 512' \
>>> +         | _filter_qemu_io
>>> +
>>> +echo
>>> +echo "=== Testing blkdebug latency through file blockref ==="
>>> +echo
>>> +
>>> +$QEMU_IO -c "open -o
>>> driver=$IMGFMT,file.driver=blkdebug,file.inject-delay.event=write_aio,file.inject-delay.latency=10000,file.image.filename=$TEST_IMG"
>>> \
>>> +         -c 'aio_write -P 42 0x28000 512' \
>>> +         -c 'aio_read -P 42 0x38000 512' \
>>> +         | _filter_qemu_io
>>> +
>>> +# Using QMP is synchronous by default, so even though we would
>>> +# expect reordering due to using the aio_* commands, they are
>>> +# not. The purpose of this test is to verify that the driver
>>> +# can be setup via QMP, and IO can complete. See the qemu-io
>>> +# test above to prove delay functionality
>> But it doesn't prove that because the output is filtered.  To prove it,
>> you'd probably need to use null-co as the protocol (so there is as
>> little noise as possible) and then parse the qemu-io output to show that
>> the time is always above 10 ms.
>>
>> I leave it to you whether you'd like to go through that pain.
> There's not a great way to prove it without doing a lot of parsing
> changes in testing. I'll consider an update to this patch, but I think
> this series has carried on long enough.

I agree in this instance, but I'd like to note still that "this series
has carried on long enough" is not an argument to merge bad code (or
something incomplete without promise of a follow-up).  This is just a
test for blkdebug, though, so it's OK (with the comment fixed, because
it doesn't prove anything).

(I'm sorry for not having looked at this series for so long, but qemu is
not my own, so it isn't like I could pay for my wrong by accepting
something incomplete -- if it were more important than this single test
case.)

Also, we do support Python for iotests, and parsing should be simpler
there.  Since blkdebug is just a debugging driver, I'm fine with not
knowing whether its features work, though.

Maybe I'll write the test, that would kind of pay for my wrong...

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]