| Message ID | 20181229122027.42245-6-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | backup-top filter driver for backup | expand |
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)? Wouldn't it be better to let the user specify the breakpoint name? Max > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > 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 >
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? > > 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 <vsementsov@virtuozzo.com> >> --- >> 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 >> > >
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; 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 <vsementsov@virtuozzo.com> >>> --- >>> 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 >>> >> >> > >
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 <vsementsov@virtuozzo.com> >>>> --- >>>> 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 >>>> >>> >>> >> >> > >
23.01.2019 16:22, Vladimir Sementsov-Ogievskiy wrote: > 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. > Hmmm. Or, finally, may be better to teach bdrv_debug_breakpoint() and bdrv_debug_remove_breakpoint() to skip filters with backing child?
On 23.01.19 14:31, Vladimir Sementsov-Ogievskiy wrote: > 23.01.2019 16:22, Vladimir Sementsov-Ogievskiy wrote: >> 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. >> > > Hmmm. > > Or, finally, may be better to teach bdrv_debug_breakpoint() and > bdrv_debug_remove_breakpoint() to skip filters with backing child? I suppose that would be a question for my "Deal with filters" series. It does make sense to me, though. Max
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
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. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- tests/qemu-iotests/iotests.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)