Development #112
Development #112
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a read-oriented EXT2 filesystem driver and header, wires EXT2 into VFS and kernel init, adds FastStrnCmp, updates build layout/includes (fs/FAT, fs/EXT) and Meson run target, and extends GitHub workflow artifact checks and upload behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Kernel
participant IDE as IDE Driver
participant FAT as FAT1x
participant EXT as EXT2
participant Mem as Memory Pools
Kernel->>IDE: IdeInit()
alt IDE present
Kernel->>FAT: FAT1xInit(0)
FAT-->>Kernel: status
Kernel->>EXT: Ext2Init(0)
EXT-->>Kernel: status
else No IDE
Kernel-->>Kernel: Skip FAT1x & EXT2 init
end
Kernel->>Mem: InitDefaultPools()
Mem-->>Kernel: Memory stats
Kernel->>Kernel: PrintVMemStats()
sequenceDiagram
autonumber
actor Caller
participant VFS as VFS
participant EXT as EXT2
participant IDE as IDE Driver
Caller->>VFS: VfsReadFile(path, buf, n) [VFS_EXT2]
VFS->>EXT: Ext2ReadFile(path, buf, n)
EXT->>EXT: Ext2PathToInode & Ext2FindInDir
EXT->>IDE: IdeReadSector / Ext2ReadBlock
IDE-->>EXT: block data
EXT-->>VFS: bytes read / error
VFS-->>Caller: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests
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: 8
🧹 Nitpick comments (6)
kernel/etc/StringOps.c (1)
25-40: FastStrnCmp: correct and consistent with FastStrCmp; minor clarity tweak optionalLogic and return contract look good. For readability, you could make the loop symmetric by also checking *str2, and (only if desired) align n==0 behavior with POSIX strncmp by short-circuiting before NULL checks.
Optional diffs:
- while (n > 0 && *str1 && *str1 == *str2) { + while (n > 0 && *str1 && *str2 && *str1 == *str2) { str1++; str2++; n--; }- if (!str1 || !str2) return (str1 == str2) ? 0 : (str1 ? 1 : -1); - if (n == 0) return 0; // Edge case: comparing 0 characters + if (n == 0) return 0; // Edge case: comparing 0 characters + if (!str1 || !str2) return (str1 == str2) ? 0 : (str1 ? 1 : -1);Confirm whether the current NULL-pointer tri-state precedence over n==0 is intentional across the codebase.
kernel/core/Kernel.c (1)
3-3: Fix inconsistent include path styleThe FAT1x.h include uses a relative path with
../../fs/FAT/, which is inconsistent with other includes in the file and fragile to directory structure changes.-#include "../../fs/FAT/FAT1x.h" +#include "FAT/FAT1x.h"fs/EXT/Ext2.h (2)
24-24: Document the EXT2 magic numberThe magic number is crucial for filesystem identification and should be documented.
- uint16_t s_magic; // Should be 0xEF53 + uint16_t s_magic; // EXT2 signature: 0xEF53
61-61: Consider using a constant for the block countThe magic number 15 for EXT2_N_BLOCKS represents the standard EXT2 inode block array size (12 direct + 3 indirect).
-#define EXT2_N_BLOCKS 15 +#define EXT2_N_BLOCKS 15 // 12 direct + 3 indirect block pointersfs/EXT/Ext2.c (2)
223-238: Limited to direct blocks onlyThe implementation only handles the first 12 direct blocks, limiting file size to 12 * block_size (typically 12KB-48KB). This is a significant limitation that should be documented.
The current implementation limits files to direct blocks only. Would you like me to help implement indirect block support to handle larger files? This would allow files up to several GB in size.
244-247: Consider returning ENOSYS for unimplemented functionsReturning -1 for unimplemented functions is ambiguous. Consider using a more specific error code.
int Ext2WriteFile(const char* path, const void* buffer, uint32_t size) { + (void)buffer; (void)size; // Suppress unused parameter warnings PrintKernelF("[EXT2] WriteFile: %s (Not implemented)\n", path); - return -1; + return -ENOSYS; // Function not implemented }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
fs/EXT/Ext2.c(1 hunks)fs/EXT/Ext2.h(1 hunks)fs/VFS.c(10 hunks)fs/VFS.h(1 hunks)kernel/core/Kernel.c(2 hunks)kernel/etc/StringOps.c(1 hunks)kernel/etc/StringOps.h(1 hunks)meson.build(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 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.hfs/VFS.c
📚 Learning: 2025-08-13T07:26:56.892Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:26:56.892Z
Learning: VoidFrame kernel uses direct interrupt dispatch in arch/x86_64/interrupts/Interrupts.c rather than RegisterInterruptHandler pattern. Hardware interrupts are handled in switch cases by interrupt number (32+ for IRQs), with proper PIC EOI handling required.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-31T14:19:11.366Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.366Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:32.713Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.713Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-01T01:10:22.123Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.123Z
Learning: The current VoidFrame Registers struct in arch/x86_64/cpu/Cpu.h only saves basic GPRs, segment registers, and exception context (interrupt_number, error_code, rip, cs, rflags, rsp, ss). It's missing critical registers for deep fault tracing: SIMD registers (XMM/YMM/ZMM), control registers (CR0/CR2/CR3/CR4), debug registers (DR0-DR7), and extended FPU state. The struct has a "DO NOT TOUCH" comment indicating it must match interrupt ASM stack layout.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:51.217Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.217Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-17T08:16:43.539Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#93
File: kernel/memory/VMem.c:72-79
Timestamp: 2025-08-17T08:16:43.539Z
Learning: In VoidFrame kernel, VMemGetPhysAddr and related page table walking functions were causing panics because they assumed identity mapping existed for physical addresses < IDENTITY_MAP_SIZE (4GB), but the bootloader doesn't provide this 1:1 mapping. The fix is to remove all identity mapping assumptions and always use PHYS_TO_VIRT() for page table access in functions like VMemGetPageTablePhys, VMemMap, VMemMapHuge, VMemGetPhysAddr, VMemUnmap, VMemMapMMIO, and VMemUnmapMMIO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T02:13:46.266Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#72
File: arch/x86_64/syscall/SyscallEntry.asm:24-30
Timestamp: 2025-08-12T02:13:46.266Z
Learning: User assembler-0 has existing ELF infrastructure in VoidFrame kernel with Elf.h header definitions and CreateProcess() function that properly sets up process context including stack allocation, ProcessExitStub return address, and register initialization. The issue was missing ELF loader implementation, not compilation problems.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-30T07:32:52.338Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#99
File: meson.build:103-103
Timestamp: 2025-08-30T07:32:52.338Z
Learning: FAT12 filesystem has a hard mathematical limit of 4,086 clusters due to 12-bit cluster addressing. This means FAT12 cannot handle disk sizes larger than approximately 4MB, making 128MB disks incompatible with FAT12 format. The user's meson.build was creating 128M disks with FAT12 format (`mkfs.fat -F 12`), which caused kernel panics when accessing files. The solution is to either use smaller disk sizes (≤4MB, preferably 1440K for traditional floppy compatibility) with FAT12, or use FAT16/FAT32 for larger disk sizes.
Applied to files:
kernel/core/Kernel.c
🧬 Code graph analysis (5)
fs/EXT/Ext2.h (1)
fs/EXT/Ext2.c (8)
Ext2Init(38-89)Ext2ReadFile(201-242)Ext2ListDir(249-287)Ext2IsFile(304-313)Ext2IsDir(315-324)Ext2GetFileSize(326-335)Ext2ReadInode(91-115)Ext2PathToInode(154-199)
kernel/etc/StringOps.h (1)
kernel/etc/StringOps.c (1)
FastStrnCmp(25-40)
fs/VFS.c (1)
fs/EXT/Ext2.c (9)
Ext2ReadFile(201-242)Ext2ListDir(249-287)Ext2CreateFile(289-292)Ext2CreateDir(294-297)Ext2Delete(299-302)Ext2GetFileSize(326-335)Ext2IsFile(304-313)Ext2IsDir(315-324)Ext2WriteFile(244-247)
kernel/core/Kernel.c (2)
kernel/etc/Console.c (2)
PrintKernelSuccess(213-218)PrintKernelWarning(227-232)fs/EXT/Ext2.c (1)
Ext2Init(38-89)
fs/EXT/Ext2.c (5)
drivers/Ide.c (1)
IdeReadSector(145-189)kernel/etc/Console.c (2)
PrintKernelF(260-267)PrintKernelSuccess(213-218)mm/MemOps.c (1)
FastMemcpy(75-162)mm/KernelHeap.c (2)
KernelMemoryAlloc(272-330)KernelFree(377-422)kernel/etc/StringOps.c (2)
FastStrnCmp(25-40)FastStrlen(10-15)
🔇 Additional comments (11)
kernel/etc/StringOps.h (1)
10-10: Add FastStrnCmp prototype — LGTMPrototype matches the implementation and placement is consistent with existing APIs.
fs/VFS.h (1)
11-13: Introduce VFS_EXT2 enum — LGTMEnumerator and trailing comma changes are correct and non-breaking. Downstream switches appear updated in this PR.
meson.build (1)
71-73: Build wiring for FAT/EXT paths — files and paths verified; LGTMfs/VFS.c (1)
4-8: Header path updates and KernelHeap include reordering — LGTMIncludes reflect the new FAT/EXT layout and avoid duplicate KernelHeap.h.
kernel/core/Kernel.c (2)
591-594: Good addition of memory pool initializationThe early initialization of memory pools is a solid improvement for efficient small allocations. This should reduce fragmentation and improve allocation performance for kernel objects.
597-605: Excellent memory statistics reportingAdding detailed memory statistics during boot provides valuable observability into kernel memory health. The fragmentation score reporting is particularly useful for early detection of memory issues.
fs/EXT/Ext2.h (2)
87-88: Well-designed type checking macrosThe S_ISDIR and S_ISREG macros properly check the file type bits using the standard POSIX approach. Good defensive programming with the mask.
100-113: Good API design with clear VFS integrationThe function prototypes provide a clean interface for VFS integration. The separation between public API and internal helpers (Ext2ReadInode, Ext2PathToInode) is well thought out.
fs/EXT/Ext2.c (3)
79-79: Correct BGDT location logicThe Block Group Descriptor Table location logic correctly handles the special case where the superblock backup occupies block 0 when block_size > 1024.
154-199: Well-implemented path traversal logicThe path parsing correctly handles root directory special case, skips leading/trailing slashes, and validates each component is a directory before traversing. Good defensive programming.
304-335: Clean implementation of utility functionsThe IsFile, IsDir, and GetFileSize functions are well-implemented with proper error handling and early returns for uninitialized state.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores