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

Message ID C281A17C31CFD745B242416D0E96EC63AB81F535@ONWVEXCHMB04.ciena.com (mailing list archive)
State Not Applicable, archived
Headers

Commit Message

Mussar, Gary Aug. 29, 2016, 3:09 p.m. UTC
  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.

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

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 <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.
  

Comments

John McNamara Sept. 1, 2016, 10:59 a.m. UTC | #1
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mussar, Gary
> Sent: Monday, August 29, 2016 4:10 PM
> To: Dey, Souvik <sodey@sonusnet.com>; 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"] = ""
> -------------------------------------------

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:

> +    device["Interface"] = ""
> +    for base, dirs, files in os.walk("/sys/bus/pci/devices/%s/" % dev_id):
> +        

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
  
souvikdey33 Sept. 1, 2016, 10:08 p.m. UTC | #2
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 <gmussar@ciena.com>; Dey, Souvik <sodey@sonusnet.com>; Stephen Hemminger <stephen@networkplumber.org>
Cc: nhorman@tuxdriver.com; dev@dpdk.org
Subject: RE: [dpdk-dev] [PATCH v1] dpdk-devbind.py: Virtio interface issue.

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mussar, Gary
> Sent: Monday, August 29, 2016 4:10 PM
> To: Dey, Souvik <sodey@sonusnet.com>; 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"] = ""
> -------------------------------------------

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:

> +    device["Interface"] = ""
> +    for base, dirs, files in os.walk("/sys/bus/pci/devices/%s/" % dev_id):
> +        

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
  
Mussar, Gary Sept. 2, 2016, 12:57 p.m. UTC | #3
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.

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mussar, Gary
> Sent: Monday, August 29, 2016 4:10 PM
> To: Dey, Souvik <sodey@sonusnet.com>; 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"] = ""
> -------------------------------------------

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:

> +    device["Interface"] = ""
> +    for base, dirs, files in os.walk("/sys/bus/pci/devices/%s/" % dev_id):
> +        

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
  

Patch

--- 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"] = ""