-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some pci utils in avocado framework #5755
Conversation
Dear contributor,
As for the Avocado utility modules (“avocado.utils”) it is OK to introduce new functionality, |
@PraveenPenguin I can see one check failed. Can you help me out with this? |
Hi @dhsrivas, thank you for your contribution. The check which is failing is a style check. We use Black for style checking and adjustments, therefore you can fix it by running I haven't done a review of the code yet, but I noticed that there are no tests there. Do you think that it will be possible to add unit tests for these utilities? |
@richtja Corrected the style checks and updated. Pl. review. Writing unit test for these will need whole lot of things to mimic as it expects pci device and related sysfs enteries. So, for now it may not be possible to write one. |
@richtja Gentle ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dhsrivas sorry for the delay, thank you for this. Overall, it LGTM, I have just small concern about error handling during manipulation with files. Can you please look at that? Thank you.
avocado/utils/pci.py
Outdated
:param full_pci_address: Full PCI address including domain (0000:03:00.0) | ||
return: None | ||
""" | ||
genio.write_file(f"/sys/bus/pci/drivers/{driver}/unbind", full_pci_address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing to this file might lead to raising the OSError
, which could confuse the user of this util. IMO, it would be better to add Not able to unbind {full_pci_address} from {driver}
message to those errors to increase future debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the same problem to other utilities in this PR. Please update them as well.
Add the following pci utils: 1) unbind - unbind the driver from pci device. 2) bind - bind the driver to pci device. 3) get_vendor_id - get the vendor id of pci device. 4) reset - remove pci device in pci devices list in the system. 5) rescan - rescan the system for pci device. 6) reset_check - check if reset of pci device is successful. 7) rescan_check - check if rescan of the system for pci device is successful. 8) get_iommu_group - returns the iommu group of pci device. 9) change_domain - change iommu domain of pci device 10) change_domain_check - check whether whether change_domain is successful. Signed-off-by: Dheeraj Kumar Srivastava <[email protected]>
@richtja Thanks for reviewing the patch. I have addressed your comments. For file writes, i found one alternative in utils/genio itself that handles OSError for file writes. For read files i have handled it manually in the patch api itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhsrivas It LGTM, thank you
Add the following pci utils: