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

  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(+)
  

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())
>
  

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.
  

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