Skip to content

Commit 04293a1

Browse files
Jefferyyenwaynelin99fernando79513
authored
Fix NTFS SD card mount failure by adding sync and udevadm settle (BugFix) (#1717)
* Fix NTFS SD card mount failure by adding sync and udevadm settle Fix an issue where Checkbox aborts during the SDHC storage test due to a race condition when mounting an NTFS-formatted SD card. The test fails with: ntfs-3g-mount: mount failed: Device or resource busy because `mount_usb_storage()` executes before Ubuntu's auto-mount process completes. Add `sync` to flush pending writes and `udevadm settle` to wait for udev events, ensuring the device is ready before mounting. Signed-off-by: Jeffery Yen <[email protected]> Co-authored-by: Wayne Lin <[email protected]> * Fix PPA GPG key retrieval issue in CI * Fix CI: Use gpg --dearmor for PPA key handling * Add test for mount_usb_storage functionality * Format test_mount_usb_storage.py using Black * Update test contents from existing file and rename test_mount_usb_storage.py to test_usb_read_write.py * Update test contents and merge tests into test_usb_read_write.py * Apply Black formatting to test_usb_read_write.py * Delet the wrong comment * Apply Black formatting to test_usb_read_write.py * Simplify test: mock subprocess and check log Co-authored-by: Fernando Bravo <[email protected]> * keep comments under 80 lines Co-authored-by: Fernando Bravo <[email protected]> * Try to keep comments under 80 lines Co-authored-by: Fernando Bravo <[email protected]> * Merge import statements from the same module Co-authored-by: Fernando Bravo <[email protected]> * Remove unused imports Co-authored-by: Fernando Bravo <[email protected]> * Update copyright and author Co-authored-by: Fernando Bravo <[email protected]> * Refactor and cleanup code - Simplify test: mock subprocess and check log - Enforce comments under 80 characters - Merge import statements from the same module - Remove unused imports - Update copyright and add author info * Remove old test_usb_read_write.py * Revert changes to GitHub Actions configuration in deb_validator.yaml --------- Signed-off-by: Jeffery Yen <[email protected]> Co-authored-by: Wayne Lin <[email protected]> Co-authored-by: Fernando Bravo <[email protected]>
1 parent ea6c494 commit 04293a1

File tree

3 files changed

+210
-113
lines changed

3 files changed

+210
-113
lines changed
Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
#!/usr/bin/env python3
2+
# This file is part of Checkbox.
3+
#
4+
# Copyright 2024-2025 Canonical Ltd.
5+
# Authors:
6+
# Fernando Bravo <[email protected]>
7+
# Jeffery Yen <songpao2262gmail.com>
8+
#
9+
# Checkbox is free software: you can redistribute it and/or modify
10+
# it under the terms of the GNU General Public License version 3,
11+
# as published by the Free Software Foundation.
12+
#
13+
# Checkbox is distributed in the hope that it will be useful,
14+
# but WITHOUT ANY WARRANTY; without even the implied warranty of
15+
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
16+
# GNU General Public License for more details.
17+
#
18+
# You should have received a copy of the GNU General Public License
19+
# along with Checkbox. If not, see <http://www.gnu.org/licenses/>.
20+
21+
import unittest
22+
import subprocess
23+
import contextlib
24+
from unittest.mock import patch, MagicMock, call
25+
26+
from checkbox_support.scripts.usb_read_write import (
27+
mount_usb_storage,
28+
write_test_unit,
29+
)
30+
31+
32+
class TestUsbReadWrite(unittest.TestCase):
33+
34+
@patch("os.path")
35+
@patch("subprocess.Popen")
36+
@patch("subprocess.check_output")
37+
@patch("subprocess.run")
38+
def test_write_test_unit(
39+
self, mock_run, mock_check_output, mock_popen, mock_os
40+
):
41+
mock_os.join.return_value = "output_file"
42+
43+
mock_process = MagicMock()
44+
mock_process.communicate.return_value = (
45+
b"2048+1 records in\n2048+1 records out\n1049076 bytes (1.0 MB) "
46+
b"copied, 0.00473357 s, 222 MB/s\n",
47+
None,
48+
)
49+
mock_popen.return_value = mock_process
50+
51+
random_file = MagicMock()
52+
random_file.tfile.name = "random_file"
53+
write_test_unit(random_file)
54+
55+
mock_popen.assert_called_once_with(
56+
[
57+
"dd",
58+
"if=random_file",
59+
"of=output_file",
60+
"bs=1M",
61+
"oflag=sync",
62+
],
63+
stderr=subprocess.STDOUT,
64+
stdout=subprocess.PIPE,
65+
env={"LC_NUMERIC": "C"},
66+
)
67+
mock_popen.return_value.communicate.assert_called_with()
68+
69+
@patch("os.path")
70+
@patch("subprocess.Popen")
71+
@patch("subprocess.check_output")
72+
@patch("subprocess.run")
73+
def test_write_test_unit_wrong_units(
74+
self, mock_run, mock_check_output, mock_popen, mock_os
75+
):
76+
mock_os.join.return_value = "output_file"
77+
78+
mock_process = MagicMock()
79+
mock_process.communicate.return_value = (
80+
b"2048+1 records in\n2048+1 records out\n1049076 bytes (1.0 MB) "
81+
b"copied, 0.00473357 s, 222 ***/s\n",
82+
None,
83+
)
84+
mock_popen.return_value = mock_process
85+
86+
random_file = MagicMock()
87+
random_file.tfile.name = "random_file"
88+
with self.assertRaises(SystemExit):
89+
write_test_unit(random_file)
90+
91+
@patch("os.path")
92+
@patch("subprocess.Popen")
93+
@patch("subprocess.check_output")
94+
@patch("subprocess.run")
95+
def test_write_test_unit_io_error(
96+
self, mock_run, mock_check_output, mock_popen, mock_os
97+
):
98+
mock_os.join.return_value = "output_file"
99+
100+
mock_process = MagicMock()
101+
mock_process.communicate.return_value = (
102+
b"2048+1 records in\n2048+1 records out\n1049076 bytes (1.0 MB) "
103+
b"copied, 0.00473357 s, 222 MBs\n",
104+
None,
105+
)
106+
mock_popen.return_value = mock_process
107+
108+
dmesg = MagicMock()
109+
dmesg.stdout.decode.return_value = "I/O error"
110+
mock_run.return_value = dmesg
111+
112+
random_file = MagicMock()
113+
random_file.tfile.name = "random_file"
114+
with self.assertRaises(SystemExit):
115+
write_test_unit(random_file)
116+
117+
118+
class TestMountUsbStorage(unittest.TestCase):
119+
@patch("checkbox_support.scripts.usb_read_write.subprocess.call")
120+
@patch(
121+
"checkbox_support.scripts.usb_read_write.os.path.join",
122+
return_value="/dev/sda1",
123+
)
124+
@patch("checkbox_support.scripts.usb_read_write.sys.exit")
125+
def test_mount_usb_storage_success(self, mock_exit, mock_join, mock_call):
126+
"""
127+
Test the success scenario:
128+
- Simulate that all subprocess.call calls return 0 (success),
129+
with the mount command returning 0 to indicate a successful mount.
130+
- Verify that the following commands are called in order:
131+
1. ["sync"]
132+
2. ["udevadm", "settle", "--timeout=10"]
133+
3. ["umount", FOLDER_TO_MOUNT] (here we patch FOLDER_TO_MOUNT to
134+
"/mnt/usb")
135+
4. ["umount", "/dev/sda1"]
136+
5. ["mount", "/dev/sda1", "/mnt/usb"]
137+
- Upon exiting the context, the finally block should also call
138+
unmount on the folder (["umount", "/mnt/usb"]).
139+
- And sys.exit should not be called.
140+
"""
141+
142+
# Simulate that all the calls are passing
143+
mock_call.return_value = 0
144+
145+
# Patch FOLDER_TO_MOUNT to be "/mnt/usb"
146+
with patch(
147+
"checkbox_support.scripts.usb_read_write.FOLDER_TO_MOUNT",
148+
"/mnt/usb",
149+
):
150+
# Enter the mount_usb_storage context using a context manager
151+
with contextlib.ExitStack() as stack:
152+
stack.enter_context(mount_usb_storage("sda1"))
153+
# When entering the context, the following commands should be
154+
# executed in order:
155+
expected_calls_entry = [
156+
call(["sync"]),
157+
call(["udevadm", "settle", "--timeout=10"]),
158+
call(["umount", "/mnt/usb"], stderr=subprocess.PIPE),
159+
call(["umount", "/dev/sda1"], stderr=subprocess.PIPE),
160+
call(["mount", "/dev/sda1", "/mnt/usb"]),
161+
]
162+
for expected in expected_calls_entry:
163+
self.assertIn(expected, mock_call.call_args_list)
164+
# When exiting the context, the finally block should execute:
165+
expected_final_call = call(["umount", "/mnt/usb"])
166+
self.assertIn(expected_final_call, mock_call.call_args_list)
167+
# Ensure that sys.exit was not called
168+
mock_exit.assert_not_called()
169+
170+
@patch("checkbox_support.scripts.usb_read_write.subprocess.call")
171+
@patch("logging.error")
172+
def test_mount_usb_storage_failure(self, mock_log, mock_call):
173+
"""
174+
Test the failure scenario:
175+
- Simulate that the mount command returns 1 (failure).
176+
- The program should call sys.exit(1) and raise a SystemExit.
177+
"""
178+
179+
def call_side_effect(args, **kwargs):
180+
if args[0] == "mount":
181+
return 1 # Simulate mount failure
182+
else:
183+
return 0
184+
185+
mock_call.side_effect = call_side_effect
186+
187+
with patch(
188+
"checkbox_support.scripts.usb_read_write.FOLDER_TO_MOUNT",
189+
"/mnt/usb",
190+
):
191+
with self.assertRaises(SystemExit) as context:
192+
with mount_usb_storage("sda1"):
193+
# As soon as we enter the context, a non-zero return from
194+
# mount should trigger sys.exit(1)
195+
pass
196+
197+
self.assertEqual(context.exception.code, 1)
198+
199+
# Verify that the program logs the error message
200+
mock_log.assert_called_once_with("mount /dev/sda1 on /mnt/usb failed.")
201+
202+
203+
if __name__ == "__main__":
204+
unittest.main()

checkbox-support/checkbox_support/scripts/usb_read_write.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,12 @@ def mount_usb_storage(partition):
168168
try:
169169
device_to_mount = os.path.join("/dev", partition)
170170

171+
# Flush pending writes to prevent "Device or resource busy" errors.
172+
subprocess.call(["sync"])
173+
174+
# Wait for udev events to complete before mounting.
175+
subprocess.call(["udevadm", "settle", "--timeout=10"])
176+
171177
# Unmounting the folder ignoring any "not mounted" error messages.
172178
subprocess.call(["umount", FOLDER_TO_MOUNT], stderr=subprocess.PIPE)
173179

checkbox-support/checkbox_support/tests/test_usb_read_write.py

Lines changed: 0 additions & 113 deletions
This file was deleted.

0 commit comments

Comments
 (0)