Re: [Qemu-devel] [PATCH v5 05/11] iotests: allow resume_drive by node name

[Vladimir Sementsov-Ogievskiy]

16.01.2019 16:11, Max Reitz wrote:
> On 14.01.19 17:06, Vladimir Sementsov-Ogievskiy wrote:
>> 14.01.2019 17:46, Max Reitz wrote:
>>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>>> After node graph changes, we may not be able to resume_drive by device
>>>> name (backing files are not recursively searched). So, lets allow to
>>>> resume by node-name. Set constant name for breakpoints, to avoid
>>>> introducing extra parameters.
>>>
>>> Hm, I don't quite understand this reason.  Is this so you can create
>>> breakpoints on one node (which falls through to the first blkdebug node)
>>> and then remove them from another (falling through to the same blkdebug
>>> node)?
>>
>> add/remove breakpoint goes through ->file children, but my filter links
>> active disk as ->backing. So, before block-job start we can insert breakpoint
>> by device name. But then, when filter inserted, we can't remove breakpoint,
>> because my filter hides blkdebug with active disk under ->backing link.
>>
>> Maybe, right solution would be support backing links in 
>> bdrv_debug_breakpoint()
>> and bdrv_debug_remove_breakpoint() for the case when there is no file child.
>>
>> But being unsure about right behavior, I've decided to adjust the test.
>>
>> What about just do both  add/remove breakpoint through blkdebug node-name, to
>> make it less weird?
> 
> Yes, I was mostly wondering about the "set constant name for
> breakpoints".  It doesn't seem necessary to me;

It's just to not create extra parameters or something. Current code don't allow
to create several breakpoints in one blkdebug anyway (as @drive is used as
breakpoint name), so there is no reason for different names at all.

I'll improve commit message to make it clean.

if you use node names,
> you could address the blkdebug node directly, I would think.
> 
> Max
> 
>>> Wouldn't it be better to let the user specify the breakpoint name?
>>
>> it's not needed now. with current naming we can have one break-point in 
>> device,
>> so we don't need different names.
>>
>>>
>>> Max
>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> ---
>>>>    tests/qemu-iotests/iotests.py | 9 +++++----
>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>> index 76877ad584..c9779f432f 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
>>>> @@ -415,11 +415,11 @@ class VM(qtest.QEMUQtestMachine):
>>>>                self.pause_drive(drive, "write_aio")
>>>>                return
>>>>            self.qmp('human-monitor-command',
>>>> -                    command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>>>> event, drive))
>>>> +                    command_line='qemu-io %s "break %s bp_0"' % (drive, 
>>>> event))
>>>>    
>>>>        def resume_drive(self, drive):
>>>>            self.qmp('human-monitor-command',
>>>> -                    command_line='qemu-io %s "remove_break bp_%s"' % 
>>>> (drive, drive))
>>>> +                    command_line='qemu-io %s "remove_break bp_0"' % 
>>>> (drive))
>>>>    
>>>>        def hmp_qemu_io(self, drive, cmd):
>>>>            '''Write to a given drive using an HMP command'''
>>>> @@ -543,13 +543,14 @@ class QMPTestCase(unittest.TestCase):
>>>>            
>>>> self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
>>>>                             self.vm.flatten_qmp_object(reference))
>>>>    
>>>> -    def cancel_and_wait(self, drive='drive0', force=False, resume=False):
>>>> +    def cancel_and_wait(self, drive='drive0', force=False, resume=False,
>>>> +                        resume_node=None):
>>>>            '''Cancel a block job and wait for it to finish, returning the 
>>>> event'''
>>>>            result = self.vm.qmp('block-job-cancel', device=drive, 
>>>> force=force)
>>>>            self.assert_qmp(result, 'return', {})
>>>>    
>>>>            if resume:
>>>> -            self.vm.resume_drive(drive)
>>>> +            self.vm.resume_drive(resume_node or drive)
>>>>    
>>>>            cancelled = False
>>>>            result = None
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir