[dpdk-dev] [PATCH 0/2] Small usability improvements for devbind

[dpdk-dev] [PATCH 0/2] Small usability improvements for devbind

[Anatoly Burakov]

Over the course of using devbind, i find myself frequently bumping up
against two common errors (with the assumption being that i'm not the
only person who hits these errors).

First happens when i forget to specify the driver. The error message in
this case looks something like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/08:00.0/bind
Error: unbind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers//unbind

This is confusing to anyone who isn't intimately familiar with how driver binding
through sysfs works. The first patch in this series changes the error message to
instead look like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
ERROR: Driver '08:00.0' does not look like a valid driver. Did you forget to specify the driver to bind devices to?

We do that by assuming that no one in their right mind will name their PCI driver
with something that looks like a PCI address, so we check if the driver string is
actually a valid device string. If it is, we error out.

The second error i often come across is forgetting to load the driver. This
error looks something like this:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/vfio-pci/bind

This too isn't very informative. The second patch in this patchset changes this error
to look like this instead:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
ERROR: Driver 'vfio-pci' is not loaded.

Nice and informative!

Anatoly Burakov (2):
  usertools/devbind: add error on forgetting to specify driver
  usertools/devbind: check if module is loaded before binding

 usertools/dpdk-devbind.py | 81 ++++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 26 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/2] usertools/devbind: add error on forgetting to specify driver

[Anatoly Burakov]

A common user error is to forget driver to which the PCI devices should
be bound to. Currently, the error message in this case looks unhelpful
misleading and indecipherable to anyone but people who know how devbind
works.

Fix this by checking if the driver string is actually a valid device
string. If it is, we assume that the user has just forgot to specify the
driver, and display appropriate error. We also assume that no one will
name their driver in a format that looks like a PCI address, but that
seems like a reasonable assumption to make.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 542ecffcc..f7c4c6434 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -342,9 +342,8 @@ def dev_id_from_dev_name(dev_name):
             if dev_name in devices[d]["Interface"].split(","):
                 return devices[d]["Slot"]
     # if nothing else matches - error
-    print("Unknown device: %s. "
-          "Please specify device in \"bus:slot.func\" format" % dev_name)
-    sys.exit(1)
+    raise ValueError("Unknown device: %s. "
+	    "Please specify device in \"bus:slot.func\" format" % dev_name)
 
 
 def unbind_one(dev_id, force):
@@ -493,7 +492,12 @@ def unbind_all(dev_list, force=False):
                     unbind_one(devices[d]["Slot"], force)
         return
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        print(ex)
+        sys.exit(1)
+
     for d in dev_list:
         unbind_one(d, force)
 
@@ -502,7 +506,26 @@ def bind_all(dev_list, driver, force=False):
     """Bind method, takes a list of device locations"""
     global devices
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    # a common user error is to forget to specify the driver the devices need to
+    # be bound to. check if the driver is a valid device, and if it is, show
+    # a meaningful error.
+    try:
+        dev_id_from_dev_name(driver)
+        # if we've made it this far, this means that the "driver" was a valid
+        # device string, so it's probably not a valid driver name.
+        print("ERROR: Driver '%s' does not look like a valid driver. "
+              "Did you forget to specify the driver to bind devices to?" %
+              driver)
+        sys.exit(1)
+    except ValueError:
+        # driver generated error - it's not a valid device ID, so all is well
+        pass
+
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        print(ex)
+        sys.exit(1)
 
     for d in dev_list:
         bind_one(d, driver, force)
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/2] usertools/devbind: add error on forgetting to specify driver

[Stephen Hemminger]

On Wed, 24 Jul 2019 16:34:43 +0100
Anatoly Burakov <anatoly.burakov@intel.com> wrote:

> A common user error is to forget driver to which the PCI devices should
> be bound to. Currently, the error message in this case looks unhelpful
> misleading and indecipherable to anyone but people who know how devbind
> works.
> 
> Fix this by checking if the driver string is actually a valid device
> string. If it is, we assume that the user has just forgot to specify the
> driver, and display appropriate error. We also assume that no one will
> name their driver in a format that looks like a PCI address, but that
> seems like a reasonable assumption to make.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  usertools/dpdk-devbind.py | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
> index 542ecffcc..f7c4c6434 100755
> --- a/usertools/dpdk-devbind.py
> +++ b/usertools/dpdk-devbind.py
> @@ -342,9 +342,8 @@ def dev_id_from_dev_name(dev_name):
>              if dev_name in devices[d]["Interface"].split(","):
>                  return devices[d]["Slot"]
>      # if nothing else matches - error
> -    print("Unknown device: %s. "
> -          "Please specify device in \"bus:slot.func\" format" % dev_name)
> -    sys.exit(1)
> +    raise ValueError("Unknown device: %s. "
> +	    "Please specify device in \"bus:slot.func\" format" % dev_name)
>  
>  
>  def unbind_one(dev_id, force):
> @@ -493,7 +492,12 @@ def unbind_all(dev_list, force=False):
>                      unbind_one(devices[d]["Slot"], force)
>          return
>  
> -    dev_list = map(dev_id_from_dev_name, dev_list)
> +    try:
> +        dev_list = map(dev_id_from_dev_name, dev_list)
> +    except ValueError as ex:
> +        print(ex)
> +        sys.exit(1)
> +
>      for d in dev_list:
>          unbind_one(d, force)
>  
> @@ -502,7 +506,26 @@ def bind_all(dev_list, driver, force=False):
>      """Bind method, takes a list of device locations"""
>      global devices
>  
> -    dev_list = map(dev_id_from_dev_name, dev_list)
> +    # a common user error is to forget to specify the driver the devices need to
> +    # be bound to. check if the driver is a valid device, and if it is, show
> +    # a meaningful error.
> +    try:
> +        dev_id_from_dev_name(driver)
> +        # if we've made it this far, this means that the "driver" was a valid
> +        # device string, so it's probably not a valid driver name.
> +        print("ERROR: Driver '%s' does not look like a valid driver. "
> +              "Did you forget to specify the driver to bind devices to?" %
> +              driver)
> +        sys.exit(1)
> +    except ValueError:
> +        # driver generated error - it's not a valid device ID, so all is well
> +        pass
> +
> +    try:
> +        dev_list = map(dev_id_from_dev_name, dev_list)
> +    except ValueError as ex:
> +        print(ex)
> +        sys.exit(1)
>  
>      for d in dev_list:
>          bind_one(d, driver, force)

It would be better print error messages to stderr.
If you call sys.exit() with a string it will do that.

Re: [dpdk-dev] [PATCH 1/2] usertools/devbind: add error on forgetting to specify driver

[Burakov, Anatoly]

