Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,36 @@ def check_device_status(net_ip, server_ip, netmask):
# ping to server from each function
for val in bus_info:
nic_name = str(utils_misc.get_interface_from_pci_id(val, session))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Check the return value before converting to string.

Converting the result to string before checking for None makes the logic harder to follow. The check on line 212 has to compare against the string "None" instead of the Python None object.

♻️ Cleaner approach
-                nic_name = str(utils_misc.get_interface_from_pci_id(val, session))
-                
+                nic_name = utils_misc.get_interface_from_pci_id(val, session)
+
                 # If get_interface_from_pci_id returns None, use uevent file
-                if nic_name == "None" or not nic_name:
+                if not nic_name:
                     logging.warning("get_interface_from_pci_id returned None for %s, trying uevent method", val)

Then ensure nic_name is converted to string only after the fallback succeeds (it's already a string from line 231, so this works correctly).

📝 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.

Suggested change
nic_name = str(utils_misc.get_interface_from_pci_id(val, session))
nic_name = utils_misc.get_interface_from_pci_id(val, session)
# If get_interface_from_pci_id returns None, use uevent file
if not nic_name:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py` at line 209,
The code currently assigns nic_name =
str(utils_misc.get_interface_from_pci_id(val, session)) which forces a "None"
string and requires string comparison later; change it to call
utils_misc.get_interface_from_pci_id(val, session) without str(), check if the
returned value is None (nic_name is None) and only convert to str() after the
fallback logic succeeds (or where the value is guaranteed to be a string),
referencing the utils_misc.get_interface_from_pci_id call and the nic_name
variable to locate and update the logic.


# If get_interface_from_pci_id returns None, use uevent file
if nic_name == "None" or not nic_name:
logging.warning("get_interface_from_pci_id returned None for %s, trying uevent method", val)
try:
# Get all network interfaces using modern 'ip' command instead of deprecated 'ifconfig'
ifaces_output = session.cmd_output("ip -o link show | awk -F': ' '{print $2}'")
interfaces = [iface.strip() for iface in ifaces_output.strip().split("\n") if iface.strip()]

# Find interface matching PCI address using uevent file
for iface in interfaces:
if iface in ["lo", "sit0"]: # Skip loopback
continue
Comment on lines +221 to +222

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify the comment to reflect both interface types.

The comment says "Skip loopback" but the code also skips sit0, which is an IPv6-in-IPv4 tunnel interface, not a loopback interface.

📝 Suggested fix
-                            if iface in ["lo", "sit0"]:  # Skip loopback
+                            if iface in ["lo", "sit0"]:  # Skip loopback and tunnel interfaces
                                 continue
📝 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.

Suggested change
if iface in ["lo", "sit0"]: # Skip loopback
continue
if iface in ["lo", "sit0"]: # Skip loopback and tunnel interfaces
continue
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py` around lines
221 - 222, Update the inline comment on the conditional that checks if iface in
["lo", "sit0"] to accurately describe both cases (i.e., skip the loopback
interface "lo" and the "sit0" IPv6-in-IPv4 tunnel interface) so the intent of
the check around the iface variable is clear; reference the conditional using
the exact list ["lo", "sit0"] and the variable name iface when making the
comment change near the PCI passthrough interface filtering logic.

# Read PCI address from uevent file
pci_cmd = "cat /sys/class/net/{}/device/uevent 2>/dev/null | grep PCI_SLOT_NAME | cut -d= -f2".format(iface)
status, pci_addr = session.cmd_status_output(pci_cmd)
if status == 0 and pci_addr.strip():
# Normalize for comparison (case-insensitive)
pci_normalized = pci_addr.strip().lower()
val_normalized = val.lower()
if pci_normalized == val_normalized:
nic_name = iface
logging.info("Found interface %s for PCI %s using uevent method", nic_name, val)
break
except Exception as e:
logging.error("Failed to find interface for PCI %s: %s", val, str(e))

if nic_name == "None" or not nic_name:
test.error("Could not determine interface name for PCI device {}".format(val))
continue
session.cmd("ip addr flush dev %s" % nic_name)
session.cmd("ip addr add %s/%s dev %s"
% (net_ip, netmask, nic_name))
Expand Down
Loading