| Message ID | 20210621103337.36637-7-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu_iotests: improve debugging options | expand |
21.06.2021 13:33, Emanuele Giuseppe Esposito wrote: > Attaching gdbserver implies that the qmp socket > should wait indefinitely for an answer from QEMU. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Please, let this patch go without my r-b. I don't like it, I explained my thoughts, and you even used my suggested way in previous version. What I don't like: good component - Timeout class, which is currently independent of outer code and can be simply moved to another module becomes dependent on global variable, which doesn't relate to the class itself. Neither I like logic of the dependency which just make the whole class do nothing by skipping any action internally. So, IMHO Timeout class becomes worse. I'm not a maintainer here anyway, so my r-b isn't necessary :) > --- > tests/qemu-iotests/iotests.py | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index c86f239d81..e176a84620 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -477,10 +477,14 @@ def __init__(self, seconds, errmsg="Timeout"): > self.seconds = seconds > self.errmsg = errmsg > def __enter__(self): > + if qemu_gdb: > + return self > signal.signal(signal.SIGALRM, self.timeout) > signal.setitimer(signal.ITIMER_REAL, self.seconds) > return self > def __exit__(self, exc_type, value, traceback): > + if qemu_gdb: > + return False > signal.setitimer(signal.ITIMER_REAL, 0) > return False > def timeout(self, signum, frame): > @@ -575,7 +579,7 @@ class VM(qtest.QEMUQtestMachine): > > def __init__(self, path_suffix=''): > name = "qemu%s-%d" % (path_suffix, os.getpid()) > - timer = 15.0 > + timer = 15.0 if not qemu_gdb else None > super().__init__(qemu_prog, qemu_opts, name=name, > base_temp_dir=test_dir, > socket_scm_helper=socket_scm_helper, >
On 22/06/2021 12:50, Vladimir Sementsov-Ogievskiy wrote: > 21.06.2021 13:33, Emanuele Giuseppe Esposito wrote: >> Attaching gdbserver implies that the qmp socket >> should wait indefinitely for an answer from QEMU. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Please, let this patch go without my r-b. I don't like it, I explained > my thoughts, and you even used my suggested way in previous version. > > What I don't like: good component - Timeout class, which is currently > independent of outer code and can be simply moved to another module > becomes dependent on global variable, which doesn't relate to the class > itself. Neither I like logic of the dependency which just make the whole > class do nothing by skipping any action internally. So, IMHO Timeout > class becomes worse. > > I'm not a maintainer here anyway, so my r-b isn't necessary :) Oh okay. Sorry I understood from previous versions that you liked it even without your suggested change. Apologies. Emanuele > >> --- >> tests/qemu-iotests/iotests.py | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/tests/qemu-iotests/iotests.py >> b/tests/qemu-iotests/iotests.py >> index c86f239d81..e176a84620 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py >> @@ -477,10 +477,14 @@ def __init__(self, seconds, errmsg="Timeout"): >> self.seconds = seconds >> self.errmsg = errmsg >> def __enter__(self): >> + if qemu_gdb: >> + return self >> signal.signal(signal.SIGALRM, self.timeout) >> signal.setitimer(signal.ITIMER_REAL, self.seconds) >> return self >> def __exit__(self, exc_type, value, traceback): >> + if qemu_gdb: >> + return False >> signal.setitimer(signal.ITIMER_REAL, 0) >> return False >> def timeout(self, signum, frame): >> @@ -575,7 +579,7 @@ class VM(qtest.QEMUQtestMachine): >> def __init__(self, path_suffix=''): >> name = "qemu%s-%d" % (path_suffix, os.getpid()) >> - timer = 15.0 >> + timer = 15.0 if not qemu_gdb else None >> super().__init__(qemu_prog, qemu_opts, name=name, >> base_temp_dir=test_dir, >> socket_scm_helper=socket_scm_helper, >> > >
22.06.2021 14:04, Emanuele Giuseppe Esposito wrote: > > > On 22/06/2021 12:50, Vladimir Sementsov-Ogievskiy wrote: >> 21.06.2021 13:33, Emanuele Giuseppe Esposito wrote: >>> Attaching gdbserver implies that the qmp socket >>> should wait indefinitely for an answer from QEMU. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> >> Please, let this patch go without my r-b. I don't like it, I explained my thoughts, and you even used my suggested way in previous version. >> >> What I don't like: good component - Timeout class, which is currently independent of outer code and can be simply moved to another module becomes dependent on global variable, which doesn't relate to the class itself. Neither I like logic of the dependency which just make the whole class do nothing by skipping any action internally. So, IMHO Timeout class becomes worse. >> >> I'm not a maintainer here anyway, so my r-b isn't necessary :) > > Oh okay. Sorry I understood from previous versions that you liked it even without your suggested change. Apologies. > No problem. I really gave an r-b to earlier version, so formally you had it until now :) I thought I was explicit enough in my last comment to this patch in v5. apologies if not(
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index c86f239d81..e176a84620 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -477,10 +477,14 @@ def __init__(self, seconds, errmsg="Timeout"): self.seconds = seconds self.errmsg = errmsg def __enter__(self): + if qemu_gdb: + return self signal.signal(signal.SIGALRM, self.timeout) signal.setitimer(signal.ITIMER_REAL, self.seconds) return self def __exit__(self, exc_type, value, traceback): + if qemu_gdb: + return False signal.setitimer(signal.ITIMER_REAL, 0) return False def timeout(self, signum, frame): @@ -575,7 +579,7 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) - timer = 15.0 + timer = 15.0 if not qemu_gdb else None super().__init__(qemu_prog, qemu_opts, name=name, base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper,