Skip to content
Open
Show file tree
Hide file tree
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
9 changes: 9 additions & 0 deletions libvirt/tests/cfg/virtual_network/qemu/nicdriver_unload.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
- virtual_network.qemu_test.nicdriver_unload: image_copy
only Linux
type = nicdriver_unload
create_vm_libvirt = yes
kill_vm_libvirt = yes
filesize = 512
rules = 'SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="%s", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0"'
transfer_timeout = 1000
sessions_num = 5
184 changes: 184 additions & 0 deletions libvirt/tests/src/virtual_network/qemu/nicdriver_unload.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
import os
import random
import time

from avocado.utils import crypto, process
from virttest import data_dir, error_context, utils_misc, utils_net


@error_context.context_aware
def run(test, params, env):
"""
Test nic driver load/unload.

1) Boot a VM.
2) Get the NIC driver name.
3) Multi-session TCP transfer on test interface.
4) Repeatedly unload/load NIC driver during file transfer.
5) Check whether the test interface should still work.

:param test: QEMU test object.
:param params: Dictionary with the test parameters.
:param env: Dictionary with test environment.
"""

def reset_guest_udevrules(session, rules_file, rules_content):
"""
Write guest udev rules, then reboot the guest and
return the new session
"""
set_cmd = "echo '%s' > %s" % (rules_content, rules_file)
session.cmd_output_safe(set_cmd)
return vm.reboot()

def all_threads_done(threads):
"""
Check whether all threads have finished
"""
for thread in threads:
if thread.is_alive():
return False
else:
continue
return True

def all_threads_alive(threads):
"""
Check whether all threads is alive
"""
for thread in threads:
if not thread.is_alive():
return False
else:
continue
return True

timeout = params.get_numeric("login_timeout", 360)
transfer_timeout = params.get_numeric("transfer_timeout", 1000)
filesize = params.get_numeric("filesize", 512)

vm = env.get_vm(params["main_vm"])
vm.verify_alive()
session = vm.wait_for_login(timeout=timeout)
vm_mac_address = vm.get_mac_address()
udev_rules_file = "/etc/udev/rules.d/70-persistent-net.rules"
rules = params.get("rules")
if not session.cmd_status("[ -e %s ]" % udev_rules_file):
if not rules:
test.cancel("You must set udev rules before test")
rules = rules % vm_mac_address
session = reset_guest_udevrules(session, udev_rules_file, rules)
Comment on lines +66 to +70
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the udev rules file existence check logic.

The condition if not session.cmd_status("[ -e %s ]") evaluates to True when the file EXISTS (cmd_status returns 0). The current logic sets rules when the file already exists, but does nothing when the file is missing. This seems inverted.

Expected behavior:

  • If file is missing and rules not provided → cancel test
  • If file is missing and rules provided → create file with rules and reboot
  • If file exists → use existing rules (no action needed)

Run the following script to verify the intended behavior:


🏁 Script executed:

#!/bin/bash
# Check similar udev rules handling patterns in the codebase
rg -n -C3 "70-persistent-net.rules" --type=py

Length of output: 728


The web search didn't find the specific function. Let me search for it directly in the codebase.


🏁 Script executed:

#!/bin/bash
# Find reset_guest_udevrules function definition
rg -n "def reset_guest_udevrules" --type=py

Length of output: 132


🏁 Script executed:

#!/bin/bash
# View the reset_guest_udevrules function and surrounding context
sed -n '25,55p' libvirt/tests/src/virtual_network/qemu/nicdriver_unload.py

Length of output: 843


🏁 Script executed:

#!/bin/bash
# View the full test setup flow including what happens after line 70
sed -n '56,85p' libvirt/tests/src/virtual_network/qemu/nicdriver_unload.py

Length of output: 1474


Fix the inverted file existence check logic.

The condition on line 66 is backwards. if not session.cmd_status("[ -e %s ]") executes when the file exists, but the test should set up rules when the file is missing. Remove the not operator:

if session.cmd_status("[ -e %s ]" % udev_rules_file):

This way, the block executes only when the file is missing, and the test can set required rules before proceeding.

🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/nicdriver_unload.py around lines 66 to
70, the file-existence check is inverted: the current condition uses "if not
session.cmd_status('[ -e %s ]' % udev_rules_file)" which runs when the file
exists; change it to "if session.cmd_status('[ -e %s ]' % udev_rules_file)" so
the block runs only when the udev rules file is missing, then keep the existing
logic to require rules, format rules with the VM MAC address, and call
reset_guest_udevrules to create/setup the file.


error_context.base_context("Test env prepare")
error_context.context("Get NIC interface name in guest.", test.log.info)
ethname = utils_net.get_linux_ifname(session, vm.get_mac_address(0))
# get ethernet driver from '/sys' directory.
# ethtool can do the same thing and doesn't care about os type.
# if we make sure all guests have ethtool, we can make a change here.
sys_path = params.get("sys_path") % (ethname)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Missing required parameter sys_path in configuration.

Line 78 retrieves sys_path from params but this parameter is not defined in the configuration file. This will cause the test to fail with a TypeError when attempting string formatting.

Add the sys_path parameter to libvirt/tests/cfg/virtual_network/qemu/nicdriver_unload.cfg:

     rules = 'SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="%s", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0"'
     transfer_timeout = 1000
     sessions_num = 5
+    sys_path = "/sys/class/net/%s/device/driver"

Committable suggestion skipped: line range outside the PR's diff.

readlink_cmd = params.get("readlink_command", "readlink -e")
driver = os.path.basename(session.cmd("%s %s" % (readlink_cmd, sys_path)).strip())
test.log.info("The guest interface %s using driver %s", ethname, driver)

error_context.context(
"Host test file prepare, create %dMB file on host" % filesize, test.log.info
)
tmp_dir = data_dir.get_tmp_dir()
host_path = os.path.join(
tmp_dir, "host_file_%s" % utils_misc.generate_random_string(8)
)
guest_path = os.path.join(
"/home", "guest_file_%s" % utils_misc.generate_random_string(8)
)
cmd = "dd if=/dev/zero of=%s bs=1M count=%d" % (host_path, filesize)
process.run(cmd)
file_checksum = crypto.hash_file(host_path, algorithm="md5")

error_context.context(
"Guest test file prepare, Copy file %s from host to guest" % host_path,
test.log.info,
)
vm.copy_files_to(host_path, guest_path, timeout=transfer_timeout)
if session.cmd_status("md5sum %s | grep %s" % (guest_path, file_checksum)):
test.cancel("File MD5SUMs changed after copy to guest")
Comment on lines +102 to +103
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use test.error() instead of test.cancel() for file integrity failure.

When MD5 verification fails after copying the file to the guest, it indicates a real problem with the transfer mechanism, not a missing test requirement. Use test.error() to report this as a test setup failure rather than test.cancel().

Apply this diff:

-    if session.cmd_status("md5sum %s | grep %s" % (guest_path, file_checksum)):
-        test.cancel("File MD5SUMs changed after copy to guest")
+    if session.cmd_status("md5sum %s | grep %s" % (guest_path, file_checksum)):
+        test.error("File MD5SUMs changed after copy to guest")
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/nicdriver_unload.py around lines
102-103, the code calls test.cancel when the MD5 checksum verification fails;
change this to test.error to report a real test failure. Replace the
test.cancel("File MD5SUMs changed after copy to guest") call with
test.error("File MD5SUMs changed after copy to guest") (optionally include the
checksum or command output for context) so the failure is reported as an error
rather than a cancellation.

test.log.info("Test env prepare successfully")

error_context.base_context("Nic driver load/unload testing", test.log.info)
session_serial = vm.wait_for_serial_login(timeout=timeout)
try:
error_context.context("Transfer file between host and guest", test.log.info)
threads = []
file_paths = []
host_file_paths = []
for sess_index in range(int(params.get("sessions_num", "10"))):
sess_path = os.path.join("/home", "dst-%s" % sess_index)
host_sess_path = os.path.join(tmp_dir, "dst-%s" % sess_index)

thread1 = utils_misc.InterruptedThread(
vm.copy_files_to, (host_path, sess_path), {"timeout": transfer_timeout}
)

thread2 = utils_misc.InterruptedThread(
vm.copy_files_from,
(guest_path, host_sess_path),
{"timeout": transfer_timeout},
)
thread1.start()
threads.append(thread1)
thread2.start()
threads.append(thread2)
file_paths.append(sess_path)
host_file_paths.append(host_sess_path)

utils_misc.wait_for(lambda: all_threads_alive(threads), 60, 10, 1)

time.sleep(5)
error_context.context(
"Repeatedly unload/load NIC driver during file transfer", test.log.info
)
while not all_threads_done(threads):
error_context.context(
"Shutdown the driver for NIC interface.", test.log.info
)
sleep_time = random.randint(1, 5)
error_context.context("Unload NIC driver.", test.log.info)
session_serial.cmd_output_safe("modprobe -r %s" % driver)
time.sleep(sleep_time)
error_context.context("Load NIC driver.", test.log.info)
session_serial.cmd_output_safe("modprobe %s" % driver)
time.sleep(sleep_time)

# files md5sums check
error_context.context(
"File transfer finished, checking files md5sums", test.log.info
)
err_info = []
for copied_file in file_paths:
if session_serial.cmd_status(
"md5sum %s | grep %s" % (copied_file, file_checksum)
):
err_msg = "Guest file %s md5sum changed"
err_info.append(err_msg % copied_file)
for copied_file in host_file_paths:
if process.system(
"md5sum %s | grep %s" % (copied_file, file_checksum), shell=True
):
err_msg = "Host file %s md5sum changed"
err_info.append(err_msg % copied_file)
if err_info:
test.error("files MD5SUMs changed after copying %s" % err_info)
except Exception:
for thread in threads:
thread.join(suppress_exception=True)
raise
Comment on lines +170 to +173
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix the exception handling indentation.

The raise statement at line 173 is inside the for-loop, which means only the first thread will be joined before re-raising the exception. All threads should be joined before re-raising.

Apply this diff:

     except Exception:
         for thread in threads:
             thread.join(suppress_exception=True)
-            raise
+        raise
📝 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
except Exception:
for thread in threads:
thread.join(suppress_exception=True)
raise
except Exception:
for thread in threads:
thread.join(suppress_exception=True)
raise
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/nicdriver_unload.py around lines 170
to 173, the exception handler currently re-raises inside the for-loop causing
only the first thread to be joined; modify the handler so that all threads are
joined first and then the exception is re-raised (i.e., move the raise outside
the for-loop), ensuring thread.join(suppress_exception=True) runs for every
thread before re-raising the caught exception.

else:
for thread in threads:
thread.join()
for copied_file in file_paths:
session_serial.cmd("rm -rf %s" % copied_file)
for copied_file in host_file_paths:
process.system("rm -rf %s" % copied_file)
session_serial.cmd("%s %s" % ("rm -rf", guest_path))
os.remove(host_path)
session.close()
session_serial.close()