utils_net: add get_dhcp_client for dhcp commands #4278

quanwenli · 2025-12-04T09:19:37Z

Introduce "get_dhcp_client" function.
utils_net: update network functions to use "get_dhcp_client".

[LIBVIRTAT-22186]
Signed-off-by: Wenli Quan wquan@redhat.com

Summary by CodeRabbit

Refactor
- Unified DHCP client discovery and handling across Linux guest network and bridge operations, replacing multiple per-client checks with a single, consistent discovery flow to improve reliability and error handling when no DHCP client is available.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-04T09:19:53Z

Walkthrough

This change adds a new public function get_dhcp_client(session) in virttest/utils_net.py that detects available DHCP clients (e.g., dhclient, dhcpcd) and returns a tuple (command, release_argument). Multiple Linux network/bridge routines (restart_guest_network, create_ovs_bridge, delete_ovs_bridge, create_linux_bridge_tmux, delete_linux_bridge_tmux) were updated to call this function and use its returned dhcp_cmd and release_cmd instead of ad-hoc or hard-coded dhclient sequences, unifying DHCP client handling and modifying related control and error paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Verify correctness of get_dhcp_client() detection logic and its error path (raises TestError when no client found).
Confirm all replaced call sites correctly use both dhcp_cmd and release_cmd for IPv4/IPv6 flows.
Inspect command string composition where dhcp_cmd and release_cmd are interpolated to ensure quoting/escaping and ordering are preserved.
Check bridge creation/deletion and network restart flows for unchanged semantics and proper cleanup on failure.

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: introducing a new get_dhcp_client function and updating utils_net to use it for DHCP commands.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 825c66f and ba9f1aa.

📒 Files selected for processing (1)

virttest/utils_net.py (6 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

virttest/utils_net.py (2)

virttest/utils_misc.py (1)

cmd_status_output (157-195)

virttest/remote.py (1)

cmd_status_output (718-727)

🪛 Ruff (0.14.7)

virttest/utils_net.py

1704-1704: Function call with shell=True parameter identified, security issue

(S604)

1710-1710: 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). (2)

GitHub Check: fedora_40 AVOCADO_SRC:avocado-framework<104.0 SETUP:-m pip install . VT_TYPE:qemu
GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install . VT_TYPE:libvirt

🔇 Additional comments (5)

virttest/utils_net.py (5)
1694-1711: Function design looks good, but the docstring could clarify that release_cmd is a flag, not a complete command.

The return value description says "release argument" but some callers might expect a complete command. Consider making the docstring more explicit:
:return: tuple of (dhcp_command, release_flag) - the release_flag must be
         combined with the command, e.g., f"{dhcp_cmd} {release_flag} {interface}"
Regarding the static analysis hints:

The shell=True (S604) is a false positive since the command string uses only values from a fixed list (["dhclient", "dhcpcd"]), not user-controlled input.

The TRY003 hint about exception message length is a style nit and acceptable here.

4648-4656: LGTM!

The DHCP client handling is correctly implemented here. The format string {2} {3} properly combines dhcp_cmd with release_cmd to form a complete release command (e.g., dhclient -r), and {2} {0} correctly requests DHCP on the new bridge.

4693-4701: LGTM!

The DHCP client handling correctly mirrors the pattern used in create_ovs_bridge, properly combining dhcp_cmd with release_cmd for the release operation.

4800-4816: LGTM!

The change correctly retrieves the DHCP command and intentionally discards the release flag. The function uses pkill to terminate the DHCP client instead of the release command, which is a valid approach (though less graceful than using the proper release mechanism). This maintains consistency with the function's purpose of switching network connectivity.

4849-4862: LGTM!

The DHCP client handling is correctly implemented. The format string properly combines dhcp_cmd, release_cmd, and interface names to form complete commands like dhclient -r bridge_name.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

1. Introduce "get_dhcp_client" function. 2. utils_net: update network functions to use "get_dhcp_client". Signed-off-by: Wenli Quan <wquan@redhat.com>

smitterl · 2025-12-19T16:24:38Z

virttest/utils_net.py

+    """
+    dhcp_clients = [("dhclient", "-r"), ("dhcpcd", "-k")]
+
+    for cmd, release_cmd in dhcp_clients:


Please change the naming from 'release_cmd' to 'release_flag'; IMO, release_cmd = f"{cmd} {release_flag}". If you agree, please change it here and also where it's used further below.

smitterl · 2025-12-19T16:27:20Z

virttest/utils_net.py

    :param timeout: timeout value for command.
    """
    if os_type == "linux":
-        dhcp_clients = [("dhclient", "-r"), ("dhcpcd", "-k")]


Are these commands equivalent? If so, please mention it in the commit message and bring some proof that you tested that.

smitterl · 2025-12-19T16:30:34Z

virttest/utils_net.py

    cmd = (
-        "ovs-vsctl add-br {0};ovs-vsctl add-port {0} {1};dhclient -r;"
-        "sleep 5 ;dhclient {0}".format(ovs_bridge_name, iface_name)
+        "ovs-vsctl add-br {0};ovs-vsctl add-port {0} {1};{2} {3};"


Using these placeholders is confusing IMO, because as a human I have to always go back to the list in format at the end and for this long string with 4 elements it's really hard. How about you changed the way this command is constructed to make it more readable? You can do for example f"...; sleep 5;{dhcp_cmd} {release_flag}" and also

cmd = f"ovs-vsctl add-br {ovs_bridge_name}" cmd += f"... ... cmd += f"{dhcp_cmd} {dhcp_flag}"

qiankehan · 2025-12-23T06:14:48Z

virttest/utils_net.py

            "ip link set {1} master {0}; ip link set {0} up; "
-            "pkill dhclient; sleep 6; "
-            "dhclient {0};".format(linux_bridge_name, iface_name)
+            "pkill {2}; sleep 6; "


I suggest use f-strings instead of .format() for better readabilty.
E.g.

f"pkill {dhcp_cmd}; sleep 6; "

The same for line 4851-4853

@qiankehan @smitterl I understand that f-strings improve readability, and I also considered using them during the design. However, using .format() or % formatting is safer because the framework may run tests on older product versions with lower python versions. What do you think—should we be concerned about version compatibility?

utils_net: add get_dhcp_client for dhcp commands

ba9f1aa

1. Introduce "get_dhcp_client" function. 2. utils_net: update network functions to use "get_dhcp_client". Signed-off-by: Wenli Quan <wquan@redhat.com>

quanwenli force-pushed the dhcp branch from 825c66f to ba9f1aa Compare December 4, 2025 09:22

smitterl requested changes Dec 19, 2025

View reviewed changes

qiankehan suggested changes Dec 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

utils_net: add get_dhcp_client for dhcp commands #4278

utils_net: add get_dhcp_client for dhcp commands #4278

Uh oh!

quanwenli commented Dec 4, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 4, 2025 •

edited

Loading

Uh oh!

smitterl Dec 19, 2025

Uh oh!

smitterl Dec 19, 2025

Uh oh!

smitterl Dec 19, 2025

Uh oh!

qiankehan Dec 23, 2025

Uh oh!

quanwenli Dec 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

utils_net: add get_dhcp_client for dhcp commands #4278

Are you sure you want to change the base?

utils_net: add get_dhcp_client for dhcp commands #4278

Uh oh!

Conversation

quanwenli commented Dec 4, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks

Uh oh!

smitterl Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

smitterl Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

smitterl Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

qiankehan Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

quanwenli Dec 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

quanwenli commented Dec 4, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 4, 2025 •

edited

Loading