fs: support non-ASCII VFAT labels via locale-derived codepage - #1202
fs: support non-ASCII VFAT labels via locale-derived codepage#1202Johnson-zs wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesVFAT label handling detects locale-specific OEM codepages, converts labels between OEM bytes and UTF-8, and validates labels by byte length. Tests cover ASCII, UTF-8, forbidden characters, and CJK round trips. VFAT label handling
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/fs_tests/vfat_test.py (3)
332-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test does not cover the negative case in its docstring.
The docstring states that 6 CJK characters do not fit, but the test only checks 5. Add the failing case so the byte-limit boundary is covered.
💚 Proposed addition
fi = BlockDev.fs_vfat_get_info(self.loop_devs[0]) self.assertEqual(fi.label, five) + + # 6 CJK chars = 12 GBK bytes > 11 -- should fail + with self.assertRaises(GLib.GError): + BlockDev.fs_vfat_set_label(self.loop_devs[0], "测" * 6)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fs_tests/vfat_test.py` around lines 332 - 343, Extend test_vfat_set_label_max_cjk after the successful five-character assertion to set a six-character CJK label and assert the operation fails, covering the documented GBK byte-limit boundary while preserving the existing success and readback checks.
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestrict the exception handler and match the codepage name exactly.
Two points:
except Exceptionhides real failures. CatchOSErrorandsubprocess.SubprocessErroronly.- The substring test
"CP936" in p.stdoutalso matchesCP936X-style names, and it ignores theiconv -lexit status. Compare tokens instead.♻️ Proposed refactor
def _has_codepage(cp): """Check whether the given DOS codepage is available via iconv.""" try: - p = subprocess.run(["iconv", "-l"], capture_output=True, text=True) - return "CP%d" % cp in p.stdout or "CP%d//" % cp in p.stdout - except Exception: + p = subprocess.run(["iconv", "-l"], capture_output=True, text=True, + check=True) + except (OSError, subprocess.SubprocessError): return False + + names = {n.rstrip("/") for n in p.stdout.split()} + return "CP%d" % cp in names🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fs_tests/vfat_test.py` around lines 29 - 35, Update _has_codepage to catch only OSError and subprocess.SubprocessError, allowing unrelated failures to propagate. Require a successful iconv -l result, then tokenize its stdout and compare exact normalized entries for the CP%d and CP%d// forms instead of using substring checks.Source: Linters/SAST tools
283-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
unittest.mock.patch.dictfor the environment override.
setUpClasssaves the variables andtearDownClassrestores them, butsetUpmutatesos.environfor each test without a per-test restore. If a test raises beforetearDownClassruns in a shared process,LC_ALLstays set.unittest.mock.patch.dict(os.environ, {"LC_ALL": "zh_CN.UTF-8"})insetUpwithself.addCleanupremoves the manual bookkeeping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fs_tests/vfat_test.py` around lines 283 - 303, Update VfatSetLabelNonAscii.setUp to apply the LC_ALL override with unittest.mock.patch.dict and register its stop/restore cleanup via self.addCleanup. Remove the _saved_env bookkeeping and corresponding setUpClass/tearDownClass restoration logic, while preserving the zh_CN.UTF-8 value for each test.src/plugins/fs/vfat.c (1)
315-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a table-driven mapping and add the remaining Latin/Greek/Turkish codepages.
The mapping covers CJK and Cyrillic only. Locales such as
pl_PL,cs_CZ,tr_TR, andel_GRfall back to 850, but their characters are not all encodable in CP850. For those locales,fatlabelwill either fail or store wrong bytes. A static array of{prefix, codepage}pairs makes the list easier to extend (852 for Central European, 857 for Turkish, 737 for Greek, 862 for Hebrew).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/fs/vfat.c` around lines 315 - 327, Replace the locale condition chain in the codepage-mapping function with a static prefix-to-codepage table, preserving the existing CJK and Cyrillic mappings while adding Central European locales such as pl_PL/cs_CZ to 852, Turkish tr_TR to 857, Greek el_GR to 737, and Hebrew locales to 862. Iterate over the table and retain 850 as the default for unmatched locales.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/fs/vfat.c`:
- Around line 341-363: Remove the cp == 850 early return in
_vfat_label_from_codepage and ensure every g_convert failure fallback is valid
UTF-8 before returning. In src/plugins/fs/vfat.c lines 341-363, apply this to
all codepages; in lines 393-402, update the write path to pass -c <cp> for every
non-ASCII label, including codepage 850, so encoding matches decoding.
- Around line 447-454: Update bd_fs_vfat_check_label around the current
strlen(label) > 11 validation to convert the UTF-8 label to the selected VFAT
target codepage first, then enforce the 11-byte limit on the converted byte
sequence. Reuse the existing codepage-conversion behavior or helper used by
bd_fs_vfat_set_label/fatlabel, and preserve the existing
BD_FS_ERROR_LABEL_INVALID result for conversion failures or labels exceeding 11
target-codepage bytes so BD_FS_LABEL_CHECK matches BD_FS_LABEL.
In `@tests/fs_tests/vfat_test.py`:
- Around line 344-347: Move the newly added VfatSetLabelNonAscii-related class
definitions below VfatSetLabel, ensuring test_vfat_set_uuid remains defined
within VfatSetLabel and retains its original setup and execution conditions.
- Around line 262-269: Correct test_vfat_check_label_non_ascii_byte_count so its
docstring matches the actual byte counts and expected outcomes, then add an
assertion covering a label with four CJK characters that exceeds the 11-byte
limit and must be rejected, while retaining the existing accepted under-limit
case.
---
Nitpick comments:
In `@src/plugins/fs/vfat.c`:
- Around line 315-327: Replace the locale condition chain in the
codepage-mapping function with a static prefix-to-codepage table, preserving the
existing CJK and Cyrillic mappings while adding Central European locales such as
pl_PL/cs_CZ to 852, Turkish tr_TR to 857, Greek el_GR to 737, and Hebrew locales
to 862. Iterate over the table and retain 850 as the default for unmatched
locales.
In `@tests/fs_tests/vfat_test.py`:
- Around line 332-343: Extend test_vfat_set_label_max_cjk after the successful
five-character assertion to set a six-character CJK label and assert the
operation fails, covering the documented GBK byte-limit boundary while
preserving the existing success and readback checks.
- Around line 29-35: Update _has_codepage to catch only OSError and
subprocess.SubprocessError, allowing unrelated failures to propagate. Require a
successful iconv -l result, then tokenize its stdout and compare exact
normalized entries for the CP%d and CP%d// forms instead of using substring
checks.
- Around line 283-303: Update VfatSetLabelNonAscii.setUp to apply the LC_ALL
override with unittest.mock.patch.dict and register its stop/restore cleanup via
self.addCleanup. Remove the _saved_env bookkeeping and corresponding
setUpClass/tearDownClass restoration logic, while preserving the zh_CN.UTF-8
value for each test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d18ffee-e3a3-4962-adab-c9b08e2a78ca
📒 Files selected for processing (2)
src/plugins/fs/vfat.ctests/fs_tests/vfat_test.py
33f43b0 to
a8f025e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/fs_tests/vfat_test.py`:
- Around line 382-393: Update test_vfat_set_label_max_cjk to use a round-trip
label within 11 UTF-8 bytes, such as three CJK characters followed by “AB”,
instead of "测" * 5; keep the existing success and label-equality assertions
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d330197d-8c82-458d-a92a-653914317aaa
📒 Files selected for processing (2)
src/plugins/fs/vfat.ctests/fs_tests/vfat_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/fs/vfat.c
d4c9003 to
ab42fcb
Compare
fatlabel defaults to DOS codepage 850, which cannot encode characters outside Western European languages. When a user sets a VFAT label containing CJK, Cyrillic or other non-ASCII characters, fatlabel fails with "Cannot convert input sequence" and the label is not changed. Pass the OEM codepage matching the system locale to fatlabel via the -c option when the label contains non-ASCII characters. Keep the byte-count check (strlen) in bd_fs_vfat_check_label as a conservative upper bound; fatlabel performs the exact codepage-length check during iconv conversion. Preserve the original "at most 11 characters long" error wording so downstream consumers such as udisks that match on this string keep working. Decode the raw OEM bytes returned by blkid back to UTF-8 in bd_fs_vfat_get_info so labels round-trip correctly. Also add a NULL check to bd_fs_vfat_check_label and tests for non-ASCII label set/get and the byte-limit check. The non-ASCII end-to-end tests are gated by a runtime probe (_cjk_label_supported) that actually tries fatlabel -c 936 with a CJK label: iconv may advertise CP936 yet fatlabel's locale-aware conversion can still fail on some hosts (e.g. glibc/dosfstools combinations where the CJK locale is not fully functional), so the tests are skipped rather than reported as failures in such environments. This mirrors how Windows derives the OEM codepage from the system locale. Note: the codepage is derived from the daemon environment (udisksd), not the calling user's session — this is a known limitation documented in the code.
ab42fcb to
51ae081
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/fs/vfat.c (1)
444-453: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
strlenbyte-count check rejects valid CJK/Cyrillic labels via the generic API.The comment calls
strlen"a conservative upper bound," but for non-ASCII input the relation is the opposite: UTF-8 encodes most non-ASCII characters using more bytes than the target OEM codepage (e.g. GBK uses 2 bytes per CJK character; UTF-8 uses 3). Sostrlenoverestimates the OEM byte count and rejects labels that would fit in the real 11-byte OEM field, such as"测" * 5(15 UTF-8 bytes, 10 GBK bytes).
bd_fs_vfat_set_labeldoes not callbd_fs_vfat_check_label, so it accepts such a label directly. Butbd_fs_set_label(the generic API) callsBD_FS_LABEL_CHECKbeforeBD_FS_LABEL, perdevice_operationinsrc/plugins/fs/generic.c. A caller using the generic API is rejected for a label the VFAT-specific API accepts. This is a real contract mismatch between the two entry points, not just a documentation choice.Count the label's byte length after converting it to the locale-derived OEM codepage, instead of using
strlenon the UTF-8 string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/fs/vfat.c` around lines 444 - 453, Update the VFAT label validation around bd_fs_vfat_check_label to convert the UTF-8 label to the locale-derived OEM codepage before measuring its length, and enforce the 11-byte limit on the converted result rather than strlen(label). Preserve the existing invalid-label error and ensure generic bd_fs_set_label and bd_fs_vfat_set_label accept the same labels.
🧹 Nitpick comments (1)
src/plugins/fs/vfat.c (1)
330-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale docstring about codepage 850.
The docstring states:
"Pure ASCII labels and the default codepage (850) need no conversion."The code no longer special-cases codepage 850:g_convertruns unconditionally for any non-ASCII label, regardless ofcp. This wording describes the old behavior that a previous review flagged as a round-trip bug. Update the comment so it does not describe the removed special case, to avoid a future regression.📝 Proposed fix
* Convert a label read from a VFAT filesystem (raw OEM codepage bytes, as * returned by blkid) to UTF-8, using the locale-derived codepage. Pure - * ASCII labels and the default codepage (850) need no conversion. + * ASCII labels need no conversion; non-ASCII labels are always converted + * from the locale-derived codepage, including the default codepage (850).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/fs/vfat.c` around lines 330 - 340, Update the documentation for _vfat_label_from_codepage to remove the claim that codepage 850 requires no conversion. Describe only the current behavior: pure ASCII labels bypass conversion, while non-ASCII labels are passed through g_convert regardless of cp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/fs_tests/vfat_test.py`:
- Around line 424-436: Add a six-CJK-character boundary case to
test_vfat_set_label_max_cjk after the existing five-character success
assertions. Call BlockDev.fs_vfat_set_label with "测" multiplied by 6 and assert
the operation is rejected, preserving the existing successful five-character and
label verification checks.
---
Outside diff comments:
In `@src/plugins/fs/vfat.c`:
- Around line 444-453: Update the VFAT label validation around
bd_fs_vfat_check_label to convert the UTF-8 label to the locale-derived OEM
codepage before measuring its length, and enforce the 11-byte limit on the
converted result rather than strlen(label). Preserve the existing invalid-label
error and ensure generic bd_fs_set_label and bd_fs_vfat_set_label accept the
same labels.
---
Nitpick comments:
In `@src/plugins/fs/vfat.c`:
- Around line 330-340: Update the documentation for _vfat_label_from_codepage to
remove the claim that codepage 850 requires no conversion. Describe only the
current behavior: pure ASCII labels bypass conversion, while non-ASCII labels
are passed through g_convert regardless of cp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6060bc83-86b8-4a0a-9fb4-440045ddad8a
📒 Files selected for processing (2)
src/plugins/fs/vfat.ctests/fs_tests/vfat_test.py
| def test_vfat_set_label_max_cjk(self): | ||
| """5 CJK characters (10 GBK bytes) fit in 11 DOS bytes; 6 do not.""" | ||
|
|
||
| succ = BlockDev.fs_vfat_mkfs(self.loop_devs[0], self._mkfs_options) | ||
| self.assertTrue(succ) | ||
|
|
||
| # 5 CJK chars = 10 GBK bytes <= 11 -- should succeed | ||
| five = "测" * 5 | ||
| succ = BlockDev.fs_vfat_set_label(self.loop_devs[0], five) | ||
| self.assertTrue(succ) | ||
| fi = BlockDev.fs_vfat_get_info(self.loop_devs[0]) | ||
| self.assertEqual(fi.label, five) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the missing boundary assertion for 6 CJK characters.
The docstring states 6 CJK characters (12 GBK bytes) "do not" fit, but the test only exercises the 5-character success case. Add the rejection case to verify the real OEM byte limit end-to-end.
✅ Proposed fix
five = "测" * 5
succ = BlockDev.fs_vfat_set_label(self.loop_devs[0], five)
self.assertTrue(succ)
fi = BlockDev.fs_vfat_get_info(self.loop_devs[0])
self.assertEqual(fi.label, five)
+
+ # 6 CJK chars = 12 GBK bytes > 11 -- should fail
+ six = "测" * 6
+ with self.assertRaises(GLib.GError):
+ BlockDev.fs_vfat_set_label(self.loop_devs[0], six)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_vfat_set_label_max_cjk(self): | |
| """5 CJK characters (10 GBK bytes) fit in 11 DOS bytes; 6 do not.""" | |
| succ = BlockDev.fs_vfat_mkfs(self.loop_devs[0], self._mkfs_options) | |
| self.assertTrue(succ) | |
| # 5 CJK chars = 10 GBK bytes <= 11 -- should succeed | |
| five = "测" * 5 | |
| succ = BlockDev.fs_vfat_set_label(self.loop_devs[0], five) | |
| self.assertTrue(succ) | |
| fi = BlockDev.fs_vfat_get_info(self.loop_devs[0]) | |
| self.assertEqual(fi.label, five) | |
| def test_vfat_set_label_max_cjk(self): | |
| """5 CJK characters (10 GBK bytes) fit in 11 DOS bytes; 6 do not.""" | |
| succ = BlockDev.fs_vfat_mkfs(self.loop_devs[0], self._mkfs_options) | |
| self.assertTrue(succ) | |
| # 5 CJK chars = 10 GBK bytes <= 11 -- should succeed | |
| five = "测" * 5 | |
| succ = BlockDev.fs_vfat_set_label(self.loop_devs[0], five) | |
| self.assertTrue(succ) | |
| fi = BlockDev.fs_vfat_get_info(self.loop_devs[0]) | |
| self.assertEqual(fi.label, five) | |
| # 6 CJK chars = 12 GBK bytes > 11 -- should fail | |
| six = "测" * 6 | |
| with self.assertRaises(GLib.GError): | |
| BlockDev.fs_vfat_set_label(self.loop_devs[0], six) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/fs_tests/vfat_test.py` around lines 424 - 436, Add a six-CJK-character
boundary case to test_vfat_set_label_max_cjk after the existing five-character
success assertions. Call BlockDev.fs_vfat_set_label with "测" multiplied by 6 and
assert the operation is rejected, preserving the existing successful
five-character and label verification checks.
1 participant
fatlabel defaults to DOS codepage 850, which cannot encode characters outside Western European languages. When a user sets a VFAT label containing CJK, Cyrillic or other non-ASCII characters, fatlabel fails with "Cannot convert input sequence" and the label is not changed.
Pass the OEM codepage matching the system locale to fatlabel via the -c option when the label contains non-ASCII characters. Keep the byte-count check (strlen) in bd_fs_vfat_check_label as a conservative upper bound; fatlabel performs the exact codepage-length check during iconv conversion.
Decode the raw OEM bytes returned by blkid back to UTF-8 in bd_fs_vfat_get_info so labels round-trip correctly. Also add a NULL check to bd_fs_vfat_check_label and tests for non-ASCII label set/get and the byte-limit check.
This mirrors how Windows derives the OEM codepage from the system locale. Note: the codepage is derived from the daemon environment (udisksd), not the calling user's session — this is a known limitation documented in the code.
Summary by CodeRabbit
New Features
fatlabelautomatically use the appropriate code page.Bug Fixes