On Wed, 4 Dec 2024 09:47:36 +0000 Anatoly Burakov <[email protected]> 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.bura...@intel.com/ > [2] > https://patches.dpdk.org/project/dpdk/cover/[email protected]/ > > 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 <[email protected]> **Reviewer**: Stephen Hemminger <[email protected]> --- ## 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 <[email protected]> ``` Should be: ``` Reviewed-by: Stephen Hemminger <[email protected]> ``` 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
