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

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 [off-list ref]

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

 From d9e8937b8d88a22ee5519fde2c728b377bc8fb1f Mon Sep 17 00:00:00 2001
From: souvikdey33 <redacted>
Date: Wed, 24 Aug 2016 19:56:36 -0400
Subject: [PATCH v1] Signed-off-by: souvikdey33 [off-list ref]

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

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

-- 
2.9.3.windows.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 <redacted>
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 [off-list ref]

You should add your real name to the sign off.

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.

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

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

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Thursday, August 25, 2016 11:19 AM
To: Mcnamara, John <redacted>
Cc: dev@dpdk.org; souvikdey33 <redacted>; nhorman@tuxdriver.com
Subject: Re: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface
issue.

2016-08-25 09:51, Mcnamara, John:

quoted

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

On Wed, 24 Aug 2016 22:25:46 -0400
souvikdey33 [off-list ref] wrote:

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.

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: Souvik Dey [off-list ref]
  Fixes: 3da038604009 ("Signed-off-by: Souvik Dey [off-list ref]")
  
 tools/dpdk-devbind.py | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 

From 3da0386040092fcd54ee333ceff8c427a36c6b45 Mon Sep 17 00:00:00 2001

From: souvikdey33 <redacted>
Date: Thu, 25 Aug 2016 23:31:28 -0400
Subject: [PATCH v2] Signed-off-by: Souvik Dey [off-list ref]

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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index 9829e25..c0b46ee 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py

@@ -36,7 +36,6 @@ import sys
 import os
 import getopt
 import subprocess
-import commands
 
 from os.path import exists, abspath, dirname, basename
 

@@ -224,14 +223,18 @@ 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)
+    # the path for virtio devices are different, so get the correct path
+    virtio = "/sys/bus/pci/devices/%s/" % dev_id
+    ls = subprocess.Popen(['ls', virtio], stdout=subprocess.PIPE)
+    grep = subprocess.Popen('grep virt'.split(), stdin=ls.stdout,
+                            stdout=subprocess.PIPE)
+    ls.stdout.close()
+    virtio = grep.communicate()[0].rstrip()
+    ls.wait()
+    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):
+    elif exists(virtio_sys_path):
         device["Interface"] = ",".join(os.listdir(virtio_sys_path))
     else:
         device["Interface"] = ""

-- 
2.9.3.windows.1

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: Souvik Dey [off-list ref]
  Fixes: 3da038604009 ("Signed-off-by: Souvik Dey [off-list ref]")
  
 tools/dpdk-devbind.py | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 

From 3da0386040092fcd54ee333ceff8c427a36c6b45 Mon Sep 17 00:00:00 2001

From: souvikdey33 <redacted>
Date: Thu, 25 Aug 2016 23:31:28 -0400
Subject: [PATCH v2] Signed-off-by: Souvik Dey [off-list ref]

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 | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index 9829e25..c0b46ee 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py

@@ -36,7 +36,6 @@ import sys
 import os
 import getopt
 import subprocess
-import commands
 
 from os.path import exists, abspath, dirname, basename
 

@@ -224,14 +223,18 @@ 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)
+    # the path for virtio devices are different, so get the correct path
+    virtio = "/sys/bus/pci/devices/%s/" % dev_id
+    ls = subprocess.Popen(['ls', virtio], stdout=subprocess.PIPE)
+    grep = subprocess.Popen('grep virt'.split(), stdin=ls.stdout,
+                            stdout=subprocess.PIPE)
+    ls.stdout.close()
+    virtio = grep.communicate()[0].rstrip()
+    ls.wait()
+    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):
+    elif exists(virtio_sys_path):
         device["Interface"] = ",".join(os.listdir(virtio_sys_path))
     else:
         device["Interface"] = ""

-- 
2.9.3.windows.1

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: Souvik Dey [off-list ref]

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

-- 

From 1fb8e8896ca8d5b33bdcc875231bfb5ff72550c6 Mon Sep 17 00:00:00 2001

From: souvikdey33 <redacted>
Date: Fri, 26 Aug 2016 07:27:43 -0400
Subject: [PATCH v3] Signed-off-by: Souvik Dey [off-list ref]

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.

Fixes: e2af2c716077 ("Signed-off-by: Souvik Dey [off-list ref]")
---
 tools/dpdk-devbind.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index b69ca2a..c0b46ee 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py

@@ -36,6 +36,7 @@ import sys
 import os
 import getopt
 import subprocess
+
 from os.path import exists, abspath, dirname, basename
 
 # The PCI base class for NETWORK devices

@@ -222,8 +223,19 @@ 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, so get the correct path
+    virtio = "/sys/bus/pci/devices/%s/" % dev_id
+    ls = subprocess.Popen(['ls', virtio], stdout=subprocess.PIPE)
+    grep = subprocess.Popen('grep virt'.split(), stdin=ls.stdout,
+                            stdout=subprocess.PIPE)
+    ls.stdout.close()
+    virtio = grep.communicate()[0].rstrip()
+    ls.wait()
+    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(virtio_sys_path):
+        device["Interface"] = ",".join(os.listdir(virtio_sys_path))
     else:
         device["Interface"] = ""
     # check if a port is used for ssh connection

-- 
2.9.3.windows.1

On Wed, 24 Aug 2016 22:25:46 -0400
souvikdey33 [off-list ref] wrote:

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.

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 <redacted>
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 [off-list ref] wrote:

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.

We did this slightly differently. This is 100% python and is a bit more general. We search for the first "net" directory under the specific device directory.

-------------------------------------------

--- tools/dpdk-devbind.py       2016-08-29 11:02:35.594202888 -0400
+++ ../dpdk/tools/dpdk-devbind.py 2016-08-29 11:00:34.897677233 -0400

@@ -221,11 +221,11 @@
         name = name.strip(":") + "_str"
         device[name] = value
     # check for a unix interface name
-    sys_path = "/sys/bus/pci/devices/%s/net/" % dev_id
-    if exists(sys_path):
-        device["Interface"] = ",".join(os.listdir(sys_path))
-    else:
-        device["Interface"] = ""
+    device["Interface"] = ""
+    for base, dirs, files in os.walk("/sys/bus/pci/devices/%s/" % dev_id):
+        if "net" in dirs:
+            device["Interface"] = ",".join(os.listdir(os.path.join(base,"net")))
+            break
     # check if a port is used for ssh connection
     device["Ssh_if"] = False
     device["Active"] = ""
-------------------------------------------

Gary

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dey, Souvik
Sent: Friday, August 26, 2016 8:21 PM
To: Stephen Hemminger
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

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 <redacted>
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 [off-list ref] wrote:

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.

Hi,

I already followed the 100% python way and submitted the v3 of this patch. http://dpdk.org/dev/patchwork/patch/15378/
How will your patch be different in solving the issue. There will always be multiple ways to solving things right.


V3 of my submitted patch:

diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index b69ca2a..c0b46ee 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py

@@ -36,6 +36,7 @@  import sys
 import os
 import getopt
 import subprocess
+
 from os.path import exists, abspath, dirname, basename
 
 # The PCI base class for NETWORK devices

@@ -222,8 +223,19 @@  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, so get the correct path
+    virtio = "/sys/bus/pci/devices/%s/" % dev_id
+    ls = subprocess.Popen(['ls', virtio], stdout=subprocess.PIPE)
+    grep = subprocess.Popen('grep virt'.split(), stdin=ls.stdout,
+                            stdout=subprocess.PIPE)
+    ls.stdout.close()
+    virtio = grep.communicate()[0].rstrip()
+    ls.wait()
+    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(virtio_sys_path):
+        device["Interface"] = ",".join(os.listdir(virtio_sys_path))
     else:
         device["Interface"] = ""
     # check if a port is used for ssh connection


-----Original Message-----

From: Mussar, Gary [mailto:gmussar@ciena.com] 
Sent: Monday, August 29, 2016 11:10 AM
To: Dey, Souvik <redacted>; Stephen Hemminger <stephen@networkplumber.org>
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: RE: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

We did this slightly differently. This is 100% python and is a bit more general. We search for the first "net" directory under the specific device directory.

-------------------------------------------

--- tools/dpdk-devbind.py       2016-08-29 11:02:35.594202888 -0400
+++ ../dpdk/tools/dpdk-devbind.py 2016-08-29 11:00:34.897677233 -0400

@@ -221,11 +221,11 @@
         name = name.strip(":") + "_str"
         device[name] = value
     # check for a unix interface name
-    sys_path = "/sys/bus/pci/devices/%s/net/" % dev_id
-    if exists(sys_path):
-        device["Interface"] = ",".join(os.listdir(sys_path))
-    else:
-        device["Interface"] = ""
+    device["Interface"] = ""
+    for base, dirs, files in os.walk("/sys/bus/pci/devices/%s/" % dev_id):
+        if "net" in dirs:
+            device["Interface"] = ",".join(os.listdir(os.path.join(base,"net")))
+            break
     # check if a port is used for ssh connection
     device["Ssh_if"] = False
     device["Active"] = ""
-------------------------------------------

Gary

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dey, Souvik
Sent: Friday, August 26, 2016 8:21 PM
To: Stephen Hemminger
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

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 <redacted>
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 [off-list ref] wrote:

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.

On Mon, 29 Aug 2016 23:16:35 +0000
"Dey, Souvik" [off-list ref] wrote:

When I was suggesting pure python, I meant do it without a sub shell. Popen is just
another wrapper around a sub-shell.

On Mon, Aug 29, 2016 at 11:16:35PM +0000, Dey, Souvik wrote:

Hi,

I already followed the 100% python way and submitted the v3 of this patch. http://dpdk.org/dev/patchwork/patch/15378/
How will your patch be different in solving the issue. There will always be multiple ways to solving things right.

As stephen says, using popen is a bit of a hack here.  You could easily use one
of several python-sysfs libraries to simplify the sysfs enumeration and
discovery process
Neil

