diff mbox series

usertools/dpdk-telemetry: print name of app when connected

Message ID 20210216094415.28000-1-bruce.richardson@intel.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series usertools/dpdk-telemetry: print name of app when connected | expand

Checks

Context Check Description
ci/github-robot success github build: passed
ci/travis-robot fail travis build: failed
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/checkpatch success coding style OK

Commit Message

Bruce Richardson Feb. 16, 2021, 9:44 a.m. UTC
When the dpdk-telemetry client connects to a DPDK instance, we can use the
PID provided in the initial connection message to query from /proc the name
of the process we are connected to, and display that to the user. We use
the "cmdline" procfs entry for the query since that is available on both
Linux and FreeBSD (assuming procfs is mounted on the BSD instance).

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 usertools/dpdk-telemetry.py | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Burakov, Anatoly Feb. 16, 2021, 10:40 a.m. UTC | #1
On 16-Feb-21 9:44 AM, Bruce Richardson wrote:
> When the dpdk-telemetry client connects to a DPDK instance, we can use the
> PID provided in the initial connection message to query from /proc the name
> of the process we are connected to, and display that to the user. We use
> the "cmdline" procfs entry for the query since that is available on both
> Linux and FreeBSD (assuming procfs is mounted on the BSD instance).
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   usertools/dpdk-telemetry.py | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-telemetry.py
> index 181859658f..82b91f346f 100755
> --- a/usertools/dpdk-telemetry.py
> +++ b/usertools/dpdk-telemetry.py
> @@ -45,6 +45,11 @@ def handle_socket(path):
>           return
>       json_reply = read_socket(sock, 1024)
>       output_buf_len = json_reply["max_output_len"]
> +    pid = json_reply["pid"]
> +    if os.path.exists('/proc/' + str(pid) + '/cmdline'):
> +        with open('/proc/' + str(pid) + '/cmdline') as f:

First of all, this is better done using os.path.join:

path = os.path.join('/proc', str(pid), 'cmdline')
if os.path.exists(path):
     with open(path) as f:
         ...

More importantly this isn't terribly Pythonic as it's not over-using 
exceptions :) IMO a better way would be:

try:
     with open(path) as f:
         ...
except IOError as e:
     # ignore if doesn't exist
     if e.errno != errno.ENOENT:
         raise

> +            argv0 = f.read(1024).split('\0')[0]
> +            print("Connected to application: '" + os.path.basename(argv0) + "'")

Also, formatting is better than concatenation, e.g. at least:

bname = os.path.basename(argv0)
print("Connected to application: '{}'".format(bname))

>   
>       # get list of commands for readline completion
>       sock.send("/".encode())
>
Bruce Richardson Feb. 16, 2021, 11:02 a.m. UTC | #2
On Tue, Feb 16, 2021 at 10:40:36AM +0000, Burakov, Anatoly wrote:
> On 16-Feb-21 9:44 AM, Bruce Richardson wrote:
> > When the dpdk-telemetry client connects to a DPDK instance, we can use the
> > PID provided in the initial connection message to query from /proc the name
> > of the process we are connected to, and display that to the user. We use
> > the "cmdline" procfs entry for the query since that is available on both
> > Linux and FreeBSD (assuming procfs is mounted on the BSD instance).
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >   usertools/dpdk-telemetry.py | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-telemetry.py
> > index 181859658f..82b91f346f 100755
> > --- a/usertools/dpdk-telemetry.py
> > +++ b/usertools/dpdk-telemetry.py
> > @@ -45,6 +45,11 @@ def handle_socket(path):
> >           return
> >       json_reply = read_socket(sock, 1024)
> >       output_buf_len = json_reply["max_output_len"]
> > +    pid = json_reply["pid"]
> > +    if os.path.exists('/proc/' + str(pid) + '/cmdline'):
> > +        with open('/proc/' + str(pid) + '/cmdline') as f:
> 
> First of all, this is better done using os.path.join:
> 
> path = os.path.join('/proc', str(pid), 'cmdline')
> if os.path.exists(path):
>     with open(path) as f:
>         ...

Ok, I forgot that os.path.join can take > 2 parameters.

> 
> More importantly this isn't terribly Pythonic as it's not over-using
> exceptions :) IMO a better way would be:
> 
> try:
>     with open(path) as f:
>         ...
> except IOError as e:
>     # ignore if doesn't exist
>     if e.errno != errno.ENOENT:
>         raise
> 

Yes, I was thinking that I just wanted any exceptions to be raised, but you
right that I should just ignore the not-found one and skip the printout.

