On 26.02.20 18:58, John Snow wrote: > > > On 2/26/20 6:18 AM, Max Reitz wrote: >> On 26.02.20 01:44, John Snow wrote: >>> The idea is that instead of increasing the arguments to job_run all the >>> time, create a more general-purpose job runner that can be subclassed to >>> do interesting things with. >>> >>> Signed-off-by: John Snow <js...@redhat.com> >>> --- >>> tests/qemu-iotests/255 | 9 +- >>> tests/qemu-iotests/257 | 12 ++- >>> tests/qemu-iotests/287 | 19 +++- >>> tests/qemu-iotests/iotests.py | 176 ++++++++++++++++++++++++---------- >>> 4 files changed, 158 insertions(+), 58 deletions(-) >> >> I like it! >> > > High praise! > >> [...] >> >>> diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287 >>> index 0ab58dc011..f06e6ff084 100755 >>> --- a/tests/qemu-iotests/287 >>> +++ b/tests/qemu-iotests/287 >>> @@ -165,13 +165,22 @@ def test_bitmap_populate(config): >>> if not config.disabled: >>> ebitmap.dirty_group(2) >>> >>> + >>> + class TestJobRunner(iotests.JobRunner): >>> + def on_pending(self, event): >>> + if config.mid_writes: >>> + perform_writes(drive0, 2) >>> + if not config.disabled: >>> + ebitmap.dirty_group(2) >> >> I actually prefer inlining the pre_finalize() functions (over calling >> the existing one), but then we can also remove the original function. :) >> > > Not sure I understand you correctly. You're saying you prefer this > strategy where I inline the logic vs others where I call out to a function?
Yes.
> If so, I agree if only for purity -- the function looks and acts like a
> callback instead of a callback-that-calls-another-callback.
That was my thinking.
>>> + super().on_pending(event)
>>> +
>>> job = populate(drive0, 'target', 'bitpop0')
>>> assert job['return'] == {'return': {}}
>>> - vm.run_job(job['id'],
>>> - auto_dismiss=job['auto-dismiss'],
>>> - auto_finalize=job['auto-finalize'],
>>> - pre_finalize=pre_finalize,
>>> - cancel=config.cancel)
>>> + job_runner = TestJobRunner(vm, job['id'],
>>> + auto_dismiss=job['auto-dismiss'],
>>> + auto_finalize=job['auto-finalize'],
>>> + cancel=config.cancel)
>>> + job_runner.run()
>>> log('')
>>>
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3390fab021..37a8b4d649 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -460,6 +460,130 @@ def remote_filename(path):
>>> else:
>>> raise Exception("Protocol %s not supported" % (imgproto))
>>>
>>> +
>>> +class JobRunner:
>>
>> [...]
>>
>>> + def on_ready(self, event):
>>> + if self.logging:
>>> + self._vm.qmp_log('job-complete', id=self._id)
>>> + else:
>>> + self._vm.qmp('job-complete', id=self._id)
>>
>> I suppose this is a bug fix. (The old version always called qmp_log.)
>>
>
> Technically yes. It was needed for 040.
>
>> But what about adding a do_qmp method to JobRunner that does the
>> “if self.logging { self._vm.qmp_log() } else { self._vm.qmp }” part so
>> we don’t have to inline that everywhere?
>>
>> Max
>>
>
> I'll just clean up the logging series I had to do it at a more
> fundamental level.
OK. So you’re looking to basically get VM.qmp() to log automatically if
necessary? (or maybe qmp_log() to not log unless necessary)
Max
signature.asc
Description: OpenPGP digital signature
