nickcooper-zhangtonghao <n...@opencloud.tech> writes: > Hi Aaron > You ignore an error case. Run the tcpdump with invalid option or > argument.
I agree, that error case is being skipped currently. > In this case, process will raise directly KeyboardInterrupt exception, not > run the while loop any more. We should return an error code with > sys.exit(1). Yes, but only in the error case. The code makes both come into the same path - perhaps it would be good to check pipes.returncode before setting the value to exit. Possibly doing something like: sys.exit(pipes.returncode) but I don't know - whatever you choose will be fine. > What’s more, in this error case, the subprocess has exited already before we > terminate it. We should check subprocess state before call the > 'pipes.terminate()'. Agreed. > I will submit v2, can you review it? Yes, I will. > Don’t you see this issue on your target? > # ovs-tcpdump -i eth1 -nnn -w > tcpdump: option requires an argument -- 'w' > tcpdump version 4.5.1 > libpcap version 1.5.3 > Usage: tcpdump [-aAbdDefhHIJKlLnNOpqRStuUvxX] [ -B size ] [ -c count ] > [ -C file_size ] [ -E algo:secret ] [ -F file ] [ -G seconds ] > [ -i interface ] [ -j tstamptype ] [ -M secret ] > [ -P in|out|inout ] > [ -r file ] [ -s snaplen ] [ -T type ] [ -V file ] [ -w file ] > [ -W filecount ] [ -y datalinktype ] [ -z command ] > [ -Z user ] [ expression ] > Traceback (most recent call last): > File "/root/ovs_master_dev_new/utilities/ovs-tcpdump", line 453, in <module> > main() > File "/root/ovs_master_dev_new/utilities/ovs-tcpdump", line 439, in main > pipes.terminate() > File "/usr/lib64/python2.7/subprocess.py", line 1551, in terminate > self.send_signal(signal.SIGTERM) > File "/usr/lib64/python2.7/subprocess.py", line 1546, in send_signal > os.kill(self.pid, sig) > OSError: [Errno 3] No such process I hadn't tried this case, tbh. But, I agree it's valuable to handle this error. > # ovs-tcpdump -i eth1 -nnn -1 > tcpdump: invalid option -- '1' > tcpdump version 4.5.1 > libpcap version 1.5.3 > Usage: tcpdump [-aAbdDefhHIJKlLnNOpqRStuUvxX] [ -B size ] [ -c count ] > [ -C file_size ] [ -E algo:secret ] [ -F file ] [ -G seconds ] > [ -i interface ] [ -j tstamptype ] [ -M secret ] > [ -P in|out|inout ] > [ -r file ] [ -s snaplen ] [ -T type ] [ -V file ] [ -w file ] > [ -W filecount ] [ -y datalinktype ] [ -z command ] > [ -Z user ] [ expression ] > Traceback (most recent call last): > File "/root/ovs_master_dev_new/utilities/ovs-tcpdump", line 453, in <module> > main() > File "/root/ovs_master_dev_new/utilities/ovs-tcpdump", line 439, in main > pipes.terminate() > File "/usr/lib64/python2.7/subprocess.py", line 1551, in terminate > self.send_signal(signal.SIGTERM) > File "/usr/lib64/python2.7/subprocess.py", line 1546, in send_signal > os.kill(self.pid, sig) > OSError: [Errno 3] No such process > > > > Thanks. > Nick Thank you! >> On Nov 10, 2016, at 12:47 AM, Aaron Conole <acon...@redhat.com> wrote: >> >> 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. > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
