fs: support non-ASCII VFAT labels via locale-derived codepage - #1202
Open
Johnson-zs wants to merge 1 commit into
Open
fs: support non-ASCII VFAT labels via locale-derived codepage#1202Johnson-zs wants to merge 1 commit into
Johnson-zs wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesVFAT label handling now 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
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.
Johnson-zs
force-pushed
the
vfat-non-ascii-label
branch
from
July 31, 2026 03:29
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
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.
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