[ovs-dev,v2] ovs-tcpdump: add dump_cmd checker before _doexec()
diff mbox series

Message ID 84918eb0-af97-44ee-a97b-8f4773b4dd6d.txfh2007@aliyun.com
State Superseded
Headers show
Series
  • [ovs-dev,v2] ovs-tcpdump: add dump_cmd checker before _doexec()
Related show

Commit Message

Surya Rudra via dev March 7, 2019, 2:18 p.m. UTC
Sorry , I forgot to add signed-off messages.

v1->v2: add signed-off messages.

Signed-off-by: Liu Chang <txfh2007@aliyun.com>

Comments

0-day Robot March 7, 2019, 2:59 p.m. UTC | #1
Bleep bloop.  Greetings txfh2007 via dev, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author should not be mailing list.
Lines checked: 33, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Aaron Conole March 7, 2019, 3:04 p.m. UTC | #2
Hi Liu,

Thanks for working on this issue.

txfh2007 via dev <ovs-dev@openvswitch.org> writes:

> Sorry , I forgot to add signed-off messages.
>
> v1->v2: add signed-off messages.
>
> diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
> index 269c252f8..f28ecf5b2 100755
> --- a/utilities/ovs-tcpdump.in
> +++ b/utilities/ovs-tcpdump.in
> @@ -416,6 +416,10 @@ def main():
>          print("Error: must at least specify an interface with '-i' option")
>          sys.exit(1)
>
> +    if os.system("command -v %s" % (dump_cmd)):
> +        print("Error: %s is not installed !!"%dump_cmd)
> +        sys.exit(1)
> +
>      if '-l' not in tcpdargs:
>          tcpdargs.insert(0, '-l')
>
> Signed-off-by: Liu Chang <txfh2007@aliyun.com>

Typically commits are formatted with the 'Signed-off-by: ...' lines
coming before the diff.  While I think it's possible for a
maintainer/committer to fix this before applying when dealing with the
mail file, it's nicer when it is formatted correctly from the beginning.
In the future, using 'git format-patch' (and using 'git commit --amend'
to fix the commit message first) could make this easier for you.

OTOH, your proposal will generate a new flake8 error:

   utilities/ovs-tcpdump.in:420:46: E228 missing whitespace around modulo operator

Additionally, I think rather than spawning another task to search the
path, python provides us a mechanism to search the path.  I suggest
something like the following (please feel free to test+submit).

---
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 269c252f8..11624c54b 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -372,6 +372,11 @@ def argv_tuples(lst):
         pass
 
 
+def py_which(executable):
+    return any(os.access(os.path.join(path, executable), os.X_OK)
+               for path in os.environ["PATH"].split(os.pathsep))
+
+
 def main():
     db_sock = 'unix:@RUNDIR@/db.sock'
     interface = None
@@ -416,6 +421,10 @@ def main():
         print("Error: must at least specify an interface with '-i' option")
         sys.exit(1)
 
+    if not py_which(dump_cmd):
+        print("Error: unable to execute '%s' (check PATH)" % dump_cmd)
+        sys.exit(1)
+
     if '-l' not in tcpdargs:
         tcpdargs.insert(0, '-l')
 
---

Patch
diff mbox series

diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 269c252f8..f28ecf5b2 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -416,6 +416,10 @@  def main():
         print("Error: must at least specify an interface with '-i' option")
         sys.exit(1)

+    if os.system("command -v %s" % (dump_cmd)):
+        print("Error: %s is not installed !!"%dump_cmd)
+        sys.exit(1)
+
     if '-l' not in tcpdargs:
         tcpdargs.insert(0, '-l')