fix(fs): rmdir() removed nothing, it called unlink() - #77
Conversation
VFSImpl::rmdir() built the native path and then passed it to unlink(), which cannot remove a directory: EISDIR on Linux, EACCES on Windows. The call always failed, so FSCom.rmdir() never removed a directory on any portduino platform and rmDir() left the whole directory tree behind. arduino-esp32, which this file is ported from, uses ::rmdir() here, and the nRF52 Adafruit_LittleFS backend uses lfs_remove(), which removes an empty directory. Both platforms therefore implement "remove an empty directory" and portduino was the only backend that did not, so firmware written against the Arduino FS API behaved differently there. Qualified as ::rmdir because an unqualified call inside VFSImpl::rmdir resolves to the member function and recurses. That is the likely reason unlink() was reached for originally. Verified on Windows against the meshtastic firmware test suite: test_fscommon_getfiles previously passed only against a clean tree and failed on every later run, because the directories its depth-limit case creates survived setUp()'s rmDir() and made getFiles() report a depth truncation. With this change the suite passes with a stale tree seeded, passes on consecutive runs, and leaves no directories behind.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesVFS directory removal
Estimated code review effort: 1 (Trivial) | ~2 minutes Mergeability Score: ⚪ Minimal · up to This localized change makes directory removal work correctly without altering other filesystem behavior; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
1 participant
VFSImpl::rmdir()built the native path and then passed it tounlink(), which cannot remove a directory. Measured on both hosts:unlink(empty dir)rmdir(empty dir)So the call always failed.
FSCom.rmdir()has never removed a directory on any portduino platform, andrmDir()leaves the whole tree behind.Parity with the other backends
This file is a port of arduino-esp32's
libraries/FS/src/vfs_api.cpp, which uses::rmdir()at the same spot:The nRF52
Adafruit_LittleFS::rmdir()backend callslfs_remove(), which removes an empty directory and returnsLFS_ERR_NOTEMPTYotherwise. Both platforms therefore implement "remove an empty directory". portduino was the only backend that did not, so firmware written against the Arduino FS API behaved differently there.The call is qualified as
::rmdirbecause an unqualifiedrmdir()insideVFSImpl::rmdirresolves to the member function and recurses. That is the likely reasonunlink()was reached for originally.Verification
Against the meshtastic firmware native test suite on Windows,
test_fscommon_getfilespreviously passed only against a clean tree and failed on every later run: the directories its depth-limit case creates survivedsetUp()'srmDir(), sogetFiles()reported a depth truncation and the "not limited" case failed.With this change, seeding the stale tree first, the suite passes with the stale tree present, passes on two consecutive runs, and leaves zero entries behind each time.
Linux does not currently show the failure because each suite there runs in its own scratch
$HOME, so leftovers never survive into a later run, and within a single run the directory-creating case happens to run after the directory-sensitive one. The underlying defect is the same on both platforms.Summary by CodeRabbit