utils/vfio: Add APIs for VFIO container and device management #6226
Conversation
📝 WalkthroughWalkthroughAdds a new VFIO utilities module providing functions to manage VFIO containers, IOMMU groups, device FDs, and IRQ queries; introduces unit tests exercising success/failure paths; updates selftests metadata to increase the unit test count. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.0)avocado/utils/vfio.pyselftests/unit/utils/vfio.pyselftests/check.pyThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
avocado/utils/vfio.py (3)
40-70: Consider removing the unnecessary return statement.Line 70 returns
True, but since the function raisesValueErroron failure, the caller can assume success if no exception is raised. The return value adds no semantic value.Apply this diff to simplify:
if not ioctl(container_fd, vfio_check_extension, vfio_type_iommu): raise ValueError("does not support type 1 iommu") - - return True
37-157: Optional: Consider custom exception classes for better error categorization.Static analysis flags TRY003 warnings about long error messages in
ValueErrorexceptions. While the current approach is acceptable, you could optionally define custom exception types (e.g.,VfioContainerError,VfioGroupError,VfioDeviceError) to better categorize failures and allow callers to handle different error types separately.This is a low-priority style improvement and can be deferred to maintain consistency with the rest of the codebase.
27-159: Consider documenting resource cleanup responsibilities.The functions return file descriptors that need to be closed by the caller. Consider adding notes in the docstrings about cleanup responsibilities, especially for:
get_vfio_container_fd()- caller must close the container FDget_iommu_group_fd()- caller must close the group FD and handle detachmentget_device_fd()- caller must close the device FDExample for
get_vfio_container_fd:def get_vfio_container_fd(): """get vfio container file descriptor + Note: The caller is responsible for closing the returned file descriptor + when done. + :return: file descriptor of the vfio container. :rtype: int :raises ValueError: if the vfio container cannot be opened. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
avocado/utils/vfio.py(1 hunks)selftests/check.py(1 hunks)selftests/unit/utils/vfio.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
selftests/unit/utils/vfio.py (1)
avocado/utils/vfio.py (7)
get_vfio_container_fd(27-37)check_vfio_container(40-70)get_iommu_group_fd(73-100)attach_group_to_container(103-119)detach_group_from_container(122-138)get_device_fd(141-159)vfio_device_supports_irq(162-185)
avocado/utils/vfio.py (1)
avocado/utils/pci.py (1)
get_iommu_group(455-468)
🪛 Ruff (0.14.0)
avocado/utils/vfio.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Avoid specifying long messages outside the exception class
(TRY003)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
136-138: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: Podman Egg task
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Egg task ubuntu:24.04
- GitHub Check: Egg task debian:12.4
- GitHub Check: Version task debian:11.0
- GitHub Check: Fedora selftests
- GitHub Check: Egg task ubi:9.2
- GitHub Check: Egg task fedora:40
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task ubi:8.8
- GitHub Check: Windows with Python 3.13
- GitHub Check: Version task debian:12.4
- GitHub Check: Windows with Python 3.9
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (5)
selftests/check.py (1)
30-30: LGTM! Test count correctly reflects new VFIO tests.The increment of 15 tests accurately matches the 15 test methods added in
selftests/unit/utils/vfio.py(VfioUtilsTests class).selftests/unit/utils/vfio.py (1)
80-176: LGTM! Comprehensive test coverage for VFIO utilities.The remaining test methods properly cover:
- Successful IOMMU group FD retrieval with viable group status
- Group attach/detach operations with both success and failure paths
- Device FD retrieval with appropriate error handling
- IRQ support checks with varying interrupt counts
The mocking strategy correctly simulates ioctl responses and verifies expected behavior.
avocado/utils/vfio.py (3)
27-37: LGTM! Proper error handling for container FD retrieval.The function correctly:
- Uses
os.O_CLOEXECto ensure the FD is closed on exec- Wraps
OSErrorin aValueErrorwith context- Includes clear error messaging
103-138: LGTM! Proper IOMMU group container management.Both
attach_group_to_containeranddetach_group_from_containercorrectly:
- Use
ctypes.c_void_p()to pass the container FD as a pointer argument- Check ioctl return values (non-zero indicates failure)
- Provide clear error messages
162-185: LGTM! Proper IRQ support verification.The function correctly:
- Packs the IRQ info request with appropriate struct format
- Calls ioctl to retrieve interrupt information
- Unpacks the response and extracts the interrupt count (4th field)
- Returns a boolean indicating whether the device supports the requested count
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6226 +/- ##
==========================================
+ Coverage 73.46% 73.53% +0.06%
==========================================
Files 206 207 +1
Lines 22487 22541 +54
==========================================
+ Hits 16521 16576 +55
+ Misses 5966 5965 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dhsrivas
force-pushed
the
utilies_vfio
branch
2 times, most recently
from
3936e5a to
5acdb5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
avocado/utils/vfio.py (2)
91-93: Fix incorrect error handling -os.open()raises exceptions, doesn't return -1.Line 92 checks
if group_fd < 0, but in Python,os.open()raisesOSErroron failure rather than returning a negative value. This error check is unreachable and ineffective.Apply this diff to handle errors properly:
vfio_group = f"/dev/vfio/{pci.get_iommu_group(device)}" - group_fd = os.open(vfio_group, os.O_RDWR) - if group_fd < 0: - raise ValueError(f"Failed to open {vfio_group}, {group_fd}") + try: + group_fd = os.open(vfio_group, os.O_RDWR) + except OSError as e: + raise ValueError(f"Failed to open {vfio_group}: {e}") from e
158-160: Replace return-value error check with exception handling for ioctl.
fcntl.ioctlraisesOSErroron failure rather than returning negative values. Remove theif device_fd < 0block and wrap theioctlcall in atry/except OSErrorto handle errors.Apply this diff:
buf = ctypes.create_string_buffer(device.encode("utf-8") + b"\x00") - device_fd = ioctl(group_fd, vfio_group_get_device_fd, buf) - if device_fd < 0: - raise ValueError("failed to get vfio device fd") + try: + device_fd = ioctl(group_fd, vfio_group_get_device_fd, buf) + except OSError as e: + raise ValueError(f"Failed to get vfio device fd: {e}") from e return device_fdselftests/unit/utils/vfio.py (1)
49-60: Fix the mock to raise OSError instead of returning -1.Line 52 sets
mock_open.return_value = -1, butos.open()raisesOSErroron failure rather than returning -1. This test doesn't properly simulate the real failure scenario. The past review comment indicated this was addressed in commit 3936e5a, but the issue remains.Apply this diff:
@mock.patch("avocado.utils.vfio.os.open") @mock.patch("avocado.utils.vfio.pci.get_iommu_group") def test_get_group_fd_fail_1(self, mock_get_group, mock_open): - mock_open.return_value = -1 + mock_open.side_effect = OSError("No such file or directory") mock_get_group.return_value = "42" with self.assertRaises(ValueError) as e: vfio.get_iommu_group_fd(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
avocado/utils/vfio.py(1 hunks)selftests/check.py(1 hunks)selftests/unit/utils/vfio.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
selftests/unit/utils/vfio.py (1)
avocado/utils/vfio.py (7)
get_vfio_container_fd(28-38)check_vfio_container(41-73)get_iommu_group_fd(76-103)attach_group_to_container(106-122)detach_group_from_container(125-141)get_device_fd(144-162)vfio_device_supports_irq(165-188)
avocado/utils/vfio.py (1)
avocado/utils/pci.py (1)
get_iommu_group(455-468)
🪛 Ruff (0.14.0)
avocado/utils/vfio.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
93-93: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
120-122: Avoid specifying long messages outside the exception class
(TRY003)
139-141: Avoid specifying long messages outside the exception class
(TRY003)
160-160: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (51)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Egg task debian:11.0
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Fedora selftests
- GitHub Check: Egg task fedora:41
- GitHub Check: Egg task ubi:9.2
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Windows with Python 3.13
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task ubi:8.8
- GitHub Check: Version task debian:12.4
- GitHub Check: Windows with Python 3.12
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (13)
selftests/check.py (1)
30-30: LGTM!The unit test count has been correctly updated from 874 to 889, reflecting the addition of 15 new VFIO utility tests in
selftests/unit/utils/vfio.py.avocado/utils/vfio.py (6)
28-38: LGTM!The function properly handles
os.open()failures by catchingOSErrorand re-raising asValueErrorwith context. Exception chaining usingfrom epreserves the original error details.
67-71: Verify error handling for ioctl calls.The
ioctl()calls on lines 67 and 70 do not wrap exception handling. Whileioctl()typically raisesOSErroron failure, the current implementation assumes success or that exceptions will propagate naturally. Confirm whether callers are expected to handleOSError, or consider wrapping these calls in try/except blocks for more explicit error messages.
97-101: Verify error handling for ioctl call.The
ioctl()call on line 97 does not wrap exception handling. If the ioctl fails,OSErrorwill propagate. Consider whether you want to catch and convert this to aValueErrorwith a more specific message, or allow theOSErrorto propagate naturally.
118-122: Verify error handling for ioctl call.The
ioctl()call on line 118 may raiseOSErroron failure. The current code checks the return value, but ifioctl()raises an exception instead of returning a non-zero value, the check on line 119 becomes unreachable. Verify the expected behavior and consider wrapping in try/except for explicit error handling.
137-141: Verify error handling for ioctl call.The
ioctl()call on line 137 may raiseOSErroron failure. The current code checks the return value, but ifioctl()raises an exception instead of returning a non-zero value, the check on line 138 becomes unreachable. Verify the expected behavior and consider wrapping in try/except for explicit error handling.
184-185: Verify error handling for ioctl call.The
ioctl()call on line 184 does not wrap exception handling. If the ioctl fails,OSErrorwill propagate. Confirm whether you want this behavior or prefer to catch and convert to a more specific exception.selftests/unit/utils/vfio.py (6)
11-21: LGTM!Both success and failure test cases for
get_vfio_container_fd()are correct. The failure case properly mocksos.open()to raiseOSError, which aligns with the actual behavior of the function.
23-47: LGTM!The tests for
check_vfio_container()properly exercise both failure paths (invalid API version and missing IOMMU extension) using appropriate mock return values.
62-98: LGTM!The tests for
get_iommu_group_fd()success and non-viable group failure cases are well-structured. The use ofstruct.pack()to simulate ioctl responses correctly tests the binary protocol handling.
100-131: LGTM!The tests for
attach_group_to_container()anddetach_group_from_container()properly cover both success and failure paths by mocking ioctl return values.
133-139: LGTM!The success test for
get_device_fd()correctly mocks the ioctl return value and validates the returned file descriptor.
150-176: LGTM!Both tests for
vfio_device_supports_irq()properly exercise the success and failure paths using structured ioctl responses. The tests correctly validate IRQ count comparisons.
This patch introduces a set of VFIO (Virtual Function I/O) helper APIs to simplify container, IOMMU group, and device management. The new functions provide a clean abstraction over common VFIO operations: * get_vfio_container_fd() – open and validate a VFIO container * check_vfio_container() – verify API version and IOMMU extension support * get_iommu_group_fd() – obtain an IOMMU group file descriptor * attach_group_to_container()/detach_group_from_container() – manage group association with a VFIO container * get_device_fd() – retrieve a VFIO device file descriptor * vfio_device_supports_irq() – check if a device supports the requested number of MSI-X interrupts Signed-off-by: Dheeraj Kumar Srivastava <[email protected]>
dhsrivas
force-pushed
the
utilies_vfio
branch
from
5acdb5a to
32dfd3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
avocado/utils/vfio.py(1 hunks)selftests/check.py(1 hunks)selftests/unit/utils/vfio.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- selftests/check.py
🧰 Additional context used
🧬 Code graph analysis (2)
selftests/unit/utils/vfio.py (1)
avocado/utils/vfio.py (7)
get_vfio_container_fd(28-38)check_vfio_container(41-73)get_iommu_group_fd(76-104)attach_group_to_container(107-123)detach_group_from_container(126-142)get_device_fd(145-164)vfio_device_supports_irq(167-190)
avocado/utils/vfio.py (1)
avocado/utils/pci.py (1)
get_iommu_group(455-468)
🪛 Ruff (0.14.0)
avocado/utils/vfio.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
121-123: Avoid specifying long messages outside the exception class
(TRY003)
140-142: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (36)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: Egg task ubi:9.2
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Egg task debian:12.4
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Podman Egg task
- GitHub Check: Egg task debian:11.0
- GitHub Check: Egg task fedora:41
- GitHub Check: Egg task ubuntu:24.04
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Fedora selftests
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (10)
avocado/utils/vfio.py (3)
28-38: LGTM! Proper exception handling for container FD.The function correctly wraps
os.open()in a try/except block and raises a descriptive ValueError with exception chaining. This properly handles the OSError thatos.open()raises on failure.
126-142: Verify ioctl error handling - return value check may be ineffective.Line 138 checks
if ret:after callingioctl(). Similar toattach_group_to_container(), this return value check may be ineffective iffcntl.ioctl()raisesOSErroron failure rather than returning error codes.If ioctl raises OSError on failure (as verified by the script in the previous comment), apply this diff:
def detach_group_from_container(group_fd, container_fd, vfio_group_unset_container): """detach the iommu group of pci device from vfio container :param group_fd: iommu group file descriptor :type group_fd: int :param container_fd: vfio container file descriptor :type container_fd: int :param vfio_group_unset_container: vfio ioctl to detach iommu group from the container fd :type vfio_group_unset_container: int :raises ValueError: if detaching the group to the container fails. """ - ret = ioctl(group_fd, vfio_group_unset_container, ctypes.c_void_p(container_fd)) - if ret: - raise ValueError( - "failed to detach pci device's iommu group from vfio container" - ) + try: + ioctl(group_fd, vfio_group_unset_container, ctypes.c_void_p(container_fd)) + except OSError as e: + raise ValueError( + f"failed to detach pci device's iommu group from vfio container: {e}" + ) from e
145-164: LGTM! Proper exception handling for device FD retrieval.The function correctly wraps
ioctl()in a try/except block and raises a descriptive ValueError with exception chaining. This is the correct pattern for handling ioctl failures.selftests/unit/utils/vfio.py (7)
10-21: LGTM! Container FD tests properly verify exception handling.Both success and failure test cases are well-structured:
- Success case mocks
os.open()to return a valid FD- Failure case properly mocks
os.open()to raiseOSErrorand verifies the ValueError is raised with the correct message
23-47: Add tests for ioctl exception path.While these tests verify the validation logic (API version mismatch and missing IOMMU extension), they don't test the case where
ioctl()itself raises anOSError. If the implementation is updated to wrap ioctl in try/except (as suggested in the review ofavocado/utils/vfio.py), tests should verify that OSError is properly caught and converted to ValueError.Consider adding tests like:
@mock.patch("avocado.utils.vfio.ioctl") def test_check_vfio_container_ioctl_failure(self, mock_ioctl): mock_ioctl.side_effect = OSError("ioctl failed") with self.assertRaises(ValueError) as e: vfio.check_vfio_container( container_fd=3, vfio_get_api_version=15204, vfio_api_version=0, vfio_check_extension=15205, vfio_type_iommu=1, ) self.assertIn("Failed to get API version", str(e.exception))
49-98: Add test for ioctl exception path and verify FD cleanup.The tests cover open failure and non-viable group, but don't test the case where the
ioctl()call itself raises anOSError. Additionally, if the implementation is updated to close the FD on error (as suggested in the review ofavocado/utils/vfio.py), tests should verify that cleanup occurs.Consider adding a test like:
@mock.patch("avocado.utils.vfio.os.close") @mock.patch("avocado.utils.vfio.ioctl") @mock.patch("avocado.utils.vfio.os.open") @mock.patch("avocado.utils.vfio.pci.get_iommu_group") def test_get_group_fd_ioctl_failure(self, mock_get_group, mock_open, mock_ioctl, mock_close): mock_open.return_value = 3 mock_get_group.return_value = "42" mock_ioctl.side_effect = OSError("ioctl failed") with self.assertRaises(ValueError) as e: vfio.get_iommu_group_fd( device="0000:03:00.0", vfio_group_get_status=100, vfio_group_flags_viable=0x1, ) self.assertIn("Failed to get group status", str(e.exception)) mock_close.assert_called_once_with(3) # Verify FD was closed
100-131: Tests may not match implementation behavior.These tests mock
ioctl()to return 0 or 1, but in Python,fcntl.ioctl()typically raisesOSErroron failure rather than returning error codes. If the implementation is corrected to use exception handling (as suggested in the review ofavocado/utils/vfio.py), these tests will need updates:If the implementation is updated to catch OSError, modify the failure tests:
@mock.patch("avocado.utils.vfio.ioctl") def test_attach_group_to_container_failure(self, mock_ioctl): mock_ioctl.side_effect = OSError("Attach failed") with self.assertRaises(ValueError) as ctx: vfio.attach_group_to_container( group_fd=10, container_fd=20, vfio_group_set_container=100 ) self.assertIn("failed to attach pci device", str(ctx.exception)) @mock.patch("avocado.utils.vfio.ioctl") def test_detach_group_from_container_failure(self, mock_ioctl): mock_ioctl.side_effect = OSError("Detach failed") with self.assertRaises(ValueError) as ctx: vfio.detach_group_from_container( group_fd=10, container_fd=20, vfio_group_unset_container=200 ) self.assertIn("failed to detach pci device", str(ctx.exception))Keep the success tests as-is since successful ioctl calls typically return normally.
133-148: LGTM! Device FD tests properly verify exception handling.Both success and failure test cases are well-structured:
- Success case mocks
ioctl()to return a valid device FD- Failure case properly mocks
ioctl()to raiseOSErrorand verifies the ValueError is raised with the correct messageThis aligns with the implementation which already has proper exception handling.
150-176: Add test for ioctl exception path.While these tests verify the success and insufficient IRQ count scenarios, they don't test the case where
ioctl()itself raises anOSError. If the implementation is updated to wrap ioctl in try/except (as suggested in the review ofavocado/utils/vfio.py), tests should verify that OSError is properly caught and converted to ValueError.Consider adding a test like:
@mock.patch("avocado.utils.vfio.ioctl") def test_vfio_device_supports_irq_ioctl_failure(self, mock_ioctl): mock_ioctl.side_effect = OSError("ioctl failed") with self.assertRaises(ValueError) as e: vfio.vfio_device_supports_irq( device_fd=30, vfio_pci_msix_irq_index=100, vfio_device_get_irq_info=300, count=4, ) self.assertIn("Failed to get device IRQ info", str(e.exception))
179-180: LGTM! Standard unittest entry point.The standard
unittest.main()entry point is appropriate for running the tests as a standalone module.
| def check_vfio_container( | ||
| container_fd, | ||
| vfio_get_api_version, | ||
| vfio_api_version, | ||
| vfio_check_extension, | ||
| vfio_type_iommu, | ||
| ): | ||
| """validate the vfio container by verifying the api version and ensuring the required | ||
| iommu extension is supported. if either validation fails, the test is canceled with | ||
| an appropriate message. | ||
|
|
||
| :param container_fd: vfio container file descriptor | ||
| :type container_fd: int | ||
| :param vfio_get_api_version: ioctl to retrieve the vfio container api version | ||
| :type vfio_get_api_version: int | ||
| :param vfio_api_version: expected vfio api version | ||
| :type vfio_api_version: int | ||
| :param vfio_check_extension: ioctl to check iommu extension support | ||
| :type vfio_check_extension: int | ||
| :param vfio_type_iommu: expected vfio iommu type | ||
| :type vfio_type_iommu: int | ||
| :raises ValueError: if the vfio api version is invalid or if the required | ||
| iommu extension is not supported. | ||
| :return: True if vfio container fd check passes. | ||
| :rtype: bool | ||
| """ | ||
| if ioctl(container_fd, vfio_get_api_version) != vfio_api_version: | ||
| raise ValueError("Failed to get right API version") | ||
|
|
||
| if not ioctl(container_fd, vfio_check_extension, vfio_type_iommu): | ||
| raise ValueError("does not support type 1 iommu") | ||
|
|
||
| return True |
There was a problem hiding this comment.
Add exception handling for ioctl calls.
The two ioctl() calls (lines 67 and 70) are not wrapped in try/except blocks. In Python, fcntl.ioctl() raises OSError on failure rather than returning error codes. If either ioctl fails, an unhandled OSError will propagate instead of the intended ValueError.
Apply this diff to add proper exception handling:
def check_vfio_container(
container_fd,
vfio_get_api_version,
vfio_api_version,
vfio_check_extension,
vfio_type_iommu,
):
"""validate the vfio container by verifying the api version and ensuring the required
iommu extension is supported. if either validation fails, the test is canceled with
an appropriate message.
:param container_fd: vfio container file descriptor
:type container_fd: int
:param vfio_get_api_version: ioctl to retrieve the vfio container api version
:type vfio_get_api_version: int
:param vfio_api_version: expected vfio api version
:type vfio_api_version: int
:param vfio_check_extension: ioctl to check iommu extension support
:type vfio_check_extension: int
:param vfio_type_iommu: expected vfio iommu type
:type vfio_type_iommu: int
:raises ValueError: if the vfio api version is invalid or if the required
iommu extension is not supported.
:return: True if vfio container fd check passes.
:rtype: bool
"""
- if ioctl(container_fd, vfio_get_api_version) != vfio_api_version:
- raise ValueError("Failed to get right API version")
+ try:
+ api_version = ioctl(container_fd, vfio_get_api_version)
+ except OSError as e:
+ raise ValueError(f"Failed to get API version: {e}") from e
+
+ if api_version != vfio_api_version:
+ raise ValueError("Failed to get right API version")
- if not ioctl(container_fd, vfio_check_extension, vfio_type_iommu):
- raise ValueError("does not support type 1 iommu")
+ try:
+ has_extension = ioctl(container_fd, vfio_check_extension, vfio_type_iommu)
+ except OSError as e:
+ raise ValueError(f"Failed to check IOMMU extension: {e}") from e
+
+ if not has_extension:
+ raise ValueError("does not support type 1 iommu")
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def check_vfio_container( | |
| container_fd, | |
| vfio_get_api_version, | |
| vfio_api_version, | |
| vfio_check_extension, | |
| vfio_type_iommu, | |
| ): | |
| """validate the vfio container by verifying the api version and ensuring the required | |
| iommu extension is supported. if either validation fails, the test is canceled with | |
| an appropriate message. | |
| :param container_fd: vfio container file descriptor | |
| :type container_fd: int | |
| :param vfio_get_api_version: ioctl to retrieve the vfio container api version | |
| :type vfio_get_api_version: int | |
| :param vfio_api_version: expected vfio api version | |
| :type vfio_api_version: int | |
| :param vfio_check_extension: ioctl to check iommu extension support | |
| :type vfio_check_extension: int | |
| :param vfio_type_iommu: expected vfio iommu type | |
| :type vfio_type_iommu: int | |
| :raises ValueError: if the vfio api version is invalid or if the required | |
| iommu extension is not supported. | |
| :return: True if vfio container fd check passes. | |
| :rtype: bool | |
| """ | |
| if ioctl(container_fd, vfio_get_api_version) != vfio_api_version: | |
| raise ValueError("Failed to get right API version") | |
| if not ioctl(container_fd, vfio_check_extension, vfio_type_iommu): | |
| raise ValueError("does not support type 1 iommu") | |
| return True | |
| def check_vfio_container( | |
| container_fd, | |
| vfio_get_api_version, | |
| vfio_api_version, | |
| vfio_check_extension, | |
| vfio_type_iommu, | |
| ): | |
| """validate the vfio container by verifying the api version and ensuring the required | |
| iommu extension is supported. if either validation fails, the test is canceled with | |
| an appropriate message. | |
| :param container_fd: vfio container file descriptor | |
| :type container_fd: int | |
| :param vfio_get_api_version: ioctl to retrieve the vfio container api version | |
| :type vfio_get_api_version: int | |
| :param vfio_api_version: expected vfio api version | |
| :type vfio_api_version: int | |
| :param vfio_check_extension: ioctl to check iommu extension support | |
| :type vfio_check_extension: int | |
| :param vfio_type_iommu: expected vfio iommu type | |
| :type vfio_type_iommu: int | |
| :raises ValueError: if the vfio api version is invalid or if the required | |
| iommu extension is not supported. | |
| :return: True if vfio container fd check passes. | |
| :rtype: bool | |
| """ | |
| try: | |
| api_version = ioctl(container_fd, vfio_get_api_version) | |
| except OSError as e: | |
| raise ValueError(f"Failed to get API version: {e}") from e | |
| if api_version != vfio_api_version: | |
| raise ValueError("Failed to get right API version") | |
| try: | |
| has_extension = ioctl(container_fd, vfio_check_extension, vfio_type_iommu) | |
| except OSError as e: | |
| raise ValueError(f"Failed to check IOMMU extension: {e}") from e | |
| if not has_extension: | |
| raise ValueError("does not support type 1 iommu") | |
| return True |
🧰 Tools
🪛 Ruff (0.14.0)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In avocado/utils/vfio.py around lines 41 to 73, the two ioctl() calls are not
exception-handled and will raise OSError instead of the intended ValueError on
failure; wrap each ioctl call in a try/except OSError block, catch the OSError,
and re-raise a ValueError with a clear message that includes the original
exception information (e.g., using f-strings or chaining) so failures produce
the intended ValueError rather than an unhandled OSError.
| def get_iommu_group_fd(device, vfio_group_get_status, vfio_group_flags_viable): | ||
| """get iommu group fd for the pci device | ||
|
|
||
| :param device: full pci address including domain (0000:03:00.0) | ||
| :type device: str | ||
| :param vfio_group_get_status: ioctl to get iommu group status | ||
| :type vfio_group_get_status: int | ||
| :param vfio_group_flags_viable: ioctl to check if iommu group is viable | ||
| :type vfio_group_flags_viable: int | ||
| :raises ValueError: if the vfio group device cannot be opened or the group | ||
| is not viable. | ||
| :return: file descriptor for the iommu group. | ||
| :rtype: int | ||
| """ | ||
| vfio_group = f"/dev/vfio/{pci.get_iommu_group(device)}" | ||
| try: | ||
| group_fd = os.open(vfio_group, os.O_RDWR) | ||
| except OSError as e: | ||
| raise ValueError(f"Failed to open {vfio_group}: {e}") from e | ||
|
|
||
| argsz = struct.calcsize("II") | ||
| group_status_request = struct.pack("II", argsz, 2) | ||
| group_status_response = ioctl(group_fd, vfio_group_get_status, group_status_request) | ||
| group_status = struct.unpack("II", group_status_response) | ||
|
|
||
| if not group_status[1] & vfio_group_flags_viable: | ||
| raise ValueError("Group not viable, are all devices attached to vfio?") | ||
|
|
||
| return group_fd |
There was a problem hiding this comment.
Add exception handling for ioctl call.
The ioctl() call at line 98 is not wrapped in a try/except block. If the ioctl fails, an unhandled OSError will propagate instead of a descriptive ValueError. This is inconsistent with other functions in this module (e.g., get_device_fd() which properly wraps ioctl).
Apply this diff to add proper exception handling:
vfio_group = f"/dev/vfio/{pci.get_iommu_group(device)}"
try:
group_fd = os.open(vfio_group, os.O_RDWR)
except OSError as e:
raise ValueError(f"Failed to open {vfio_group}: {e}") from e
argsz = struct.calcsize("II")
group_status_request = struct.pack("II", argsz, 2)
- group_status_response = ioctl(group_fd, vfio_group_get_status, group_status_request)
+ try:
+ group_status_response = ioctl(group_fd, vfio_group_get_status, group_status_request)
+ except OSError as e:
+ os.close(group_fd)
+ raise ValueError(f"Failed to get group status: {e}") from e
+
group_status = struct.unpack("II", group_status_response)
if not group_status[1] & vfio_group_flags_viable:
+ os.close(group_fd)
raise ValueError("Group not viable, are all devices attached to vfio?")
return group_fdNote: Added os.close(group_fd) in error paths to prevent file descriptor leaks.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_iommu_group_fd(device, vfio_group_get_status, vfio_group_flags_viable): | |
| """get iommu group fd for the pci device | |
| :param device: full pci address including domain (0000:03:00.0) | |
| :type device: str | |
| :param vfio_group_get_status: ioctl to get iommu group status | |
| :type vfio_group_get_status: int | |
| :param vfio_group_flags_viable: ioctl to check if iommu group is viable | |
| :type vfio_group_flags_viable: int | |
| :raises ValueError: if the vfio group device cannot be opened or the group | |
| is not viable. | |
| :return: file descriptor for the iommu group. | |
| :rtype: int | |
| """ | |
| vfio_group = f"/dev/vfio/{pci.get_iommu_group(device)}" | |
| try: | |
| group_fd = os.open(vfio_group, os.O_RDWR) | |
| except OSError as e: | |
| raise ValueError(f"Failed to open {vfio_group}: {e}") from e | |
| argsz = struct.calcsize("II") | |
| group_status_request = struct.pack("II", argsz, 2) | |
| group_status_response = ioctl(group_fd, vfio_group_get_status, group_status_request) | |
| group_status = struct.unpack("II", group_status_response) | |
| if not group_status[1] & vfio_group_flags_viable: | |
| raise ValueError("Group not viable, are all devices attached to vfio?") | |
| return group_fd | |
| def get_iommu_group_fd(device, vfio_group_get_status, vfio_group_flags_viable): | |
| """get iommu group fd for the pci device | |
| :param device: full pci address including domain (0000:03:00.0) | |
| :type device: str | |
| :param vfio_group_get_status: ioctl to get iommu group status | |
| :type vfio_group_get_status: int | |
| :param vfio_group_flags_viable: ioctl to check if iommu group is viable | |
| :type vfio_group_flags_viable: int | |
| :raises ValueError: if the vfio group device cannot be opened or the group | |
| is not viable. | |
| :return: file descriptor for the iommu group. | |
| :rtype: int | |
| """ | |
| vfio_group = f"/dev/vfio/{pci.get_iommu_group(device)}" | |
| try: | |
| group_fd = os.open(vfio_group, os.O_RDWR) | |
| except OSError as e: | |
| raise ValueError(f"Failed to open {vfio_group}: {e}") from e | |
| argsz = struct.calcsize("II") | |
| group_status_request = struct.pack("II", argsz, 2) | |
| try: | |
| group_status_response = ioctl(group_fd, vfio_group_get_status, group_status_request) | |
| except OSError as e: | |
| os.close(group_fd) | |
| raise ValueError(f"Failed to get group status: {e}") from e | |
| group_status = struct.unpack("II", group_status_response) | |
| if not group_status[1] & vfio_group_flags_viable: | |
| os.close(group_fd) | |
| raise ValueError("Group not viable, are all devices attached to vfio?") | |
| return group_fd |
🧰 Tools
🪛 Ruff (0.14.0)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In avocado/utils/vfio.py around lines 76 to 104, the ioctl() call that checks
group status is not exception-handled so an OSError would propagate and leak the
opened group_fd; wrap the ioctl and subsequent unpack in a try/except OSError
block, on exception close group_fd and raise a ValueError with a descriptive
message (including the original error), and likewise ensure that if the group is
not viable you close group_fd before raising the ValueError to avoid file
descriptor leaks.
| def attach_group_to_container(group_fd, container_fd, vfio_group_set_container): | ||
| """attach the iommu group of pci device to the vfio container. | ||
|
|
||
| :param group_fd: iommu group file descriptor | ||
| :type group_fd: int | ||
| :param container_fd: vfio container file descriptor | ||
| :type container_fd: int | ||
| :param vfio_group_set_container: vfio ioctl to add iommu group to the container fd | ||
| :type vfio_group_set_container: int | ||
| :raises ValueError: if attaching the group to the container fails. | ||
| """ | ||
|
|
||
| ret = ioctl(group_fd, vfio_group_set_container, ctypes.c_void_p(container_fd)) | ||
| if ret: | ||
| raise ValueError( | ||
| "failed to attached pci device's iommu group to the vfio container" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify ioctl error handling - return value check may be ineffective.
Line 119 checks if ret: after calling ioctl(). In Python, fcntl.ioctl() typically raises OSError on failure rather than returning non-zero error codes. This return value check may never trigger as intended.
Run the following script to examine ioctl usage patterns in similar code:
If ioctl raises OSError on failure, apply this diff:
def attach_group_to_container(group_fd, container_fd, vfio_group_set_container):
"""attach the iommu group of pci device to the vfio container.
:param group_fd: iommu group file descriptor
:type group_fd: int
:param container_fd: vfio container file descriptor
:type container_fd: int
:param vfio_group_set_container: vfio ioctl to add iommu group to the container fd
:type vfio_group_set_container: int
:raises ValueError: if attaching the group to the container fails.
"""
- ret = ioctl(group_fd, vfio_group_set_container, ctypes.c_void_p(container_fd))
- if ret:
- raise ValueError(
- "failed to attached pci device's iommu group to the vfio container"
- )
+ try:
+ ioctl(group_fd, vfio_group_set_container, ctypes.c_void_p(container_fd))
+ except OSError as e:
+ raise ValueError(
+ f"failed to attach pci device's iommu group to the vfio container: {e}"
+ ) from e🏁 Script executed:
#!/bin/bash
# Search for ioctl error handling patterns in the codebase
echo "=== Searching for ioctl usage patterns ==="
rg -n -C3 'ioctl\(' --type=py -g 'avocado/utils/*.py' | head -50
echo "=== Checking if ioctl is imported ==="
rg -n 'from fcntl import ioctl' --type=py
echo "=== Looking for ioctl exception handling ==="
rg -n -B2 -A5 'try:.*ioctl' --type=py -g 'avocado/utils/*.py'Length of output: 3161
Refactor attach_group_to_container to use exception-based error handling
In avocado/utils/vfio.py (lines 119–122), replace the ret = ioctl…; if ret: check with a try/except OSError that raises a ValueError including the caught error. For example:
try:
ioctl(group_fd, vfio_group_set_container, ctypes.c_void_p(container_fd))
except OSError as e:
raise ValueError(f"failed to attach pci device's iommu group to the vfio container: {e}") from e🧰 Tools
🪛 Ruff (0.14.0)
121-123: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In avocado/utils/vfio.py around lines 107 to 123, the function
attach_group_to_container currently checks ioctl's integer return and raises a
ValueError; change it to call ioctl inside a try/except OSError block and on
exception raise a ValueError that includes the caught error message and uses
"from e" to preserve the original traceback, so failures surface as a clear
ValueError with underlying OSError details.
There was a problem hiding this comment.
@dhsrivas this comment from CodeRabbit seems very appropriate.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def vfio_device_supports_irq( | ||
| device_fd, vfio_pci_msix_irq_index, vfio_device_get_irq_info, count | ||
| ): | ||
| """Check if device supports at least count number of interrupts | ||
|
|
||
| :param device_fd: device file descriptor | ||
| :type device_fd: int | ||
| :param vfio_pci_msix_irq_index: vfio ioctl to get irq index for msix | ||
| :type vfio_pci_msix_irq_index: int | ||
| :param vfio_device_get_irq_info: vfio ioctl to get vfio device irq information | ||
| :type vfio_device_get_irq_info: int | ||
| :param count: number of irqs the device should support | ||
| :type count: int | ||
| :return: true if supported, false otherwise | ||
| :rtype: bool | ||
| """ | ||
| argsz = struct.calcsize("IIII") | ||
| index = vfio_pci_msix_irq_index | ||
| irq_info_request = struct.pack("IIII", argsz, 1, index, 1) | ||
| irq_info_response = ioctl(device_fd, vfio_device_get_irq_info, irq_info_request) | ||
| nirq = (struct.unpack("IIII", irq_info_response))[3] | ||
| if nirq < count: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Add exception handling for ioctl call.
The ioctl() call at line 186 is not wrapped in a try/except block. If the ioctl fails, an unhandled OSError will propagate. For consistency with get_device_fd() and proper error handling, this should be wrapped in try/except.
Apply this diff to add proper exception handling:
def vfio_device_supports_irq(
device_fd, vfio_pci_msix_irq_index, vfio_device_get_irq_info, count
):
"""Check if device supports at least count number of interrupts
:param device_fd: device file descriptor
:type device_fd: int
:param vfio_pci_msix_irq_index: vfio ioctl to get irq index for msix
:type vfio_pci_msix_irq_index: int
:param vfio_device_get_irq_info: vfio ioctl to get vfio device irq information
:type vfio_device_get_irq_info: int
:param count: number of irqs the device should support
:type count: int
+ :raises ValueError: if unable to query device IRQ information
:return: true if supported, false otherwise
:rtype: bool
"""
argsz = struct.calcsize("IIII")
index = vfio_pci_msix_irq_index
irq_info_request = struct.pack("IIII", argsz, 1, index, 1)
- irq_info_response = ioctl(device_fd, vfio_device_get_irq_info, irq_info_request)
+ try:
+ irq_info_response = ioctl(device_fd, vfio_device_get_irq_info, irq_info_request)
+ except OSError as e:
+ raise ValueError(f"Failed to get device IRQ info: {e}") from e
+
nirq = (struct.unpack("IIII", irq_info_response))[3]
if nirq < count:
return False
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def vfio_device_supports_irq( | |
| device_fd, vfio_pci_msix_irq_index, vfio_device_get_irq_info, count | |
| ): | |
| """Check if device supports at least count number of interrupts | |
| :param device_fd: device file descriptor | |
| :type device_fd: int | |
| :param vfio_pci_msix_irq_index: vfio ioctl to get irq index for msix | |
| :type vfio_pci_msix_irq_index: int | |
| :param vfio_device_get_irq_info: vfio ioctl to get vfio device irq information | |
| :type vfio_device_get_irq_info: int | |
| :param count: number of irqs the device should support | |
| :type count: int | |
| :return: true if supported, false otherwise | |
| :rtype: bool | |
| """ | |
| argsz = struct.calcsize("IIII") | |
| index = vfio_pci_msix_irq_index | |
| irq_info_request = struct.pack("IIII", argsz, 1, index, 1) | |
| irq_info_response = ioctl(device_fd, vfio_device_get_irq_info, irq_info_request) | |
| nirq = (struct.unpack("IIII", irq_info_response))[3] | |
| if nirq < count: | |
| return False | |
| return True | |
| def vfio_device_supports_irq( | |
| device_fd, vfio_pci_msix_irq_index, vfio_device_get_irq_info, count | |
| ): | |
| """Check if device supports at least count number of interrupts | |
| :param device_fd: device file descriptor | |
| :type device_fd: int | |
| :param vfio_pci_msix_irq_index: vfio ioctl to get irq index for msix | |
| :type vfio_pci_msix_irq_index: int | |
| :param vfio_device_get_irq_info: vfio ioctl to get vfio device irq information | |
| :type vfio_device_get_irq_info: int | |
| :param count: number of irqs the device should support | |
| :type count: int | |
| :raises ValueError: if unable to query device IRQ information | |
| :return: true if supported, false otherwise | |
| :rtype: bool | |
| """ | |
| argsz = struct.calcsize("IIII") | |
| index = vfio_pci_msix_irq_index | |
| irq_info_request = struct.pack("IIII", argsz, 1, index, 1) | |
| try: | |
| irq_info_response = ioctl(device_fd, vfio_device_get_irq_info, irq_info_request) | |
| except OSError as e: | |
| raise ValueError(f"Failed to get device IRQ info: {e}") from e | |
| nirq = (struct.unpack("IIII", irq_info_response))[3] | |
| if nirq < count: | |
| return False | |
| return True |
🤖 Prompt for AI Agents
In avocado/utils/vfio.py around lines 167 to 190, the ioctl() call that fetches
IRQ info is not protected and will let OSError propagate; wrap the
ioctl(device_fd, vfio_device_get_irq_info, irq_info_request) call in a
try/except OSError block and on failure raise a RuntimeError (or rethrow a
contextual exception) that includes a clear message and the original exception
details so callers get consistent, descriptive error handling similar to
get_device_fd().
Adds functionality to test interrupt remapping support for a PCI device using VFIO ioctls. - Verifies if the PCI device supports "count" MSI-X interrupts. - Binds the input PCI device to the vfio-pci driver. - Attempts to allocate "count" number of IRQs with IOMMU Interrupt remapping enabled; the test fails if allocation is unsuccessful. This patch is depended on 1. avocado-framework/avocado#6225 2. avocado-framework/avocado#6226 Signed-off-by: Dheeraj Kumar Srivastava <[email protected]>
|
@richtja In continuation to #6205. This PR contains new utility vfio.py file added in avocado. I have addressed all your previous review comments for utilities in this PR. Please review and let me know if you have any further review comments. Q. vfio.py depends on few pci.py avocado utils. Have we moved pci.py file utilities to AAutils. |
| def attach_group_to_container(group_fd, container_fd, vfio_group_set_container): | ||
| """attach the iommu group of pci device to the vfio container. | ||
|
|
||
| :param group_fd: iommu group file descriptor | ||
| :type group_fd: int | ||
| :param container_fd: vfio container file descriptor | ||
| :type container_fd: int | ||
| :param vfio_group_set_container: vfio ioctl to add iommu group to the container fd | ||
| :type vfio_group_set_container: int | ||
| :raises ValueError: if attaching the group to the container fails. | ||
| """ | ||
|
|
||
| ret = ioctl(group_fd, vfio_group_set_container, ctypes.c_void_p(container_fd)) | ||
| if ret: | ||
| raise ValueError( | ||
| "failed to attached pci device's iommu group to the vfio container" | ||
| ) |
There was a problem hiding this comment.
@dhsrivas this comment from CodeRabbit seems very appropriate.
| from avocado.utils import pci | ||
|
|
||
|
|
||
| def get_vfio_container_fd(): |
There was a problem hiding this comment.
Do you see this function being used publicly or only by the vfio module itself? If this is probably not going to be used by end users, maybe making it a private function can give more API flexiblity. And, TBH, it seems to provide little value other than standardization.
| vfio_type_iommu, | ||
| ): | ||
| """validate the vfio container by verifying the api version and ensuring the required | ||
| iommu extension is supported. if either validation fails, the test is canceled with |
There was a problem hiding this comment.
As a generic utility, it's better to talk about the behavior of this function without strictly talking about "the test".
| @mock.patch("os.open") | ||
| def test_get_vfio_container_fd_success(self, mock_open): | ||
| mock_open.return_value = 10 | ||
| vfio.get_vfio_container_fd() |
There was a problem hiding this comment.
Given the simple logic on get_vfio_container_fd(), the use of mocking and lack of checks here, I can't see actual value in this particular test.
2 participants
This patch introduces a set of VFIO (Virtual Function I/O) helper APIs to simplify container, IOMMU group, and device management. The new functions provide a clean abstraction over common VFIO operations:
Summary by CodeRabbit
New Features
Tests
Chores