loop: Remove the loop device when setting sector size fails#1173
Conversation
📝 WalkthroughWalkthroughAdded LOOP_CLR_FD cleanup attempts in bd_loop_setup_from_fd for two error paths (failure to set loop status and failure to set sector size), and added a test that tears down the loop and verifies no loop device remains associated after an invalid sector_size setup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/loop.c (1)
391-402:⚠️ Potential issue | 🟠 Major
errnois clobbered beforeg_set_errorreads%mfor the sector-size errorWhen
LOOP_CLR_FDitself fails (line 392), the ioctl overwriteserrno. Theg_set_errorcall at line 395 then embeds the wrong errno via%m— it reports theLOOP_CLR_FDfailure reason instead of the originalLOOP_SET_BLOCK_SIZEfailure reason. On the happy path (LOOP_CLR_FDsucceeds), a successful ioctl does not modifyerrnoon Linux, so the bug is only triggered in the failure sub-branch. Still, it silently corrupts the error message surfaced to callers.Fix: save and restore
errnoaround the cleanup ioctl.🐛 Proposed fix: save/restore errno
if (status != 0) { - if (ioctl (loop_fd, LOOP_CLR_FD) < 0) - bd_utils_log_format (BD_UTILS_LOG_WARNING, - "Failed to clear the %s device: %m", loop_device); + int saved_errno = errno; + if (ioctl (loop_fd, LOOP_CLR_FD) < 0) + bd_utils_log_format (BD_UTILS_LOG_WARNING, + "Failed to clear the %s device: %m", loop_device); + errno = saved_errno; g_set_error (&l_error, BD_LOOP_ERROR, BD_LOOP_ERROR_FAIL, "Failed to set sector size for the %s device: %m", loop_device);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/loop.c` around lines 391 - 402, The cleanup ioctl (ioctl(loop_fd, LOOP_CLR_FD)) can overwrite errno, so preserve the original errno from the earlier LOOP_SET_BLOCK_SIZE failure before doing the cleanup and restore it before calling g_set_error/bd_utils_report_finished; specifically, in the error branch where status != 0 save int saved_errno = errno before the ioctl on loop_fd, perform the ioctl (and still log its own failure), then restore errno = saved_errno prior to calling g_set_error (and any other error-reporting that uses %m) so the reported message refers to the original sector-size failure; adjust references around loop_fd, LOOP_CLR_FD, g_set_error, bd_utils_log_format and l_error accordingly.
🧹 Nitpick comments (2)
tests/loop_test.py (1)
149-155: Misleading comment and overly long sleepTwo minor nits:
Comment (line 149):
"setup should fail, but we need to remove the loop device after"implies the test must do cleanup. With this fix the C code now performs the cleanup automatically; the actual intent is to verify cleanup happened. Consider something like:"invalid sector size -- setup should fail and the loop device should be cleared automatically".
time.sleep(2)(line 153):LOOP_CLR_FDis a synchronous ioctl and sysfsbacking_fileis cleared immediately. A 2-second sleep is very conservative and will slow down the test suite. Even0.1seconds is ample as a safety margin, or you could poll with a short timeout.♻️ Suggested comment + sleep improvement
- # invalid sector size -- setup should fail, but we need to remove the loop device after + # invalid sector size -- setup should fail and the loop device should be automatically cleared with self.assertRaisesRegex(GLib.GError, "Failed to set sector size"): BlockDev.loop_setup(self.dev_file, sector_size=7) - time.sleep(2) + time.sleep(0.5) loop_name = BlockDev.loop_get_loop_name(self.dev_file) self.assertIsNone(loop_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/loop_test.py` around lines 149 - 155, Update the misleading comment and shorten the artificial delay: change the comment above the BlockDev.loop_setup call to reflect that invalid sector size should fail and the loop device should be cleared automatically (e.g., "invalid sector size -- setup should fail and the loop device should be cleared automatically"), and replace the long time.sleep(2) after BlockDev.loop_setup with a much shorter wait (e.g., time.sleep(0.1)) or implement a short polling loop that checks BlockDev.loop_get_loop_name(self.dev_file) with a small timeout to avoid slowing the test suite.src/plugins/loop.c (1)
372-380: Same missingLOOP_CLR_FDcleanup exists in theLOOP_SET_STATUS64failure pathAt this point
LOOP_SET_FDhas already succeeded (line 350), so the backing file is associated. IfLOOP_SET_STATUS64fails, the code frees resources and returnsFALSEwithout issuingLOOP_CLR_FD, leaving an orphaned loop device — the identical class of bug this PR is fixing forLOOP_SET_BLOCK_SIZE. This is pre-existing and out of scope for this PR, but worth a follow-up issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/loop.c` around lines 372 - 380, In the LOOP_SET_STATUS64 failure path make sure to clear the association before returning: after LOOP_SET_FD has succeeded but if ioctl with LOOP_SET_STATUS64 fails, call the LOOP_CLR_FD ioctl on loop_fd (handle/log any error but continue cleanup), then close(loop_fd), free(loop_device) and report/propagate the original l_error (as done with LOOP_SET_BLOCK_SIZE fix). Update the error cleanup in that branch (references: LOOP_SET_STATUS64, LOOP_CLR_FD, loop_fd, loop_device, l_error, progress_id, error) so the loop device is not left orphaned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/plugins/loop.c`:
- Around line 391-402: The cleanup ioctl (ioctl(loop_fd, LOOP_CLR_FD)) can
overwrite errno, so preserve the original errno from the earlier
LOOP_SET_BLOCK_SIZE failure before doing the cleanup and restore it before
calling g_set_error/bd_utils_report_finished; specifically, in the error branch
where status != 0 save int saved_errno = errno before the ioctl on loop_fd,
perform the ioctl (and still log its own failure), then restore errno =
saved_errno prior to calling g_set_error (and any other error-reporting that
uses %m) so the reported message refers to the original sector-size failure;
adjust references around loop_fd, LOOP_CLR_FD, g_set_error, bd_utils_log_format
and l_error accordingly.
---
Nitpick comments:
In `@src/plugins/loop.c`:
- Around line 372-380: In the LOOP_SET_STATUS64 failure path make sure to clear
the association before returning: after LOOP_SET_FD has succeeded but if ioctl
with LOOP_SET_STATUS64 fails, call the LOOP_CLR_FD ioctl on loop_fd (handle/log
any error but continue cleanup), then close(loop_fd), free(loop_device) and
report/propagate the original l_error (as done with LOOP_SET_BLOCK_SIZE fix).
Update the error cleanup in that branch (references: LOOP_SET_STATUS64,
LOOP_CLR_FD, loop_fd, loop_device, l_error, progress_id, error) so the loop
device is not left orphaned.
In `@tests/loop_test.py`:
- Around line 149-155: Update the misleading comment and shorten the artificial
delay: change the comment above the BlockDev.loop_setup call to reflect that
invalid sector size should fail and the loop device should be cleared
automatically (e.g., "invalid sector size -- setup should fail and the loop
device should be cleared automatically"), and replace the long time.sleep(2)
after BlockDev.loop_setup with a much shorter wait (e.g., time.sleep(0.1)) or
implement a short polling loop that checks
BlockDev.loop_get_loop_name(self.dev_file) with a small timeout to avoid slowing
the test suite.
vojtechtrefny
force-pushed
the
master_loop-setup-fail-clear
branch
from
dc76d25 to
302002e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/plugins/loop.c (1)
394-399: Consider DRYing the duplicate cleanup block.The
LOOP_CLR_FD+ warning sequence is repeated in multiple error paths; extracting a small helper would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/loop.c` around lines 394 - 399, Extract the repeated cleanup sequence (ioctl(loop_fd, LOOP_CLR_FD) < 0 then bd_utils_log_format(..., "Failed to clear the %s device: %m", loop_device)) into a small helper (e.g., clear_loop_fd_and_warn(loop_fd, loop_device)) and call it from each error path where LOOP_CLR_FD cleanup is performed (including the block referencing l_error and BD_LOOP_ERROR_FAIL), ensuring the helper performs the ioctl and logs the warning on failure; update all occurrences to use this helper to remove duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/plugins/loop.c`:
- Around line 394-399: Extract the repeated cleanup sequence (ioctl(loop_fd,
LOOP_CLR_FD) < 0 then bd_utils_log_format(..., "Failed to clear the %s device:
%m", loop_device)) into a small helper (e.g., clear_loop_fd_and_warn(loop_fd,
loop_device)) and call it from each error path where LOOP_CLR_FD cleanup is
performed (including the block referencing l_error and BD_LOOP_ERROR_FAIL),
ensuring the helper performs the ioctl and logs the warning on failure; update
all occurrences to use this helper to remove duplication.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/loop.ctests/loop_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/loop_test.py
2 participants
Fixes: #1170
Summary by CodeRabbit
Bug Fixes
Tests