Skip to content

Conversation

@Anushree-Mathur
Copy link
Contributor

@Anushree-Mathur Anushree-Mathur commented Dec 4, 2025

In the latest versions of QEMU, the QEMU process may run as qemu-system-, where represents any supported architecture. However, in some cases, it may still run as qemu-kvm. To ensure a more generic and reliable approach for identifying the QEMU process ID, it is recommended to replace the pidof command with pgrep, as pgrep provides broader compatibility and flexibility.

Signed-off-by: Anushree-Mathur anushree.mathur@linux.ibm.com

Summary by CodeRabbit

  • Improvements
    • Enhanced QEMU process detection mechanism to support a broader range of QEMU variants, improving compatibility with additional QEMU invocation types beyond the standard variant.

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

In the latest versions of QEMU, the QEMU process may run as qemu-system-<arch>, where <arch> represents any supported architecture. However, in some cases, it may still run as qemu-kvm. To ensure a more generic and reliable approach for identifying the QEMU process ID, it is recommended to replace the pidof command with pgrep, as pgrep provides broader compatibility and flexibility.
Signed-off-by: Anushree-Mathur <anushree.mathur@linux.ibm.com>
@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Walkthrough

This change modifies a test file to alter how the QEMU process ID is obtained. The previous implementation used a direct pidof call to retrieve the process ID, while the updated approach uses pgrep combined with ps to identify the top-level qemu-kvm or qemu-system-* process. This modification expands compatibility to support additional QEMU process invocation types. The remainder of the logic, which uses the resulting PID for cpu_allowed_list_by_task and logging operations, remains unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the pgrep and ps command combination correctly identifies both qemu-kvm and qemu-system-* processes across expected environments
  • Confirm the resulting PID is properly utilized by downstream logic (cpu_allowed_list_by_task)
  • Check for potential edge cases where multiple QEMU processes might exist and ensure the top-level process is correctly selected

Pre-merge checks and finishing touches

✅ 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 'Replacing pidof with pgrep to make it more generic!' directly and clearly describes the main change: replacing pidof with pgrep for more generic QEMU process identification, which matches the core objective of the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 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 b52d338 and 5c5677e.

📒 Files selected for processing (1)
  • libvirt/tests/src/virsh_cmd/domain/virsh_emulatorpin.py (1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
libvirt/tests/src/virsh_cmd/domain/virsh_emulatorpin.py

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

(S604)

⏰ 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). (4)
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.8
  • GitHub Check: Python 3.12
🔇 Additional comments (1)
libvirt/tests/src/virsh_cmd/domain/virsh_emulatorpin.py (1)

223-225: Critical: Command retrieves arbitrary QEMU process, not the specific VM under test.

The pgrep-based approach has a fundamental flaw: it retrieves the first qemu-system-* or qemu-kvm process it finds, regardless of which VM is being tested. If multiple VMs are running, this will check the wrong VM's emulator affinity, causing test failures or false positives.

Additionally:

  • pgrep without additional filtering can return multiple PIDs (one per line), but the code treats the result as a single PID
  • The nested command structure with shell=True is unnecessarily complex and flagged by static analysis (S604)

The recommended fix is to use the VM object's PID retrieval method instead of the generic process search command, and pass the VM object to the check_allowed_list function.


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.

@Anushree-Mathur
Copy link
Contributor Author

Before applying the patch :

 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: STARTED
 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: PASS (51.99 s)
 (2/3) type_specific.io-github-autotest-libvirt.virsh.emulatorpin.positive_testing.set_emulatorpin_parameter.running_guest.cpulist.emulatorpin_options.current.single.start_with_cpuset_config: STARTED
 (2/3) type_specific.io-github-autotest-libvirt.virsh.emulatorpin.positive_testing.set_emulatorpin_parameter.running_guest.cpulist.emulatorpin_options.current.single.start_with_cpuset_config: ERROR: Command 'pidof qemu-kvm' failed.\nstdout: b''\nstderr: b''\nadditional_info: None (51.55 s)
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: STARTED
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: PASS (9.08 s)
RESULTS    : PASS 2 | ERROR 1 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

After applying the patch:

JOB LOG    : /home/Anu/tests/results/job-2025-12-04T00.23-16808e9/job.log
 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: STARTED
 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: PASS (51.60 s)
 (2/3) type_specific.io-github-autotest-libvirt.virsh.emulatorpin.positive_testing.set_emulatorpin_parameter.running_guest.cpulist.emulatorpin_options.current.single.start_with_cpuset_config: STARTED
 (2/3) type_specific.io-github-autotest-libvirt.virsh.emulatorpin.positive_testing.set_emulatorpin_parameter.running_guest.cpulist.emulatorpin_options.current.single.start_with_cpuset_config: PASS (22.52 s)
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: STARTED
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: PASS (4.35 s)
RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant