On 8/24/22 15:33, Aaron Conole wrote: > Hi Daniel, > > Thanks for the patch! I have some comments for re-spin. > > The subject includes "(Daniel Ding)" but that should be trimmed. I'm > not sure if your email system added this. If so, perhaps you can use > something like "git send-email" or attach a plaintext copy of the > patchfile on a new spin. > > Additionally, I think $subject should be written something like: > > "[PATCH] ovs-tcpdump: Destroy mirror port on SIGHUP/SIGTERM" > > "Daniel Ding" <[email protected]> writes: > >> If ovs-tcpdump received HUP or TERM signal, mirror and mirror interface >> should be destroyed. This often happens, when controlling terminal is >> closed, like ssh session closed, and other users use kill to terminate it. > > Please keep commit messages to shorter lines. I don't know if your > email client made this change or not. > >> Signed-off-by: Daniel Ding <[email protected]> >> --- > > Otherwise, change LGTM. :) > >> utilities/ovs-tcpdump.in | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in >> index 7fd26e405..211d50fc5 100755 >> --- a/utilities/ovs-tcpdump.in >> +++ b/utilities/ovs-tcpdump.in >> @@ -17,6 +17,7 @@ >> import os >> import pwd >> from random import randint >> +import signal >> import subprocess >> import sys >> import time >> @@ -489,6 +490,17 @@ def main(): >> print("ERROR: Mirror port (%s) exists for port %s." % >> (mirror_interface, interface)) >> sys.exit(1) >> + >> + def signal_handler(_signo, _stack_frame): >> + ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface)) >> + ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
I suppose, you need to check if the database connection is still open and re-open it here, because it is likely already closed, as we're closing it before calling tcpdump. And should we also delete taps? >> + >> + sys.exit(0) >> + >> + if sys.platform in ['linux', 'linux2']: >> + signal.signal(signal.SIGHUP, signal_handler) >> + signal.signal(signal.SIGTERM, signal_handler) I think, we should import and use ovs.fatal_signal module instead of managing signals manually. It will take care of all the different signals and will also handle platform differences and the correct re-rising of the signal for us. The whole teardown sequence can be just moved to a separate function and registered with add_hook(), so it will always be called at exit, regardless of it being a signal or normal exit. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
