Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete
On 19.09.19 19:02, John Snow wrote:
>
>
> 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
---
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.
I have to respin anyway. ;-)
+
+ 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.
The point is that they’re special language constructs whereas map/filter
are available in basically any decent language.
>> For the tuple: This is a test, nobody cares whether it uses 60 bytes
>> more memory or is 10 µs slower or lets so
Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete
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
>>> ---
>>> 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!
Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete
On 18.09.19 20:46, John Snow wrote:
>
>
> On 9/12/19 9:56 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz
>> ---
>> 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.
>> +
>> + 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.
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.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete
On 18.09.19 18:30, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 16:56, Max Reitz wrote:
>> Signed-off-by: Max Reitz
>> ---
>> 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))
>
> using list comprehension is a bit more pythonic:
> nodes = [node['node-name'] for node in nodes]
OK. I can never remember, so I rarely bother using list/dict
comprehensions and instead use what I would do in any other language.
(Hence the “Sadly” from John.)
(And then wait for some kind reviewer to tell me how to do it right. :-))
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete
On 9/12/19 9:56 AM, Max Reitz wrote:
Signed-off-by: Max Reitz
---
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.
+
+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?"
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.
Reviewed-by: John Snow
Re: [Qemu-block] [Qemu-devel] [PATCH 4/4] iotests: Add test for failing mirror complete
12.09.2019 16:56, Max Reitz wrote:
> Signed-off-by: Max Reitz
> ---
> 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))
using list comprehension is a bit more pythonic:
nodes = [node['node-name'] for node in nodes]
> +
> +for expect in ['src', 'src-perm', 'dest', 'dest-perm']:
> +self.assertTrue(expect in nodes, '%s disappeared' % expect)
> +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
>
With or without my suggestion:
Reviewed-by: Vladimir Sementsov-Ogievskiy
I checked, that it pass, and that fails (generates segfault) if drop patch 01:
Tested-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
