Skip to content

Conversation

assembler-0
Copy link
Owner

@assembler-0 assembler-0 commented Oct 13, 2025

NVMe driver, vsnprintf, and strings

Summary by CodeRabbit

  • New Features

    • NVMe storage support added; VM run and extra-image build now include an NVMe disk and auxiliary disks use ext2.
    • Per-device volume support added for EXT2, FAT1x, and NTFS with activation before filesystem operations.
  • Refactor

    • Safer, size‑bounded kernel formatting (vsnprintf) and added string utility functions.
  • Documentation

    • README: NTFS operations marked complete; NVMe listed under Storage.
  • Style

    • Reduced directory-list log noise, longer scheduler separators, and corrected stack usage display.

Copy link

coderabbitai bot commented Oct 13, 2025

Walkthrough

Adds full NVMe support: build/run integration (CMake/QEMU), feature flag, sources, driver (PCI/MMIO, admin/IO queues, read/write, block registration) and kernel init path. Switches image tooling to create/format NVMeDisk.img and use ext2 for disks. Introduces per-device volume registries and SetActive APIs for Ext2, FAT1x, and NTFS with VFS call-site activations. Reworks formatting to size-bounded vsnprintf, adds string utilities (strspn/strpbrk/strsep), and adds a generic vector API (include/Vector.h). README updates reflect NVMe and NTFS checklist changes.

Poem

In my burrow I nibble code and light,
Doorbells drum softly through the night.
Queues I plant, and sectors sing,
Ext2 wakes, and blocks take wing.
Hop! NVMe carrots—fast and bright. 🥕🐇

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.55% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title “Development” is too vague to convey the primary changes in this pull request, which include adding an NVMe driver, implementing a bounded vsnprintf function, and extending string utilities; it does not help reviewers quickly understand the main update. Please revise the title to a concise summary that highlights the key additions, for example “Add NVMe driver with vsnprintf and string utility enhancements.”
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (3)
README.md (1)

132-135: Fix list indentation to satisfy markdownlint (MD007) and confirm NTFS write support

  • Indentation is 4 spaces; markdownlint expects 2. Please reduce indent by 2 spaces for these items.
  • Also, flipping NTFS Write/Create/Delete/List to checked implies full support. Confirm these operations are implemented/stable to avoid misleading users.

Apply this minimal diff to fix indentation:

-    - [x] Write
-    - [x] Create
-    - [x] Delete
-    - [x] List
+  - [x] Write
+  - [x] Create
+  - [x] Delete
+  - [x] List
drivers/storage/NVMe.c (2)

318-326: Cap queue sizes to CAP.MQES+1

Queue entries must be <= (CAP.MQES + 1). Current code assumes NVME_*_QUEUE_SIZE fits all controllers.

Apply this diff to compute safe sizes:

-    // Configure admin queues
-    NVMe_WriteReg32(NVME_AQA, ((NVME_ADMIN_QUEUE_SIZE - 1) << 16) | (NVME_ADMIN_QUEUE_SIZE - 1));
+    // Configure admin queues with CAP.MQES cap
+    uint64_t cap = NVMe_ReadReg64(NVME_CAP);
+    uint32_t max_qe = (uint32_t)((cap & 0xFFFFu) + 1u);
+    uint32_t a_entries = (NVME_ADMIN_QUEUE_SIZE < max_qe) ? NVME_ADMIN_QUEUE_SIZE : max_qe;
+    NVMe_WriteReg32(NVME_AQA, ((a_entries - 1) << 16) | (a_entries - 1));

Also size allocations should match a_entries (and the IO queue sizes should respect the same cap).


338-347: Use Identify Namespace to derive logical block size (don’t hardcode 512)

Block size can be 4K (or other). Parse Identify Namespace (FLBAS/LBAF[n].LBADS) to set correct block size and total_blocks = NSZE, not bytes.

Would you like a patch to read LBADS and set block_size accordingly? It’s a few lines on top of your Identify path.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 92e6f27 and b53d3c7.

