Adding a method to check for accelerators#6294
Adding a method to check for accelerators#6294saijanani-c wants to merge 2 commits intoavocado-framework:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility function isAccelerator to check for the presence of PCI accelerator devices. The review feedback suggests renaming the function to is_accelerator to adhere to PEP 8 naming conventions and replacing the broad exception handler with a more specific one. Additionally, the reviewer noted that the current implementation is inefficient due to frequent process forking and suggested a more performant approach by reading directly from the sysfs filesystem.
avocado/utils/pci.py
Outdated
| cfg_dic["subvendor_device"] = device_subvendor.group() | ||
| return cfg_dic | ||
|
|
||
| def isAccelerator(): |
There was a problem hiding this comment.
The function name isAccelerator uses camelCase, which is inconsistent with the snake_case naming convention used throughout this module (e.g., get_pci_addresses, get_pci_class_name) and violates PEP 8 guidelines. It should be renamed to is_accelerator.
| def isAccelerator(): | |
| def is_accelerator(): |
avocado/utils/pci.py
Outdated
| try: | ||
| if get_pci_class_name(dev) == "accelerator": | ||
| return True | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Using a bare except Exception is discouraged as it can catch and hide unrelated errors (such as KeyboardInterrupt or implementation bugs). Since get_pci_class_name specifically raises ValueError when a class is not recognized or cannot be retrieved, it is better to catch that specifically.
Additionally, this implementation is inefficient because it forks an lspci process for every PCI device on the system via get_pci_class_name. For a more performant approach, consider reading the class ID directly from /sys/bus/pci/devices/<dev>/class to avoid the overhead of multiple process forks.
| try: | |
| if get_pci_class_name(dev) == "accelerator": | |
| return True | |
| except Exception: | |
| pass | |
| try: | |
| if get_pci_class_name(dev) == "accelerator": | |
| return True | |
| except ValueError: | |
| continue |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6294 +/- ##
==========================================
- Coverage 74.04% 72.30% -1.75%
==========================================
Files 206 206
Lines 22651 23258 +607
==========================================
+ Hits 16773 16817 +44
- Misses 5878 6441 +563 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f36cf1a to
6e65fd6
Compare
Adding a new function isAccelerator() to detect PCI devices with class name "accelerator". Signed-off-by: Sai Janani C <jananic@linux.ibm.com>
6e65fd6 to
1b8610f
Compare
Apply black formatting to pci.py Signed-off-by: Praveen K Pandey <praveen@linux.vnet.ibm.com>
Adding a new function isAccelerator()
to detect PCI devices with class name
"accelerator".