add "dump memory file from guest" feature#4314
add "dump memory file from guest" feature#4314nickzhq wants to merge 1 commit intoavocado-framework:masterfrom
Conversation
WalkthroughAdds a dump-file extraction framework and integrates it into environment processing. New module virttest/guest_dump_file_manager.py provides GuestDumpFileManager, QemuDumpFileManager (qemu-nbd attach, partition and filesystem detection, mount, collect .dmp files, cleanup), LibvirtDumpFileManager (placeholder), DumpFileManagerFactory, and get_dump_file_mgr(vm_type). New test setup class DumpFileSetup (virttest/test_setup/dump_file.py) loads the nbd kernel module in setup and conditionally unloads it in cleanup. virttest/env_process.py now imports guest_dump_file_manager and DumpFileSetup, registers DumpFileSetup when dumping_guest_memory == "offline", and invokes the dump-file manager in postprocess for the offline dump flow. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
virttest/env_process.py (1)
23-43: Fix import ordering (CI isort failure).Pre-commit reports isort rewrote files; the new imports in this block appear out of order. Please re-run isort (or reorder) so CI passes.
Also applies to: 77-77
🤖 Fix all issues with AI agents
In `@virttest/env_process.py`:
- Around line 538-542: The call to
guest_dump_file_manager.get_dump_file_mgr(params.get("vm_type")) can raise for
vm_type values outside supported ones (e.g., "v2v"), so guard the dump-file
extraction by first checking params.get("vm_type") is one of the supported types
(e.g., "qemu", "libvirt") or wrap get_dump_file_mgr in a try/except and skip
extraction on failure; only call storage.get_image_filename(...) and
_mgr.run(_image_path, test.outputdir) when _mgr is valid, and log or silently
ignore unsupported vm_type instead of letting the exception propagate from
get_dump_file_mgr or _mgr.run.
In `@virttest/guest_dump_file_manager.py`:
- Around line 108-118: The NBD discovery shell command in
QemuDumpFileManager.__init__ assigns a malformed awk condition ($3 == missing)
to self._nbd_dev_cmd; fix it by making the awk check explicitly compare $3 to an
empty string (e.g. $3 == "" ) so it matches unmounted devices, and ensure the
Python string quoting/escaping for the awk expression is correct when assigning
to _nbd_dev_cmd (adjust the inner quotes or escape double quotes as needed).
- Around line 141-213: In run(), attach the image with qemu-nbd (qemu_nbd_cmd /
process.run) before querying devices and partitions: move the qemu-nbd attach
block above the partition detection, then call process.run("ls /dev/%sp*" %
self._nbd_dev).stdout_text.strip().split() to populate self._partition_list and
strip self._nbd_dev from process.run(...).stdout_text; fix the fdisk call to
format both placeholders and include /dev/ (e.g. process.run("fdisk -l /dev/%s |
grep %s" % (self._nbd_dev, des))). Ensure self._nbd_dev_partition_path remains
the full "/dev/..." string and do not prepend an extra "/dev/" when mounting or
running blkid (use self._nbd_dev_partition_path directly); derive filesystem
mapping key from self._partition_fstype.strip() and use that to lookup
self._fs_support_mapping, and call
utils_misc.mount(self._nbd_dev_partition_path, self._mount_pt,
self._fs_support_mapping[self._partition_fstype.strip()]()). Also add .strip()
to all stdout_text usages.
- Around line 120-139: The _process_dump_file_from_guest function should stop
using a shell find/mv pipeline and instead build a list of existing scan
directories from self._dmp_dirs_scaned[self._guest_type] (filter with
os.path.isdir), iterate each directory and use glob.glob(os.path.join(dir,
"*.dmp")) to find files, ensure the target res_dir/dump_file_from_guest exists
(os.makedirs exist_ok=True), and move each file with shutil.move(source, dest)
so paths with spaces and missing dirs are handled safely; update references to
_dmp_dirs_scaned and _guest_type accordingly.
In `@virttest/test_setup/dump_file.py`:
- Around line 1-7: Reformat this file to satisfy black and isort: reorder and
group the imports (e.g., standard lib, third-party, local) so isort is happy and
then run black to apply its styling; specifically update the import block that
contains logging, avocado.utils.linux_modules, virttest.utils_package, and
virttest.test_setup.core.Setuper and then re-run both tools (or run your repo's
pre-commit/format script) so the LOG = logging.getLogger(__name__) line and the
imports match the CI formatting rules.
- Around line 10-33: The DumpFileSetup.setup currently ignores boolean return
values from utils_package.package_install and linux_modules.load_module and
swallows errors; update DumpFileSetup.setup to check each package_install return
and raise an exception if any install returns False, check
linux_modules.load_module("nbd") return and raise on failure, set
self._nbd_is_loaded_by_vt only after a successful load, and replace
LOG.error(..., exc_info=e) with LOG.exception(...) and re-raise the exception so
the setup manager can handle failures; keep cleanup using
self._nbd_is_loaded_by_vt to unload the module.
🧹 Nitpick comments (3)
virttest/env_process.py (1)
1039-1039: Make DumpFileSetup registration conditional.Registering this unconditionally will attempt package installs and NBD module load on every run. Consider registering only when dump collection is enabled.
💡 Example gating
- _setup_manager.register(DumpFileSetup) + if params.get("dump_file_from_guest", "no") == "yes": + _setup_manager.register(DumpFileSetup)virttest/guest_dump_file_manager.py (2)
230-241: Make the libvirt stub explicit.A silent no‑op can be confusing since
run()is invoked. Consider logging or raisingNotImplementedError.💡 Example
def run(self, image_path, res_dir): @@ - pass + LOG.debug("Dump file extraction for libvirt is not implemented yet.") + return
244-272: Cache managers per vm_type.The single
_GDF_MGR_INSinstance returns the first-created manager even ifvm_typechanges. A per‑type cache avoids cross‑test contamination.♻️ Suggested refactor
- _GDF_MGR_INS = None + _GDF_MGR_INS = {} @@ - if not cls._GDF_MGR_INS: + if vm_type not in cls._GDF_MGR_INS: _type_mgr_mapping = { "qemu": QemuDumpFileManager, "libvirt": LibvirtDumpFileManager, } if vm_type not in _type_mgr_mapping: raise ValueError(f"Unsupported vm_type: {vm_type}") - cls._GDF_MGR_INS = _type_mgr_mapping[vm_type]() - return cls._GDF_MGR_INS + cls._GDF_MGR_INS[vm_type] = _type_mgr_mapping[vm_type]() + return cls._GDF_MGR_INS[vm_type]
| def __init__(self): | ||
| """ | ||
| Initialize the QemuDumpFileManager. | ||
|
|
||
| Sets up the command for finding available NBD devices. | ||
| """ | ||
| super(QemuDumpFileManager, self).__init__() | ||
| self._nbd_dev_cmd = ( | ||
| "lsblk --output NAME,SIZE,MOUNTPOINT --noheadings | grep '^nbd' | awk '$2 == \"0B\" && $3 == " | ||
| " {print $1}' | sort -rV | head -1" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "guest_dump_file_manager.py" -type fRepository: avocado-framework/avocado-vt
Length of output: 109
🏁 Script executed:
sed -n '108,118p' ./virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 474
🏁 Script executed:
# Test the original awk command syntax
echo "test" | awk '$2 == "0B" && $3 == ' 2>&1
# Test the suggested awk command syntax
echo "nbd0 0B" | awk '$1 ~ /^nbd/ && $2 == "0B" && $3 == "" {print $1}' 2>&1Repository: avocado-framework/avocado-vt
Length of output: 191
🏁 Script executed:
# Create a more realistic test to validate the command logic
cat <<'EOF' | awk '$1 ~ /^nbd/ && $2 == "0B" && $3 == "" {print $1}'
nbd0 0B
nbd1 100M /mnt
nbd2 0B
nbd3 50G /media
nbd4 0B
EOF
# And test the full pipeline with sorting
cat <<'EOF' | awk '$1 ~ /^nbd/ && $2 == "0B" && $3 == "" {print $1}' | sort -rV | head -1
nbd0 0B
nbd1 100M /mnt
nbd2 0B
nbd3 50G /media
nbd4 0B
nbd10 0B
nbd5 0B
EOFRepository: avocado-framework/avocado-vt
Length of output: 92
🏁 Script executed:
# Check if this command is used elsewhere in the codebase
rg "_nbd_dev_cmd" --type py
# Also check for any other awk expressions in the file that might have similar issues
cat -n ./virttest/guest_dump_file_manager.py | head -150Repository: avocado-framework/avocado-vt
Length of output: 6446
Fix malformed NBD device discovery command.
The awk expression contains an incomplete condition ($3 == without a value), causing the command to fail with an awk syntax error at runtime.
🛠️ Suggested fix
- self._nbd_dev_cmd = (
- "lsblk --output NAME,SIZE,MOUNTPOINT --noheadings | grep '^nbd' | awk '$2 == \"0B\" && $3 == "
- " {print $1}' | sort -rV | head -1"
- )
+ self._nbd_dev_cmd = (
+ "lsblk --output NAME,SIZE,MOUNTPOINT --noheadings | "
+ "awk '$1 ~ /^nbd/ && $2 == \"0B\" && $3 == \"\" {print $1}' | "
+ "sort -rV | head -1"
+ )🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 108 - 118, The NBD
discovery shell command in QemuDumpFileManager.__init__ assigns a malformed awk
condition ($3 == missing) to self._nbd_dev_cmd; fix it by making the awk check
explicitly compare $3 to an empty string (e.g. $3 == "" ) so it matches
unmounted devices, and ensure the Python string quoting/escaping for the awk
expression is correct when assigning to _nbd_dev_cmd (adjust the inner quotes or
escape double quotes as needed).
| def _process_dump_file_from_guest(self, res_dir): | ||
| """ | ||
| Process dump files from the mounted guest disk filesystem. | ||
|
|
||
| Searches for dump files in predefined directories and moves | ||
| them to the results directory. | ||
|
|
||
| :param res_dir: Directory where results should be stored. | ||
| :type res_dir: str | ||
| """ | ||
| _dirs = " ".join(self._dmp_dirs_scaned[self._guest_type]) | ||
| _cmd = 'find %s -maxdepth 1 -type f -iname "*.dmp" 2>/dev/null' % _dirs | ||
| _find_dmp_in_path = process.run(_cmd).stdout_text | ||
| if _find_dmp_in_path: | ||
| _find_dmp_in_path = _find_dmp_in_path.strip().splitlines() | ||
| res_dir = os.path.join(res_dir, "dump_file_from_guest") | ||
| if not os.path.exists(res_dir): | ||
| os.makedirs(res_dir) | ||
| for _path in _find_dmp_in_path: | ||
| process.run("mv %s %s/" % (_path, res_dir)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "guest_dump_file_manager.py" -t fRepository: avocado-framework/avocado-vt
Length of output: 107
🏁 Script executed:
head -20 virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 729
🏁 Script executed:
wc -l virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 111
🏁 Script executed:
sed -n '1,50p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 1667
🏁 Script executed:
sed -n '50,150p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 3665
🏁 Script executed:
sed -n '120,145p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 1127
🏁 Script executed:
python3 << 'EOF'
# Check what process.run() does on error
import subprocess
import sys
# Simulate what avocado's process.run likely does
# It typically wraps subprocess.run and raises on non-zero exit
try:
result = subprocess.run("find /nonexistent/path -type f 2>/dev/null", shell=True, capture_output=True, text=True)
print(f"Exit code: {result.returncode}")
print(f"Stdout: {result.stdout}")
print(f"Stderr: {result.stderr}")
except Exception as e:
print(f"Exception: {e}")
EOFRepository: avocado-framework/avocado-vt
Length of output: 102
🏁 Script executed:
# Check the process module from avocado to understand its behavior
rg "class.*process" --type py -A 10 -m 5Repository: avocado-framework/avocado-vt
Length of output: 3245
🏁 Script executed:
# Look for how process.run() is used elsewhere in the codebase to understand error handling
rg "process\.run\(" --type py -B 2 -A 2 | head -80Repository: avocado-framework/avocado-vt
Length of output: 4374
🏁 Script executed:
# Check what imports are currently in the file
sed -n '1,30p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 992
🏁 Script executed:
# Verify that glob and shutil are standard library modules
python3 << 'EOF'
import glob
import shutil
print("glob available:", hasattr(glob, 'glob'))
print("shutil available:", hasattr(shutil, 'move'))
EOFRepository: avocado-framework/avocado-vt
Length of output: 115
🏁 Script executed:
# Check how process.run() behaves with non-zero exit codes by looking at avocado documentation
rg "def run" virttest -A 5 -m 3 | head -40Repository: avocado-framework/avocado-vt
Length of output: 2349
🏁 Script executed:
# Look for any test files that might use this function to understand expected behavior
fd "test.*guest_dump" -t fRepository: avocado-framework/avocado-vt
Length of output: 54
🏁 Script executed:
# Check if there are any test files for guest_dump_file_manager
fd ".*guest_dump.*test" -t fRepository: avocado-framework/avocado-vt
Length of output: 54
🏁 Script executed:
# Check the specific behavior of process.run() by searching for its usage with error handling
rg "process\.run.*ignore_status" --type py -c | head -20Repository: avocado-framework/avocado-vt
Length of output: 697
🏁 Script executed:
# Search for process.run() without ignore_status to understand the default behavior
rg "process\.run\(" --type py -B 1 -A 1 | grep -v "ignore_status" | head -50Repository: avocado-framework/avocado-vt
Length of output: 2275
🏁 Script executed:
# Look at avocado's process module documentation or implementation
python3 << 'EOF'
# Check the behavior of process.run() by looking at its signature
try:
from avocado.utils import process
import inspect
sig = inspect.signature(process.run)
print("process.run signature:", sig)
# Check if there's a default for ignore_status
params = sig.parameters
if 'ignore_status' in params:
print("ignore_status default:", params['ignore_status'].default)
except Exception as e:
print(f"Could not inspect: {e}")
EOFRepository: avocado-framework/avocado-vt
Length of output: 116
🏁 Script executed:
# Verify the actual problem: does find with 2>/dev/null still exit non-zero?
python3 << 'EOF'
import subprocess
# Simulate the exact command that would be run
dirs = "/mnt/guest/Windows /mnt/guest/nonexistent"
cmd = 'find %s -maxdepth 1 -type f -iname "*.dmp" 2>/dev/null' % dirs
result = subprocess.run(cmd, shell=True, capture_output=True, text=True)
print(f"Exit code with non-existent dir and stderr redirect: {result.returncode}")
print(f"Stdout: '{result.stdout}'")
print(f"Stderr: '{result.stderr}'")
# Now test without the redirect to show the actual error
cmd2 = 'find %s -maxdepth 1 -type f -iname "*.dmp"' % dirs
result2 = subprocess.run(cmd2, shell=True, capture_output=True, text=True)
print(f"\nExit code without stderr redirect: {result2.returncode}")
print(f"Stderr: '{result2.stderr}'")
EOFRepository: avocado-framework/avocado-vt
Length of output: 309
🏁 Script executed:
# Check if the suggested fix properly handles the empty directories case
sed -n '50,65p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 652
🏁 Script executed:
# Verify the structure of _dmp_dirs_scaned to ensure the fix handles it correctly
sed -n '45,65p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 813
🏁 Script executed:
# Check if there are any calls to _process_dump_file_from_guest to understand context
rg "_process_dump_file_from_guest" --type py -B 2 -A 2Repository: avocado-framework/avocado-vt
Length of output: 744
🏁 Script executed:
# Verify the complete method to understand the full context
sed -n '115,200p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 3625
Use glob and shutil.move() to avoid shell issues and missing directory errors.
find exits non-zero if any configured path is missing (common on Windows), causing process.run() to raise; mv also fails on paths with spaces. Filter existing directories with os.path.isdir(), then use glob.glob() + shutil.move() instead.
🛠️ Suggested fix
-import logging
-import os
+import glob
+import logging
+import os
+import shutil
@@
- _dirs = " ".join(self._dmp_dirs_scaned[self._guest_type])
- _cmd = 'find %s -maxdepth 1 -type f -iname "*.dmp" 2>/dev/null' % _dirs
- _find_dmp_in_path = process.run(_cmd).stdout_text
- if _find_dmp_in_path:
- _find_dmp_in_path = _find_dmp_in_path.strip().splitlines()
- res_dir = os.path.join(res_dir, "dump_file_from_guest")
- if not os.path.exists(res_dir):
- os.makedirs(res_dir)
- for _path in _find_dmp_in_path:
- process.run("mv %s %s/" % (_path, res_dir))
+ _dirs = self._dmp_dirs_scaned.get(self._guest_type, [])
+ if not _dirs:
+ LOG.debug("No dump directories configured for guest type %s", self._guest_type)
+ return
+ res_dir = os.path.join(res_dir, "dump_file_from_guest")
+ if not os.path.exists(res_dir):
+ os.makedirs(res_dir)
+ for _dir in _dirs:
+ if not os.path.isdir(_dir):
+ continue
+ for _path in glob.glob(os.path.join(_dir, "*.dmp")):
+ shutil.move(_path, res_dir)🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 120 - 139, The
_process_dump_file_from_guest function should stop using a shell find/mv
pipeline and instead build a list of existing scan directories from
self._dmp_dirs_scaned[self._guest_type] (filter with os.path.isdir), iterate
each directory and use glob.glob(os.path.join(dir, "*.dmp")) to find files,
ensure the target res_dir/dump_file_from_guest exists (os.makedirs
exist_ok=True), and move each file with shutil.move(source, dest) so paths with
spaces and missing dirs are handled safely; update references to
_dmp_dirs_scaned and _guest_type accordingly.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
virttest/env_process.py (1)
23-43: Resolve isort failure in the import block.Pre-commit reports isort reformatting here; rerun isort/pre-commit to fix ordering.
Also applies to: 77-77
🤖 Fix all issues with AI agents
In `@virttest/env_process.py`:
- Around line 538-542: Guard the dump-file collection so a missing/unsupported
vm_type or failures in _mgr.run do not abort postprocess_image: retrieve vm_type
via params.get("vm_type") and skip get_dump_file_mgr if None; call
guest_dump_file_manager.get_dump_file_mgr(vm_type) inside a try/except catching
ValueError and log/ignore it, and wrap the call to _mgr.run(_image_path,
test.outputdir) in a separate try/except Exception that logs the error but does
not re-raise, ensuring the remainder of postprocess_image (cleanup/restore
logic) still executes; reference guest_dump_file_manager.get_dump_file_mgr,
_mgr, and _mgr.run to locate the code to modify.
In `@virttest/guest_dump_file_manager.py`:
- Around line 75-97: The _lvm2_member method currently returns an empty string
which will flow into mounting logic; change this to explicitly surface
unsupported LVM2 handling by either raising a NotImplementedError from
_lvm2_member (so callers like any mount or probe functions receive a clear
exception), or have _lvm2_member log a descriptive warning and return a distinct
sentinel (e.g., None or a dedicated value) so the mount logic can skip/handle it
explicitly; update callers of _lvm2_member in the mount/probe flow to check for
that sentinel or catch the NotImplementedError and skip LVM volumes with a clear
log message.
- Around line 15-20: Reorder the import block to satisfy isort: group standard
library imports (logging, os) first, then third-party imports
(avocado.utils.linux_modules, avocado.utils.process), then local application
imports (virttest.utils_misc), with a single blank line between each group;
update the import lines in guest_dump_file_manager.py accordingly and re-run
isort/pre-commit to confirm formatting.
- Around line 255-272: The get_mgr method currently uses a single class-level
cache _GDF_MGR_INS which returns the same manager regardless of vm_type; update
get_mgr (in class where _GDF_MGR_INS and method get_mgr are defined) to either
(a) cache instances per vm_type (e.g., use a dict keyed by vm_type) or (b)
simply return a new instance each call, so requests for "qemu" and "libvirt"
produce the correct manager; ensure you still validate vm_type against the
_type_mgr_mapping that maps "qemu"->QemuDumpFileManager and
"libvirt"->LibvirtDumpFileManager and remove or replace the single-instance
behavior that shares mutable state across calls.
- Around line 108-118: The _nbd_dev_cmd string in QemuDumpFileManager.__init__
contains an incomplete awk test (`$3 ==`) causing a syntax error; update the
command assigned to self._nbd_dev_cmd to compare $3 to an empty string (e.g. `$3
== ""`) so awk matches devices with no mount point and ensure the Python string
quoting/concatenation remains valid; modify the _nbd_dev_cmd value in the
QemuDumpFileManager.__init__ method accordingly.
- Around line 196-200: The mount call incorrectly indexes _fs_support_mapping
with self._guest_type; change it to use the detected filesystem type by indexing
_fs_support_mapping with self._partition_fstype.strip() (use .strip() because
_partition_fstype comes from process.run().stdout_text and may contain a
trailing newline) when calling utils_misc.mount(self._nbd_dev_partition_path,
self._mount_pt, ...); ensure any fallback lookup also uses the stripped
filesystem string and keep references to _fs_support_mapping, _partition_fstype,
_guest_type, utils_misc.mount, and self._mount_pt when making the change.
- Around line 159-186: Strip whitespace from the output of self._nbd_dev after
running process.run(self._nbd_dev_cmd), then move all partition detection and
filesystem/probe logic (the code that sets self._partition_list, computes
_partition, loops over _mapping to set self._guest_type and
self._nbd_dev_partition_path, and sets self._partition_fstype) to after the
qemu-nbd attach call (process.run(qemu_nbd_cmd)) so device nodes exist; also fix
the fdisk invocation in the loop to pass both values into the format string
(e.g. "fdisk -l %s | grep %s" % (self._nbd_dev, des)) or otherwise format
correctly so fdisk gets the nbd device and grep gets the descriptive string.
Ensure all uses reference the stripped self._nbd_dev when building "/dev/%s" and
when calling blkid on self._nbd_dev_partition_path.
- Around line 120-139: The _process_dump_file_from_guest method currently builds
_dirs from self._dmp_dirs_scaned[self._guest_type] without checking that the key
exists or that the list is non-empty, which makes find search the current dir;
add a guard at the start of _process_dump_file_from_guest to verify that
self._guest_type is in self._dmp_dirs_scaned and that the resulting list is
non-empty, and if not log a warning (use an existing logger on the object, e.g.
self.logger.warning, or fallback to logging.warning) and return early; only
construct _dirs and run the find/mv commands after that check to avoid moving
host files (refer to symbols _process_dump_file_from_guest, _dmp_dirs_scaned,
_guest_type, _find_dmp_in_path).
In `@virttest/test_setup/dump_file.py`:
- Around line 1-5: The file fails isort/black formatting; reorder and format
imports and file with the project's formatter (e.g., run isort then black or
invoke the repository pre-commit hooks) so imports are grouped/sorted (ensure
"import logging" and third-party imports like "from avocado.utils import
linux_modules" and "from virttest import utils_package" are ordered per project
rules) and overall file adheres to Black formatting, then re-run tests/CI to
confirm.
- Around line 12-28: The setup() method currently ignores return values from
utils_package.package_install and linux_modules.load_module and swallows errors;
update it to check each utils_package.package_install(pkg) result and if False
raise exceptions.TestSetupFail with a descriptive message, and likewise check
linux_modules.load_module("nbd") return value and raise exceptions.TestSetupFail
on failure (still set self._nbd_is_loaded_by_vt only on success); when catching
exceptions use LOG.exception(...) to log the traceback and re-raise the
exception so failures propagate to the test framework.
🧹 Nitpick comments (1)
virttest/env_process.py (1)
1039-1039: Consider gatingDumpFileSetupbehind a feature flag.Registering it unconditionally installs packages and loads
nbdfor every run. A param likeenable_dump_filewould avoid side effects when the feature isn’t used.
| def _ntfs(self): | ||
| """ | ||
| Get the filesystem type for NTFS partitions. | ||
|
|
||
| :return: String representing the NTFS filesystem type ("ntfs-3g"). | ||
| """ | ||
| # NOTE: | ||
| # ntfs-3g permission means rw | ||
| # ntfs permission means read-only | ||
| return "ntfs-3g" | ||
|
|
||
| def _lvm2_member(self): | ||
| """ | ||
| Get the filesystem type for LVM2 partitions. | ||
|
|
||
| Note: LVM2_member cannot be mounted directly and requires | ||
| additional steps to access the logical volumes. | ||
|
|
||
| :return: Empty string (not yet implemented). | ||
| """ | ||
| # The lvm2_member type (guest type is Linux) is generated by vt default | ||
| # TODO: Support the linux guest. lvm2_member can NOT be used directly. And some steps are needed. | ||
| return "" |
There was a problem hiding this comment.
Make Linux LVM handling explicit.
Line 95-97 returns an empty string with a TODO, which will later flow into mount logic. If Linux guests are in scope, consider raising a clear NotImplementedError or skipping with an explicit log until it’s supported.
If you want, I can sketch the LVM probe/mount flow and open a follow-up issue.
🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 75 - 97, The _lvm2_member
method currently returns an empty string which will flow into mounting logic;
change this to explicitly surface unsupported LVM2 handling by either raising a
NotImplementedError from _lvm2_member (so callers like any mount or probe
functions receive a clear exception), or have _lvm2_member log a descriptive
warning and return a distinct sentinel (e.g., None or a dedicated value) so the
mount logic can skip/handle it explicitly; update callers of _lvm2_member in the
mount/probe flow to check for that sentinel or catch the NotImplementedError and
skip LVM volumes with a clear log message.
| def __init__(self): | ||
| """ | ||
| Initialize the QemuDumpFileManager. | ||
|
|
||
| Sets up the command for finding available NBD devices. | ||
| """ | ||
| super(QemuDumpFileManager, self).__init__() | ||
| self._nbd_dev_cmd = ( | ||
| "lsblk --output NAME,SIZE,MOUNTPOINT --noheadings | grep '^nbd' | awk '$2 == \"0B\" && $3 == " | ||
| " {print $1}' | sort -rV | head -1" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n virttest/guest_dump_file_manager.py | sed -n '108,125p'Repository: avocado-framework/avocado-vt
Length of output: 842
🏁 Script executed:
python3 << 'EOF'
import subprocess
# Test the problematic awk command (with the supposed error)
cmd_invalid = 'echo "nbd0 0B" | awk \'$2 == "0B" && $3 == {print $1}\''
# Test the proposed fix
cmd_valid = 'echo "nbd0 0B " | awk \'$2 == "0B" && $3 == "" {print $1}\''
print("Testing INVALID awk command (from review):")
result = subprocess.run(cmd_invalid, shell=True, capture_output=True, text=True)
print(f"Return code: {result.returncode}")
print(f"Stdout: {result.stdout}")
print(f"Stderr: {result.stderr}")
print("\nTesting PROPOSED awk command (with fix):")
result = subprocess.run(cmd_valid, shell=True, capture_output=True, text=True)
print(f"Return code: {result.returncode}")
print(f"Stdout: {result.stdout}")
print(f"Stderr: {result.stderr}")
EOFRepository: avocado-framework/avocado-vt
Length of output: 329
Fix incomplete awk condition in NBD device discovery command.
The NBD device discovery command at lines 116-117 contains a syntax error. The awk condition $3 == is incomplete—it has no operand to compare against. This causes awk to fail with a syntax error when the command executes. The comparison should be $3 == "" to match devices with no mount point.
Proposed fix
self._nbd_dev_cmd = (
- "lsblk --output NAME,SIZE,MOUNTPOINT --noheadings | grep '^nbd' | awk '$2 == \"0B\" && $3 == "
- " {print $1}' | sort -rV | head -1"
+ "lsblk --output NAME,SIZE,MOUNTPOINT --noheadings | grep '^nbd' | awk '$2 == \"0B\" && $3 == \"\" {print $1}' | sort -rV | head -1"
)🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 108 - 118, The _nbd_dev_cmd
string in QemuDumpFileManager.__init__ contains an incomplete awk test (`$3 ==`)
causing a syntax error; update the command assigned to self._nbd_dev_cmd to
compare $3 to an empty string (e.g. `$3 == ""`) so awk matches devices with no
mount point and ensure the Python string quoting/concatenation remains valid;
modify the _nbd_dev_cmd value in the QemuDumpFileManager.__init__ method
accordingly.
| def _process_dump_file_from_guest(self, res_dir): | ||
| """ | ||
| Process dump files from the mounted guest disk filesystem. | ||
|
|
||
| Searches for dump files in predefined directories and moves | ||
| them to the results directory. | ||
|
|
||
| :param res_dir: Directory where results should be stored. | ||
| :type res_dir: str | ||
| """ | ||
| _dirs = " ".join(self._dmp_dirs_scaned[self._guest_type]) | ||
| _cmd = 'find %s -maxdepth 1 -type f -iname "*.dmp" 2>/dev/null' % _dirs | ||
| _find_dmp_in_path = process.run(_cmd).stdout_text | ||
| if _find_dmp_in_path: | ||
| _find_dmp_in_path = _find_dmp_in_path.strip().splitlines() | ||
| res_dir = os.path.join(res_dir, "dump_file_from_guest") | ||
| if not os.path.exists(res_dir): | ||
| os.makedirs(res_dir) | ||
| for _path in _find_dmp_in_path: | ||
| process.run("mv %s %s/" % (_path, res_dir)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -150 virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 5227
🏁 Script executed:
sed -n '140,250p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 3978
🏁 Script executed:
sed -n '163,198p' virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 1601
🏁 Script executed:
# Check if there are any calls to _process_dump_file_from_guest to understand if it could be called in other contexts
rg "_process_dump_file_from_guest" virttest/guest_dump_file_manager.pyRepository: avocado-framework/avocado-vt
Length of output: 181
Guard against empty scan paths to avoid moving host files.
When _guest_type is not detected or defaults to "Linux", the _dmp_dirs_scaned dictionary value is either missing (KeyError) or empty. An empty _dirs causes find to search the current directory instead of intended paths, risking accidental movement of host .dmp files. Add a guard before accessing the dictionary and log when directories are unavailable.
Proposed fix
- _dirs = " ".join(self._dmp_dirs_scaned[self._guest_type])
+ if not self._guest_type or not self._dmp_dirs_scaned.get(self._guest_type):
+ LOG.debug(
+ "No dump-file directories configured for guest type: %s",
+ self._guest_type,
+ )
+ return
+ _dirs = " ".join(self._dmp_dirs_scaned[self._guest_type])
_cmd = 'find %s -maxdepth 1 -type f -iname "*.dmp" 2>/dev/null' % _dirs🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 120 - 139, The
_process_dump_file_from_guest method currently builds _dirs from
self._dmp_dirs_scaned[self._guest_type] without checking that the key exists or
that the list is non-empty, which makes find search the current dir; add a guard
at the start of _process_dump_file_from_guest to verify that self._guest_type is
in self._dmp_dirs_scaned and that the resulting list is non-empty, and if not
log a warning (use an existing logger on the object, e.g. self.logger.warning,
or fallback to logging.warning) and return early; only construct _dirs and run
the find/mv commands after that check to avoid moving host files (refer to
symbols _process_dump_file_from_guest, _dmp_dirs_scaned, _guest_type,
_find_dmp_in_path).
| def setup(self): | ||
| # "qemu-nbd" is required by qemu in vm_type. | ||
| # "ntfs-3g" is required by ntfs filesystem. | ||
| # TODO: add the required package by libvirt. | ||
| _pkg = ["qemu-nbd", "ntfs-3g"] | ||
| try: | ||
| for pkg in _pkg: | ||
| if not utils_package.is_installed(pkg): | ||
| utils_package.package_install(pkg) | ||
|
|
||
| if not linux_modules.module_is_loaded("nbd"): | ||
| linux_modules.load_module("nbd") | ||
| self._nbd_is_loaded_by_vt = True | ||
|
|
||
| LOG.debug("The dump_file is set up successfully.") | ||
| except Exception as e: | ||
| LOG.error("The dump_file failed to set up.", exc_info=e) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file in question
find . -name "dump_file.py" -type f | head -20Repository: avocado-framework/avocado-vt
Length of output: 106
🏁 Script executed:
# Also check the repository structure
git ls-files | grep -E "(dump_file|utils_package|linux_modules)" | head -20Repository: avocado-framework/avocado-vt
Length of output: 198
🏁 Script executed:
cat -n ./virttest/test_setup/dump_file.py | head -40Repository: avocado-framework/avocado-vt
Length of output: 1484
🏁 Script executed:
cat -n ./virttest/utils_package.py | grep -A 30 "def package_install"Repository: avocado-framework/avocado-vt
Length of output: 1341
🏁 Script executed:
cat -n ./virttest/utils_linux_modules.py | grep -A 30 "def load_module"Repository: avocado-framework/avocado-vt
Length of output: 54
🏁 Script executed:
# Search for load_module in the codebase
rg "def load_module" --type pythonRepository: avocado-framework/avocado-vt
Length of output: 102
🏁 Script executed:
# Also check if there's documentation or usage patterns
rg "load_module\(" --type python -A 2 -B 2 | head -50Repository: avocado-framework/avocado-vt
Length of output: 102
🏁 Script executed:
rg "def load_module" -A 5 | head -30Repository: avocado-framework/avocado-vt
Length of output: 1611
🏁 Script executed:
# Check how these functions are used elsewhere in the codebase
rg "load_module\(" -A 2 -B 2 | head -60Repository: avocado-framework/avocado-vt
Length of output: 3802
🌐 Web query:
avocado linux_modules load_module return value raises exception
💡 Result:
In Avocado, avocado.utils.linux_modules.load_module(module_name) is documented to only check whether the module is already loaded and to return a boolean (True if loaded, False otherwise). It is not documented to raise on “module not found/not loaded”. [1]
If you’re seeing an exception, it’s typically because Avocado’s linux-module helpers invoke external commands (such as lsmod/modprobe) through avocado.utils.process.*. Those helpers raise avocado.utils.process.CmdError when a command returns non‑zero unless they were called with ignore_status=True. [2]
Sources:
[1] Avocado utils docs: linux_modules.load_module() return value (bool) https://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html
[2] Avocado utils docs: process.system() / process.system_output() raises CmdError when ignore_status=False https://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html
🌐 Web query:
avocado-framework utils linux_modules documentation
💡 Result:
The Avocado Framework Linux kernel modules utilities are documented in the Utilities API reference under avocado.utils.linux_modules. It includes:
ModuleConfigenum:BUILTIN,MODULE,NOT_SETcheck_kernel_config(config_name)– report how a kernel config is set in the running kernelget_loaded_modules()– list currently loaded modulesget_modules_dir()– return the modules directory for the running kernelget_submodules(module_name)– list submodules of a given moduleload_module(module_name)/unload_module(module_name)– load/unload modulesmodule_is_loaded(module_name)– check if a module is loadedloaded_module_info(module_name)– fetch details (size, deps, params, etc.)parse_lsmod_for_module(...)– parselsmodoutput for a specific module [1]
Docs location: Avocado “Utilities APIs” → avocado.utils.linux_modules module (latest docs) [1]
References:
[1] (avocado-framework.readthedocs.io)
Citations:
🏁 Script executed:
# Check the Setuper base class to understand expected behavior
rg "class Setuper" -A 20Repository: avocado-framework/avocado-vt
Length of output: 1169
🏁 Script executed:
# Check for other setup methods and their error handling patterns
rg "def setup\(" -A 15 | head -80Repository: avocado-framework/avocado-vt
Length of output: 4408
🏁 Script executed:
# Verify if package_install can raise exceptions
rg "def package_install" -A 50 | head -60Repository: avocado-framework/avocado-vt
Length of output: 2338
🏁 Script executed:
# Check other test_setup subclasses for error handling patterns
ls -la ./virttest/test_setup/ | grep -E "\.py$"Repository: avocado-framework/avocado-vt
Length of output: 1151
🏁 Script executed:
# Get list of all setup subclasses and their error handling
find ./virttest/test_setup -name "*.py" -exec grep -l "class.*Setuper" {} \;Repository: avocado-framework/avocado-vt
Length of output: 607
🏁 Script executed:
# Check if there are any imports of TestError in test_setup
rg "from avocado.*exceptions|import.*TestError" ./virttest/test_setup/Repository: avocado-framework/avocado-vt
Length of output: 221
🏁 Script executed:
# Check how requirement_checks.py uses exceptions in setup
rg "def setup" ./virttest/test_setup/requirement_checks.py -A 20 | head -100Repository: avocado-framework/avocado-vt
Length of output: 4155
🏁 Script executed:
# Check the __init__.py to see how exceptions are used
rg "TestError|exceptions\." ./virttest/test_setup/__init__.py | head -30Repository: avocado-framework/avocado-vt
Length of output: 1339
🏁 Script executed:
# Check what exception types are available in avocado.core.exceptions
rg "class Test.*Error|class Test.*Fail" virttest/test_setup/__init__.py -B 2 -A 5 | head -40Repository: avocado-framework/avocado-vt
Length of output: 54
🏁 Script executed:
# Search for TestSetupFail usage in the codebase
rg "TestSetupFail" ./virttest/test_setup/ | head -20Repository: avocado-framework/avocado-vt
Length of output: 1370
🌐 Web query:
avocado.core.exceptions TestError TestSetupFail available exception types
💡 Result:
In avocado.core.exceptions (per the current “latest” Avocado docs, v113.0), the available exception classes are: JobBaseException, JobError, JobFailFast, JobTestSuiteDuplicateNameError, JobTestSuiteEmptyError, JobTestSuiteError, JobTestSuiteReferenceResolutionError, OptionValidationError, TestAbortError, TestBaseException, TestCancel, TestError, TestFail, TestInterrupt, TestSetupFail, TestSkipError, TestWarn. [1]
Notes on the two you mentioned:
TestError: raised for a test that did not fully execute due to a fatal condition (setup/config/etc.). [1]TestSetupFail: indicates an error during setup or cleanup. [1]
Source
- [1] Avocado docs (latest):
avocado.core.exceptionsAPI reference. [1]
Validate return values of package install and module load, then raise on failure.
package_install() returns True/False but line 20 ignores the result. load_module() also returns a boolean but line 23 doesn't check if it succeeded. Both failures are silently swallowed by the broad exception handler at lines 27–28, which only logs the error without re-raising.
Check both return values and raise exceptions.TestSetupFail when installation or module loading fails. If catching exceptions, use LOG.exception() instead of LOG.error() for proper exception logging, and re-raise to signal setup failure to the test framework.
🛠️ Proposed fix
import logging
+from avocado.core import exceptions
from avocado.utils import linux_modules
from virttest import utils_package
from virttest.test_setup.core import Setuper
@@
_pkg = ["qemu-nbd", "ntfs-3g"]
try:
for pkg in _pkg:
if not utils_package.is_installed(pkg):
- utils_package.package_install(pkg)
+ if not utils_package.package_install(pkg):
+ raise exceptions.TestSetupFail(
+ f"Failed to install required package: {pkg}"
+ )
if not linux_modules.module_is_loaded("nbd"):
linux_modules.load_module("nbd")
+ if not linux_modules.module_is_loaded("nbd"):
+ raise exceptions.TestSetupFail("Failed to load nbd module")
self._nbd_is_loaded_by_vt = True
LOG.debug("The dump_file is set up successfully.")
- except Exception as e:
- LOG.error("The dump_file failed to set up.", exc_info=e)
+ except Exception:
+ LOG.exception("The dump_file failed to set up.")
+ raise🧰 Tools
🪛 Ruff (0.14.14)
28-28: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In `@virttest/test_setup/dump_file.py` around lines 12 - 28, The setup() method
currently ignores return values from utils_package.package_install and
linux_modules.load_module and swallows errors; update it to check each
utils_package.package_install(pkg) result and if False raise
exceptions.TestSetupFail with a descriptive message, and likewise check
linux_modules.load_module("nbd") return value and raise exceptions.TestSetupFail
on failure (still set self._nbd_is_loaded_by_vt only on success); when catching
exceptions use LOG.exception(...) to log the traceback and re-raise the
exception so failures propagate to the test framework.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/env_process.py`:
- Around line 1039-1040: The code currently always registers DumpFileSetup via
_setup_manager.register(DumpFileSetup) causing unnecessary package installs;
change this to register DumpFileSetup only when the dump feature is enabled by
checking the feature flag (e.g., dump_file_from_guest) before calling
_setup_manager.register(DumpFileSetup), leaving _setup_manager.do_setup() as-is
so setup runs for only the enabled features; locate the registration site that
calls _setup_manager.register and wrap that call in a conditional that queries
the feature flag (or equivalent config) to gate DumpFileSetup registration.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 15-22: The file virttest/guest_dump_file_manager.py fails Black
formatting; run the project's formatter (e.g., black . or pre-commit run
--all-files) and reformat this module so imports and lines match Black's style
(affecting the top-level imports and LOG declaration). After running Black,
stage and commit the updated virttest/guest_dump_file_manager.py so CI no longer
reports formatting differences.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 1-22: The file fails formatting checks—run the project formatter
to fix Black/isort issues: run `black` and `isort` (or `pre-commit run
--all-files`) and commit the changes so imports and code formatting in
guest_dump_file_manager.py (e.g., the imports and LOG declaration) match the
repository style; ensure imports are grouped/sorted (stdlib, third-party, local)
and spacing around top-level declarations is corrected before pushing.
🧹 Nitpick comments (2)
virttest/guest_dump_file_manager.py (2)
87-98: Consider raising an explicit error for unsupported LVM.Returning an empty string will flow into
utils_misc.mount()and likely cause a confusing error. Consider raisingNotImplementedErrorwith a clear message, or logging a warning and returning early fromrun()when LVM is detected.
18-18: Remove unused import.
linux_modulesis imported but not used in this file (it's used indump_file.pyinstead).♻️ Proposed fix
-from avocado.utils import linux_modules, process +from avocado.utils import process
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 165-172: The file formatting fails Black at the block that builds
and runs qemu-nbd (look for qemu_nbd_cmd, process.run, self._nbd_dev,
image_path, and LOG.debug in guest_dump_file_manager.py); fix it by running the
project's formatter (run black on the repo or run pre-commit run --all-files) to
apply Black's style changes, then commit the reformatted file so CI passes.
nickzhq
force-pushed
the
win_dump_mem
branch
2 times, most recently
from
6c80b72 to
c5a7a17
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 186-216: The fdisk/blkid detection can leave self._guest_type or
self._nbd_dev_partition_path unset and cause KeyError or blkid failures; update
the detection code around the fdisk loop (referenced by _mapping,
self._guest_type, self._nbd_dev_partition_path) to validate results after the
loop and raise/log a clear error if no partition was found, then run blkid and
check that self._partition_fstype is non-empty; before calling utils_misc.mount,
verify that self._partition_fstype exists in self._fs_support_mapping and
raise/log an explicit unsupported-filesystem error if not, so mount is only
attempted with validated values.
🧹 Nitpick comments (1)
virttest/guest_dump_file_manager.py (1)
246-257: Prefer explicit NotImplementedError over silent no-op.♻️ Suggested change
def run(self, image_path, res_dir): @@ - pass + raise NotImplementedError("Libvirt dump-file extraction is not implemented yet")
| # Detect the guest type( Linux or Windows ) and partition | ||
| _partition = "/dev/" + self._nbd_dev + "p" + str(len(self._partition_list)) | ||
| _mapping = {"Microsoft basic data": "Windows", "Linux LVM": "Linux"} | ||
| for des, platform in _mapping.items(): | ||
| _output = ( | ||
| process.run("fdisk -l /dev/%s" % self._nbd_dev) | ||
| .stdout_text.strip() | ||
| .splitlines() | ||
| ) | ||
| for row in _output: | ||
| if des in row: | ||
| self._guest_type = platform | ||
| self._nbd_dev_partition_path = row.split()[0] | ||
|
|
||
| LOG.debug("Detect the guest type: %s", self._guest_type) | ||
|
|
||
| # Detect the filesystem | ||
| self._partition_fstype = process.run( | ||
| "blkid -o value -s TYPE %s" % self._nbd_dev_partition_path | ||
| ).stdout_text.strip() | ||
| LOG.debug("Detect the guest fs type: %s", self._partition_fstype) | ||
|
|
||
| if not os.path.exists(self._mount_pt): | ||
| os.makedirs(self._mount_pt) | ||
|
|
||
| # mount the nbd device partition path | ||
| utils_misc.mount( | ||
| "%s" % self._nbd_dev_partition_path, | ||
| self._mount_pt, | ||
| fstype = self._fs_support_mapping[self._partition_fstype]() | ||
| ) |
There was a problem hiding this comment.
Validate partition/fs detection before mount to avoid KeyError/blkid failures.
If the fdisk scan doesn’t match, _guest_type/_nbd_dev_partition_path stay empty and blkid or _fs_support_mapping[...] will fail. Also handle unsupported filesystem types explicitly.
🛠️ Proposed fix
for des, platform in _mapping.items():
_output = (
process.run("fdisk -l /dev/%s" % self._nbd_dev)
.stdout_text.strip()
.splitlines()
)
for row in _output:
if des in row:
self._guest_type = platform
self._nbd_dev_partition_path = row.split()[0]
LOG.debug("Detect the guest type: %s", self._guest_type)
+ if not self._guest_type or not self._nbd_dev_partition_path:
+ raise RuntimeError("Failed to detect guest partition on %s" % self._nbd_dev)
# Detect the filesystem
self._partition_fstype = process.run(
"blkid -o value -s TYPE %s" % self._nbd_dev_partition_path
).stdout_text.strip()
LOG.debug("Detect the guest fs type: %s", self._partition_fstype)
+ fs_handler = self._fs_support_mapping.get(self._partition_fstype)
+ if not fs_handler:
+ raise RuntimeError("Unsupported filesystem type: %s" % self._partition_fstype)
+ fstype = fs_handler()
+ if not fstype:
+ raise RuntimeError("Unsupported filesystem handler for: %s" % self._partition_fstype)
@@
utils_misc.mount(
"%s" % self._nbd_dev_partition_path,
self._mount_pt,
- fstype = self._fs_support_mapping[self._partition_fstype]()
+ fstype=fstype,
)🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 186 - 216, The fdisk/blkid
detection can leave self._guest_type or self._nbd_dev_partition_path unset and
cause KeyError or blkid failures; update the detection code around the fdisk
loop (referenced by _mapping, self._guest_type, self._nbd_dev_partition_path) to
validate results after the loop and raise/log a clear error if no partition was
found, then run blkid and check that self._partition_fstype is non-empty; before
calling utils_misc.mount, verify that self._partition_fstype exists in
self._fs_support_mapping and raise/log an explicit unsupported-filesystem error
if not, so mount is only attempted with validated values.
|
Hello @zhencliu @YongxueHong @leidwang , let me schedule a meeting for review. Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 129-142: The code incorrectly prefixes items with self._mount_pt
when building the existence check and can invoke find with an empty path list;
update the loop that builds _dirs to test existence using the item values
directly (do not os.path.join(self._mount_pt, item) since items already include
the mount point) and keep _dirs as a list of valid directories; before
constructing _cmd, if _dirs is empty return/skip early (or raise) to avoid
running find in CWD; finally build the find command from the validated _dirs
list (joining/quoting paths safely) and proceed to copy results as before
(references: variables _dirs, _find_dmp_in_path, _cmd and res_dir).
- Around line 88-99: The _lvm2_member method currently returns None which
propagates into utils_misc.mount() as fstype and can break mounting; update
_lvm2_member (the method name to locate) to raise NotImplementedError with a
clear message like "LVM2 member handling not implemented" (so callers get an
explicit error) and/or update callers of _lvm2_member (e.g., code that calls
_lvm2_member before utils_misc.mount()) to catch that exception and handle it
gracefully; ensure the raised NotImplementedError is documented in the method
docstring so behavior is explicit.
| def _lvm2_member(self): | ||
| """ | ||
| Get the filesystem type for LVM2 partitions. | ||
|
|
||
| Note: LVM2_member cannot be mounted directly and requires | ||
| additional steps to access the logical volumes. | ||
|
|
||
| :return: Empty string (not yet implemented). | ||
| """ | ||
| # The lvm2_member type (guest type is Linux) is generated by vt default | ||
| # TODO: Support the linux guest. lvm2_member can NOT be used directly. And some steps are needed. | ||
| return None |
There was a problem hiding this comment.
LVM2 handler returns None which will cause mount() to fail.
The _lvm2_member() method returns None (Line 99), which flows into utils_misc.mount() at Line 215 as the fstype parameter. This will likely cause mount to fail or behave unexpectedly. Consider either raising NotImplementedError or adding explicit handling in the mount logic.
🛠️ Proposed fix - Option 1: Raise explicitly
def _lvm2_member(self):
# The lvm2_member type (guest type is Linux) is generated by vt default
# TODO: Support the linux guest. lvm2_member can NOT be used directly. And some steps are needed.
- return None
+ raise NotImplementedError("LVM2 member mounting is not yet supported")🛠️ Proposed fix - Option 2: Guard in mount logic
+ fs_handler = self._fs_support_mapping.get(self._partition_fstype)
+ fstype = fs_handler() if fs_handler else None
+ if not fstype:
+ LOG.warning("Unsupported filesystem type: %s, skipping mount", self._partition_fstype)
+ return
utils_misc.mount(
"%s" % self._nbd_dev_partition_path,
self._mount_pt,
- fstype=self._fs_support_mapping[self._partition_fstype](),
+ fstype=fstype,
)🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 88 - 99, The _lvm2_member
method currently returns None which propagates into utils_misc.mount() as fstype
and can break mounting; update _lvm2_member (the method name to locate) to raise
NotImplementedError with a clear message like "LVM2 member handling not
implemented" (so callers get an explicit error) and/or update callers of
_lvm2_member (e.g., code that calls _lvm2_member before utils_misc.mount()) to
catch that exception and handle it gracefully; ensure the raised
NotImplementedError is documented in the method docstring so behavior is
explicit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 162-216: The code lacks validation after selecting NBD device,
discovering partitions, detecting guest type/partition, and mapping filesystem
types; update the block around self._nbd_dev selection and subsequent steps
(qemu_nbd_cmd, self._partition_list, partition detection loop that sets
self._guest_type and self._nbd_dev_partition_path, blkid call that sets
self._partition_fstype, and the utils_misc.mount call) to explicitly check for
failure at each step: if self._nbd_dev is empty abort with a clear error/log and
do not run qemu-nbd; after running qemu-nbd ensure self._partition_list is
non-empty and raise/log and disconnect on failure; ensure the
partition-detection loop sets self._guest_type and self._nbd_dev_partition_path
and fail with a descriptive error if not; validate that self._partition_fstype
is found and that self._fs_support_mapping contains the key before calling it
(otherwise raise/log a useful error or fallback), and perform proper cleanup
(disconnect qemu-nbd) on any failure.
| # Choose the nbd device (Such as nbd15, nbd14) | ||
| _output = process.run(self._nbd_dev_cmd).stdout_text | ||
| for row in _output.strip().splitlines()[::-1]: | ||
| if "nbd" in row and "0B" in row and "/" not in row: | ||
| self._nbd_dev = row.split()[0] | ||
| break | ||
| LOG.debug("Find the available nbd device: %s", self._nbd_dev) | ||
|
|
||
| try: | ||
| qemu_nbd_cmd = "qemu-nbd -c /dev/%s %s" % ( | ||
| self._nbd_dev, | ||
| image_path, | ||
| ) | ||
| process.run(qemu_nbd_cmd) | ||
| LOG.debug("Connect the nbd device and image: %s", image_path) | ||
|
|
||
| # Detect the partition list (such as ["/dev/nba15p1", "/dev/nba15p2", "/dev/nba15p3"] ) | ||
| self._partition_list = ( | ||
| process.run('find /dev -name "%sp*"' % self._nbd_dev) | ||
| .stdout_text.strip() | ||
| .split() | ||
| ) | ||
| LOG.debug("Detect the nbd partition list: %s", self._partition_list) | ||
|
|
||
| # Detect the guest type( Linux or Windows ) and partition | ||
| _partition = "/dev/" + self._nbd_dev + "p" + str(len(self._partition_list)) | ||
| _mapping = {"Microsoft basic data": "Windows", "Linux LVM": "Linux"} | ||
| for des, platform in _mapping.items(): | ||
| _output = ( | ||
| process.run("fdisk -l /dev/%s" % self._nbd_dev) | ||
| .stdout_text.strip() | ||
| .splitlines() | ||
| ) | ||
| for row in _output: | ||
| if des in row: | ||
| self._guest_type = platform | ||
| self._nbd_dev_partition_path = row.split()[0] | ||
|
|
||
| LOG.debug("Detect the guest type: %s", self._guest_type) | ||
|
|
||
| # Detect the filesystem | ||
| self._partition_fstype = process.run( | ||
| "blkid -o value -s TYPE %s" % self._nbd_dev_partition_path | ||
| ).stdout_text.strip() | ||
| LOG.debug("Detect the guest fs type: %s", self._partition_fstype) | ||
|
|
||
| if not os.path.exists(self._mount_pt): | ||
| os.makedirs(self._mount_pt) | ||
|
|
||
| # mount the nbd device partition path | ||
| utils_misc.mount( | ||
| "%s" % self._nbd_dev_partition_path, | ||
| self._mount_pt, | ||
| fstype=self._fs_support_mapping[self._partition_fstype](), | ||
| ) |
There was a problem hiding this comment.
Missing validation for NBD device, partition detection, and filesystem type.
Several failure modes lack validation:
-
Lines 162-167: If no suitable NBD device is found,
self._nbd_devremains an empty string. The subsequentqemu-nbd -c /dev/ ...command (Line 171-175) will fail. -
Lines 186-198: The loop may exit without finding a matching partition, leaving
self._guest_typeandself._nbd_dev_partition_pathempty. This causesblkid(Line 203-204) and mount (Line 212-216) to fail. -
Line 215:
self._fs_support_mapping[self._partition_fstype]()raisesKeyErrorif the filesystem type is not in the mapping (only "ntfs" and "LVM2_member" are supported).
🛠️ Proposed validation
for row in _output.strip().splitlines()[::-1]:
if "nbd" in row and "0B" in row and "/" not in row:
self._nbd_dev = row.split()[0]
break
+ if not self._nbd_dev:
+ raise RuntimeError("No available NBD device found")
LOG.debug("Find the available nbd device: %s", self._nbd_dev)
@@
LOG.debug("Detect the guest type: %s", self._guest_type)
+ if not self._guest_type or not self._nbd_dev_partition_path:
+ raise RuntimeError("Failed to detect guest partition on %s" % self._nbd_dev)
# Detect the filesystem
@@
LOG.debug("Detect the guest fs type: %s", self._partition_fstype)
+ if self._partition_fstype not in self._fs_support_mapping:
+ raise RuntimeError("Unsupported filesystem type: %s" % self._partition_fstype)🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 162 - 216, The code lacks
validation after selecting NBD device, discovering partitions, detecting guest
type/partition, and mapping filesystem types; update the block around
self._nbd_dev selection and subsequent steps (qemu_nbd_cmd,
self._partition_list, partition detection loop that sets self._guest_type and
self._nbd_dev_partition_path, blkid call that sets self._partition_fstype, and
the utils_misc.mount call) to explicitly check for failure at each step: if
self._nbd_dev is empty abort with a clear error/log and do not run qemu-nbd;
after running qemu-nbd ensure self._partition_list is non-empty and raise/log
and disconnect on failure; ensure the partition-detection loop sets
self._guest_type and self._nbd_dev_partition_path and fail with a descriptive
error if not; validate that self._partition_fstype is found and that
self._fs_support_mapping contains the key before calling it (otherwise raise/log
a useful error or fallback), and perform proper cleanup (disconnect qemu-nbd) on
any failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/env_process.py`:
- Around line 712-723: The dump extraction block is currently dead code:
base_dir and _img are assigned but unused and the important call _mgr.run(...)
is commented out; either remove this whole conditional or implement it properly
by getting the dump manager via
guest_dump_file_manager.get_dump_file_mgr(params.get("vm_type")), constructing
the image object (use qemu_storage.QemuImg(params, base_dir, image_name) or the
existing image if present) using base_dir (params.get("images_base_dir",
data_dir.get_data_dir())) and the first image from params.objects("images")[0],
then call _mgr.run(image.image_filename, test.outputdir) (or the correct output
dir variable in scope) so the dump manager actually runs; update or remove the
commented lines and ensure no unused variables remain.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/env_process.py`:
- Around line 646-648: The file fails Black formatting; run the repository
formatter (e.g., pre-commit run --all-files or black .) and commit the
reformatted changes so CI passes; specifically reformat the block handling
params.get("dumping_guest_memory") and the LOG.debug call in env_process.py (the
section referencing dumping_guest_memory and LOG.debug) to comply with Black
style and include the resulting formatted file in your commit.
71f7361 to
a190f44
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 52-60: The code builds _dmp_dirs_scaned with capitalized keys
("Windows", "Linux") but looks up os type from params.get("os_type") which is
lowercase, causing KeyError in _process_dump_file_from_guest; normalize the OS
key before indexing (e.g., map params.get("os_type") or _guest_type to the
title-cased key or change _dmp_dirs_scaned to use lowercase keys) so
_process_dump_file_from_guest uses the same normalized key when accessing
self._dmp_dirs_scaned; update all lookups (including the similar site at lines
~222-223) to use the same normalization function or pattern.
- Around line 67-86: The run method builds a dispatch dict _run and indexes it
with params.get("dumping_guest_memory") which can raise KeyError for
missing/unsupported modes; update guest_dump_file_manager.run to validate the
dumping_guest_memory value (e.g., read into a variable mode =
params.get("dumping_guest_memory")) and if mode not in _run, raise a clear
ValueError (or log and raise) mentioning the invalid mode, otherwise call
_run[mode](params, res_dir, image_path); reference the run method and the _run
dict to locate the change.
- Around line 87-117: The base class methods _run_online and _run_offline
currently silently no-op; change both implementations to raise
NotImplementedError with a clear message (e.g., "Subclasses must implement
_run_online" / "Subclasses must implement _run_offline") so callers fail loudly
when a subclass hasn't provided the online/offline dump-collection logic; update
the methods _run_online and _run_offline in guest_dump_file_manager.py
accordingly.
| def run(self, params, res_dir, image_path=None): | ||
| """ | ||
| Execute the dump file collection process. | ||
|
|
||
| This method should be implemented by subclasses to perform | ||
| the complete workflow of mounting the image and extracting dump files. | ||
|
|
||
| :param image_path: Path to the guest disk image. | ||
| :type image_path: str | ||
| :param res_dir: The path where results should be stored. | ||
| :type res_dir: str | ||
| :param params: Parameters of vm. | ||
| :type params: dict | ||
| """ | ||
| _run = { | ||
| "online": self._run_online, | ||
| "offline": self._run_offline, | ||
| } | ||
| _run[params.get("dumping_guest_memory")](params, res_dir, image_path) | ||
|
|
There was a problem hiding this comment.
Guard against unsupported dumping_guest_memory values.
A missing or unexpected mode causes a KeyError. Fail fast with a clear error.
🛠️ Proposed fix
_run = {
"online": self._run_online,
"offline": self._run_offline,
}
- _run[params.get("dumping_guest_memory")](params, res_dir, image_path)
+ mode = params.get("dumping_guest_memory")
+ runner = _run.get(mode)
+ if not runner:
+ raise ValueError("Unsupported dumping_guest_memory mode: %s" % mode)
+ runner(params, res_dir, image_path)🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 67 - 86, The run method
builds a dispatch dict _run and indexes it with
params.get("dumping_guest_memory") which can raise KeyError for
missing/unsupported modes; update guest_dump_file_manager.run to validate the
dumping_guest_memory value (e.g., read into a variable mode =
params.get("dumping_guest_memory")) and if mode not in _run, raise a clear
ValueError (or log and raise) mentioning the invalid mode, otherwise call
_run[mode](params, res_dir, image_path); reference the run method and the _run
dict to locate the change.
|
Hello @zhencliu @YongxueHong @leidwang , this patch is ready to review. Please take a look when you're available. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@virttest/guest_dump_file_manager.py`:
- Around line 317-328: Add a final newline at the end of the file to satisfy the
end-of-file-fixer; open virttest/guest_dump_file_manager.py (e.g., near the
get_dump_file_mgr function) and ensure the file ends with a single trailing
newline character, then save or rerun pre-commit to confirm the EOF fixer
passes.
🧹 Nitpick comments (1)
virttest/guest_dump_file_manager.py (1)
258-268: Narrow cleanup exception handling (avoid blanketException).
Catching all exceptions can hide real cleanup failures; consider catching specific exceptions (e.g., process/umount errors) and logging with stack traces.
| def get_dump_file_mgr(vm_type): | ||
| """ | ||
| Get dump file manager instance for the specified VM type. | ||
|
|
||
| This is a convenience function that delegates to the | ||
| DumpFileManagerFactory. | ||
|
|
||
| :param vm_type: Type of virtual machine ("qemu" or "libvirt"). | ||
| :type vm_type: str | ||
| :return: Instance of the appropriate dump file manager. | ||
| """ | ||
| return DumpFileManagerFactory.get_mgr(vm_type) |
There was a problem hiding this comment.
Fix end-of-file-fixer failure (missing trailing newline).
CI shows end-of-file-fixer modified this file; please add a final newline or rerun pre-commit.
🧰 Tools
🪛 GitHub Actions: pre-commit
[error] 326-326: pre-commit end-of-file-fixer hook modified the file. CI failed due to hook exit code 1.
🤖 Prompt for AI Agents
In `@virttest/guest_dump_file_manager.py` around lines 317 - 328, Add a final
newline at the end of the file to satisfy the end-of-file-fixer; open
virttest/guest_dump_file_manager.py (e.g., near the get_dump_file_mgr function)
and ensure the file ends with a single trailing newline character, then save or
rerun pre-commit to confirm the EOF fixer passes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@virttest/env_process.py`:
- Around line 714-718: Wrap the call to _mgr.run(params, test.outputdir,
_sys_image.image_filename) inside a try/except within postprocess_vm so that any
exception from _mgr.run (e.g., NBD/mount failures) is caught, logged, and
suppressed (or re-raised after cleanup) to ensure the subsequent enable_strace
cleanup block always runs; specifically, catch Exception around the _mgr.run
invocation, log the error including exception details, and continue so
enable_strace cleanup at the end of postprocess_vm executes.
- Around line 646-647: The call that instantiates the dump file manager (_mgr =
guest_dump_file_manager.get_dump_file_mgr(params.get("vm_type"))) should not run
unconditionally at the top of postprocess_vm; move this call into the branch
that checks dumping_guest_memory so the manager is only obtained when needed
(i.e., inside the conditional that uses _mgr later), and ensure any subsequent
uses of _mgr (the code that writes or cleans up guest memory dumps) still
reference the moved variable and handle a None/absent manager safely; remove the
top-level call to avoid the cached singleton side-effect and keep the variable
scoped to the dumping_guest_memory path.
🧹 Nitpick comments (1)
virttest/test_setup/dump_file.py (1)
27-32: Minor grammar nit in cleanup log message.Line 30:
"cleanup successfully"→"cleaned up successfully".
| _mgr = guest_dump_file_manager.get_dump_file_mgr(params.get("vm_type")) | ||
|
|
There was a problem hiding this comment.
Manager instantiated unconditionally for every VM postprocess.
get_dump_file_mgr is called at the top of postprocess_vm for every test, even when dumping_guest_memory is not configured. This is unnecessary overhead and creates a cached singleton as a side-effect. Move the call inside the conditional block at line 714 where it's actually needed.
Proposed fix
- _mgr = guest_dump_file_manager.get_dump_file_mgr(params.get("vm_type"))
-
if params.get("start_vm") == "yes":Then at lines 714-718:
if params.get("dumping_guest_memory", "") == "offline":
+ _mgr = guest_dump_file_manager.get_dump_file_mgr(params.get("vm_type"))
- if params.get("dumping_guest_memory", "") == "offline" and _mgr:
+ if _mgr:
base_dir = params.get("images_base_dir", data_dir.get_data_dir())🤖 Prompt for AI Agents
In `@virttest/env_process.py` around lines 646 - 647, The call that instantiates
the dump file manager (_mgr =
guest_dump_file_manager.get_dump_file_mgr(params.get("vm_type"))) should not run
unconditionally at the top of postprocess_vm; move this call into the branch
that checks dumping_guest_memory so the manager is only obtained when needed
(i.e., inside the conditional that uses _mgr later), and ensure any subsequent
uses of _mgr (the code that writes or cleans up guest memory dumps) still
reference the moved variable and handle a None/absent manager safely; remove the
top-level call to avoid the cached singleton side-effect and keep the variable
scoped to the dumping_guest_memory path.
18eeb79 to
178d546
Compare
nickzhq
force-pushed
the
win_dump_mem
branch
7 times, most recently
from
dbe4ed9 to
e0b853d
Compare
nickzhq
force-pushed
the
win_dump_mem
branch
2 times, most recently
from
c5a87d4 to
77eb97c
Compare
|
Hello @zhencliu @YongxueHong @leidwang , this patch is ready to review. Please take a look when you're available. |
nickzhq
force-pushed
the
win_dump_mem
branch
2 times, most recently
from
8d74165 to
f61658c
Compare
The guest dump file extraction architecture to
support multiple dump methods through a flexible dump object pattern.
Architecture changes:
- Introduce BaseDumpObject interface for different dump implementations
- Create dedicated dump object classes for each method:
* QemuNBDDump: NBD-based offline dump extraction
* QemuQMPDump: QMP-based online dump
* QemuBackupImageDump: Backup image dump
* Libvirt dump objects
- Simplify GuestDumpFileManager as orchestrator:
* Add get_dump_object(params) to create appropriate dump object
* The dump extraction is optional - only runs
if vm_memory_dump_method is set in params
- Update env_process.py to use new extract_dump_files() API
Benefits:
- Clear separation of concerns: manager orchestrates, dump objects execute
- Easy to extend with new dump methods (just add new dump object class)
- Better code organization with dump-method specific logic isolated
- Optional execution model via vm_memory_dump_method parameter
Signed-off-by: Houqi (Nick) Zuo <hzuo@redhat.com>
5 participants
Add "dump memory file" step in postprocess_vm function The guest_dump_file_manager.py implements the main function The dump_file.py implements "prepare the environment"
ID: KVMAUTOMA-1304
Summary by CodeRabbit
New Features
Chores