storage: fix VFS corruption, ISO9660 hang, OOM; add notepad, file picker, fdtable, GPT, async I/O
continuous-integration/drone/push Build is passing
continuous-integration/drone/push Build is passing
VFS fixes: - GetDirectoryListing(otVDIRECTORY) now returns a heap-allocated snapshot instead of a raw live-tree pointer; FreeDirectoryListing no longer corrupts the VFS tree (fixes page fault on repeated navigation) - volumeGetDirectories: guard LL_Size > 0 before loop to prevent uint32 underflow (0-1 = 0xFFFFFFFF) causing 4B kalloc iterations -> OOM - Free entry^.fileName and dirList in all branches (memory leak fix) iso9660: - equalsIgnoreCase: guard la=0 before loop (same uint32 underflow bug) - readDirectoryEntries/resolvePath: add 65536/256-iter safety caps storagemanager: - Strip debug syslog calls that were allocating 6+ strings per I/O (hundreds of I/Os x 6 allocs exhausted the heap) Process manager: - Per-process FDTable and Cwd initialised on create, freed on reap - proc_await: use pointer-indirection spin per lesson #6 (FPC caches non-volatile reads; raw while flag=0 returns immediately) - graphicsrefresh: move LVGL handler from timer ISR (IF=0) to a dedicated process so LVGL callbacks can call blocking I/O New units: - fdtable.pas: per-process file descriptor table (fd_alloc/get/close) - iorequest.pas: heap allocation helpers for TIORequest - gpt.pas: GPT header/entry read + CRC validation - filepicker.pas: reusable OS-level open/save file picker dialog - notepad.pas: GUI text editor with selection, indent, save-as Shell commands (diskcmd, partcmd, volcmd, diskutil): - Route output to stdout_buf instead of syslog (lesson #4) - diskutil: async add/remove/format partition; cache-based device display Add Apache 2.0 license headers to storagemanager.pas, iso9660.pas, fat32.pas, storagetest.pas Lessons learnt: #9 VFS listing must return snapshot (not live pointer)
This commit is contained in:
1 parent
aa0f537b5b
commit
3e3d001a49
33 files changed
+5125
-1873
No files matched your search
+70
-48
File diff suppressed because it is too large.
Load diff
Binary file not shown.
@@ -0,0 +1,26 @@
|
||||
# Lessons Learnt
|
||||
|
||||
## 1. File Pickers Must Be Proper Reusable OS Components
|
||||
Inline pickers hacked into app code are broken and not reusable. Build file pickers as a standalone unit (`filepicker.pas`) that any program can call via a callback.
|
||||
|
||||
## 2. All Errors Need Visible In-App Feedback
|
||||
Never use `syslog.logln` as the only error output. Always show the user a dialog, msgbox, or status bar message. Syslog is supplemental only.
|
||||
|
||||
## 4. Shell Command Output Goes to stdout_buf, Not syslog
|
||||
Use `stdio.bufWriteStr(stdout_buf, ...)` for all shell command output. `syslog` is for kernel diagnostics only.
|
||||
|
||||
## 5. Unit Test Convention
|
||||
New subsystem units get a `procedure UnitTest` (called at boot from kernel.pas) and a `procedure init` that registers a CLI command so tests can be re-run at runtime. Tests use a nested `Assert(condition, name)` that increments `passed`/`failed` and logs failures via `syslog.logln`. At-boot tests must not do disk I/O — only test in-memory/virtual structures.
|
||||
|
||||
## 6. ISR-Visible Spin-Wait Flags Must Use Pointer Dereferencing
|
||||
FPC has no `volatile` keyword. Any `while flag = 0 do asm hlt end` loop where `flag` is set by an ISR will hang forever because FPC hoists the load into a register before the loop and never re-reads memory. Always access the flag through a pointer: `while puint32(@flag)^ = 0 do proc_yield()`. This applies everywhere in the kernel that spins waiting for ISR-written state (e.g. `submit_io_wait`, `proc_await`-style loops, DMA done flags).
|
||||
|
||||
## 7. Never Spawn a Process Per I/O Operation
|
||||
Spawning a new kernel process for each FAT32 read/write/dir operation is pure overhead: process creation (kalloc 8 KB stack, fake IRET frame, DList insertion), an extra context switch, and a double-wait (VFS spins on the worker, worker spins on AHCI). Instead, submit the I/O directly and park the calling process via `psAwaiting` until the AHCI ISR fires the completion callback — zero extra processes, zero wasted scheduler ticks.
|
||||
|
||||
## 8. Use psAwaiting to Park a Process Pending Async I/O (not proc_yield spin)
|
||||
The correct way to block a process on an async I/O result is: (1) set up a completion callback that sets `State := psReady` then `Done := 1`, (2) atomically check if the operation is already done (under CLI) and set `CurrentProcess^.State := psAwaiting` if not, (3) spin on `Done` via `puint32(@wait.Done)^` per Lesson #6 with `asm hlt end` inside the loop. The process is removed from the runnable set while hardware works. **Do NOT call `proc_await` from `submit_io_wait`** — `proc_await` spins on `CurrentProcess^.State = psAwaiting` without pointer indirection and FPC caches the value in a register, causing it to return immediately before the I/O completes. The calling code then reads garbage from the DMA buffer, causing invalid pointer dereferences and page faults.
|
||||
|
||||
## 9. VFS GetDirectoryListing Must Return a Snapshot, Not a Live Tree Reference
|
||||
`GetDirectoryListing` for `otVDIRECTORY` nodes previously returned `PHashMap(Obj^.Reference)` — a direct pointer into the live VFS tree. When callers called `FreeDirectoryListing` on the returned map, it freed the live VFSObjects and their name strings, destroying the VFS tree. Any subsequent `GetObjectFromPath` call would then dereference the trashed memory and page fault. **Fix**: always return a heap-allocated snapshot copy of the directory (new `PHashMap`, new `PVFSObject` entries with `stringCopy`'d keys). `FreeDirectoryListing` can then safely own and free the snapshot without touching the live tree. The rule: any function that returns a `PHashMap` to a caller who will call `FreeDirectoryListing` must allocate a fresh map — never return a raw interior pointer to a live data structure.
|
||||
|
||||
-603
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
@@ -23,15 +23,15 @@ unit flatfs;
|
||||
interface
|
||||
|
||||
uses
|
||||
tracer,
|
||||
strings,
|
||||
filesystemmanager,
|
||||
lists,
|
||||
syslog,
|
||||
lmemorymanager,
|
||||
stdio,
|
||||
storagemanager,
|
||||
storagetypes,
|
||||
lmemorymanager,
|
||||
strings,
|
||||
syslog,
|
||||
tracer,
|
||||
util,
|
||||
volumemanager;
|
||||
|
||||
@@ -592,15 +592,18 @@ end;
|
||||
procedure detect_volumes(disk : PStorage_Device);
|
||||
var
|
||||
buffer : puint32;
|
||||
bufSize : uint32;
|
||||
volume : PStorage_volume;
|
||||
begin
|
||||
push_trace('flatfs.detectVolumes()');
|
||||
|
||||
buffer := puint32(kalloc(512));
|
||||
memset(uint32(buffer), 0, 512);
|
||||
bufSize := disk^.sectorSize;
|
||||
if bufSize < 512 then bufSize := 512;
|
||||
buffer := puint32(kalloc(bufSize));
|
||||
memset(uint32(buffer), 0, bufSize);
|
||||
|
||||
if (disk^.readCallback = nil) and (disk^.readCallbackAsync = nil) then begin
|
||||
syslog.writestringln('FlatFS: detect_volumes: device has no read callback.');
|
||||
if disk^.dispatchRead = nil then begin
|
||||
syslog.writestringln('FlatFS: detect_volumes: device has no read dispatch.');
|
||||
kfree(buffer);
|
||||
exit;
|
||||
end;
|
||||
@@ -759,14 +762,17 @@ end;
|
||||
|
||||
function identify_volume(volume : PStorage_Volume) : boolean;
|
||||
var
|
||||
buffer : puint32;
|
||||
buffer : puint32;
|
||||
bufSize : uint32;
|
||||
begin
|
||||
push_trace('flatfs.identify_volume');
|
||||
identify_volume := false;
|
||||
if (volume^.device^.readCallback = nil) and (volume^.device^.readCallbackAsync = nil) then exit;
|
||||
if volume^.device^.dispatchRead = nil then exit;
|
||||
|
||||
buffer := puint32(kalloc(512));
|
||||
memset(uint32(buffer), 0, 512);
|
||||
bufSize := volume^.device^.sectorSize;
|
||||
if bufSize < 512 then bufSize := 512;
|
||||
buffer := puint32(kalloc(bufSize));
|
||||
memset(uint32(buffer), 0, bufSize);
|
||||
|
||||
storagemanager.storage_read(volume^.device, volume^.sectorStart, 1, buffer);
|
||||
|
||||
|
||||
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
@@ -38,12 +38,10 @@ begin
|
||||
if size = 0 then exit;
|
||||
|
||||
{ Round up to a whole number of sectors so DMA / block I/O is safe }
|
||||
sectorSize := volume^.device^.sectorSize; //TODO need fileystsem function to get thing size, like cluster size, for file-level IO buffer allocation. For now we just assume sector size.
|
||||
sectorSize := volume^.device^.sectorSize;
|
||||
if sectorSize = 0 then
|
||||
sectorSize := 512;
|
||||
|
||||
sectorSize := 512*4; { safe default } //TODO
|
||||
|
||||
alignedSize := ((size + sectorSize - 1) div sectorSize) * sectorSize;
|
||||
|
||||
buf := puint32(kalloc(alignedSize));
|
||||
|
||||
@@ -0,0 +1,93 @@
|
||||
{
|
||||
Driver->Storage->IORequest - I/O request allocation and lifecycle helpers.
|
||||
|
||||
Provides heap allocation/free for TIORequest records used by the
|
||||
submit_io / complete_io path in storagemanager.pas.
|
||||
|
||||
@author(Aaron Hance <[email protected]>)
|
||||
}
|
||||
unit iorequest;
|
||||
|
||||
interface
|
||||
|
||||
uses
|
||||
lmemorymanager,
|
||||
storagetypes;
|
||||
|
||||
{ Allocate a new TIORequest on the heap and initialise its core fields.
|
||||
Caller, State, Error and ByteCount are zeroed (caller fills as needed). }
|
||||
function ioreq_alloc(reqType : TIORequestType;
|
||||
device : PStorage_Device;
|
||||
lba : uint32;
|
||||
sectors : uint32;
|
||||
buf : pointer) : PIORequest;
|
||||
|
||||
{ Free a heap-allocated TIORequest. Safe to call with nil. }
|
||||
procedure ioreq_free(req : PIORequest);
|
||||
|
||||
{ Copy an existing TIORequest into a new heap allocation (deep copy of the
|
||||
record, NOT the buffer it points to). Used by dispatch_next to create an
|
||||
ISR-safe copy from the CFIFO value-element. }
|
||||
function ioreq_copy(src : PIORequest) : PIORequest;
|
||||
|
||||
implementation
|
||||
|
||||
function ioreq_alloc(reqType : TIORequestType;
|
||||
device : PStorage_Device;
|
||||
lba : uint32;
|
||||
sectors : uint32;
|
||||
buf : pointer) : PIORequest;
|
||||
var
|
||||
req : PIORequest;
|
||||
begin
|
||||
req := PIORequest(kalloc(SizeOf(TIORequest)));
|
||||
if req = nil then begin
|
||||
ioreq_alloc := nil;
|
||||
exit;
|
||||
end;
|
||||
req^.RequestType := reqType;
|
||||
req^.State := iosPending;
|
||||
req^.Device := device;
|
||||
req^.LBA := lba;
|
||||
req^.SectorCount := sectors;
|
||||
req^.Buffer := buf;
|
||||
req^.ByteCount := 0;
|
||||
req^.Error := eNone;
|
||||
req^.Caller := nil;
|
||||
req^.UserData := nil;
|
||||
req^.Callback := nil;
|
||||
req^.CallbackData := nil;
|
||||
ioreq_alloc := req;
|
||||
end;
|
||||
|
||||
procedure ioreq_free(req : PIORequest);
|
||||
begin
|
||||
if req <> nil then
|
||||
kfree(void(req));
|
||||
end;
|
||||
|
||||
function ioreq_copy(src : PIORequest) : PIORequest;
|
||||
var
|
||||
dst : PIORequest;
|
||||
begin
|
||||
dst := PIORequest(kalloc(SizeOf(TIORequest)));
|
||||
if dst = nil then begin
|
||||
ioreq_copy := nil;
|
||||
exit;
|
||||
end;
|
||||
dst^.RequestType := src^.RequestType;
|
||||
dst^.State := src^.State;
|
||||
dst^.Device := src^.Device;
|
||||
dst^.LBA := src^.LBA;
|
||||
dst^.SectorCount := src^.SectorCount;
|
||||
dst^.Buffer := src^.Buffer;
|
||||
dst^.ByteCount := src^.ByteCount;
|
||||
dst^.Error := src^.Error;
|
||||
dst^.Caller := src^.Caller;
|
||||
dst^.UserData := src^.UserData;
|
||||
dst^.Callback := src^.Callback;
|
||||
dst^.CallbackData := src^.CallbackData;
|
||||
ioreq_copy := dst;
|
||||
end;
|
||||
|
||||
end.
|
||||
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
+638
-234
File diff suppressed because it is too large.
Load diff
File diff suppressed because it is too large.
Load diff
@@ -1,8 +1,20 @@
|
||||
{
|
||||
Driver->Timer->GraphicsRefresh - Timer-driven graphics refresh at ~120FPS.
|
||||
Driver->Timer->GraphicsRefresh - Process-driven UI/graphics refresh at ~120FPS.
|
||||
|
||||
Hooks into the 1024Hz timer (TMR_0_ISR) and calls the desktop, uidebug,
|
||||
LVGL and video flush routines every ~9 ticks (1024/9 ≈ 113.8 FPS).
|
||||
Spawns a dedicated UI process that calls desktop, uidebug, LVGL and video
|
||||
flush routines in a loop (~8ms sleep ≈ 120 FPS).
|
||||
|
||||
Running LVGL from a process (not the timer ISR) is critical: LVGL event
|
||||
callbacks (e.g. notepad save, disk format) may call blocking storage I/O
|
||||
via submit_io_wait. Blocking I/O needs proc_yield → hlt, which requires
|
||||
interrupts to be enabled (IF=1). Inside the timer ISR, IF=0, so hlt
|
||||
would hang forever. A process runs with IF=1 between preemptions, so
|
||||
blocking I/O works correctly.
|
||||
|
||||
LVGL's internal tick counter (tick_accumulator) is still driven by the
|
||||
1024Hz timer ISR hook registered in lvgl_init — that just increments a
|
||||
counter and is safe from ISR context. Only lv_timer_handler (event
|
||||
dispatch) and video.Flush move to the process.
|
||||
|
||||
@author(Kieron Morris <kjm@kieronmorris.me>)
|
||||
}
|
||||
@@ -15,8 +27,8 @@ procedure init;
|
||||
implementation
|
||||
|
||||
uses
|
||||
util,
|
||||
TMR_0_ISR,
|
||||
proctypes,
|
||||
processmanager,
|
||||
desktop,
|
||||
uidebug,
|
||||
lvgl,
|
||||
@@ -24,31 +36,25 @@ uses
|
||||
windows,
|
||||
syslog;
|
||||
|
||||
const
|
||||
{ 1024 / 4 ≈ 256 FPS }
|
||||
TICKS_PER_FRAME = 4;
|
||||
|
||||
var
|
||||
TickCounter : uint32;
|
||||
|
||||
procedure on_tick(data : void);
|
||||
{ UI loop — runs as a dedicated process so LVGL event callbacks can safely
|
||||
call blocking storage I/O (proc_yield → hlt needs IF=1). }
|
||||
procedure ui_loop(ctx : PProcessContext);
|
||||
begin
|
||||
TickCounter := TickCounter + 1;
|
||||
if TickCounter >= TICKS_PER_FRAME then begin
|
||||
TickCounter := 0;
|
||||
while true do begin
|
||||
windows.reapOrphanedWindows;
|
||||
desktop.update;
|
||||
uidebug.update;
|
||||
lvgl_handler;
|
||||
video.Flush;
|
||||
{ ~120 FPS: sleep 8ms between frames }
|
||||
processmanager.proc_sleep_ms(8);
|
||||
end;
|
||||
end;
|
||||
|
||||
procedure init;
|
||||
begin
|
||||
TickCounter := 0;
|
||||
TMR_0_ISR.hook(uint32(@on_tick));
|
||||
syslog.logln('GFXREFRESH', 'Hooked into 1024Hz timer (~120 FPS).');
|
||||
processmanager.create('ui', @ui_loop, nil, 1);
|
||||
syslog.logln('GFXREFRESH', 'UI process created (~120 FPS).');
|
||||
end;
|
||||
|
||||
end.
|
||||
@@ -5,6 +5,7 @@
|
||||
bindings used by processmanager.pas and contextswitcher.pas.
|
||||
|
||||
@author(Kieron Morris <kjm@kieronmorris.me>)
|
||||
@author(Aaron Hance <ah@aaronhance.me>)
|
||||
}
|
||||
unit proctypes;
|
||||
|
||||
@@ -105,6 +106,12 @@ type
|
||||
{ Resource bindings (PDList of TResourceBinding) }
|
||||
Resources : void;
|
||||
|
||||
{ Per-process file descriptor table (PFDTable from fdtable.pas) }
|
||||
FDTable : void;
|
||||
|
||||
{ Per-process working directory — heap-allocated, defaults to '/' }
|
||||
Cwd : pchar;
|
||||
|
||||
{ User-defined state }
|
||||
Local : void;
|
||||
end;
|
||||
|
||||
Loaded 20 of 33 files, more files were not shown because too many files have changed in this diff.
Show more
Reference in new issue
Block a user