-----Original Message-----
From: Dey, Souvik [mailto:sodey@sonusnet.com] 
Sent: Monday, August 29, 2016 7:17 PM
To: Mussar, Gary; Stephen Hemminger
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: RE: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

Hi,

I already followed the 100% python way and submitted the v3 of this patch. http://dpdk.org/dev/patchwork/patch/15378/
How will your patch be different in solving the issue. There will always be multiple ways to solving things right.

GM> When I first tackled this problem I used Popen() and got the exact same feedback about using 100% python. The version I posted yesterday satisfied the internal reviewers. 


V3 of my submitted patch:

diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py index b69ca2a..c0b46ee 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py

@@ -36,6 +36,7 @@  import sys
 import os
 import getopt
 import subprocess
+
 from os.path import exists, abspath, dirname, basename
 
 # The PCI base class for NETWORK devices @@ -222,8 +223,19 @@  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, so get the correct path
+    virtio = "/sys/bus/pci/devices/%s/" % dev_id
+    ls = subprocess.Popen(['ls', virtio], stdout=subprocess.PIPE)
+    grep = subprocess.Popen('grep virt'.split(), stdin=ls.stdout,
+                            stdout=subprocess.PIPE)
+    ls.stdout.close()
+    virtio = grep.communicate()[0].rstrip()
+    ls.wait()
+    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(virtio_sys_path):
+        device["Interface"] = ",".join(os.listdir(virtio_sys_path))
     else:
         device["Interface"] = ""
     # check if a port is used for ssh connection


-----Original Message-----

From: Mussar, Gary [mailto:gmussar@ciena.com]
Sent: Monday, August 29, 2016 11:10 AM
To: Dey, Souvik <redacted>; Stephen Hemminger <stephen@networkplumber.org>
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: RE: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

We did this slightly differently. This is 100% python and is a bit more general. We search for the first "net" directory under the specific device directory.

-------------------------------------------

--- tools/dpdk-devbind.py       2016-08-29 11:02:35.594202888 -0400
+++ ../dpdk/tools/dpdk-devbind.py 2016-08-29 11:00:34.897677233 -0400

@@ -221,11 +221,11 @@
         name = name.strip(":") + "_str"
         device[name] = value
     # check for a unix interface name
-    sys_path = "/sys/bus/pci/devices/%s/net/" % dev_id
-    if exists(sys_path):
-        device["Interface"] = ",".join(os.listdir(sys_path))
-    else:
-        device["Interface"] = ""
+    device["Interface"] = ""
+    for base, dirs, files in os.walk("/sys/bus/pci/devices/%s/" % dev_id):
+        if "net" in dirs:
+            device["Interface"] = ",".join(os.listdir(os.path.join(base,"net")))
+            break
     # check if a port is used for ssh connection
     device["Ssh_if"] = False
     device["Active"] = ""
-------------------------------------------

Gary

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dey, Souvik
Sent: Friday, August 26, 2016 8:21 PM
To: Stephen Hemminger
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

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 <redacted>
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 [off-list ref] wrote:

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.

Hi Gary,

That looks like a cleaner solution. Could you submit that as a patch.

Souvik, could you test this patch and confirm it fixes your issue.


Gary, if you submit a patch could you make a few minor changes:

If "files" is unused, and it looks like it is, then replace it with "_".

+            device["Interface"] = ",".join(os.listdir(os.path.join(base,"net")))

There is a space required after "," for PEP8 compliance.

John

Yes this patch definitely solves my issue too. 

-----Original Message-----
From: Mcnamara, John [mailto:john.mcnamara@intel.com] 
Sent: Thursday, September 1, 2016 7:00 AM
To: Mussar, Gary <redacted>; Dey, Souvik <redacted>; Stephen Hemminger <stephen@networkplumber.org>
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: RE: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

Hi Gary,

That looks like a cleaner solution. Could you submit that as a patch.

Souvik, could you test this patch and confirm it fixes your issue.


Gary, if you submit a patch could you make a few minor changes:

If "files" is unused, and it looks like it is, then replace it with "_".

+            device["Interface"] = 
+ ",".join(os.listdir(os.path.join(base,"net")))

There is a space required after "," for PEP8 compliance.

John

I will get a proper patch sent hopefully today.

Gary

-----Original Message-----
From: Mcnamara, John [mailto:john.mcnamara@intel.com] 
Sent: Thursday, September 01, 2016 7:00 AM
To: Mussar, Gary; Dey, Souvik; Stephen Hemminger
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: RE: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

Hi Gary,

That looks like a cleaner solution. Could you submit that as a patch.

Souvik, could you test this patch and confirm it fixes your issue.


Gary, if you submit a patch could you make a few minor changes:

If "files" is unused, and it looks like it is, then replace it with "_".

+            device["Interface"] = 
+ ",".join(os.listdir(os.path.join(base,"net")))

There is a space required after "," for PEP8 compliance.

John

2016-08-26 07:35, souvikdey33:

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: Souvik Dey [off-list ref]

The patch from Gary - which do not use subprocess - has been preferred:
	http://dpdk.org/patch/15595