[ovs-dev] utilities/ovs-tcpdump.in: Fix not destroy mirror (Daniel Ding)

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.

Signed-off-by: Daniel Ding <zhihui.ding@easystack.cn>
---
utilities/ovs-tcpdump.in | 12 ++++++++++++
 1 file changed, 12 insertions(+)


---

Best regards, Daniel Ding

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" <zhihui.ding@easystack.cn> 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 <zhihui.ding@easystack.cn>
> ---

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))
> +
> +        sys.exit(0)
> +
> +    if sys.platform in ['linux', 'linux2']:
> +        signal.signal(signal.SIGHUP, signal_handler)
> +    signal.signal(signal.SIGTERM, signal_handler)
> +
>      try:
>          ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
>          ovsdb.bridge_mirror(interface, mirror_interface,
>
> ---

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" <zhihui.ding@easystack.cn> 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 <zhihui.ding@easystack.cn>
>> ---
> 
> 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.