diff mbox

[dpdk-dev,v1] dpdk-devbind.py: Virtio interface issue.

Message ID 20160825022546.96468-1-sodey@sonusnet.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

souvikdey33 Aug. 25, 2016, 2:25 a.m. UTC 
  This change is required to have the interface name for virtio interfaces.
When we execute the status command the for virtio inerfaces we get
Sample output without the change:
0000:00:04.0 'Virtio network device' if= drv=virtio-pci unused=virtio_pci,igb_uio
Though for other drivers this works.
Sample output with the change:
0000:00:04.0 'Virtio network device' if=eth0 drv=virtio-pci unused=virtio_pci,igb_uio

souvikdey33 (1):
  Signed-off-by: souvikdey33 <sodey@sonusnet.com>

 tools/dpdk-devbind.py | 9 +++++++++
 1 file changed, 9 insertions(+)

 From d9e8937b8d88a22ee5519fde2c728b377bc8fb1f Mon Sep 17 00:00:00 2001
From: souvikdey33 <sodey@sonusnet.com>
Date: Wed, 24 Aug 2016 19:56:36 -0400
Subject: [PATCH v1] Signed-off-by: souvikdey33 <sodey@sonusnet.com>

When we execute the status command the for virtio inerfaces the interface name is not shown.
Sample output without the change.
0000:00:04.0 'Virtio network device' if= drv=virtio-pci unused=virtio_pci,igb_uio
Though for other this works.
---
 tools/dpdk-devbind.py | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

John McNamara Aug. 25, 2016, 9:51 a.m. UTC | #1 
Hi,

Welcome to DPDK and thanks for the contribution. It looks like a useful fix.

Since you are a new contributor the user guide on "Contributing Code to DPDK"
explains some of the steps involved:

    http://dpdk.org/doc/guides/contributing/patches.html

Some comments below.


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of souvikdey33
> Sent: Thursday, August 25, 2016 3:26 AM
> To: nhorman@tuxdriver.com; dev@dpdk.org
> Cc: souvikdey33 <sodey@sonusnet.com>
> Subject: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

As you will see in the guide above the subject line should be lowercase and
shouldn't end with a full stop. Also, the prefix would be better as "tools". 
Something like this:

    tools: fix issue with virtio interfaces

The word fix on the command line normally means you should add a "Fixes" line
to the body but in this case the issue was probably always there (or at least
since virtio support was added) so you can probably omit it.

> 
> This change is required to have the interface name for virtio interfaces.
> When we execute the status command the for virtio inerfaces we get Sample
> output without the change:
> 0000:00:04.0 'Virtio network device' if= drv=virtio-pci
> unused=virtio_pci,igb_uio Though for other drivers this works.
> Sample output with the change:
> 0000:00:04.0 'Virtio network device' if=eth0 drv=virtio-pci
> unused=virtio_pci,igb_uio
> 
> souvikdey33 (1):
>   Signed-off-by: souvikdey33 <sodey@sonusnet.com>

You should add your real name to the sign off.



> diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py index
> b69ca2a..9829e25 100755
> --- a/tools/dpdk-devbind.py
> +++ b/tools/dpdk-devbind.py
> @@ -36,6 +36,8 @@ import sys
>  import os
>  import getopt
>  import subprocess
> +import commands

The commands module is deprecated in Python 2 and removed in Python 3.
Python 2 and 3 should both be supported by the DPDK tools. In which case
you can use subprocess.check_output(), or similar, instead.
 

> +
>  from os.path import exists, abspath, dirname, basename
> 
>  # The PCI base class for NETWORK devices @@ -222,8 +224,15 @@ def
> get_pci_device_details(dev_id):
>          device[name] = value
>      # check for a unix interface name
>      sys_path = "/sys/bus/pci/devices/%s/net/" % dev_id
> +    #The path for virtio devices are different. Get the correct path.
> +	virtio = "/sys/bus/pci/devices/%s/" % dev_id

This space/tab indentation gives a Python error.


> +    cmd = " ls %s | grep 'virt' " %virtio
> +    virtio = commands.getoutput(cmd)
> +    virtio_sys_path = "/sys/bus/pci/devices/%s/%s/net/" %
> +(dev_id,virtio)
>      if exists(sys_path):
>          device["Interface"] = ",".join(os.listdir(sys_path))
> +    elif exists(virt_path):
> +        device["Interface"] = ",".join(os.listdir(virtio_sys_path))
>      else:
>          device["Interface"] = ""
>      # check if a port is used for ssh connection

There a number of small Python formatting issues in the patch. The DPDK Python
code follows the pep8 guidelines:

    http://dpdk.org/doc/guides/contributing/coding_style.html#python-code

Here are the warnings:

    $ pep8 tools/dpdk-devbind.py 
    tools/dpdk-devbind.py:227:5:  E265 block comment should start with '# '
    tools/dpdk-devbind.py:228:1:  E101 indentation contains mixed spaces and tabs
    tools/dpdk-devbind.py:228:1:  W191 indentation contains tabs
    tools/dpdk-devbind.py:228:2:  E113 unexpected indentation
    tools/dpdk-devbind.py:229:1:  E101 indentation contains mixed spaces and tabs
    tools/dpdk-devbind.py:229:36: E225 missing whitespace around operator
    tools/dpdk-devbind.py:231:66: E231 missing whitespace after ','

