On 13.02.19 21:49, Marc Olson wrote: > On 2/13/19 7:48 AM, Max Reitz wrote: >> 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. > > IFF I submit by the end of the week, what version number shall I choose?
4.0 is still the next one. >> [...] >> >>>>> 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 wasn't implying that it's ok to merge bad code, but that at some point > we have to just paint the shed a color and move on. At the risk of > excess drama, at what point does a small addition become a complete > tear-down and rebuild? Depends on the case whether a tear down is necessary. :-) >> (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... > > I think the real fix is to make QMP support async IO, but if you'd like > to do more parsing, that's fine too. Hm, yeah, that would work, too. But that's definitely more complicated than a bit of parsing... Max
signature.asc
Description: OpenPGP digital signature
