[ovs-dev] [PATCH 3/4] ovs-tcpdump: Fix error handling when parsing parameter.

[Aaron Conole]

nickcooper-zhangtonghao <nic at opencloud.tech> writes:

>  On Nov 9, 2016, at 1:47 AM, Aaron Conole <aconole at redhat.com> wrote:
>
>  nickcooper-zhangtonghao <nic at opencloud.tech> writes:
>
>  Signed-off-by: nickcooper-zhangtonghao <nic at opencloud.tech>
>  ---
>  utilities/ovs-tcpdump.in | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
>  diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
>  index c189bc8..d03568d 100755
>  --- a/utilities/ovs-tcpdump.in
>  +++ b/utilities/ovs-tcpdump.in
>  @@ -432,9 +432,12 @@ def main():
>              print(data)
>          raise KeyboardInterrupt
>      except KeyboardInterrupt:
>  -        pipes.terminate()
>  +        if pipes.poll() is None:
>  +            pipes.terminate()
>  +
>          ovsdb.destroy_mirror('m%s' % interface, ovsdb.port_bridge(interface))
>          ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
>
>  +        sys.exit(1)
>
>  NAK to this - this will make the normal termination path read like an error
>  on the terminal.
>
>  Try it out:
>
>   ovs-tcpdump -i port0 -c 1
>   echo $?
>
> Hi Aaron
> Thanks for your reply. If you run the ovs-tcpdump with invalid option or
> option without an argument, sometimes ovs-tcpdump will crash. 
> When run the “pipes.terminate()”, parent process does’t known whether
> the subprocess has exited. We should check it before running the “pipes.terminate()”.
>

Sorry for being unclear - I had separated the sys.exit() line because I
disagree with that change.

You need to accommodate both error case for exits, and normal case.  I
agree it's important.