> > +            argv0 = f.read(1024).split('\0')[0]
> > +            print("Connected to application: '" + os.path.basename(argv0) + "'")
> 
> Also, formatting is better than concatenation, e.g. at least:
> 
> bname = os.path.basename(argv0)
> print("Connected to application: '{}'".format(bname))
> 
And f-strings are best of all, but we need python 3.6 for those. :-) I
consider use of format vs concat a matter of taste, but I'll update as you
suggest.

Thanks for the review.

/Bruce

PS: Python 3.5 has had its final release late last year:
https://www.python.org/downloads/release/python-3510/
We should soon consider updating our minimum required version to Python
3.6, allowing us to use f-strings in our python code rather than
concatenation or explicit format calls.
Burakov, Anatoly Feb. 16, 2021, 11:13 a.m. UTC | #3
On 16-Feb-21 11:02 AM, Bruce Richardson wrote:
> On Tue, Feb 16, 2021 at 10:40:36AM +0000, Burakov, Anatoly wrote:
>> On 16-Feb-21 9:44 AM, Bruce Richardson wrote:
>>> When the dpdk-telemetry client connects to a DPDK instance, we can use the
>>> PID provided in the initial connection message to query from /proc the name
>>> of the process we are connected to, and display that to the user. We use
>>> the "cmdline" procfs entry for the query since that is available on both
>>> Linux and FreeBSD (assuming procfs is mounted on the BSD instance).
>>>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>>    usertools/dpdk-telemetry.py | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-telemetry.py
>>> index 181859658f..82b91f346f 100755
>>> --- a/usertools/dpdk-telemetry.py
>>> +++ b/usertools/dpdk-telemetry.py
>>> @@ -45,6 +45,11 @@ def handle_socket(path):
>>>            return
>>>        json_reply = read_socket(sock, 1024)
>>>        output_buf_len = json_reply["max_output_len"]
>>> +    pid = json_reply["pid"]
>>> +    if os.path.exists('/proc/' + str(pid) + '/cmdline'):
>>> +        with open('/proc/' + str(pid) + '/cmdline') as f:
>>
>> First of all, this is better done using os.path.join:
>>
>> path = os.path.join('/proc', str(pid), 'cmdline')
>> if os.path.exists(path):
>>      with open(path) as f:
>>          ...
> 
> Ok, I forgot that os.path.join can take > 2 parameters.
> 
>>
>> More importantly this isn't terribly Pythonic as it's not over-using
>> exceptions :) IMO a better way would be:
>>
>> try:
>>      with open(path) as f:
>>          ...
>> except IOError as e:
>>      # ignore if doesn't exist
>>      if e.errno != errno.ENOENT:
>>          raise
>>
> 
> Yes, I was thinking that I just wanted any exceptions to be raised, but you
> right that I should just ignore the not-found one and skip the printout.

'raise' will raise the exception up the stack, so you'll still get your 
exceptions that way - you'll just skip the one that says the file 
doesn't exist. plus, it also has a benefit of avoiding TOCTOU race :)

> 
>>> +            argv0 = f.read(1024).split('\0')[0]
>>> +            print("Connected to application: '" + os.path.basename(argv0) + "'")
>>
>> Also, formatting is better than concatenation, e.g. at least:
>>
>> bname = os.path.basename(argv0)
>> print("Connected to application: '{}'".format(bname))
>>
> And f-strings are best of all, but we need python 3.6 for those. :-) I
> consider use of format vs concat a matter of taste, but I'll update as you
> suggest.
> 
> Thanks for the review.
> 
> /Bruce
> 
> PS: Python 3.5 has had its final release late last year:
> https://www.python.org/downloads/release/python-3510/
> We should soon consider updating our minimum required version to Python
> 3.6, allowing us to use f-strings in our python code rather than
> concatenation or explicit format calls.
> 

I seem to have a lot of difficulty remembering syntax for f-strings :P
diff mbox series

Patch

diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-telemetry.py
index 181859658f..82b91f346f 100755
--- a/usertools/dpdk-telemetry.py
+++ b/usertools/dpdk-telemetry.py
@@ -45,6 +45,11 @@  def handle_socket(path):
         return
     json_reply = read_socket(sock, 1024)
     output_buf_len = json_reply["max_output_len"]
+    pid = json_reply["pid"]
+    if os.path.exists('/proc/' + str(pid) + '/cmdline'):
+        with open('/proc/' + str(pid) + '/cmdline') as f:
+            argv0 = f.read(1024).split('\0')[0]
+            print("Connected to application: '" + os.path.basename(argv0) + "'")
 
     # get list of commands for readline completion
     sock.send("/".encode())