Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete

[John Snow]

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 <address@hidden>
>>> ---
>>>   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!