On 3/18/24 09:38, Daniel Ding wrote: > > >> 2024年3月16日 上午9:17，Ilya Maximets <[email protected] >> <mailto:[email protected]>> 写道： >> >> On 2/23/24 04:37, Daniel Ding wrote: >>> After running ovs-tcpdump and inputs multiple CTRL+C, the program will >>> raise the following exception. >>> >>> Error in atexit._run_exitfuncs: >>> Traceback (most recent call last): >>> File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror >>> ovsdb = OVSDB(db_sock) >>> File "/usr/bin/ovs-tcpdump", line 168, in __init__ >>> OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB >>> File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change >>> while idl.change_seqno == seq and not idl.run(): >>> >>> The default handler of SIGINT is default_int_handler, so it not >>> register the signal handler successfully. When received CTRL+C again, >>> the program be break, and calling hook can't execute completely. >>> >>> Signed-off-by: Daniel Ding <[email protected] >>> <mailto:[email protected]>> >>> --- >>> python/ovs/fatal_signal.py | 24 +++++++++++++----------- >>> utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> | 38 >>> +++++++++++++++++--------------------- >>> 2 files changed, 30 insertions(+), 32 deletions(-) >> >> Sorry for the late response. Thanks for v2! >> >> I did some testing and it seem to fix the problem, though in case the ovsdb >> connection is no longer available for some reason, we would block forever in >> the handler waiting for the server to reply while the signals are blocked. >> So, it's getting harder to kill the ovs-tcpdump in this situation. But I >> don't really have a solution for this. We will have either this problem or >> lingering ports and mirrors as long as we're trying to communicate with the >> external process in the signal handler. >> >> I think, it's an OK trade-off to make. Double Ctrl+C seems like a more >> frequent event than paused or stuck ovsdb-server, so I think it's OK to have >> this patch. >> >> See a few minor comments below. >> > > Please help me review the following patch again, Thanks! > >> Best regards, Ilya Maximets. >> >>> >>> diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py >>> index cb2e99e87..077f50dd5 100644 >>> --- a/python/ovs/fatal_signal.py >>> +++ b/python/ovs/fatal_signal.py >>> @@ -16,6 +16,7 @@ import atexit >>> import os >>> import signal >>> import sys >>> +import threading >>> >>> import ovs.vlog >>> >>> @@ -112,29 +113,29 @@ def _unlink(file_): >>> def _signal_handler(signr, _): >>> _call_hooks(signr) >>> >>> - # Re-raise the signal with the default handling so that the program >>> - # termination status reflects that we were killed by this signal. >>> - signal.signal(signr, signal.SIG_DFL) >>> - os.kill(os.getpid(), signr) >>> - >>> >>> def _atexit_handler(): >>> _call_hooks(0) >>> >>> >>> -recurse = False >>> +mutex = threading.Lock() >>> >>> >>> def _call_hooks(signr): >>> - global recurse >>> - if recurse: >>> + global mutex >>> + if not mutex.acquire(blocking=False): >>> return >>> - recurse = True >>> >>> for hook, cancel, run_at_exit in _hooks: >>> if signr != 0 or run_at_exit: >>> hook() >>> >>> + if signr != 0: >>> + # Re-raise the signal with the default handling so that the program >>> + # termination status reflects that we were killed by this signal. >>> + signal.signal(signr, signal.SIG_DFL) >>> + os.kill(os.getpid(), signr) >> >> The indentation here is incorrect: >> >> python/ovs/fatal_signal.py:134:8: E114 indentation is not a multiple of 4 >> (comment) >> python/ovs/fatal_signal.py:135:8: E114 indentation is not a multiple of 4 >> (comment) >> python/ovs/fatal_signal.py:136:8: E111 indentation is not a multiple of 4 >> python/ovs/fatal_signal.py:137:8: E111 indentation is not a multiple of 4 > > Done > >> >>> + >>> >>> _inited = False >>> >>> @@ -150,7 +151,9 @@ def _init(): >>> signal.SIGALRM] >>> >>> for signr in signals: >>> - if signal.getsignal(signr) == signal.SIG_DFL: >>> + handler = signal.getsignal(signr) >>> + if (handler == signal.SIG_DFL or >>> + handler == signal.default_int_handler): >>> signal.signal(signr, _signal_handler) >>> atexit.register(_atexit_handler) >>> >>> @@ -165,7 +168,6 @@ def signal_alarm(timeout): >>> >>> if sys.platform == "win32": >>> import time >>> - import threading >>> >>> class Alarm (threading.Thread): >>> def __init__(self, timeout): >>> diff --git a/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> >>> b/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> >>> index 4cbd9a5d3..0731b4ac8 100755 >>> --- a/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> >>> +++ b/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> >>> @@ -534,29 +534,25 @@ def main(): >>> ovsdb.close_idl() >>> >>> pipes = _doexec(*([dump_cmd, '-i', mirror_interface] + tcpdargs)) >>> + while pipes.poll() is None: >>> + data = pipes.stdout.readline().strip(b'\n') >>> + if len(data) == 0: >>> + break >>> + print(data.decode('utf-8')) >>> + >>> + # If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump >>> + # -i eth0 | grep "192.168.1.1"), the pipe is no longer available >>> + # after received Ctrl+C. >>> + # If we write data to an unavailable pipe, a pipe error will be >>> + # reported, so we turn off stdout to avoid subsequent flushing >>> + # of data into the pipe. >> >> I'm not sure if we need this comment. >> The code below will not be executed after Ctrl+C. We will go >> directly to the signal handler, execute hooks and raise the >> signal that will terminate the program. >> >> The only way we can end up here is if underlying tcpdump exited >> for some reason and we're in the process of graceful termination. >> >> I think, we should keep the code below, but it seems that the >> comment lost its meaning. >> >> Does that make sense? >> > > OK, I removed the comment and keep the code bellow. > >> >>> try: >>> - while pipes.poll() is None: >>> - data = pipes.stdout.readline().strip(b'\n') >>> - if len(data) == 0: >>> - raise KeyboardInterrupt >>> - print(data.decode('utf-8')) >>> - raise KeyboardInterrupt >>> - except KeyboardInterrupt: >>> - # If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump >>> - # -i eth0 | grep "192.168.1.1"), the pipe is no longer available >>> - # after received Ctrl+C. >>> - # If we write data to an unavailable pipe, a pipe error will be >>> - # reported, so we turn off stdout to avoid subsequent flushing >>> - # of data into the pipe. >>> - try: >>> - sys.stdout.close() >>> - except IOError: >>> - pass >>> - >>> - if pipes.poll() is None: >>> - pipes.terminate() >>> + sys.stdout.close() >>> + except IOError: >>> + pass >> >> And here the indentation is not right: >> >> utilities/ovs-tcpdump.in:550 <http://ovs-tcpdump.in:550/>:8: E111 >> indentation is not a multiple of 4 >> utilities/ovs-tcpdump.in:552 <http://ovs-tcpdump.in:552/>:8: E111 >> indentation is not a multiple of 4 >> >>> >>> - sys.exit(0) >>> + if pipes.poll() is None: >>> + pipes.terminate() >>> >>> >>> if __name__ == '__main__’: > > > After running ovs-tcpdump and inputs multiple CTRL+C, the program will > raise the following exception. > > Error in atexit._run_exitfuncs: > Traceback (most recent call last): > File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror > ovsdb = OVSDB(db_sock) > File "/usr/bin/ovs-tcpdump", line 168, in __init__ > OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB > File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change > while idl.change_seqno == seq and not idl.run(): > > The default handler of SIGINT is default_int_handler, so it was not > registered to the signal handler. When received CTRL+C again, the program > was broken, and calling hook could not be executed completely. > > Signed-off-by: Daniel Ding <[email protected] > <mailto:[email protected]>> > --- > python/ovs/fatal_signal.py | 24 +++++++++++++----------- > utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> | 32 > +++++++++++--------------------- > 2 files changed, 24 insertions(+), 32 deletions(-)
Thanks! This seems correct to me. Could you send it as a new PATCH v3 ? Best regards, Ilya Maximets. > > diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py > index cb2e99e87..16a7e78a0 100644 > --- a/python/ovs/fatal_signal.py > +++ b/python/ovs/fatal_signal.py > @@ -16,6 +16,7 @@ import atexit > import os > import signal > import sys > +import threading > > import ovs.vlog > > @@ -112,29 +113,29 @@ def _unlink(file_): > def _signal_handler(signr, _): > _call_hooks(signr) > > - # Re-raise the signal with the default handling so that the program > - # termination status reflects that we were killed by this signal. > - signal.signal(signr, signal.SIG_DFL) > - os.kill(os.getpid(), signr) > - > > def _atexit_handler(): > _call_hooks(0) > > > -recurse = False > +mutex = threading.Lock() > > > def _call_hooks(signr): > - global recurse > - if recurse: > + global mutex > + if not mutex.acquire(blocking=False): > return > - recurse = True > > for hook, cancel, run_at_exit in _hooks: > if signr != 0 or run_at_exit: > hook() > > + if signr != 0: > + # Re-raise the signal with the default handling so that the program > + # termination status reflects that we were killed by this signal. > + signal.signal(signr, signal.SIG_DFL) > + os.kill(os.getpid(), signr) > + > > _inited = False > > @@ -150,7 +151,9 @@ def _init(): > signal.SIGALRM] > > for signr in signals: > - if signal.getsignal(signr) == signal.SIG_DFL: > + handler = signal.getsignal(signr) > + if (handler == signal.SIG_DFL or > + handler == signal.default_int_handler): > signal.signal(signr, _signal_handler) > atexit.register(_atexit_handler) > > @@ -165,7 +168,6 @@ def signal_alarm(timeout): > > if sys.platform == "win32": > import time > - import threading > > class Alarm (threading.Thread): > def __init__(self, timeout): > diff --git a/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> > b/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> > index 4cbd9a5d3..eada803bb 100755 > --- a/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> > +++ b/utilities/ovs-tcpdump.in <http://ovs-tcpdump.in> > @@ -534,29 +534,19 @@ def main(): > ovsdb.close_idl() > > pipes = _doexec(*([dump_cmd, '-i', mirror_interface] + tcpdargs)) > - try: > - while pipes.poll() is None: > - data = pipes.stdout.readline().strip(b'\n') > - if len(data) == 0: > - raise KeyboardInterrupt > - print(data.decode('utf-8')) > - raise KeyboardInterrupt > - except KeyboardInterrupt: > - # If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump > - # -i eth0 | grep "192.168.1.1"), the pipe is no longer available > - # after received Ctrl+C. > - # If we write data to an unavailable pipe, a pipe error will be > - # reported, so we turn off stdout to avoid subsequent flushing > - # of data into the pipe. > - try: > - sys.stdout.close() > - except IOError: > - pass > + while pipes.poll() is None: > + data = pipes.stdout.readline().strip(b'\n') > + if len(data) == 0: > + break > + print(data.decode('utf-8')) > > - if pipes.poll() is None: > - pipes.terminate() > + try: > + sys.stdout.close() > + except IOError: > + pass > > - sys.exit(0) > + if pipes.poll() is None: > + pipes.terminate() > > > if __name__ == '__main__': > -- > 2.43.0 > > > > Regards, > Daniel Ding > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