On 24-Jul-19 5:29 PM, Stephen Hemminger wrote:
> On Wed, 24 Jul 2019 16:34:43 +0100
> Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> 
>> A common user error is to forget driver to which the PCI devices should
>> be bound to. Currently, the error message in this case looks unhelpful
>> misleading and indecipherable to anyone but people who know how devbind
>> works.
>>
>> Fix this by checking if the driver string is actually a valid device
>> string. If it is, we assume that the user has just forgot to specify the
>> driver, and display appropriate error. We also assume that no one will
>> name their driver in a format that looks like a PCI address, but that
>> seems like a reasonable assumption to make.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   usertools/dpdk-devbind.py | 33 ++++++++++++++++++++++++++++-----
>>   1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
>> index 542ecffcc..f7c4c6434 100755
>> --- a/usertools/dpdk-devbind.py
>> +++ b/usertools/dpdk-devbind.py
>> @@ -342,9 +342,8 @@ def dev_id_from_dev_name(dev_name):
>>               if dev_name in devices[d]["Interface"].split(","):
>>                   return devices[d]["Slot"]
>>       # if nothing else matches - error
>> -    print("Unknown device: %s. "
>> -          "Please specify device in \"bus:slot.func\" format" % dev_name)
>> -    sys.exit(1)
>> +    raise ValueError("Unknown device: %s. "
>> +	    "Please specify device in \"bus:slot.func\" format" % dev_name)
>>   
>>   
>>   def unbind_one(dev_id, force):
>> @@ -493,7 +492,12 @@ def unbind_all(dev_list, force=False):
>>                       unbind_one(devices[d]["Slot"], force)
>>           return
>>   
>> -    dev_list = map(dev_id_from_dev_name, dev_list)
>> +    try:
>> +        dev_list = map(dev_id_from_dev_name, dev_list)
>> +    except ValueError as ex:
>> +        print(ex)
>> +        sys.exit(1)
>> +
>>       for d in dev_list:
>>           unbind_one(d, force)
>>   
>> @@ -502,7 +506,26 @@ def bind_all(dev_list, driver, force=False):
>>       """Bind method, takes a list of device locations"""
>>       global devices
>>   
>> -    dev_list = map(dev_id_from_dev_name, dev_list)
>> +    # a common user error is to forget to specify the driver the devices need to
>> +    # be bound to. check if the driver is a valid device, and if it is, show
>> +    # a meaningful error.
>> +    try:
>> +        dev_id_from_dev_name(driver)
>> +        # if we've made it this far, this means that the "driver" was a valid
>> +        # device string, so it's probably not a valid driver name.
>> +        print("ERROR: Driver '%s' does not look like a valid driver. "
>> +              "Did you forget to specify the driver to bind devices to?" %
>> +              driver)
>> +        sys.exit(1)
>> +    except ValueError:
>> +        # driver generated error - it's not a valid device ID, so all is well
>> +        pass
>> +
>> +    try:
>> +        dev_list = map(dev_id_from_dev_name, dev_list)
>> +    except ValueError as ex:
>> +        print(ex)
>> +        sys.exit(1)
>>   
>>       for d in dev_list:
>>           bind_one(d, driver, force)
> 
> It would be better print error messages to stderr.
> If you call sys.exit() with a string it will do that.
> 

Will fix in v2.

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH 2/2] usertools/devbind: check if module is loaded before binding

[Anatoly Burakov]

Currently, if an attempt is made to bind a device to a driver that
is not loaded, a confusing and misleading error message appears.
Fix it so that, before binding to the driver, we actually check if
it is loaded in the kernel first.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 48 ++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index f7c4c6434..53112c999 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -146,6 +146,25 @@ def check_output(args, stderr=None):
     return subprocess.Popen(args, stdout=subprocess.PIPE,
                             stderr=stderr).communicate()[0]
 
+# check if a specific kernel module is loaded
+def module_is_loaded(module):
+    # Get list of sysfs modules (both built-in and dynamically loaded)
+    sysfs_path = '/sys/module/'
+
+    # Get the list of directories in sysfs_path
+    sysfs_mods = [os.path.join(sysfs_path, o) for o
+                  in os.listdir(sysfs_path)
+                  if os.path.isdir(os.path.join(sysfs_path, o))]
+
+    # get module names
+    sysfs_mods = [os.path.basename(a) for a in sysfs_mods]
+
+    # special case for vfio_pci (module is named vfio-pci,
+    # but its .ko is named vfio_pci)
+    sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
+
+    return module in sysfs_mods
+
 
 def check_modules():
     '''Checks that igb_uio is loaded'''
@@ -155,27 +174,9 @@ def check_modules():
     mods = [{"Name": driver, "Found": False} for driver in dpdk_drivers]
 
     # first check if module is loaded
-    try:
-        # Get list of sysfs modules (both built-in and dynamically loaded)
-        sysfs_path = '/sys/module/'
-
-        # Get the list of directories in sysfs_path
-        sysfs_mods = [os.path.join(sysfs_path, o) for o
-                      in os.listdir(sysfs_path)
-                      if os.path.isdir(os.path.join(sysfs_path, o))]
-
-        # Extract the last element of '/sys/module/abc' in the array
-        sysfs_mods = [a.split('/')[-1] for a in sysfs_mods]
-
-        # special case for vfio_pci (module is named vfio-pci,
-        # but its .ko is named vfio_pci)
-        sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
-
-        for mod in mods:
-            if mod["Name"] in sysfs_mods:
-                mod["Found"] = True
-    except:
-        pass
+    for mod in mods:
+        if module_is_loaded(mod["Name"]):
+            mod["Found"] = True
 
     # check if we have at least one loaded module
     if True not in [mod["Found"] for mod in mods] and b_flag is not None:
@@ -521,6 +522,11 @@ def bind_all(dev_list, driver, force=False):
         # driver generated error - it's not a valid device ID, so all is well
         pass
 
+    # check if we're attempting to bind to a driver that isn't loaded
+    if not module_is_loaded(driver):
+        print("ERROR: Driver '%s' is not loaded." % driver)
+        sys.exit(1)
+
     try:
         dev_list = map(dev_id_from_dev_name, dev_list)
     except ValueError as ex:
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/2] usertools/devbind: check if module is loaded before binding

[Stephen Hemminger]

On Wed, 24 Jul 2019 16:34:44 +0100
Anatoly Burakov <anatoly.burakov@intel.com> wrote:

> +# check if a specific kernel module is loaded
> +def module_is_loaded(module):
> +    # Get list of sysfs modules (both built-in and dynamically loaded)
> +    sysfs_path = '/sys/module/'
> +
> +    # Get the list of directories in sysfs_path
> +    sysfs_mods = [os.path.join(sysfs_path, o) for o
> +                  in os.listdir(sysfs_path)
> +                  if os.path.isdir(os.path.join(sysfs_path, o))]
> +
> +    # get module names
> +    sysfs_mods = [os.path.basename(a) for a in sysfs_mods]
> +
> +    # special case for vfio_pci (module is named vfio-pci,
> +    # but its .ko is named vfio_pci)
> +    sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
> +
> +    return module in sysfs_mods
> +

You could just check if the one module exist rather than getting full
list.

Maybe vfio-pci should be renamed vfil_pci earlier in the code.

Re: [dpdk-dev] [PATCH 2/2] usertools/devbind: check if module is loaded before binding

[Burakov, Anatoly]

On 24-Jul-19 5:28 PM, Stephen Hemminger wrote:
> On Wed, 24 Jul 2019 16:34:44 +0100
> Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> 
>> +# check if a specific kernel module is loaded
>> +def module_is_loaded(module):
>> +    # Get list of sysfs modules (both built-in and dynamically loaded)
>> +    sysfs_path = '/sys/module/'
>> +
>> +    # Get the list of directories in sysfs_path
>> +    sysfs_mods = [os.path.join(sysfs_path, o) for o
>> +                  in os.listdir(sysfs_path)
>> +                  if os.path.isdir(os.path.join(sysfs_path, o))]
>> +
>> +    # get module names
>> +    sysfs_mods = [os.path.basename(a) for a in sysfs_mods]
>> +
>> +    # special case for vfio_pci (module is named vfio-pci,
>> +    # but its .ko is named vfio_pci)
>> +    sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
>> +
>> +    return module in sysfs_mods
>> +
> 
> You could just check if the one module exist rather than getting full
> list.

I tried to reduce the amount of changes made, but i suppose i could fix 
this.

> 
> Maybe vfio-pci should be renamed vfil_pci earlier in the code.
> 

Good point.

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2 0/3] Small usability improvements for devbind

[Anatoly Burakov]

Over the course of using devbind, i find myself frequently bumping up
against two common errors (with the assumption being that i'm not the
only person who hits these errors).

First happens when i forget to specify the driver. The error message in
this case looks something like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/08:00.0/bind
Error: unbind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers//unbind

This is confusing to anyone who isn't intimately familiar with how driver binding
through sysfs works. The first patch in this series changes the error message to
instead look like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
ERROR: Driver '08:00.0' does not look like a valid driver. Did you forget to specify the driver to bind devices to?

We do that by assuming that no one in their right mind will name their PCI driver
with something that looks like a PCI address, so we check if the driver string is
actually a valid device string. If it is, we error out.

The second error i often come across is forgetting to load the driver. This
error looks something like this:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/vfio-pci/bind

This too isn't very informative. The second patch in this patchset changes this error
to look like this instead:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
ERROR: Driver 'vfio-pci' is not loaded.

Nice and informative!

Additionally, since we're outputting our new error messages to stderr, i took
the liberty of adjusting all the rest of the error messages to also output to
stderr.

v2:
- Addressed Stephen's feedbkac
- Added new patch adjusting error output to stderr

Anatoly Burakov (3):
  usertools/devbind: add error on forgetting to specify driver
  usertools/devbind: check if module is loaded before binding
  usertools/devbind: print all errors to stderr

 usertools/dpdk-devbind.py | 144 ++++++++++++++++++++++----------------
 1 file changed, 83 insertions(+), 61 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v3 0/3] Small usability improvements for devbind

[Anatoly Burakov]

Over the course of using devbind, i find myself frequently bumping up
against two common errors (with the assumption being that i'm not the
only person who hits these errors).

First happens when i forget to specify the driver. The error message in
this case looks something like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/08:00.0/bind
Error: unbind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers//unbind

This is confusing to anyone who isn't intimately familiar with how driver binding
through sysfs works. The first patch in this series changes the error message to
instead look like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
ERROR: Driver '08:00.0' does not look like a valid driver. Did you forget to specify the driver to bind devices to?

We do that by assuming that no one in their right mind will name their PCI driver
with something that looks like a PCI address, so we check if the driver string is
actually a valid device string. If it is, we error out.

The second error i often come across is forgetting to load the driver. This
error looks something like this:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/vfio-pci/bind

This too isn't very informative. The second patch in this patchset changes this error
to look like this instead:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
ERROR: Driver 'vfio-pci' is not loaded.

Nice and informative!

Additionally, since we're outputting our new error messages to stderr, i took
the liberty of adjusting all the rest of the error messages to also output to
stderr.

v3:
- Corrected mixing of tabs and spaces. Spaces all the way down!

v2:
- Addressed Stephen's feedback
- Added new patch adjusting error output to stderr

Anatoly Burakov (3):
  usertools/devbind: add error on forgetting to specify driver
  usertools/devbind: check if module is loaded before binding
  usertools/devbind: print all errors to stderr

 usertools/dpdk-devbind.py | 144 ++++++++++++++++++++++----------------
 1 file changed, 83 insertions(+), 61 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v4 0/3] Small usability improvements for devbind

[Anatoly Burakov]

Over the course of using devbind, i find myself frequently bumping up
against two common errors (with the assumption being that i'm not the
only person who hits these errors).

First happens when i forget to specify the driver. The error message in
this case looks something like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/08:00.0/bind
Error: unbind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers//unbind

This is confusing to anyone who isn't intimately familiar with how driver binding
through sysfs works. The first patch in this series changes the error message to
instead look like the following:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b 08:00.0 08:00.1
ERROR: Driver '08:00.0' does not look like a valid driver. Did you forget to specify the driver to bind devices to?

We do that by assuming that no one in their right mind will name their PCI driver
with something that looks like a PCI address, so we check if the driver string is
actually a valid device string. If it is, we error out.

The second error i often come across is forgetting to load the driver. This
error looks something like this:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
Error: bind failed for 0000:08:00.1 - Cannot open /sys/bus/pci/drivers/vfio-pci/bind

This too isn't very informative. The second patch in this patchset changes this error
to look like this instead:

anatoly@xxxx:~$ sudo DPDK/usertools/dpdk-devbind.py -b vfio-pci 08:00.1
ERROR: Driver 'vfio-pci' is not loaded.

Nice and informative!

Additionally, since we're outputting our new error messages to stderr, i took
the liberty of adjusting all the rest of the error messages to also output to
stderr.

v4:
- Corrected mixing of tabs and spaces in the first patch as well

v3:
- Corrected mixing of tabs and spaces. Spaces all the way down!

v2:
- Addressed Stephen's feedback
- Added new patch adjusting error output to stderr

Anatoly Burakov (3):
  usertools/devbind: add error on forgetting to specify driver
  usertools/devbind: check if module is loaded before binding
  usertools/devbind: print all errors to stderr

 usertools/dpdk-devbind.py | 144 ++++++++++++++++++++++----------------
 1 file changed, 83 insertions(+), 61 deletions(-)

-- 
2.17.1

Re: [dpdk-dev] [PATCH v4 0/3] Small usability improvements for devbind

[Thomas Monjalon]

> Anatoly Burakov (3):
>   usertools/devbind: add error on forgetting to specify driver
>   usertools/devbind: check if module is loaded before binding
>   usertools/devbind: print all errors to stderr

Applied, thanks

[dpdk-dev] [PATCH v4 1/3] usertools/devbind: add error on forgetting to specify driver

[Anatoly Burakov]

A common user error is to forget driver to which the PCI devices should
be bound to. Currently, the error message in this case looks unhelpful
misleading and indecipherable to anyone but people who know how devbind
works.

Fix this by checking if the driver string is actually a valid device
string. If it is, we assume that the user has just forgot to specify the
driver, and display appropriate error. We also assume that no one will
name their driver in a format that looks like a PCI address, but that
seems like a reasonable assumption to make.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 542ecffcc..b32506e3d 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -342,9 +342,8 @@ def dev_id_from_dev_name(dev_name):
             if dev_name in devices[d]["Interface"].split(","):
                 return devices[d]["Slot"]
     # if nothing else matches - error
-    print("Unknown device: %s. "
-          "Please specify device in \"bus:slot.func\" format" % dev_name)
-    sys.exit(1)
+    raise ValueError("Unknown device: %s. "
+                     "Please specify device in \"bus:slot.func\" format" % dev_name)
 
 
 def unbind_one(dev_id, force):
@@ -493,7 +492,12 @@ def unbind_all(dev_list, force=False):
                     unbind_one(devices[d]["Slot"], force)
         return
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        print(ex)
+        sys.exit(1)
+
     for d in dev_list:
         unbind_one(d, force)
 
@@ -502,7 +506,23 @@ def bind_all(dev_list, driver, force=False):
     """Bind method, takes a list of device locations"""
     global devices
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    # a common user error is to forget to specify the driver the devices need to
+    # be bound to. check if the driver is a valid device, and if it is, show
+    # a meaningful error.
+    try:
+        dev_id_from_dev_name(driver)
+        # if we've made it this far, this means that the "driver" was a valid
+        # device string, so it's probably not a valid driver name.
+        sys.exit("Error: Driver '%s' does not look like a valid driver. " \
+                 "Did you forget to specify the driver to bind devices to?" % driver)
+    except ValueError:
+        # driver generated error - it's not a valid device ID, so all is well
+        pass
+
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        sys.exit(ex)
 
     for d in dev_list:
         bind_one(d, driver, force)