📒 Files selected for processing (12)
  • CMakeLists.txt (2 hunks)
  • README.md (1 hunks)
  • cmake/features.cmake (1 hunks)
  • cmake/source.cmake (1 hunks)
  • drivers/storage/NVMe.c (1 hunks)
  • drivers/storage/NVMe.h (1 hunks)
  • kernel/core/Kernel.c (3 hunks)
  • kernel/etc/Format.c (2 hunks)
  • kernel/etc/Format.h (2 hunks)
  • kernel/etc/StringOps.c (1 hunks)
  • kernel/etc/StringOps.h (1 hunks)
  • kernel/sched/EEVDF.c (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
kernel/sched/EEVDF.c (1)
kernel/etc/Console.c (1)
  • PrintKernel (169-188)
drivers/storage/NVMe.c (6)
drivers/TSC.c (1)
  • delay_us (36-43)
mm/MemOps.c (2)
  • FastMemcpy (212-451)
  • FastMemset (23-210)
mm/KernelHeap.c (2)
  • KernelMemoryAlloc (362-438)
  • KernelFree (485-530)
drivers/PCI/PCI.c (4)
  • PciFindByClass (194-206)
  • PciReadConfig16 (27-30)
  • PciWriteConfig16 (55-64)
  • GetPCIMMIOSize (208-284)
mm/VMem.c (1)
  • VMemMapMMIO (622-673)
fs/BlockDevice.c (2)
  • BlockDeviceRegister (18-54)
  • BlockDeviceDetectAndRegisterPartitions (79-84)
drivers/storage/NVMe.h (1)
drivers/storage/NVMe.c (3)
  • NVMe_Init (235-361)
  • NVMe_ReadSectors (193-207)
  • NVMe_WriteSectors (209-223)
kernel/core/Kernel.c (2)
drivers/storage/NVMe.c (1)
  • NVMe_Init (235-361)
kernel/etc/Console.c (3)
  • PrintKernelSuccess (202-207)
  • PrintKernelWarning (216-221)
  • PrintKernelF (249-256)
kernel/etc/StringOps.h (1)
kernel/etc/StringOps.c (3)
  • strsep (267-280)
  • strpbrk (243-254)
  • strspn (219-236)
kernel/etc/Format.c (1)
kernel/etc/StringOps.c (1)
  • StringLength (3-8)
🪛 markdownlint-cli2 (0.18.1)
README.md

132-132: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


133-133: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


134-134: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


135-135: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

⏰ 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 (4)
kernel/sched/EEVDF.c (1)

1327-1327: Formatting-only separator update LGTM

Longer dashed line improves readability; no functional impact.

Also applies to: 1357-1357

cmake/features.cmake (1)

14-14: NVMe feature flag addition looks good

Define aligns with other storage flags. Assuming Kernel.c gates NVMe init on this define, this is fine.

If not already, ensure Kernel.c only calls NVMe_Init() under VF_CONFIG_ENABLE_NVME.

kernel/etc/StringOps.h (1)

17-19: New string prototypes match implementations

Signatures for strsep/strpbrk/strspn look correct and consistent with the C variants used in StringOps.c.

kernel/etc/StringOps.c (1)

213-280: strspn/strpbrk/strsep implementations look correct

Logic matches standard behavior, including strsep updating the input pointer and returning empty tokens.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (6)
drivers/storage/NVMe.c (4)

122-168: Verify physical contiguity of I/O queue allocations.

Lines 126-127 use VMemAlloc for I/O queues and set PC=1 (physically contiguous) in the CREATE_IO_CQ/SQ commands. Per past review comments, VMemAlloc may not guarantee physically contiguous pages. Consider using a contiguous allocator or clearing PC=0 and implementing PRP lists.

The cdw0 packing (lines 145, 157) now correctly places opcode in the low byte and CID in bits 16-31, which addresses previous concerns.

Based on past review comments.


241-382: Verify physically contiguous admin queue allocation.

The initialization sequence is well-structured with proper error handling. However, lines 321-322 use VMemAlloc for admin queues, which may not guarantee physical contiguity as required by the PC=1 flag set during queue configuration. This is the same concern flagged for I/O queues.

Otherwise, the MMIO mapping, controller reset, and block device registration logic are correctly implemented.

Based on past review comments.


199-213: Guard against zero count and handle PRP boundary crossings.

Critical issues remain:

  1. Line 210: (count - 1) underflows when count==0
  2. Only PRP1 is set; transfers crossing 4KB boundaries will corrupt data

These issues were flagged in past review comments. Consider implementing the suggested chunking approach to split transfers at page boundaries.

Based on past review comments.


215-229: Apply same zero-count guard and PRP boundary handling for writes.

The same critical issues from NVMe_ReadSectors apply here: underflow on count==0 and missing PRP boundary handling.

Based on past review comments.

kernel/etc/Format.c (2)

116-129: Restore full flag/width/precision parsing (and remove the bogus SMALL check).

Flags are skipped and never applied; width/precision are ignored. Also, checking *fmt == 'x' inside the flags loop is wrong. This regresses %08x, %-4d, %.3u, %#x, etc. Parse and record flags, width (incl. *), and precision, then use them downstream.

Apply this diff to correctly parse flags, width, and precision:

-        /* Skip flags */
-        while (*fmt == '-' || *fmt == '+' || *fmt == ' ' || *fmt == '#' || *fmt == '0') {
-            if (*fmt == 'x') flags |= SMALL;
-            ++fmt;
-        }
-
-        /* Skip field width */
-        while (is_digit(*fmt)) ++fmt;
-
-        /* Skip precision */
-        if (*fmt == '.') {
-            ++fmt;
-            while (is_digit(*fmt)) ++fmt;
-        }
+        /* Parse flags */
+        while (1) {
+            if (*fmt == '-') { flags |= LEFT;    ++fmt; continue; }
+            if (*fmt == '+') { flags |= PLUS;    ++fmt; continue; }
+            if (*fmt == ' ') { flags |= SPACE;   ++fmt; continue; }
+            if (*fmt == '#') { flags |= SPECIAL; ++fmt; continue; }
+            if (*fmt == '0') { flags |= ZEROPAD; ++fmt; continue; }
+            break;
+        }
+
+        /* Field width */
+        if (*fmt == '*') {
+            width = va_arg(args, int);
+            if (width < 0) { width = -width; flags |= LEFT; }
+            ++fmt;
+        } else {
+            width = skip_atoi(&fmt);
+        }
+
+        /* Precision */
+        if (*fmt == '.') {
+            ++fmt;
+            if (*fmt == '*') {
+                precision = va_arg(args, int);
+                ++fmt;
+            } else {
+                precision = skip_atoi(&fmt);
+            }
+            if (precision < 0) precision = 0;
+        }

And initialize per-spec parse state after %:

-        flags = 0;
+        flags = 0;
+        width = 0;
+        precision = -1;

159-172: Fix LLONG_MIN overflow and honor sign/plus/space with width/precision.

Negating LLONG_MIN overflows. Use unsigned magnitude; also emit sign per '+', ' ' flags and pass all formatting params.

-        case 'i': {
-            long long snum;
-            if (is_longlong) snum = va_arg(args, long long);
-            else if (is_long) snum = va_arg(args, long);
-            else snum = va_arg(args, int);
-
-            if (snum < 0 && pos < (int)size - 1) {
-                buf[pos++] = '-';
-                snum = -snum;
-            }
-            number_to_str(buf, size, &pos, (unsigned long long)snum, 10, 0);
+        case 'i': {
+            long long snum;
+            if (is_longlong) snum = va_arg(args, long long);
+            else if (is_long) snum = va_arg(args, long);
+            else snum = va_arg(args, int);
+
+            unsigned long long unum = (unsigned long long)snum;
+            char sign = 0;
+            if (snum < 0) {
+                sign = '-';
+                unum = (~unum) + 1; /* absolute value without UB */
+            } else if (flags & PLUS) {
+                sign = '+';
+            } else if (flags & SPACE) {
+                sign = ' ';
+            }
+            number_to_str(buf, size, &pos, unum, 10, flags, width, precision, sign);
             break;
         }
🧹 Nitpick comments (6)
README.md (1)

188-189: Document NVMe driver addition accurately.

The NVMe driver documentation is correctly added. Consider fixing the indentation to use 2 spaces instead of 4 for consistency with markdown linting rules.

Apply this diff to fix indentation:

-- [x] AHCI
-- [x] NVMe
+  - [x] AHCI
+  - [x] NVMe

As per static analysis hints.

kernel/etc/Format.c (5)

100-104: Track width/precision locals for each conversion.

Declare width and precision alongside flags to be used across cases.

-    int len, i;
-    int flags;
+    int len, i;
+    int flags, width = 0, precision = -1;

146-148: Apply width/precision and alignment for 'c' and 's'.

Both specifiers ignore width; 's' ignores precision. Implement standard padding and truncation.

 case 'c':
-            if (pos < (int)size - 1) buf[pos++] = (char)va_arg(args, int);
+            {
+                char ch = (char)va_arg(args, int);
+                int pad = (width > 1) ? (width - 1) : 0;
+                if (!(flags & LEFT)) while (pad-- > 0 && pos < (int)size - 1) buf[pos++] = ' ';
+                if (pos < (int)size - 1) buf[pos++] = ch;
+                if (flags & LEFT) while (pad-- > 0 && pos < (int)size - 1) buf[pos++] = ' ';
+            }
             break;
@@
         case 's':
             s = va_arg(args, char *);
-            if (!s) s = "(null)";
-            len = StringLength(s);
-            for (i = 0; i < len && pos < (int)size - 1; i++) {
-                buf[pos++] = s[i];
-            }
+            if (!s) s = "(null)";
+            len = StringLength(s);
+            if (precision >= 0 && len > precision) len = precision;
+            {
+                int pad = (width > len) ? (width - len) : 0;
+                if (!(flags & LEFT)) while (pad-- > 0 && pos < (int)size - 1) buf[pos++] = ' ';
+                for (i = 0; i < len && pos < (int)size - 1; i++) buf[pos++] = s[i];
+                if (flags & LEFT) while (pad-- > 0 && pos < (int)size - 1) buf[pos++] = ' ';
+            }
             break;

Also applies to: 150-157


203-208: Unify %p with integer formatter and avoid manual "0x".

Let the helper add the prefix via SPECIAL, ensuring width/zero-padding works.

-        case 'p':
-            if (pos < (int)size - 1) buf[pos++] = '0';
-            if (pos < (int)size - 1) buf[pos++] = 'x';
-            number_to_str(buf, size, &pos, (unsigned long long)va_arg(args, void *), 16, SMALL);
-            break;
+        case 'p': {
+            unsigned long long addr = (unsigned long long)va_arg(args, void *);
+            number_to_str(buf, size, &pos, addr, 16, SMALL | SPECIAL, width, precision, 0);
+            break;
+        }

115-115: Initialize width/precision per-spec parse.

Make sure width/precision are reset for each % specifier.

-        flags = 0;
+        flags = 0;
+        width = 0;
+        precision = -1;

242-248: FormatS uses a static buffer (not reentrant/thread‑safe).

If called concurrently, outputs will clobber. Consider documenting this or providing a reentrant variant.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b53d3c7 and 2143344.

📒 Files selected for processing (5)
  • README.md (2 hunks)
  • drivers/storage/NVMe.c (1 hunks)
  • drivers/storage/NVMe.h (1 hunks)
  • fs/EXT/Ext2.c (0 hunks)
  • kernel/etc/Format.c (2 hunks)
💤 Files with no reviewable changes (1)
  • fs/EXT/Ext2.c
🧰 Additional context used
🧬 Code graph analysis (3)
drivers/storage/NVMe.h (1)
drivers/storage/NVMe.c (3)
  • NVMe_Init (241-382)
  • NVMe_ReadSectors (199-213)
  • NVMe_WriteSectors (215-229)
drivers/storage/NVMe.c (6)
drivers/TSC.c (1)
  • delay_us (36-43)
mm/MemOps.c (2)
  • FastMemcpy (212-451)
  • FastMemset (23-210)
mm/KernelHeap.c (2)
  • KernelMemoryAlloc (362-438)
  • KernelFree (485-530)
drivers/PCI/PCI.c (5)
  • PciFindByClass (194-206)
  • PciReadConfig16 (27-30)
  • PciWriteConfig16 (55-64)
  • PciConfigReadDWord (14-25)
  • GetPCIMMIOSize (208-284)
mm/VMem.c (1)
  • VMemMapMMIO (622-673)
fs/BlockDevice.c (2)
  • BlockDeviceRegister (18-54)
  • BlockDeviceDetectAndRegisterPartitions (79-84)
kernel/etc/Format.c (1)
kernel/etc/StringOps.c (1)
  • StringLength (3-8)
🪛 markdownlint-cli2 (0.18.1)
README.md

132-132: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


133-133: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


134-134: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


135-135: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


188-188: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


189-189: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

⏰ 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 (12)
README.md (1)

132-135: LGTM!

The documentation updates accurately reflect the completed NTFS features.

drivers/storage/NVMe.h (4)

1-43: LGTM!

The NVMe constant definitions, register offsets, and PCI class codes are correct and align with the NVMe specification.


45-58: LGTM!

The NVMeSubmissionEntry structure definition is correctly packed and matches the NVMe specification layout.


60-68: Verify phase bit handling in completion entry.

The structure packs status as a single uint16_t. Per NVMe spec, the phase tag is bit 0 of DW3. Ensure that code reading this structure correctly extracts phase from bit 0 of status and the actual status code from bits 1-15. The past review comments flag concerns about phase bit handling in the implementation.

Based on past review comments.


70-107: LGTM!

The NVMeController structure and public API declarations are well-defined and appropriate for the driver implementation.

drivers/storage/NVMe.c (6)

1-11: LGTM!

The includes and global controller instance are appropriate for this driver implementation.


17-31: LGTM!

The MMIO register access functions correctly use volatile pointers to prevent compiler optimizations.


33-42: LGTM!

The ready-wait logic with timeout is correctly implemented.


44-81: Doorbell stride computation is correct.

The doorbell offset computation correctly uses CAP.DSTRD via db_shift = 2 + ctrl->dstrd. This addresses the concern raised in past review comments.

However, verify that the phase bit comparison on line 64 ((status & 1) == ctrl->admin_cq_phase) correctly interprets the phase tag from bit 0 of the status field per NVMe spec.


83-120: I/O command submission correctly uses doorbell stride.

The implementation properly computes doorbell offsets using DSTRD. The phase bit handling should be verified as noted in the admin command path.


170-197: LGTM!

The namespace size retrieval logic is correctly implemented with appropriate error handling and fallback.

kernel/etc/Format.c (1)

98-106: vsnprintf: track full length and safe NUL-termination

  • Use a total counter to count all characters that would be written, continue parsing beyond buffer limits; only write when pos < size-1.
  • At end, null-terminate with if (size) buf[(pos < (int)size ? pos : (int)size - 1)] = '\0'; and return total instead of early return 0.
  • Audit call sites to ensure no code depends on the old truncated return value.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
fs/EXT/Ext2.h (1)

2-2: Remove duplicate include.

BlockDevice.h is included twice (lines 2 and 5). While harmless due to header guards, the duplication is unnecessary.

Apply this diff to remove the duplicate:

 #pragma once
 #include "BlockDevice.h"
 #include "stdint.h"
 
-#include "BlockDevice.h"
-
 // Minimal EXT2 data structures

Also applies to: 5-5

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2143344 and eb07cfb.

📒 Files selected for processing (4)
  • cmake/features.cmake (1 hunks)
  • fs/EXT/Ext2.c (2 hunks)
  • fs/EXT/Ext2.h (1 hunks)
  • fs/VFS.c (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • cmake/features.cmake
  • fs/EXT/Ext2.c
🧰 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 (2)
fs/VFS.c (1)
fs/EXT/Ext2.c (1)
  • Ext2SetActive (32-43)
fs/EXT/Ext2.h (1)
fs/EXT/Ext2.c (1)
  • Ext2SetActive (32-43)
⏰ 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 (1)
fs/EXT/Ext2.h (1)

105-106: LGTM!

The new Ext2SetActive function prototype is well-documented and enables per-device EXT2 context management. The implementation in fs/EXT/Ext2.c properly validates the device parameter and handles NULL gracefully.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
fs/VFS.c (2)

148-188: SetActive + global state is not thread-safe

Every filesystem branch calls XxxSetActive(mount->device) and immediately invokes the driver, but the drivers still rely on a single global “active” pointer. If two threads hit different mounts, the second thread re-points the global before the first thread finishes, so the first thread ends up operating on the wrong volume—exactly the race the per-device registry was meant to fix. Please rework the driver APIs to take an explicit volume/device handle (or hold a device-scoped lock across the entire call) so concurrent VFS operations can’t trample each other’s active context.


338-347: FAT1x branch never activates its volume

Unlike the other branches, the FAT1x path in VfsIsFile skips Fat1xSetActive(). If the previous call switched g_fat1x_active to another device (or NULL), this will read the wrong FAT table or crash. Please call Fat1xSetActive(mount->device) here before querying.

-        if (FastStrCmp(mount->fs_driver->name, "FAT1x") == 0) {
-            return Fat1xGetFileSize(local_path) > 0;
+        if (FastStrCmp(mount->fs_driver->name, "FAT1x") == 0) {
+            Fat1xSetActive(mount->device);
+            return Fat1xGetFileSize(local_path) > 0;
♻️ Duplicate comments (1)
kernel/core/Kernel.c (1)

742-746: Use %zu (or cast) for size_t in PrintKernelF

used is a size_t, but the format string still uses %llu. Passing mismatched types through varargs is UB (bit-pattern reinterpretation). Please switch to %zu (and cast KERNEL_STACK_SIZE to size_t) or cast used to unsigned long long before printing.

-    PrintKernelF("Stack used: %llu/%d bytes\n", used, KERNEL_STACK_SIZE);
+    PrintKernelF("Stack used: %zu/%zu bytes\n",
+                 used,
+                 (size_t)KERNEL_STACK_SIZE);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eb07cfb and 96e4b7e.

📒 Files selected for processing (8)
  • fs/EXT/Ext2.c (2 hunks)
  • fs/FAT/FAT1x.c (2 hunks)
  • fs/FAT/FAT1x.h (1 hunks)
  • fs/NTFS/NTFS.c (2 hunks)
  • fs/NTFS/NTFS.h (1 hunks)
  • fs/VFS.c (9 hunks)
  • include/Vector.h (1 hunks)
  • kernel/core/Kernel.c (3 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 (7)
fs/FAT/FAT1x.h (1)
fs/FAT/FAT1x.c (1)
  • Fat1xSetActive (17-22)
kernel/core/Kernel.c (2)
drivers/storage/NVMe.c (1)
  • NVMe_Init (241-382)
kernel/etc/Console.c (3)
  • PrintKernelSuccess (202-207)
  • PrintKernelWarning (216-221)
  • PrintKernelF (249-256)
fs/NTFS/NTFS.h (1)
fs/NTFS/NTFS.c (1)
  • NtfsSetActive (26-31)
fs/EXT/Ext2.c (4)
kernel/atomic/rust/src/ffi.rs (2)
  • rust_rwlock_write_lock (195-199)
  • rust_rwlock_write_unlock (202-206)
kernel/etc/Console.c (1)
  • PrintKernelF (249-256)
mm/KernelHeap.c (1)
  • KernelMemoryAlloc (362-438)
mm/MemOps.c (1)
  • FastMemset (23-210)
fs/FAT/FAT1x.c (2)
mm/KernelHeap.c (1)
  • KernelMemoryAlloc (362-438)
mm/MemOps.c (1)
  • FastMemset (23-210)
fs/NTFS/NTFS.c (2)
mm/KernelHeap.c (1)
  • KernelMemoryAlloc (362-438)
mm/MemOps.c (1)
  • FastMemset (23-210)
fs/VFS.c (3)
fs/FAT/FAT1x.c (1)
  • Fat1xSetActive (17-22)
fs/EXT/Ext2.c (1)
  • Ext2SetActive (32-47)
fs/NTFS/NTFS.c (1)
  • NtfsSetActive (26-31)

@assembler-0 assembler-0 merged commit 61a1009 into main Oct 14, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant