Automate nbdkit data integrity test for NBD request size constraints and NFS truncation risk#6843
Closed
ganeshhubale wants to merge 2 commits intoautotest:masterfrom
Closed
Automate nbdkit data integrity test for NBD request size constraints and NFS truncation risk#6843ganeshhubale wants to merge 2 commits intoautotest:masterfrom
ganeshhubale wants to merge 2 commits intoautotest:masterfrom
Conversation
WalkthroughReorganized nbdkit test config by moving nbdcopy's Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
v2v/tests/src/nbdkit/nbdkit.py (3)
868-874: EnsureIMG_PATHcleanup in thefinallyblock.The cleanup removes the socket file but doesn't remove the 1G sparse image file created at
IMG_PATH. This could leave test artifacts behind.♻️ Proposed fix to clean up the image file
finally: LOG.info("Cleaning up") nbd_server.terminate() nbd_server.wait() nbd_log_file.close() if os.path.exists(SOCKET): os.remove(SOCKET) + if os.path.exists(IMG_PATH): + os.remove(IMG_PATH)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 868 - 874, The finally block cleans up the socket but not the sparse image at IMG_PATH; update the cleanup to also remove the image file by checking os.path.exists(IMG_PATH) and calling os.remove(IMG_PATH) (alongside existing cleanup of SOCKET), ensuring this is done after nbd_server termination and nbd_log_file.close() in the same finally block that references nbd_server, nbd_log_file, SOCKET and IMG_PATH.
854-858:head -cwith human-readable suffix may not be portable.The command
head -c {IMAGE_SIZE}whereIMAGE_SIZE="1G"relies on GNU coreutils supporting suffixes likeG. This works on most Linux systems but may fail on minimal environments or non-GNU systems.Consider using the numeric value directly for consistency:
♻️ Suggested fix for portability
- hash_cmd = f"nbdcopy nbd+unix:///?socket={SOCKET} - | head -c {IMAGE_SIZE} | md5sum" + # Use numeric byte count for portability (1G = 1073741824 bytes) + hash_cmd = f"nbdcopy nbd+unix:///?socket={SOCKET} - | head -c 1073741824 | md5sum" result = process.run(hash_cmd, shell=True, ignore_status=True) actual_md5 = result.stdout_text - result = process.run("head -c 1G /dev/zero | md5sum", shell=True, ignore_status=True) + result = process.run("head -c 1073741824 /dev/zero | md5sum", shell=True, ignore_status=True)Alternatively, define a constant
IMAGE_SIZE_BYTES = 1073741824at the top of the function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 854 - 858, The use of human-readable suffixes in head -c (IMAGE_SIZE = "1G") is not portable; update the code that builds hash_cmd and the /dev/zero command to use a numeric byte count instead (e.g., define IMAGE_SIZE_BYTES = 1073741824 and replace occurrences of IMAGE_SIZE in the hash_cmd string and the second process.run call), or convert IMAGE_SIZE to bytes before interpolation so hash_cmd and the expected_md5 run use the raw byte integer rather than "1G"; update references to hash_cmd, IMAGE_SIZE, and expected_md5 accordingly.
816-824: Consider usingtempfilefor the socket path.The hardcoded
/tmp/test.sockpath (flagged by static analysis as S108) could cause conflicts if multiple test instances run concurrently. While this may be acceptable for single-instance test execution, using a dynamically generated path would be safer.♻️ Suggested improvement using tempfile
# --- Configuration --- - SOCKET = "/tmp/test.sock" + SOCKET = os.path.join(data_dir.get_tmp_dir(), "test.sock") IMG_PATH = os.path.join(data_dir.get_tmp_dir(), "test.img")This aligns with how
IMG_PATHis already usingdata_dir.get_tmp_dir().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 816 - 824, Replace the hardcoded SOCKET = "/tmp/test.sock" with a dynamically generated socket path using tempfile within the same temp dir as IMG_PATH: create a temporary file in data_dir.get_tmp_dir() (e.g., tempfile.NamedTemporaryFile(delete=False) or tempfile.mkstemp alternative) and use its .name as SOCKET, ensure you close the temp file and keep existing cleanup logic (os.remove(SOCKET) if exists) so concurrent test runs don't conflict; update references to SOCKET in this block accordingly (look for SOCKET, IMG_PATH, data_dir.get_tmp_dir()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2v/tests/cfg/nbdkit/nbdkit.cfg`:
- Around line 161-168: Several version_required entries use inconsistent
package-name formats (e.g., "[nbdkit-1.46,)", "[nbdkit-1.38.0-1,)",
"[nbdkit-1.26.5-1,nbdkit-1.45)") while the file standard is the full
nbdkit-server release format like "nbdkit-server-1.42.2-4"; update the
mismatched version_required values to the canonical "nbdkit-server-X.Y.Z-R" form
(or confirm intentional variation) so all version_required keys use the same
package naming convention; specifically replace the literal strings shown in the
diff with their corresponding nbdkit-server-<version>-<release> equivalents.
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 827-833: The code shadows the nbd_log_file path with a file handle
in the with block, causing the log file to be closed immediately after Popen
starts and later double-closed; fix by opening the log file outside the with (or
rename to avoid shadowing) and keep that file handle open for the lifetime of
the nbdkit subprocess started by Popen(server_cmd, ...), e.g., create a separate
variable like nbd_log_path = os.path.join(...), open nbd_log_handle =
open(nbd_log_path, "w") before calling Popen(server_cmd, stdout=nbd_log_handle,
stderr=nbd_log_handle), and ensure the same nbd_log_handle is closed in the
finally/cleanup code rather than relying on the with block; update references to
nbd_log_file in Popen and the finally close to use the persistent handle.
---
Nitpick comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 868-874: The finally block cleans up the socket but not the sparse
image at IMG_PATH; update the cleanup to also remove the image file by checking
os.path.exists(IMG_PATH) and calling os.remove(IMG_PATH) (alongside existing
cleanup of SOCKET), ensuring this is done after nbd_server termination and
nbd_log_file.close() in the same finally block that references nbd_server,
nbd_log_file, SOCKET and IMG_PATH.
- Around line 854-858: The use of human-readable suffixes in head -c (IMAGE_SIZE
= "1G") is not portable; update the code that builds hash_cmd and the /dev/zero
command to use a numeric byte count instead (e.g., define IMAGE_SIZE_BYTES =
1073741824 and replace occurrences of IMAGE_SIZE in the hash_cmd string and the
second process.run call), or convert IMAGE_SIZE to bytes before interpolation so
hash_cmd and the expected_md5 run use the raw byte integer rather than "1G";
update references to hash_cmd, IMAGE_SIZE, and expected_md5 accordingly.
- Around line 816-824: Replace the hardcoded SOCKET = "/tmp/test.sock" with a
dynamically generated socket path using tempfile within the same temp dir as
IMG_PATH: create a temporary file in data_dir.get_tmp_dir() (e.g.,
tempfile.NamedTemporaryFile(delete=False) or tempfile.mkstemp alternative) and
use its .name as SOCKET, ensure you close the temp file and keep existing
cleanup logic (os.remove(SOCKET) if exists) so concurrent test runs don't
conflict; update references to SOCKET in this block accordingly (look for
SOCKET, IMG_PATH, data_dir.get_tmp_dir()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65a2b7a4-cde1-48d1-ba00-c41824b82e15
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/src/nbdkit/nbdkit.py
…and NFS truncation risk Signed-off-by: Ganesh Hubale <ghubale@redhat.com>
ganeshhubale
force-pushed
the
auto-data-exposure-nfs
branch
from
25b3cde to
df0bef1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
v2v/tests/src/nbdkit/nbdkit.py (1)
835-867: Reject unsupportedtest_statevalues.Anything other than
positiveornegativecurrently falls through as success, so a cfg typo can turn this into a false-positive pass.🧩 Proposed fix
elif test_state == "negative": LOG.info("Testing nbdcopy with 128M (Expected Failure)") cmd = f"nbdcopy --request-size=128M /dev/zero nbd+unix:///?socket={SOCKET}" result = process.run(cmd, shell=True, ignore_status=True) err_msg = "must be a power of 2 within 4096-33554432: 128M" if err_msg not in result.stderr_text: test.fail(f"Expected warning {err_msg}, got {result.stderr_text}") + else: + test.error(f"Unsupported test_state: {test_state}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 835 - 867, The test currently only handles test_state == "positive" and "negative" and silently succeeds for any other value; update the conditional that checks test_state (the if test_state == "positive": / elif test_state == "negative": block) to add an else branch that rejects unsupported values (e.g., call test.fail(...) with a clear message including the unexpected test_state value and/or log it via LOG.error) so a cfg typo or unknown value cannot produce a false-positive pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 816-833: Replace the fixed SOCKET and blind sleep with a per-run
unique UNIX socket and an explicit wait loop: create SOCKET using a unique name
(e.g. include pid/uuid) instead of "/tmp/test.sock", avoid unconditional
os.remove(SOCKET), start nbdkit via Popen (the existing nbd_server) and then
poll in a short loop that (a) checks for SOCKET existence, (b) checks
nbd_server.poll() for early exit and logs/raises if it exited, and (c) times out
after a short period failing the test; update the startup block that defines
SOCKET, starts nbd_server, and the time.sleep(1) to this poll-and-check logic so
callers of nbdkit (e.g. nbdcopy) only proceed when the socket is owned by this
run and nbdkit is alive.
- Around line 868-873: The finally block currently calls nbd_server.terminate()
followed by a blocking nbd_server.wait(), which can hang; update the cleanup to
mirror the terminate/wait-with-timeout/kill fallback used in
test_nbdkit_instance_name(): after calling nbd_server.terminate(), call
nbd_server.wait(timeout=some_seconds) and if it times out or the process is
still alive call nbd_server.kill() and wait again (with a short timeout),
catching and logging exceptions via LOG; still remove SOCKET if present.
Reference nbd_server.terminate(), nbd_server.wait(), nbd_server.kill(), LOG,
SOCKET and reuse the same timeout semantics as test_nbdkit_instance_name().
---
Nitpick comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 835-867: The test currently only handles test_state == "positive"
and "negative" and silently succeeds for any other value; update the conditional
that checks test_state (the if test_state == "positive": / elif test_state ==
"negative": block) to add an else branch that rejects unsupported values (e.g.,
call test.fail(...) with a clear message including the unexpected test_state
value and/or log it via LOG.error) so a cfg typo or unknown value cannot produce
a false-positive pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 621917fc-b4ee-4bd2-871f-3c4375723f93
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/src/nbdkit/nbdkit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- v2v/tests/cfg/nbdkit/nbdkit.cfg
Comment on lines
+816
to
+833
| SOCKET = "/tmp/test.sock" | ||
| IMG_PATH = os.path.join(data_dir.get_tmp_dir(), "test.img") | ||
| IMAGE_SIZE = "1G" | ||
|
|
||
| LOG.info(f"Prepare {IMAGE_SIZE} image file") | ||
| cmd = f"truncate -s {IMAGE_SIZE} {IMG_PATH}" | ||
| process.run(cmd, shell=True, ignore_status=True) | ||
| if os.path.exists(SOCKET): | ||
| os.remove(SOCKET) | ||
|
|
||
| LOG.info("Starting nbdkit server in background") | ||
| nbd_log_file = os.path.join(data_dir.get_tmp_dir(), "nbd_log_file.log") | ||
| # Start nbdkit with debug flags enabled | ||
| with open(nbd_log_file, "w") as nbd_log_file: | ||
| server_cmd = ["nbdkit", "-f", "-v", "-D", "file.zero=1", "-U", SOCKET, "file", IMG_PATH] | ||
| nbd_server = Popen(server_cmd, stdout=nbd_log_file, stderr=nbd_log_file) | ||
| # Wait for the socket file to appear | ||
| time.sleep(1) |
There was a problem hiding this comment.
Use a per-run UNIX socket and wait for it explicitly.
The fixed /tmp/test.sock can clash with another avocado job, and the unconditional os.remove(SOCKET) can unlink a socket that belongs to a different still-running test. On top of that, sleep(1) makes startup flaky on slower hosts. Please allocate a short unique socket path under /tmp and poll for socket creation or early nbdkit exit before running nbdcopy.
🧰 Tools
🪛 Ruff (0.15.6)
[error] 816-816: Probable insecure usage of temporary file or directory: "/tmp/test.sock"
(S108)
[error] 822-822: Function call with shell=True parameter identified, security issue
(S604)
[error] 831-831: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 816 - 833, Replace the fixed
SOCKET and blind sleep with a per-run unique UNIX socket and an explicit wait
loop: create SOCKET using a unique name (e.g. include pid/uuid) instead of
"/tmp/test.sock", avoid unconditional os.remove(SOCKET), start nbdkit via Popen
(the existing nbd_server) and then poll in a short loop that (a) checks for
SOCKET existence, (b) checks nbd_server.poll() for early exit and logs/raises if
it exited, and (c) times out after a short period failing the test; update the
startup block that defines SOCKET, starts nbd_server, and the time.sleep(1) to
this poll-and-check logic so callers of nbdkit (e.g. nbdcopy) only proceed when
the socket is owned by this run and nbdkit is alive.
Comment on lines
+868
to
+873
| finally: | ||
| LOG.info("Cleaning up") | ||
| nbd_server.terminate() | ||
| nbd_server.wait() | ||
| if os.path.exists(SOCKET): | ||
| os.remove(SOCKET) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "nbdkit.py" | head -5Repository: autotest/tp-libvirt
Length of output: 95
🏁 Script executed:
cd v2v/tests/src/nbdkit && wc -l nbdkit.pyRepository: autotest/tp-libvirt
Length of output: 76
🏁 Script executed:
cd v2v/tests/src/nbdkit && sed -n '860,880p' nbdkit.pyRepository: autotest/tp-libvirt
Length of output: 1065
🏁 Script executed:
cd v2v/tests/src/nbdkit && grep -n "test_nbdkit_instance_name" nbdkit.pyRepository: autotest/tp-libvirt
Length of output: 199
🏁 Script executed:
cd v2v/tests/src/nbdkit && sed -n '742,810p' nbdkit.pyRepository: autotest/tp-libvirt
Length of output: 3108
🏁 Script executed:
cd v2v/tests/src/nbdkit && sed -n '800,820p' nbdkit.pyRepository: autotest/tp-libvirt
Length of output: 846
Bound subprocess shutdown with timeout.
nbd_server.wait() can hang the whole job if nbdkit stops responding. This should use the same terminate/wait-with-timeout/kill fallback pattern already implemented in test_nbdkit_instance_name().
🛠️ Proposed fix
- from subprocess import Popen, PIPE, STDOUT
+ from subprocess import Popen, TimeoutExpired
...
finally:
LOG.info("Cleaning up")
- nbd_server.terminate()
- nbd_server.wait()
+ if nbd_server.poll() is None:
+ nbd_server.terminate()
+ try:
+ nbd_server.wait(timeout=5)
+ except TimeoutExpired:
+ nbd_server.kill()
+ nbd_server.wait(timeout=5)
if os.path.exists(SOCKET):
os.remove(SOCKET)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 868 - 873, The finally block
currently calls nbd_server.terminate() followed by a blocking nbd_server.wait(),
which can hang; update the cleanup to mirror the
terminate/wait-with-timeout/kill fallback used in test_nbdkit_instance_name():
after calling nbd_server.terminate(), call nbd_server.wait(timeout=some_seconds)
and if it times out or the process is still alive call nbd_server.kill() and
wait again (with a short timeout), catching and logging exceptions via LOG;
still remove SOCKET if present. Reference nbd_server.terminate(),
nbd_server.wait(), nbd_server.kill(), LOG, SOCKET and reuse the same timeout
semantics as test_nbdkit_instance_name().
There was a problem hiding this comment.
🧹 Nitpick comments (3)
v2v/tests/src/nbdkit/nbdkit.py (3)
854-858: UseIMAGE_SIZEvariable consistently.Line 857 hardcodes
"1G"while other lines use{IMAGE_SIZE}. This could cause a mismatch ifIMAGE_SIZEis changed.- result = process.run("head -c 1G /dev/zero | md5sum", shell=True, ignore_status=True) + result = process.run(f"head -c {IMAGE_SIZE} /dev/zero | md5sum", shell=True, ignore_status=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 854 - 858, The test computes actual_md5 using IMAGE_SIZE but computes expected_md5 with a hardcoded "1G"; update the expected_md5 command to use the IMAGE_SIZE variable consistently (e.g., build the command similar to hash_cmd using IMAGE_SIZE) so that the process.run call that sets expected_md5 references IMAGE_SIZE (the same variable used in hash_cmd) and captures its stdout_text into expected_md5.
868-873: Consider cleaning up the test image file.The
finallyblock removes the socket but doesn't clean upIMG_PATH. Whiledata_dir.get_tmp_dir()is likely cleaned elsewhere, explicit cleanup ensures no leftover 1G sparse files.if os.path.exists(SOCKET): os.remove(SOCKET) + if os.path.exists(IMG_PATH): + os.remove(IMG_PATH)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 868 - 873, The finally block currently only removes the socket and terminates nbd_server; also delete the test image file at IMG_PATH to avoid leaving the 1G sparse file behind. In the same finally block after removing SOCKET, check if os.path.exists(IMG_PATH) and remove it (os.remove(IMG_PATH)), and ensure any exceptions from removal are caught/logged (e.g., wrap removal in try/except) so cleanup doesn't mask earlier exceptions; reference the existing finally block, IMG_PATH, and nbd_server.
813-813: Remove unused imports.
PIPEandSTDOUTare imported but never used in this function.def test_nbd_data_integrity(): - from subprocess import Popen, PIPE, STDOUT + from subprocess import Popen import time🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/nbdkit/nbdkit.py` at line 813, The import statement currently brings in unused symbols PIPE and STDOUT along with Popen; remove the unused PIPE and STDOUT to avoid a lint warning by changing the import used in this file (the line that reads "from subprocess import Popen, PIPE, STDOUT") so it only imports Popen (or import subprocess and use subprocess.Popen) — target the import that defines Popen/PIPE/STDOUT in this module and drop PIPE and STDOUT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 854-858: The test computes actual_md5 using IMAGE_SIZE but
computes expected_md5 with a hardcoded "1G"; update the expected_md5 command to
use the IMAGE_SIZE variable consistently (e.g., build the command similar to
hash_cmd using IMAGE_SIZE) so that the process.run call that sets expected_md5
references IMAGE_SIZE (the same variable used in hash_cmd) and captures its
stdout_text into expected_md5.
- Around line 868-873: The finally block currently only removes the socket and
terminates nbd_server; also delete the test image file at IMG_PATH to avoid
leaving the 1G sparse file behind. In the same finally block after removing
SOCKET, check if os.path.exists(IMG_PATH) and remove it (os.remove(IMG_PATH)),
and ensure any exceptions from removal are caught/logged (e.g., wrap removal in
try/except) so cleanup doesn't mask earlier exceptions; reference the existing
finally block, IMG_PATH, and nbd_server.
- Line 813: The import statement currently brings in unused symbols PIPE and
STDOUT along with Popen; remove the unused PIPE and STDOUT to avoid a lint
warning by changing the import used in this file (the line that reads "from
subprocess import Popen, PIPE, STDOUT") so it only imports Popen (or import
subprocess and use subprocess.Popen) — target the import that defines
Popen/PIPE/STDOUT in this module and drop PIPE and STDOUT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f18e0bfd-116d-4a9d-bb0d-b438f9523b1d
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/src/nbdkit/nbdkit.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test results:
blkhash test case code modification and test rerun result:
Summary by CodeRabbit