-- 
2.17.1

[dpdk-dev] [PATCH v4 2/3] usertools/devbind: check if module is loaded before binding

[Anatoly Burakov]

Currently, if an attempt is made to bind a device to a driver that
is not loaded, a confusing and misleading error message appears.
Fix it so that, before binding to the driver, we actually check if
it is loaded in the kernel first.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 58 +++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index b32506e3d..d3b16240f 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -60,6 +60,8 @@
 devices = {}
 # list of supported DPDK drivers
 dpdk_drivers = ["igb_uio", "vfio-pci", "uio_pci_generic"]
+# list of currently loaded kernel modules
+loaded_modules = None
 
 # command-line arg flags
 b_flag = None
@@ -146,6 +148,28 @@ def check_output(args, stderr=None):
     return subprocess.Popen(args, stdout=subprocess.PIPE,
                             stderr=stderr).communicate()[0]
 
+# check if a specific kernel module is loaded
+def module_is_loaded(module):
+    global loaded_modules
+
+    if loaded_modules:
+        return module in loaded_modules
+
+    # Get list of sysfs modules (both built-in and dynamically loaded)
+    sysfs_path = '/sys/module/'
+
+    # Get the list of directories in sysfs_path
+    sysfs_mods = [m for m in os.listdir(sysfs_path)
+                  if os.path.isdir(os.path.join(sysfs_path, m))]
+
+    # special case for vfio_pci (module is named vfio-pci,
+    # but its .ko is named vfio_pci)
+    sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
+
+    loaded_modules = sysfs_mods
+
+    return module in sysfs_mods
+
 
 def check_modules():
     '''Checks that igb_uio is loaded'''
@@ -155,35 +179,13 @@ def check_modules():
     mods = [{"Name": driver, "Found": False} for driver in dpdk_drivers]
 
     # first check if module is loaded
-    try:
-        # Get list of sysfs modules (both built-in and dynamically loaded)
-        sysfs_path = '/sys/module/'
-
-        # Get the list of directories in sysfs_path
-        sysfs_mods = [os.path.join(sysfs_path, o) for o
-                      in os.listdir(sysfs_path)
-                      if os.path.isdir(os.path.join(sysfs_path, o))]
-
-        # Extract the last element of '/sys/module/abc' in the array
-        sysfs_mods = [a.split('/')[-1] for a in sysfs_mods]
-
-        # special case for vfio_pci (module is named vfio-pci,
-        # but its .ko is named vfio_pci)
-        sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
-
-        for mod in mods:
-            if mod["Name"] in sysfs_mods:
-                mod["Found"] = True
-    except:
-        pass
+    for mod in mods:
+        if module_is_loaded(mod["Name"]):
+            mod["Found"] = True
 
     # check if we have at least one loaded module
     if True not in [mod["Found"] for mod in mods] and b_flag is not None:
-        if b_flag in dpdk_drivers:
-            print("Error - no supported modules(DPDK driver) are loaded")
-            sys.exit(1)
-        else:
-            print("Warning - no supported modules(DPDK driver) are loaded")
+        print("Warning: no supported DPDK kernel modules are loaded")
 
     # change DPDK driver list to only contain drivers that are loaded
     dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
@@ -519,6 +521,10 @@ def bind_all(dev_list, driver, force=False):
         # driver generated error - it's not a valid device ID, so all is well
         pass
 
+    # check if we're attempting to bind to a driver that isn't loaded
+    if not module_is_loaded(driver):
+        sys.exit("Error: Driver '%s' is not loaded." % driver)
+
     try:
         dev_list = map(dev_id_from_dev_name, dev_list)
     except ValueError as ex:
-- 
2.17.1

[dpdk-dev] [PATCH v4 3/3] usertools/devbind: print all errors to stderr

[Anatoly Burakov]

Bring consistency to error messages and output them to stderr.
Als, whenever the script tells the user to "check usage", don't
tell the user to do it and just display usage instead.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 58 ++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index d3b16240f..7b5cbc12c 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -3,6 +3,7 @@
 # Copyright(c) 2010-2014 Intel Corporation
 #
 
+from __future__ import print_function
 import sys
 import os
 import getopt
@@ -185,7 +186,7 @@ def check_modules():
 
     # check if we have at least one loaded module
     if True not in [mod["Found"] for mod in mods] and b_flag is not None:
-        print("Warning: no supported DPDK kernel modules are loaded")
+        print("Warning: no supported DPDK kernel modules are loaded", file=sys.stderr)
 
     # change DPDK driver list to only contain drivers that are loaded
     dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
@@ -352,14 +353,14 @@ def unbind_one(dev_id, force):
     '''Unbind the device identified by "dev_id" from its current driver'''
     dev = devices[dev_id]
     if not has_driver(dev_id):
-        print("%s %s %s is not currently managed by any driver\n" %
-              (dev["Slot"], dev["Device_str"], dev["Interface"]))
+        print("Notice: %s %s %s is not currently managed by any driver" %
+              (dev["Slot"], dev["Device_str"], dev["Interface"]), file=sys.stderr)
         return
 
     # prevent us disconnecting ourselves
     if dev["Ssh_if"] and not force:
-        print("Routing table indicates that interface %s is active. "
-              "Skipping unbind" % (dev_id))
+        print("Warning: routing table indicates that interface %s is active. "
+              "Skipping unbind" % dev_id, file=sys.stderr)
         return
 
     # write to /sys to unbind
@@ -367,9 +368,8 @@ def unbind_one(dev_id, force):
     try:
         f = open(filename, "a")
     except:
-        print("Error: unbind failed for %s - Cannot open %s"
-              % (dev_id, filename))
-        sys.exit(1)
+        sys.exit("Error: unbind failed for %s - Cannot open %s" %
+                 (dev_id, filename))
     f.write(dev_id)
     f.close()
 
@@ -382,15 +382,15 @@ def bind_one(dev_id, driver, force):
 
     # prevent disconnection of our ssh session
     if dev["Ssh_if"] and not force:
-        print("Routing table indicates that interface %s is active. "
-              "Not modifying" % (dev_id))
+        print("Warning: routing table indicates that interface %s is active. "
+              "Not modifying" % dev_id, file=sys.stderr)
         return
 
     # unbind any existing drivers we don't want
     if has_driver(dev_id):
         if dev["Driver_str"] == driver:
-            print("%s already bound to driver %s, skipping\n"
-                  % (dev_id, driver))
+            print("Notice: %s already bound to driver %s, skipping" %
+                  (dev_id, driver), file=sys.stderr)
             return
         else:
             saved_driver = dev["Driver_str"]
@@ -410,14 +410,14 @@ def bind_one(dev_id, driver, force):
                 f = open(filename, "w")
             except:
                 print("Error: bind failed for %s - Cannot open %s"
-                      % (dev_id, filename))
+                      % (dev_id, filename), file=sys.stderr)
                 return
             try:
                 f.write("%s" % driver)
                 f.close()
             except:
                 print("Error: bind failed for %s - Cannot write driver %s to "
-                      "PCI ID " % (dev_id, driver))
+                      "PCI ID " % (dev_id, driver), file=sys.stderr)
                 return
         # For kernels < 3.15 use new_id to add PCI id's to the driver
         else:
@@ -426,7 +426,7 @@ def bind_one(dev_id, driver, force):
                 f = open(filename, "w")
             except:
                 print("Error: bind failed for %s - Cannot open %s"
-                      % (dev_id, filename))
+                      % (dev_id, filename), file=sys.stderr)
                 return
             try:
                 # Convert Device and Vendor Id to int to write to new_id
@@ -435,7 +435,7 @@ def bind_one(dev_id, driver, force):
                 f.close()
             except:
                 print("Error: bind failed for %s - Cannot write new PCI ID to "
-                      "driver %s" % (dev_id, driver))
+                      "driver %s" % (dev_id, driver), file=sys.stderr)
                 return
 
     # do the bind by writing to /sys
@@ -444,7 +444,7 @@ def bind_one(dev_id, driver, force):
         f = open(filename, "a")
     except:
         print("Error: bind failed for %s - Cannot open %s"
-              % (dev_id, filename))
+              % (dev_id, filename), file=sys.stderr)
         if saved_driver is not None:  # restore any previous driver
             bind_one(dev_id, saved_driver, force)
         return
@@ -459,7 +459,7 @@ def bind_one(dev_id, driver, force):
         if "Driver_str" in tmp and tmp["Driver_str"] == driver:
             return
         print("Error: bind failed for %s - Cannot bind to driver %s"
-              % (dev_id, driver))
+              % (dev_id, driver), file=sys.stderr)
         if saved_driver is not None:  # restore any previous driver
             bind_one(dev_id, saved_driver, force)
         return
@@ -472,16 +472,14 @@ def bind_one(dev_id, driver, force):
         try:
             f = open(filename, "w")
         except:
-            print("Error: unbind failed for %s - Cannot open %s"
+            sys.exit("Error: unbind failed for %s - Cannot open %s"
                   % (dev_id, filename))
-            sys.exit(1)
         try:
             f.write("\00")
             f.close()
         except:
-            print("Error: unbind failed for %s - Cannot open %s"
+            sys.exit("Error: unbind failed for %s - Cannot open %s"
                   % (dev_id, filename))
-            sys.exit(1)
 
 
 def unbind_all(dev_list, force=False):
@@ -676,8 +674,7 @@ def parse_args():
             force_flag = True
         if opt == "-b" or opt == "-u" or opt == "--bind" or opt == "--unbind":
             if b_flag is not None:
-                print("Error - Only one bind or unbind may be specified\n")
-                sys.exit(1)
+                sys.exit("Error: binding and unbinding are mutually exclusive")
             if opt == "-u" or opt == "--unbind":
                 b_flag = "none"
             else:
@@ -692,14 +689,14 @@ def do_arg_actions():
     global args
 
     if b_flag is None and not status_flag:
-        print("Error: No action specified for devices."
-              "Please give a -b or -u option")
-        print("Run '%s --usage' for further information" % sys.argv[0])
+        print("Error: No action specified for devices. "
+              "Please give a -b or -u option", file=sys.stderr)
+        usage()
         sys.exit(1)
 
     if b_flag is not None and len(args) == 0:
-        print("Error: No devices specified.")
-        print("Run '%s --usage' for further information" % sys.argv[0])
+        print("Error: No devices specified.", file=sys.stderr)
+        usage()
         sys.exit(1)
 
     if b_flag == "none" or b_flag == "None":
@@ -727,8 +724,7 @@ def main():
         ret = subprocess.call(['which', 'lspci'],
                               stdout=devnull, stderr=devnull)
         if ret != 0:
-            print("'lspci' not found - please install 'pciutils'")
-            sys.exit(1)
+            sys.exit("'lspci' not found - please install 'pciutils'")
     parse_args()
     check_modules()
     clear_data()
-- 
2.17.1

[dpdk-dev] [PATCH v3 1/3] usertools/devbind: add error on forgetting to specify driver

[Anatoly Burakov]

A common user error is to forget driver to which the PCI devices should
be bound to. Currently, the error message in this case looks unhelpful
misleading and indecipherable to anyone but people who know how devbind
works.

Fix this by checking if the driver string is actually a valid device
string. If it is, we assume that the user has just forgot to specify the
driver, and display appropriate error. We also assume that no one will
name their driver in a format that looks like a PCI address, but that
seems like a reasonable assumption to make.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 542ecffcc..fca79e66d 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -342,9 +342,8 @@ def dev_id_from_dev_name(dev_name):
             if dev_name in devices[d]["Interface"].split(","):
                 return devices[d]["Slot"]
     # if nothing else matches - error
-    print("Unknown device: %s. "
-          "Please specify device in \"bus:slot.func\" format" % dev_name)
-    sys.exit(1)
+    raise ValueError("Unknown device: %s. "
+	    "Please specify device in \"bus:slot.func\" format" % dev_name)
 
 
 def unbind_one(dev_id, force):
@@ -493,7 +492,12 @@ def unbind_all(dev_list, force=False):
                     unbind_one(devices[d]["Slot"], force)
         return
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        print(ex)
+        sys.exit(1)
+
     for d in dev_list:
         unbind_one(d, force)
 
@@ -502,7 +506,23 @@ def bind_all(dev_list, driver, force=False):
     """Bind method, takes a list of device locations"""
     global devices
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    # a common user error is to forget to specify the driver the devices need to
+    # be bound to. check if the driver is a valid device, and if it is, show
+    # a meaningful error.
+    try:
+        dev_id_from_dev_name(driver)
+        # if we've made it this far, this means that the "driver" was a valid
+        # device string, so it's probably not a valid driver name.
+        sys.exit("Error: Driver '%s' does not look like a valid driver. " \
+                 "Did you forget to specify the driver to bind devices to?" % driver)
+    except ValueError:
+        # driver generated error - it's not a valid device ID, so all is well
+        pass
+
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        sys.exit(ex)
 
     for d in dev_list:
         bind_one(d, driver, force)
-- 
2.17.1

[dpdk-dev] [PATCH v3 2/3] usertools/devbind: check if module is loaded before binding

[Anatoly Burakov]

Currently, if an attempt is made to bind a device to a driver that
is not loaded, a confusing and misleading error message appears.
Fix it so that, before binding to the driver, we actually check if
it is loaded in the kernel first.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 58 +++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index fca79e66d..55d9d1a57 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -60,6 +60,8 @@
 devices = {}
 # list of supported DPDK drivers
 dpdk_drivers = ["igb_uio", "vfio-pci", "uio_pci_generic"]
+# list of currently loaded kernel modules
+loaded_modules = None
 
 # command-line arg flags
 b_flag = None
@@ -146,6 +148,28 @@ def check_output(args, stderr=None):
     return subprocess.Popen(args, stdout=subprocess.PIPE,
                             stderr=stderr).communicate()[0]
 
+# check if a specific kernel module is loaded
+def module_is_loaded(module):
+    global loaded_modules
+
+    if loaded_modules:
+        return module in loaded_modules
+
+    # Get list of sysfs modules (both built-in and dynamically loaded)
+    sysfs_path = '/sys/module/'
+
+    # Get the list of directories in sysfs_path
+    sysfs_mods = [m for m in os.listdir(sysfs_path)
+                  if os.path.isdir(os.path.join(sysfs_path, m))]
+
+    # special case for vfio_pci (module is named vfio-pci,
+    # but its .ko is named vfio_pci)
+    sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
+
+    loaded_modules = sysfs_mods
+
+    return module in sysfs_mods
+
 
 def check_modules():
     '''Checks that igb_uio is loaded'''
@@ -155,35 +179,13 @@ def check_modules():
     mods = [{"Name": driver, "Found": False} for driver in dpdk_drivers]
 
     # first check if module is loaded
-    try:
-        # Get list of sysfs modules (both built-in and dynamically loaded)
-        sysfs_path = '/sys/module/'
-
-        # Get the list of directories in sysfs_path
-        sysfs_mods = [os.path.join(sysfs_path, o) for o
-                      in os.listdir(sysfs_path)
-                      if os.path.isdir(os.path.join(sysfs_path, o))]
-
-        # Extract the last element of '/sys/module/abc' in the array
-        sysfs_mods = [a.split('/')[-1] for a in sysfs_mods]
-
-        # special case for vfio_pci (module is named vfio-pci,
-        # but its .ko is named vfio_pci)
-        sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
-
-        for mod in mods:
-            if mod["Name"] in sysfs_mods:
-                mod["Found"] = True
-    except:
-        pass
+    for mod in mods:
+        if module_is_loaded(mod["Name"]):
+            mod["Found"] = True
 
     # check if we have at least one loaded module
     if True not in [mod["Found"] for mod in mods] and b_flag is not None:
-        if b_flag in dpdk_drivers:
-            print("Error - no supported modules(DPDK driver) are loaded")
-            sys.exit(1)
-        else:
-            print("Warning - no supported modules(DPDK driver) are loaded")
+        print("Warning: no supported DPDK kernel modules are loaded")
 
     # change DPDK driver list to only contain drivers that are loaded
     dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
@@ -519,6 +521,10 @@ def bind_all(dev_list, driver, force=False):
         # driver generated error - it's not a valid device ID, so all is well
         pass
 
+    # check if we're attempting to bind to a driver that isn't loaded
+    if not module_is_loaded(driver):
+        sys.exit("Error: Driver '%s' is not loaded." % driver)
+
     try:
         dev_list = map(dev_id_from_dev_name, dev_list)
     except ValueError as ex:
-- 
2.17.1

[dpdk-dev] [PATCH v3 3/3] usertools/devbind: print all errors to stderr

[Anatoly Burakov]

Bring consistency to error messages and output them to stderr.
Als, whenever the script tells the user to "check usage", don't
tell the user to do it and just display usage instead.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 58 ++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 55d9d1a57..c19511ffd 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -3,6 +3,7 @@
 # Copyright(c) 2010-2014 Intel Corporation
 #
 
+from __future__ import print_function
 import sys
 import os
 import getopt
@@ -185,7 +186,7 @@ def check_modules():
 
     # check if we have at least one loaded module
     if True not in [mod["Found"] for mod in mods] and b_flag is not None:
-        print("Warning: no supported DPDK kernel modules are loaded")
+        print("Warning: no supported DPDK kernel modules are loaded", file=sys.stderr)
 
     # change DPDK driver list to only contain drivers that are loaded
     dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
@@ -352,14 +353,14 @@ def unbind_one(dev_id, force):
     '''Unbind the device identified by "dev_id" from its current driver'''
     dev = devices[dev_id]
     if not has_driver(dev_id):
-        print("%s %s %s is not currently managed by any driver\n" %
-              (dev["Slot"], dev["Device_str"], dev["Interface"]))
+        print("Notice: %s %s %s is not currently managed by any driver" %
+              (dev["Slot"], dev["Device_str"], dev["Interface"]), file=sys.stderr)
         return
 
     # prevent us disconnecting ourselves
     if dev["Ssh_if"] and not force:
-        print("Routing table indicates that interface %s is active. "
-              "Skipping unbind" % (dev_id))
+        print("Warning: routing table indicates that interface %s is active. "
+              "Skipping unbind" % dev_id, file=sys.stderr)
         return
 
     # write to /sys to unbind
@@ -367,9 +368,8 @@ def unbind_one(dev_id, force):
     try:
         f = open(filename, "a")
     except:
-        print("Error: unbind failed for %s - Cannot open %s"
-              % (dev_id, filename))
-        sys.exit(1)
+        sys.exit("Error: unbind failed for %s - Cannot open %s" %
+                 (dev_id, filename))
     f.write(dev_id)
     f.close()
 
@@ -382,15 +382,15 @@ def bind_one(dev_id, driver, force):
 
     # prevent disconnection of our ssh session
     if dev["Ssh_if"] and not force:
-        print("Routing table indicates that interface %s is active. "
-              "Not modifying" % (dev_id))
+        print("Warning: routing table indicates that interface %s is active. "
+              "Not modifying" % dev_id, file=sys.stderr)
         return
 
     # unbind any existing drivers we don't want
     if has_driver(dev_id):
         if dev["Driver_str"] == driver:
-            print("%s already bound to driver %s, skipping\n"
-                  % (dev_id, driver))
+            print("Notice: %s already bound to driver %s, skipping" %
+                  (dev_id, driver), file=sys.stderr)
             return
         else:
             saved_driver = dev["Driver_str"]
@@ -410,14 +410,14 @@ def bind_one(dev_id, driver, force):
                 f = open(filename, "w")
             except:
                 print("Error: bind failed for %s - Cannot open %s"
-                      % (dev_id, filename))
+                      % (dev_id, filename), file=sys.stderr)
                 return
             try:
                 f.write("%s" % driver)
                 f.close()
             except:
                 print("Error: bind failed for %s - Cannot write driver %s to "
-                      "PCI ID " % (dev_id, driver))
+                      "PCI ID " % (dev_id, driver), file=sys.stderr)
                 return
         # For kernels < 3.15 use new_id to add PCI id's to the driver
         else:
