Development #156
Development #156
Conversation
WalkthroughSwitches SataDisk.img to NTFS in the build. Substantially expands the NTFS driver with boot-sector validation, dynamic MFT record sizing, MFT bitmap allocation, path-to-MFT resolution, and CRUD file operations. Extends VFS to dispatch read/write/list/create/delete/size/type ops to NTFS. Also increases several AHCI wait timeouts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant VFS as VFS
participant NTFS as NTFS Driver
participant Block as BlockDevice
App->>VFS: ReadFile("/foo.txt")
alt FS = NTFS
VFS->>NTFS: NtfsReadFile("/foo.txt")
NTFS->>NTFS: Resolve path → MFT record
NTFS->>Block: Read boot sector / MFT / data runs
NTFS->>NTFS: Compute MFT record size, parse attributes
NTFS-->>VFS: Data or error
else Other FS
VFS->>VFS: Delegate to FAT/EXT handlers
end
VFS-->>App: Result
sequenceDiagram
autonumber
actor App as Application
participant VFS as VFS
participant NTFS as NTFS Driver
participant Block as BlockDevice
App->>VFS: CreateFile("/new.txt")
VFS->>NTFS: NtfsCreateFile("/new.txt")
NTFS->>Block: Read $MFT::$BITMAP (resident or non-resident)
NTFS->>NTFS: Scan bitmap, allocate free MFT record
NTFS->>Block: Write updated bitmap and new MFT record (STANDARD_INFO, FILE_NAME)
NTFS-->>VFS: Status
VFS-->>App: Success/Failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(1 hunks)fs/NTFS/NTFS.c(8 hunks)fs/NTFS/NTFS.h(3 hunks)fs/VFS.c(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T02:35:28.353Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: fs/VFS.c:331-335
Timestamp: 2025-09-01T02:35:28.353Z
Learning: VfsIsFile function was initially implemented as a placeholder that only checked for mount existence. The proper implementation should follow the same pattern as VfsIsDir: find mount, strip mount path, then use filesystem-specific functions to check if the path points to a file (FS_FILE for RAMFS, Fat12 functions for FAT12).
Applied to files:
fs/VFS.c
🧬 Code graph analysis (3)
fs/NTFS/NTFS.h (1)
fs/NTFS/NTFS.c (4)
NtfsWriteFile(491-567)NtfsCreateFile(691-783)NtfsCreateDir(785-884)NtfsDelete(886-1009)
fs/VFS.c (2)
kernel/etc/StringOps.c (1)
FastStrCmp(43-53)fs/NTFS/NTFS.c (9)
NtfsReadFile(275-347)NtfsWriteFile(491-567)NtfsListDir(349-391)NtfsCreateFile(691-783)NtfsCreateDir(785-884)NtfsDelete(886-1009)NtfsIsDir(417-442)NtfsIsFile(393-415)NtfsGetFileSize(444-490)
fs/NTFS/NTFS.c (6)
kernel/etc/Console.c (3)
PrintKernel(169-188)PrintKernelHex(224-247)PrintKernelChar(190-196)kernel/atomic/rust/src/ffi.rs (3)
rust_rwlock_write_unlock(202-206)rust_rwlock_read_unlock(188-192)rust_rwlock_write_lock(195-199)mm/KernelHeap.c (2)
KernelMemoryAlloc(362-438)KernelFree(485-530)mm/MemOps.c (2)
memcpy(15-17)memset(11-13)drivers/RTC/Rtc.c (1)
RtcGetUnixTime(140-163)kernel/etc/StringOps.c (1)
StringLength(4-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
fs/NTFS/NTFS.c (3)
582-639: Fix hard-coded 1 KiB MFT buffer before reading.This is a critical issue that was flagged in a previous review but remains unfixed.
NtfsWriteFilestill allocates a 1024-byte buffer and assumes two 512-byte sectors when writing back. With typical 4096-byte MFT records, this will:
- Overflow the
recordbuffer during read- Use incorrect
record_endpointer (line 594), missing attributes- Fail the
bytes_in_usecheck incorrectly (line 596)- Write back at the wrong sector offset with incomplete data (lines 631-634)
Apply the fix from the previous review to compute
mft_record_sizedynamically, use it for allocation, bounds checks, and write-back math.As per the previous review, apply this diff:
+ uint32_t mft_record_size; + if (volume.boot_sector.clusters_per_file_record > 0) { + mft_record_size = volume.boot_sector.clusters_per_file_record * volume.bytes_per_cluster; + } else { + mft_record_size = 1u << (-(int8_t)volume.boot_sector.clusters_per_file_record); + } + - NtfsMftRecord* record = KernelMemoryAlloc(1024); + NtfsMftRecord* record = KernelMemoryAlloc(mft_record_size); @@ - uint8_t* record_end = (uint8_t*)record + 1024; + uint8_t* record_end = (uint8_t*)record + mft_record_size; @@ - if (record->bytes_in_use > 1024) { + if (record->bytes_in_use > mft_record_size) { @@ - uint64_t mft_offset = volume.mft_cluster * volume.sectors_per_cluster; - uint64_t record_offset = mft_offset + (mft_record_num * 1024 / volume.bytes_per_sector); - - if (volume.device->write_blocks(volume.device, record_offset, 2, record) != 0) { + uint64_t mft_start_sector = volume.mft_cluster * volume.sectors_per_cluster; + uint64_t record_byte_offset = mft_record_num * (uint64_t)mft_record_size; + uint64_t record_sector = mft_start_sector + (record_byte_offset / volume.bytes_per_sector); + uint32_t sectors_needed = (mft_record_size + volume.bytes_per_sector - 1) / volume.bytes_per_sector; + + if (volume.device->write_blocks(volume.device, record_sector, sectors_needed, record) != 0) {
829-862: Add a zero-length $DATA attribute when creating files.This major issue from the previous review remains unfixed. The new file record only contains $STANDARD_INFORMATION and $FILE_NAME attributes. Without a $DATA attribute, the record is invalid for regular files, and both
NtfsReadFileandNtfsWriteFilewill fail to locate data, making freshly created files unusable.Append a resident $DATA header with zero length as shown in the previous review.
Apply this diff from the previous review:
NtfsFilename* file_name = (NtfsFilename*)((uint8_t*)file_name_attr + file_name_attr->resident.value_offset); @@ - record->bytes_in_use = (uint32_t)((uint8_t*)file_name_attr + file_name_attr->length - (uint8_t*)record); + NtfsAttrHeader* data_attr = (NtfsAttrHeader*)((uint8_t*)file_name_attr + file_name_attr->length); + data_attr->type = NTFS_ATTR_DATA; + data_attr->non_resident = 0; + data_attr->name_length = 0; + data_attr->length = sizeof(NtfsAttrHeader); + data_attr->resident.value_offset = sizeof(NtfsAttrHeader); + data_attr->resident.value_length = 0; + + record->bytes_in_use = (uint32_t)((uint8_t*)data_attr + data_attr->length - (uint8_t*)record);
979-1000: Avoid 1 KiB assumptions when deleting MFT records.This critical issue from the previous review remains unfixed.
NtfsDeleterepeats the 1024-byte allocation and two-sector write for the record being deleted. With larger records, this reads past the heap buffer and writes an incomplete record back at the wrong sector offset, leaving the on-disk MFT corrupted.Apply the dynamic record-size logic shown in the previous review.
Apply this diff from the previous review:
+ uint32_t mft_record_size; + if (volume.boot_sector.clusters_per_file_record > 0) { + mft_record_size = volume.boot_sector.clusters_per_file_record * volume.bytes_per_cluster; + } else { + mft_record_size = 1u << (-(int8_t)volume.boot_sector.clusters_per_file_record); + } + - NtfsMftRecord* record = KernelMemoryAlloc(1024); + NtfsMftRecord* record = KernelMemoryAlloc(mft_record_size); @@ - uint64_t mft_offset = volume.mft_cluster * volume.sectors_per_cluster; - uint64_t record_offset = mft_offset + (mft_record_num * 1024 / volume.bytes_per_sector); - if (volume.device->write_blocks(volume.device, record_offset, 2, record) != 0) { + uint64_t mft_start_sector = volume.mft_cluster * volume.sectors_per_cluster; + uint64_t record_byte_offset = mft_record_num * (uint64_t)mft_record_size; + uint64_t record_sector = mft_start_sector + (record_byte_offset / volume.bytes_per_sector); + uint32_t sectors_needed = (mft_record_size + volume.bytes_per_sector - 1) / volume.bytes_per_sector; + if (volume.device->write_blocks(volume.device, record_sector, sectors_needed, record) != 0) {
🧹 Nitpick comments (2)
drivers/storage/AHCI.c (1)
49-54: Timeout increases improve hardware compatibility.The 10x increase (500→5000 iterations) for both CR and FR clear waits is reasonable for AHCI hardware, which can be slow during port stop operations. Since this code path is only hit during initialization and teardown, the longer wait time won't impact normal I/O performance.
Consider adding a log message when timeouts are approached (e.g., at 80% threshold) to help diagnose slow hardware without waiting for full timeout:
// Wait for CR to clear int timeout = 5000; while (timeout-- > 0) { cmd = AHCI_ReadPortReg(port, AHCI_PORT_CMD); if (!(cmd & AHCI_PORT_CMD_CR)) break; + if (timeout == 1000) { + PrintKernel("AHCI: Port CR taking longer than expected\n"); + } delay_us(500); }Also applies to: 62-67
fs/NTFS/NTFS.c (1)
251-345: Refactor duplicated MFT record size calculation.The path resolution logic is correct and handles edge cases properly. However, the MFT record size calculation (lines 256-261 and 311-316) is duplicated across many functions in this file. Extract this into a helper function to improve maintainability.
Example:
static uint32_t NtfsGetMftRecordSize(void) { if (volume.boot_sector.clusters_per_file_record > 0) { return volume.boot_sector.clusters_per_file_record * volume.bytes_per_cluster; } else { return 1u << (-(int8_t)volume.boot_sector.clusters_per_file_record); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
drivers/storage/AHCI.c(4 hunks)fs/NTFS/NTFS.c(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
fs/NTFS/NTFS.c (5)
kernel/etc/Console.c (3)
PrintKernel(169-188)PrintKernelHex(224-247)PrintKernelChar(190-196)kernel/atomic/rust/src/ffi.rs (3)
rust_rwlock_write_unlock(202-206)rust_rwlock_read_unlock(188-192)rust_rwlock_write_lock(195-199)mm/KernelHeap.c (2)
KernelMemoryAlloc(362-438)KernelFree(485-530)drivers/RTC/Rtc.c (1)
RtcGetUnixTime(140-163)kernel/etc/StringOps.c (1)
StringLength(4-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
drivers/storage/AHCI.c (1)
161-167: Command timeout increases are appropriate.The timeout adjustments are well-calibrated:
- Port ready wait: 2x increase (1→2 seconds) is conservative
- Command completion wait: 10x increase but with 10x finer granularity (50μs vs 500μs), resulting in 250ms max wait time
These values are reasonable for AHCI command execution and should accommodate slower drives without significantly impacting I/O latency. The completion timeout properly handles failures with error logging (line 210).
Also applies to: 202-212
fs/NTFS/NTFS.c (6)
10-10: LGTM!The inclusion of
Rtc.his necessary for theRtcGetUnixTime()calls used to timestamp file metadata inNtfsCreateFileandNtfsCreateDir.
83-124: LGTM!The MFT cluster validation prevents invalid filesystem access by ensuring the MFT location is non-zero and within volume boundaries. The debug output provides useful diagnostics for troubleshooting mount issues.
138-158: LGTM!The dynamic MFT record size calculation correctly handles both encoding modes (positive values as cluster counts, negative values as powers of two), and the sector offset math is accurate for variable-sized records.
347-574: LGTM with a note on the scan limit.The file operation functions correctly implement dynamic MFT record sizing with proper bounds checking and memory management. Note that
NtfsListDir(line 446) also uses the hardcodedmax_scan = 4096limit mentioned earlier, which may not list all entries in large directories.
653-773: LGTM!The MFT record allocation logic correctly handles both resident and non-resident $BITMAP attributes. The non-resident support is limited to the first contiguous run (as documented), which is a reasonable limitation for this initial implementation.
869-969: LGTM!The directory creation logic is correct and includes all necessary attributes ($STANDARD_INFORMATION, $FILE_NAME, and $INDEX_ROOT). The implementation properly uses dynamic MFT record sizing throughout.
1 participant
NTFS write
Summary by CodeRabbit
New Features
Bug Fixes
Chores