On 9/19/19 12:58 PM, Max Reitz wrote:
> On 18.09.19 20:46, John Snow wrote:
>>
>>
>> On 9/12/19 9:56 AM, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mre...@redhat.com>
>>> ---
>>> tests/qemu-iotests/041 | 44 ++++++++++++++++++++++++++++++++++++++
>>> tests/qemu-iotests/041.out | 4 ++--
>>> 2 files changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
>>> index 8568426311..84bc6d6581 100755
>>> --- a/tests/qemu-iotests/041
>>> +++ b/tests/qemu-iotests/041
>>> @@ -1121,6 +1121,50 @@ class TestOrphanedSource(iotests.QMPTestCase):
>>> target='dest-ro')
>>> self.assert_qmp(result, 'error/class', 'GenericError')
>>> + def test_failing_permission_in_complete(self):
>>> + self.assert_no_active_block_jobs()
>>> +
>>> + # Unshare consistent-read on the target
>>> + # (The mirror job does not care)
>>> + result = self.vm.qmp('blockdev-add',
>>> + driver='blkdebug',
>>> + node_name='dest-perm',
>>> + image='dest',
>>> + unshare_child_perms=['consistent-read'])
>>> + self.assert_qmp(result, 'return', {})
>>> +
>>> + result = self.vm.qmp('blockdev-mirror', job_id='job',
>>> device='src',
>>> + sync='full', target='dest',
>>> + filter_node_name='mirror-filter')
>>> + self.assert_qmp(result, 'return', {})
>>> +
>>> + # Require consistent-read on the source
>>> + # (We can only add this node once the job has started, or it
>>> + # will complain that it does not want to run on non-root nodes)
>>> + result = self.vm.qmp('blockdev-add',
>>> + driver='blkdebug',
>>> + node_name='src-perm',
>>> + image='src',
>>> + take_child_perms=['consistent-read'])
>>> + self.assert_qmp(result, 'return', {})
>>> +
>>> + # While completing, mirror will attempt to replace src by
>>> + # dest, which must fail because src-perm requires
>>> + # consistent-read but dest-perm does not share it; thus
>>> + # aborting the job when it is supposed to complete
>>> + self.complete_and_wait('job',
>>> + completion_error='Operation not
>>> permitted')
>>> +
>>> + # Assert that all of our nodes are still there (except for the
>>> + # mirror filter, which should be gone despite the failure)
>>> + nodes = self.vm.qmp('query-named-block-nodes')['return']
>>> + nodes = list(map(lambda image: image['node-name'], nodes))
>>
>> Sadly, the list comprehension is a good suggestion. Squash it in if
>> you'd like.
>
> You know I don’t, but I’ll do it anyway.
>
Don't you dare make me feel bad by re-spinning, though.
>>> +
>>> + for expect in ['src', 'src-perm', 'dest', 'dest-perm']:
>>> + self.assertTrue(expect in nodes, '%s disappeared' % expect)
>>
>> I could be a real weenie and say "why not use a tuple here?"
>
> OK.
>
>> but, I'll only pretend I didn't say that instead of couching it in a
>> self-deprecating wrapper to the same end effect.
>>
>> (I'm a weenie.)
>>
>>> + self.assertFalse('mirror-filter' in nodes,
>>> + 'Mirror filter node did not disappear')
>>> +
>>> if __name__ == '__main__':
>>> iotests.main(supported_fmts=['qcow2', 'qed'],
>>> supported_protocols=['file'])
>>> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
>>> index 2c448b4239..f496be9197 100644
>>> --- a/tests/qemu-iotests/041.out
>>> +++ b/tests/qemu-iotests/041.out
>>> @@ -1,5 +1,5 @@
>>> -..........................................................................................
>>>
>>> +...........................................................................................
>>>
>>> ----------------------------------------------------------------------
>>> -Ran 90 tests
>>> +Ran 91 tests
>>> OK
>>>
>>
>> Or don't do anything, because I'm just being obnoxious, and I owe you
>> one for giving you bad advice last round.
>
> Come on, I have better (more selfish) reasons.
>
> For the list comprehension: I want to introduce as many instances of
> map/filter as I can so using those functions becomes normal.
>
They have their uses! But also they're usually just simply longer and
aren't worth using where list comprehensions do.
> For the tuple: This is a test, nobody cares whether it uses 60 bytes
> more memory or is 10 µs slower or lets something be mutable when it is
> actually never changed. As such, I like to use the most general
> solution which is simply a list.
>
I did say I was being a weenie. You really can take the reviewed-by!