@@ -426,7 +426,7 @@ def bind_one(dev_id, driver, force):
                 f = open(filename, "w")
             except:
                 print("Error: bind failed for %s - Cannot open %s"
-                      % (dev_id, filename))
+                      % (dev_id, filename), file=sys.stderr)
                 return
             try:
                 # Convert Device and Vendor Id to int to write to new_id
@@ -435,7 +435,7 @@ def bind_one(dev_id, driver, force):
                 f.close()
             except:
                 print("Error: bind failed for %s - Cannot write new PCI ID to "
-                      "driver %s" % (dev_id, driver))
+                      "driver %s" % (dev_id, driver), file=sys.stderr)
                 return
 
     # do the bind by writing to /sys
@@ -444,7 +444,7 @@ def bind_one(dev_id, driver, force):
         f = open(filename, "a")
     except:
         print("Error: bind failed for %s - Cannot open %s"
-              % (dev_id, filename))
+              % (dev_id, filename), file=sys.stderr)
         if saved_driver is not None:  # restore any previous driver
             bind_one(dev_id, saved_driver, force)
         return
@@ -459,7 +459,7 @@ def bind_one(dev_id, driver, force):
         if "Driver_str" in tmp and tmp["Driver_str"] == driver:
             return
         print("Error: bind failed for %s - Cannot bind to driver %s"
-              % (dev_id, driver))
+              % (dev_id, driver), file=sys.stderr)
         if saved_driver is not None:  # restore any previous driver
             bind_one(dev_id, saved_driver, force)
         return
@@ -472,16 +472,14 @@ def bind_one(dev_id, driver, force):
         try:
             f = open(filename, "w")
         except:
-            print("Error: unbind failed for %s - Cannot open %s"
+            sys.exit("Error: unbind failed for %s - Cannot open %s"
                   % (dev_id, filename))
-            sys.exit(1)
         try:
             f.write("\00")
             f.close()
         except:
-            print("Error: unbind failed for %s - Cannot open %s"
+            sys.exit("Error: unbind failed for %s - Cannot open %s"
                   % (dev_id, filename))
-            sys.exit(1)
 
 
 def unbind_all(dev_list, force=False):
@@ -676,8 +674,7 @@ def parse_args():
             force_flag = True
         if opt == "-b" or opt == "-u" or opt == "--bind" or opt == "--unbind":
             if b_flag is not None:
-                print("Error - Only one bind or unbind may be specified\n")
-                sys.exit(1)
+                sys.exit("Error: binding and unbinding are mutually exclusive")
             if opt == "-u" or opt == "--unbind":
                 b_flag = "none"
             else:
@@ -692,14 +689,14 @@ def do_arg_actions():
     global args
 
     if b_flag is None and not status_flag:
-        print("Error: No action specified for devices."
-              "Please give a -b or -u option")
-        print("Run '%s --usage' for further information" % sys.argv[0])
+        print("Error: No action specified for devices. "
+              "Please give a -b or -u option", file=sys.stderr)
+        usage()
         sys.exit(1)
 
     if b_flag is not None and len(args) == 0:
-        print("Error: No devices specified.")
-        print("Run '%s --usage' for further information" % sys.argv[0])
+        print("Error: No devices specified.", file=sys.stderr)
+        usage()
         sys.exit(1)
 
     if b_flag == "none" or b_flag == "None":
@@ -727,8 +724,7 @@ def main():
         ret = subprocess.call(['which', 'lspci'],
                               stdout=devnull, stderr=devnull)
         if ret != 0:
-            print("'lspci' not found - please install 'pciutils'")
-            sys.exit(1)
+            sys.exit("'lspci' not found - please install 'pciutils'")
     parse_args()
     check_modules()
     clear_data()
-- 
2.17.1

[dpdk-dev] [PATCH v2 1/3] usertools/devbind: add error on forgetting to specify driver

[Anatoly Burakov]

A common user error is to forget driver to which the PCI devices should
be bound to. Currently, the error message in this case looks unhelpful
misleading and indecipherable to anyone but people who know how devbind
works.

Fix this by checking if the driver string is actually a valid device
string. If it is, we assume that the user has just forgot to specify the
driver, and display appropriate error. We also assume that no one will
name their driver in a format that looks like a PCI address, but that
seems like a reasonable assumption to make.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 542ecffcc..fca79e66d 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -342,9 +342,8 @@ def dev_id_from_dev_name(dev_name):
             if dev_name in devices[d]["Interface"].split(","):
                 return devices[d]["Slot"]
     # if nothing else matches - error
-    print("Unknown device: %s. "
-          "Please specify device in \"bus:slot.func\" format" % dev_name)
-    sys.exit(1)
+    raise ValueError("Unknown device: %s. "
+	    "Please specify device in \"bus:slot.func\" format" % dev_name)
 
 
 def unbind_one(dev_id, force):
@@ -493,7 +492,12 @@ def unbind_all(dev_list, force=False):
                     unbind_one(devices[d]["Slot"], force)
         return
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        print(ex)
+        sys.exit(1)
+
     for d in dev_list:
         unbind_one(d, force)
 
@@ -502,7 +506,23 @@ def bind_all(dev_list, driver, force=False):
     """Bind method, takes a list of device locations"""
     global devices
 
-    dev_list = map(dev_id_from_dev_name, dev_list)
+    # a common user error is to forget to specify the driver the devices need to
+    # be bound to. check if the driver is a valid device, and if it is, show
+    # a meaningful error.
+    try:
+        dev_id_from_dev_name(driver)
+        # if we've made it this far, this means that the "driver" was a valid
+        # device string, so it's probably not a valid driver name.
+        sys.exit("Error: Driver '%s' does not look like a valid driver. " \
+                 "Did you forget to specify the driver to bind devices to?" % driver)
+    except ValueError:
+        # driver generated error - it's not a valid device ID, so all is well
+        pass
+
+    try:
+        dev_list = map(dev_id_from_dev_name, dev_list)
+    except ValueError as ex:
+        sys.exit(ex)
 
     for d in dev_list:
         bind_one(d, driver, force)
-- 
2.17.1

[dpdk-dev] [PATCH v2 2/3] usertools/devbind: check if module is loaded before binding

[Anatoly Burakov]

Currently, if an attempt is made to bind a device to a driver that
is not loaded, a confusing and misleading error message appears.
Fix it so that, before binding to the driver, we actually check if
it is loaded in the kernel first.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 58 +++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index fca79e66d..55d9d1a57 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -60,6 +60,8 @@
 devices = {}
 # list of supported DPDK drivers
 dpdk_drivers = ["igb_uio", "vfio-pci", "uio_pci_generic"]
+# list of currently loaded kernel modules
+loaded_modules = None
 
 # command-line arg flags
 b_flag = None
@@ -146,6 +148,28 @@ def check_output(args, stderr=None):
     return subprocess.Popen(args, stdout=subprocess.PIPE,
                             stderr=stderr).communicate()[0]
 
+# check if a specific kernel module is loaded
+def module_is_loaded(module):
+    global loaded_modules
+
+    if loaded_modules:
+        return module in loaded_modules
+
+    # Get list of sysfs modules (both built-in and dynamically loaded)
+    sysfs_path = '/sys/module/'
+
+    # Get the list of directories in sysfs_path
+    sysfs_mods = [m for m in os.listdir(sysfs_path)
+                  if os.path.isdir(os.path.join(sysfs_path, m))]
+
+    # special case for vfio_pci (module is named vfio-pci,
+    # but its .ko is named vfio_pci)
+    sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
+
+    loaded_modules = sysfs_mods
+
+    return module in sysfs_mods
+
 
 def check_modules():
     '''Checks that igb_uio is loaded'''
@@ -155,35 +179,13 @@ def check_modules():
     mods = [{"Name": driver, "Found": False} for driver in dpdk_drivers]
 
     # first check if module is loaded
-    try:
-        # Get list of sysfs modules (both built-in and dynamically loaded)
-        sysfs_path = '/sys/module/'
-
-        # Get the list of directories in sysfs_path
-        sysfs_mods = [os.path.join(sysfs_path, o) for o
-                      in os.listdir(sysfs_path)
-                      if os.path.isdir(os.path.join(sysfs_path, o))]
-
-        # Extract the last element of '/sys/module/abc' in the array
-        sysfs_mods = [a.split('/')[-1] for a in sysfs_mods]
-
-        # special case for vfio_pci (module is named vfio-pci,
-        # but its .ko is named vfio_pci)
-        sysfs_mods = [a if a != 'vfio_pci' else 'vfio-pci' for a in sysfs_mods]
-
-        for mod in mods:
-            if mod["Name"] in sysfs_mods:
-                mod["Found"] = True
-    except:
-        pass
+    for mod in mods:
+        if module_is_loaded(mod["Name"]):
+            mod["Found"] = True
 
     # check if we have at least one loaded module
     if True not in [mod["Found"] for mod in mods] and b_flag is not None:
-        if b_flag in dpdk_drivers:
-            print("Error - no supported modules(DPDK driver) are loaded")
-            sys.exit(1)
-        else:
-            print("Warning - no supported modules(DPDK driver) are loaded")
+        print("Warning: no supported DPDK kernel modules are loaded")
 
     # change DPDK driver list to only contain drivers that are loaded
     dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
@@ -519,6 +521,10 @@ def bind_all(dev_list, driver, force=False):
         # driver generated error - it's not a valid device ID, so all is well
         pass
 
+    # check if we're attempting to bind to a driver that isn't loaded
+    if not module_is_loaded(driver):
+        sys.exit("Error: Driver '%s' is not loaded." % driver)
+
     try:
         dev_list = map(dev_id_from_dev_name, dev_list)
     except ValueError as ex:
-- 
2.17.1

[dpdk-dev] [PATCH v2 3/3] usertools/devbind: print all errors to stderr

[Anatoly Burakov]

Bring consistency to error messages and output them to stderr.
Als, whenever the script tells the user to "check usage", don't
tell the user to do it and just display usage instead.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 usertools/dpdk-devbind.py | 58 ++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 55d9d1a57..c19511ffd 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -3,6 +3,7 @@
 # Copyright(c) 2010-2014 Intel Corporation
 #
 
+from __future__ import print_function
 import sys
 import os
 import getopt
@@ -185,7 +186,7 @@ def check_modules():
 
     # check if we have at least one loaded module
     if True not in [mod["Found"] for mod in mods] and b_flag is not None:
-        print("Warning: no supported DPDK kernel modules are loaded")
+        print("Warning: no supported DPDK kernel modules are loaded", file=sys.stderr)
 
     # change DPDK driver list to only contain drivers that are loaded
     dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
@@ -352,14 +353,14 @@ def unbind_one(dev_id, force):
     '''Unbind the device identified by "dev_id" from its current driver'''
     dev = devices[dev_id]
     if not has_driver(dev_id):
-        print("%s %s %s is not currently managed by any driver\n" %
-              (dev["Slot"], dev["Device_str"], dev["Interface"]))
+        print("Notice: %s %s %s is not currently managed by any driver" %
+              (dev["Slot"], dev["Device_str"], dev["Interface"]), file=sys.stderr)
         return
 
     # prevent us disconnecting ourselves
     if dev["Ssh_if"] and not force:
-        print("Routing table indicates that interface %s is active. "
-              "Skipping unbind" % (dev_id))
+        print("Warning: routing table indicates that interface %s is active. "
+              "Skipping unbind" % dev_id, file=sys.stderr)
         return
 
     # write to /sys to unbind
@@ -367,9 +368,8 @@ def unbind_one(dev_id, force):
     try:
         f = open(filename, "a")
     except:
-        print("Error: unbind failed for %s - Cannot open %s"
-              % (dev_id, filename))
-        sys.exit(1)
+        sys.exit("Error: unbind failed for %s - Cannot open %s" %
+                 (dev_id, filename))
     f.write(dev_id)
     f.close()
 
@@ -382,15 +382,15 @@ def bind_one(dev_id, driver, force):
 
     # prevent disconnection of our ssh session
     if dev["Ssh_if"] and not force:
-        print("Routing table indicates that interface %s is active. "
-              "Not modifying" % (dev_id))
+        print("Warning: routing table indicates that interface %s is active. "
+              "Not modifying" % dev_id, file=sys.stderr)
         return
 
     # unbind any existing drivers we don't want
     if has_driver(dev_id):
         if dev["Driver_str"] == driver:
-            print("%s already bound to driver %s, skipping\n"
-                  % (dev_id, driver))
+            print("Notice: %s already bound to driver %s, skipping" %
+                  (dev_id, driver), file=sys.stderr)
             return
         else:
             saved_driver = dev["Driver_str"]
@@ -410,14 +410,14 @@ def bind_one(dev_id, driver, force):
                 f = open(filename, "w")
             except:
                 print("Error: bind failed for %s - Cannot open %s"
-                      % (dev_id, filename))
+                      % (dev_id, filename), file=sys.stderr)
                 return
             try:
                 f.write("%s" % driver)
                 f.close()
             except:
                 print("Error: bind failed for %s - Cannot write driver %s to "
-                      "PCI ID " % (dev_id, driver))
+                      "PCI ID " % (dev_id, driver), file=sys.stderr)
                 return
         # For kernels < 3.15 use new_id to add PCI id's to the driver
         else:
@@ -426,7 +426,7 @@ def bind_one(dev_id, driver, force):
                 f = open(filename, "w")
             except:
                 print("Error: bind failed for %s - Cannot open %s"
-                      % (dev_id, filename))
+                      % (dev_id, filename), file=sys.stderr)
                 return
             try:
                 # Convert Device and Vendor Id to int to write to new_id
@@ -435,7 +435,7 @@ def bind_one(dev_id, driver, force):
                 f.close()
             except:
                 print("Error: bind failed for %s - Cannot write new PCI ID to "
-                      "driver %s" % (dev_id, driver))
+                      "driver %s" % (dev_id, driver), file=sys.stderr)
                 return
 
     # do the bind by writing to /sys
@@ -444,7 +444,7 @@ def bind_one(dev_id, driver, force):
         f = open(filename, "a")
     except:
         print("Error: bind failed for %s - Cannot open %s"
-              % (dev_id, filename))
+              % (dev_id, filename), file=sys.stderr)
         if saved_driver is not None:  # restore any previous driver
             bind_one(dev_id, saved_driver, force)
         return
@@ -459,7 +459,7 @@ def bind_one(dev_id, driver, force):
         if "Driver_str" in tmp and tmp["Driver_str"] == driver:
             return
         print("Error: bind failed for %s - Cannot bind to driver %s"
-              % (dev_id, driver))
+              % (dev_id, driver), file=sys.stderr)
         if saved_driver is not None:  # restore any previous driver
             bind_one(dev_id, saved_driver, force)
         return
@@ -472,16 +472,14 @@ def bind_one(dev_id, driver, force):
         try:
             f = open(filename, "w")
         except:
-            print("Error: unbind failed for %s - Cannot open %s"
+            sys.exit("Error: unbind failed for %s - Cannot open %s"
                   % (dev_id, filename))
-            sys.exit(1)
         try:
             f.write("\00")
             f.close()
         except:
-            print("Error: unbind failed for %s - Cannot open %s"
+            sys.exit("Error: unbind failed for %s - Cannot open %s"
                   % (dev_id, filename))
-            sys.exit(1)
 
 
 def unbind_all(dev_list, force=False):
@@ -676,8 +674,7 @@ def parse_args():
             force_flag = True
         if opt == "-b" or opt == "-u" or opt == "--bind" or opt == "--unbind":
             if b_flag is not None:
-                print("Error - Only one bind or unbind may be specified\n")
-                sys.exit(1)
+                sys.exit("Error: binding and unbinding are mutually exclusive")
             if opt == "-u" or opt == "--unbind":
                 b_flag = "none"
             else:
@@ -692,14 +689,14 @@ def do_arg_actions():
     global args
 
     if b_flag is None and not status_flag:
-        print("Error: No action specified for devices."
-              "Please give a -b or -u option")
-        print("Run '%s --usage' for further information" % sys.argv[0])
+        print("Error: No action specified for devices. "
+              "Please give a -b or -u option", file=sys.stderr)
+        usage()
         sys.exit(1)
 
     if b_flag is not None and len(args) == 0:
-        print("Error: No devices specified.")
-        print("Run '%s --usage' for further information" % sys.argv[0])
+        print("Error: No devices specified.", file=sys.stderr)
+        usage()
         sys.exit(1)
 
     if b_flag == "none" or b_flag == "None":
@@ -727,8 +724,7 @@ def main():
         ret = subprocess.call(['which', 'lspci'],
                               stdout=devnull, stderr=devnull)
         if ret != 0:
-            print("'lspci' not found - please install 'pciutils'")
-            sys.exit(1)
+            sys.exit("'lspci' not found - please install 'pciutils'")
     parse_args()
     check_modules()
     clear_data()
-- 
2.17.1