Could you fix those issues and submit a V2 of the patch.

Thanks.

John
Thomas Monjalon Aug. 25, 2016, 10:19 a.m. UTC | #2 
2016-08-25 09:51, Mcnamara, John:
> The word fix on the command line normally means you should add a "Fixes" line
> to the body but in this case the issue was probably always there (or at least
> since virtio support was added) so you can probably omit it.

Even if it has always been there, we need to know the commit origin.
The "Fixes:" line makes things clear and helps when backporting.
Thanks
John McNamara Aug. 25, 2016, 10:27 a.m. UTC | #3 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, August 25, 2016 11:19 AM
> To: Mcnamara, John <john.mcnamara@intel.com>
> Cc: dev@dpdk.org; souvikdey33 <sodey@sonusnet.com>; nhorman@tuxdriver.com
> Subject: Re: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface
> issue.
> 
> 2016-08-25 09:51, Mcnamara, John:
> > The word fix on the command line normally means you should add a
> > "Fixes" line to the body but in this case the issue was probably
> > always there (or at least since virtio support was added) so you can
> probably omit it.
> 
> Even if it has always been there, we need to know the commit origin.
> The "Fixes:" line makes things clear and helps when backporting.
> Thanks

In that case the fixline should be:

    Fixes: 629395b063e8 ("igb_uio: remove PCI id table")

John
Stephen Hemminger Aug. 26, 2016, 12:37 a.m. UTC | #4 
On Wed, 24 Aug 2016 22:25:46 -0400
souvikdey33 <sodey@sonusnet.com> wrote:

> +    #The path for virtio devices are different. Get the correct path.
> +	virtio = "/sys/bus/pci/devices/%s/" % dev_id
> +    cmd = " ls %s | grep 'virt' " %virtio
> +    virtio = commands.getoutput(cmd)

I am not a python export but in general it is better to use native language
facilities likes os.listdir() rather than using shell pipes.
Stephen Hemminger Aug. 26, 2016, 3:55 p.m. UTC | #5 
On Wed, 24 Aug 2016 22:25:46 -0400
souvikdey33 <sodey@sonusnet.com> wrote:

> +    #The path for virtio devices are different. Get the correct path.
> +	virtio = "/sys/bus/pci/devices/%s/" % dev_id
> +    cmd = " ls %s | grep 'virt' " %virtio
> +    virtio = commands.getoutput(cmd)
> +    virtio_sys_path = "/sys/bus/pci/devices/%s/%s/net/" % (dev_id,virtio)
>      if exists(sys_path):
>          device["Interface"] = ",".join(os.listdir(sys_path))

There should be a way to do this in python without going out to shell.
This would be safer and more secure.

The code already uses os.listdir() (which is the python library version
of ls) in later section. Why not use that here to check for virtio bus.
souvikdey33 Aug. 27, 2016, 12:20 a.m. UTC | #6 
Hi ,
	I have already updated it and have re submitted the patch v3. Can you please check that http://dpdk.org/dev/patchwork/patch/15378/
--
Regards,
Souvik

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org] 
Sent: Friday, August 26, 2016 11:55 AM
To: Dey, Souvik <sodey@sonusnet.com>
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

On Wed, 24 Aug 2016 22:25:46 -0400
souvikdey33 <sodey@sonusnet.com> wrote:

> +    #The path for virtio devices are different. Get the correct path.
> +	virtio = "/sys/bus/pci/devices/%s/" % dev_id
> +    cmd = " ls %s | grep 'virt' " %virtio
> +    virtio = commands.getoutput(cmd)
> +    virtio_sys_path = "/sys/bus/pci/devices/%s/%s/net/" % 
> +(dev_id,virtio)
>      if exists(sys_path):
>          device["Interface"] = ",".join(os.listdir(sys_path))

There should be a way to do this in python without going out to shell.
This would be safer and more secure.

The code already uses os.listdir() (which is the python library version of ls) in later section. Why not use that here to check for virtio bus.
diff mbox

Patch

diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index b69ca2a..9829e25 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py
@@ -36,6 +36,8 @@  import sys
 import os
 import getopt
 import subprocess
+import commands
+
 from os.path import exists, abspath, dirname, basename
 
 # The PCI base class for NETWORK devices
@@ -222,8 +224,15 @@  def get_pci_device_details(dev_id):
         device[name] = value
     # check for a unix interface name
     sys_path = "/sys/bus/pci/devices/%s/net/" % dev_id
+    #The path for virtio devices are different. Get the correct path.
+	virtio = "/sys/bus/pci/devices/%s/" % dev_id
+    cmd = " ls %s | grep 'virt' " %virtio
+    virtio = commands.getoutput(cmd)
+    virtio_sys_path = "/sys/bus/pci/devices/%s/%s/net/" % (dev_id,virtio)
     if exists(sys_path):
         device["Interface"] = ",".join(os.listdir(sys_path))
+    elif exists(virt_path):
+        device["Interface"] = ",".join(os.listdir(virtio_sys_path))
     else:
         device["Interface"] = ""
     # check if a port is used for ssh connection