utils_sriov: get a suitable pf#4352
utils_sriov: get a suitable pf#4352dzhengfy wants to merge 1 commit intoavocado-framework:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the get_suitable_pf_pci function in virttest/utils_sriov.py to select a Physical Function (PF) PCI address based on its operational status. The review feedback identifies a potential IndexError if no PFs are found and suggests refactoring the logic for better readability and explicit error handling when no suitable PF is available.
| if len(pf_up_list) < 2: | ||
| if len(pf_down_list) >= 1: | ||
| LOG.debug("Only one PF is in up status, so a PF in " | ||
| "down status will be returned:%s", pf_down_list[0]) | ||
| return pf_down_list[0] | ||
| else: | ||
| LOG.debug("Only one PF is in up status and no PF in " | ||
| "down status, so the PF in up status will " | ||
| "be returned:%s", pf_up_list[0]) | ||
| return pf_up_list[0] | ||
| else: | ||
| LOG.debug("At least two PFs are in up status, so a PF in up status " | ||
| "will be returned:%s", pf_up_list[0]) | ||
| return pf_up_list[0] |
There was a problem hiding this comment.
The current logic has a potential IndexError if no 'up' or 'down' PFs are found. Additionally, the nested conditional statements can be simplified for better readability and maintainability. The suggested change refactors the logic to be more linear and explicitly handles the case where no suitable PF is found by raising a TestError. This aligns with the requirement to fail explicitly when a required feature or dependency is unavailable, preventing silent failures or misleading results.
if len(pf_up_list) >= 2:
LOG.debug("At least two PFs are in up status, so a PF in up status "
"will be returned:%s", pf_up_list[0])
return pf_up_list[0]
if len(pf_up_list) == 1 and pf_down_list:
LOG.debug("Only one PF is in up status, so a PF in "
"down status will be returned:%s", pf_down_list[0])
return pf_down_list[0]
if pf_up_list:
LOG.debug("Only one PF is in up status and no PF in "
"down status, so the PF in up status will "
"be returned:%s", pf_up_list[0])
return pf_up_list[0]
if pf_down_list:
LOG.debug("No PFs are in up status, so a PF in "
"down status will be returned:%s", pf_down_list[0])
return pf_down_list[0]
raise exceptions.TestError("No suitable PF found (up or down).")References
- When a test's intent is to use a specific feature, it should fail explicitly if that feature or its dependencies are unavailable to avoid masking configuration issues.
28e69fb to
797a404
Compare
|
Test result: |
797a404 to
f76b9b5
Compare
This is to add a new function to get a suitable PF. If more than one Up PF exists, return one of them. If single Up PF exists and at least one Down PF exists, return this Down PF. If single Up PF exists and no Down PF, return this Up PF. Signed-off-by: Dan Zheng <dzheng@redhat.com>
f76b9b5 to
5e20d89
Compare
Yingshun
left a comment
There was a problem hiding this comment.
LGTM, please fix the pre-commit failures
This is to add a new function to get a suitable PF.
Signed-off-by: Dan Zheng dzheng@redhat.com