Re: [PATCH v3 0/2] Rewrite devbind

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed,  4 Dec 2024 09:47:36 +0000
Anatoly Burakov <anatoly.burakov@intel.com> wrote:

> It has been suggested [1] that a major cleanup/rewrite of devbind would be
> beneficial in terms of long term maintainability of the code. I was in a
> coding mood over the weekend, and so I've went ahead and rewritten devbind.
> 
> Note that this is one giant patch, rather than a series of patches adjusting
> existing code. Making it a patch series is possible, however the internal
> code architecture diverges quite significantly from the original devbind
> script due to its copious usage of string operations/pattern matching and
> global variables, so it is unclear whether subdividing this patch would be
> worth the effort. Instead, as has been suggested [2], the patchset now
> consists of creating a new file, followed by a removal of old file and
> rename of the new file. It is expected that this will be squashed on apply.
> 
> The script has become slightly bigger - 1000 lines instead of 800, however
> I would argue that since most of that increase is infrastructure, comments,
> and sacrificing code golf for code readability (such as expanding one-liners
> into multiple lines), the trade-off between being able to read and reason
> about what happens in the script is worth the added line count.
> 
> [1] https://patches.dpdk.org/project/dpdk/patch/c2bf00195c2d43833a831a9cc9346b4606d6ea2e.1723810613.git.anatoly.burakov@intel.com/
> [2] https://patches.dpdk.org/project/dpdk/cover/cover.1733151400.git.anatoly.burakov@intel.com/
> 
> Anatoly Burakov (2):
>   usertools/devbind: update coding style
>   usertools/devbind: replace devbind
> 
>  doc/guides/tools/devbind.rst |   11 +
>  usertools/dpdk-devbind.py    | 1683 ++++++++++++++++++----------------
>  2 files changed, 924 insertions(+), 770 deletions(-)
> 

This patch seemed to get no reviews. So sent it to AI; especially since my python-fu is rudimentary.
It identified some small things, could you fix those and resubmit.

# DPDK Patch Review: usertools/devbind rewrite

**Series**: [PATCH v3 1/2] usertools/devbind: update coding style  
**Author**: Anatoly Burakov <anatoly.burakov@intel.com>  
**Reviewer**: Stephen Hemminger <stephen@networkplumber.org>

---

## Overview

This is a 2-patch series providing a complete rewrite of `dpdk-devbind.py`:

1. **Patch 1/2**: Introduces the new rewritten script as `dpdk-devbind-new.py`
2. **Patch 2/2**: Replaces the original script with the rewritten version

The rewrite brings significant improvements including PEP-484 type hints, modern Python idioms with context managers, structured classes, JSON parsing for `ip route` output, and deprecation of kernel <3.15 support.

---

## Errors (Must Fix)

### 1. Tag Formatting Error (Patch 1)
**Line 96:**
```
Reviewed-By: Stephen Hemminger <stephen@networkplumber.org>
```
Should be:
```
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
```
The `By` should be lowercase. DPDK tag format is `Reviewed-by:`.

### 2. Typo in Variable Name
**Line 325:**
```python
BASEDBAND_DEVICES = [CLASS_ACCELERATION]
```
Should be:
```python
BASEBAND_DEVICES = [CLASS_ACCELERATION]
```
This typo is referenced at line 369 as well.

### 3. Typo in Error Message  
**Line 820:**
```python
raise DevbindError(f"CLeanup failed for {dev}: {e}") from e
```
Should be:
```python
raise DevbindError(f"Cleanup failed for {dev}: {e}") from e
```

---

## Warnings (Should Fix)

### 4. Docstring Inconsistency
**Line 452-453:**
```python
def read_output(args: T.List[str]) -> str:
    """Run a subprocess, collect its output, and return it as a list of lines."""
```
The docstring says "list of lines" but the function returns a `str`. Either update the docstring to say "return it as a string" or change the return type.

### 5. `--noiommu-mode` Flag Not Checked
**Lines 876-877:**
```python
if use_vfio and not sysfs_iommu_enabled():
    sysfs_enable_unsafe_noiommu()
```
The `ctx.noiommu` flag is parsed (line 1100) but the code unconditionally enables no-IOMMU mode when IOMMU is disabled, regardless of whether the user specified `--noiommu-mode`. The original devbind required the flag to be explicitly set. Consider:
```python
if use_vfio and not sysfs_iommu_enabled():
    if not ctx.noiommu:
        raise DevbindError(
            "IOMMU not available. Use --noiommu-mode to enable unsafe no-IOMMU mode."
        )
    sysfs_enable_unsafe_noiommu()
```

### 6. No Unit Tests Added
A rewrite of this magnitude would benefit from unit tests in `app/test` or at minimum a standalone test script. Consider adding tests for:
- Device pattern matching
- lspci line parsing
- Argument parsing edge cases

### 7. Patch 2/2 Missing Commit Message Body
**Line 1213:**
The second patch has only `Signed-off-by:` with no commit message body explaining what it does. Even simple patches should have a brief description.

---

## Info (Suggestions)

### 8. File Mode for Bind Operation
**Line 807:**
```python
with open(bind_path, "a", encoding="utf-8") as f:
    f.write(dev.slot)
```
Using append mode (`"a"`) for writing to sysfs bind file is unusual. While it likely works, write mode (`"w"`) is more conventional and matches what the kernel expects. Consider changing to `"w"`.

### 9. Null Byte for driver_override Cleanup
**Line 818:**
```python
f.write("\00")
```
Writing a null byte works, but an empty string or newline might be more portable/conventional:
```python
f.write("")
```

### 10. Generator Consumed Multiple Times Risk
**Line 862:**
```python
devices = (devbind.pci_devices[dbdf] for dbdf in ctx.devices)
```
This creates a generator that can only be iterated once. If the code path were to iterate it multiple times, this would silently fail. Consider using a list comprehension instead:
```python
devices = [devbind.pci_devices[dbdf] for dbdf in ctx.devices]
```

### 11. Consider Using `shutil.which()` Instead of `subprocess`
**Lines 446-449:**
```python
if subprocess.call(
    ["which", program], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
):
```
Python 3.3+ provides `shutil.which()` which is more Pythonic:
```python
import shutil
if shutil.which(program) is None:
```

### 12. Documentation Formatting
**Lines 129-138 in doc/guides/tools/devbind.rst:**
The added documentation has a double blank line after line 128. RST typically uses single blank lines.

---

## Positive Observations

The rewrite demonstrates excellent software engineering:

- **Type Safety**: Comprehensive PEP-484 type annotations throughout
- **Modern Python**: Context managers, dataclasses pattern, proper exception hierarchy
- **Code Organization**: Clean separation with `Device`, `Devbind`, and `DevbindCtx` classes
- **Error Handling**: Consistent `DevbindError` exception with clear messages
- **Resource Management**: All file operations use context managers
- **JSON Parsing**: Using `ip -j route` for structured output is much better than text parsing
- **Reduced External Calls**: Fewer lspci invocations, unified parsing
- **Backward Compatibility**: Command-line interface remains compatible

---

## Summary

| Severity | Count |
|----------|-------|
| Error    | 3     |
| Warning  | 4     |
| Info     | 5     |

**Recommendation**: Fix the three errors (tag case, two typos) and address the `--noiommu-mode` logic, then this is ready for merge. The rewrite is a significant improvement over the original code.

---

## Checklist

- [x] SPDX license identifier present
- [x] Copyright line present  
- [x] Signed-off-by present
- [ ] Reviewed-by tag correctly formatted
- [x] Subject line ≤60 characters
- [x] Subject lowercase (except acronyms)
- [x] Imperative mood
- [x] No trailing period
- [x] Lines ≤100 characters (Python allows this)
- [x] Documentation updated
- [ ] Tests added/updated