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

["Mcnamara, John" <john.mcnamara@intel.com>]

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