Repository navigation
Conversation
fdesbiens
left a comment
fdesbiens
left a comment
There was a problem hiding this comment.
Please retarget this PR to dev.
Please add the AI assistance line below the header in both edited C files. Replace the commit's Co-Authored-By trailer with an Assisted-by trailer naming Claude Code and the tool-reported model version. The commit subject and PR title should begin with a past-tense verb.
The focused directory rename test passed in standalone no-cache and standalone fault-tolerant builds.
| ".." entry, created together with the directory. new_dir_entry | ||
| is no longer needed either and serves as the destination. */ | ||
| i = 1; | ||
| status = _fx_directory_entry_read(media_ptr, &old_dir_entry, &i, &new_dir_entry); |
There was a problem hiding this comment.
This read also runs for a rename within the same parent, after both directory entries have been written. Without fault tolerance, a read error here makes fx_directory_rename report failure even though the rename completed. Please retain the source parent's cluster before searching for the target, skip the .. read when the parent is unchanged, and add a regression case for the read-error path.
Thanks for the review. I'm not sure I'll get to your change request this week. I'll be on vacation from October 1 through October 9 (inclusive). |
|
Thanks, @schnitst. I will merge to dev once you are back and update the PR. |
When a directory is renamed into a different parent directory, fx_directory_rename relinks the entry in both parents but never updates the ".." entry inside the moved directory, which keeps pointing at the old parent's starting cluster. A ".." path component inside the moved directory then resolves to the old parent and, once that cluster is reused, to an unrelated place. fx_media_check does not detect this, as it skips the first two entries of every sub-directory. Read the moved directory's ".." entry after the relink and rewrite its starting cluster in place when the parent changed (0 when the new parent is the root). The write happens inside the existing fault tolerant transaction; a rename within one parent ends without the additional write. The regression test extension covers every parent transition - between sub-directories, into and out of the root - and pins the write count: one additional directory entry write for a move, none for a rename within one parent. Fixes Issue eclipse-threadx#98 Assisted-by: Claude Code (Claude Fable 5)
The ".." entry of the renamed directory was read on every rename, after both parent directory entries had been written. For a rename inside one parent that read is not needed: without fault tolerance, a read error at that point made fx_directory_rename report a failure for a rename that had already completed on the media. Retain the starting cluster of the source parent before the search for the new name overwrites search_directory, and read and update the ".." entry only when the parent changed. The regression test injects a directory entry read error after the last read a rename inside the root directory needs and expects success, and lets the ".." read of a move fail: the error is reported, the directory is already relinked without fault tolerance, and unchanged on the media with fault tolerance. Assisted-by: Claude Code (Claude Fable 5.1, claude-fable-5-1)
A lookup answered from the directory search cache returns the found directory itself in search_directory, not its parent. The rename relies on search_directory to tell whether the parent changed, so a warm cache made a rename inside one parent read and rewrite the ".." entry. The same cache hit on the second lookup placed the new entry inside the renamed directory when a directory was renamed to its own name, and the directory became unreachable. Clear the cache before each of the two lookups so that both report the parent. The regression test injects read faults per sector instead of per read count: a driver wrapper fails every read of one logical sector, the first sector of the renamed directory. A rename inside one parent has to succeed without that sector ever being requested, with the search cache deliberately warm; a move has to fail on it. Assisted-by: Claude Code (Claude Fable 5.1, claude-fable-5-1)
7dd4dfe to
afb0615
Compare
schnitst
changed the title
|
Thanks for the review. I think I addressed all the points you mentioned:
One finding on the way, see the third commit: a lookup answered from the directory search cache returns the found directory itself in |
This branch has not been deployed
3 participants
Problem
When fx_directory_rename moves a directory to a different parent directory, it relinks the entry in both parents but never updates the '..' entry inside the moved directory itself. '..' keeps the starting cluster of the old parent, so any '..' path component inside the moved directory resolves to the old parent - and, once that cluster is reused, to unrelated data. fx_media_check cannot detect this because it skips the first two entries of every sub-directory.
Details and reproduction: #98.
Fix
After the relink completes, read the moved directory's '..' entry (entry 1 of its first cluster) and rewrite its starting cluster in place when the parent changed - cluster 0 when the new parent is the root, matching the convention fx_directory_create writes. Design points:
Tests
filex_directory_rename_test.c gains a section covering every parent transition - between sub-directories, into the root, out of the root, and renames within one parent - verifying both path resolution through '..' and the on-media entry itself, and pinning the directory-entry write count (one additional write for a move, none for an in-place rename). The section runs on a freshly formatted media after the existing flow, so the error-injection steps calibrated against the original directory layout are unaffected.