From 1ea2f68e6a9af135e6cb3274fdf28d4ddc5812d5 Mon Sep 17 00:00:00 2001 From: salmonik Date: Sat, 1 Aug 2026 18:25:55 +0200 Subject: fix pi-lens warnings: unwraps, shell SC2046, dup code (fmtbuf/scripts/hexdump/cat/ahci/pci) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - memory.rs: self-test Layout::from_size_align no longer panics — the test fails cleanly on an invalid layout - fat.rs: v() no longer unwraps the active-volume slot — invariant violation logs + halts instead of panicking mid-disk-io - strbuf: shared FmtBuf replaces the duplicated PanicBuf/FmtBuf (main.rs + sched.rs) - test-disk.sh: SC2046 — conditional --fresh arg no longer relies on word splitting - scripts: boot.sh/boot-image.sh share lib-boot-args.sh (dup args loop) - shell: cat/hexdump stdin pairs unified with Option<&str> marker; hexdump format_row folded into format_partial - ahci: read/write_sectors unified behind dma_sectors - pci: config_read/write share config_addr --- rsdos/src/main.rs | 21 +++-------- rsnix/src/ahci.rs | 18 +++++----- rsnix/src/fat.rs | 12 ++++++- rsnix/src/memory.rs | 12 +++++-- rsnix/src/pci.rs | 28 ++++++++------- rsnix/src/sched.rs | 18 ++-------- rsnix/src/strbuf.rs | 28 +++++++++++++++ scripts/boot-image.sh | 25 ++----------- scripts/boot.sh | 22 +++--------- scripts/lib-boot-args.sh | 38 ++++++++++++++++++++ scripts/test-disk.sh | 6 +++- shell/src/lib.rs | 94 +++++++++++------------------------------------- 12 files changed, 152 insertions(+), 170 deletions(-) create mode 100644 scripts/lib-boot-args.sh diff --git a/rsdos/src/main.rs b/rsdos/src/main.rs index 7ca031c..700dcc7 100644 --- a/rsdos/src/main.rs +++ b/rsdos/src/main.rs @@ -9,20 +9,9 @@ use rsnix::uefi::*; // Panic handler // --------------------------------------------------------------------------- -/// Small fmt::Write over a byte slice (no allocation, bounded). -struct PanicBuf<'a> { - b: &'a mut [u8], - n: usize, -} - -impl core::fmt::Write for PanicBuf<'_> { - fn write_str(&mut self, s: &str) -> core::fmt::Result { - let m = s.len().min(self.b.len() - self.n); - self.b[self.n..self.n + m].copy_from_slice(&s.as_bytes()[..m]); - self.n += m; - Ok(()) - } -} +// --------------------------------------------------------------------------- +// Panic handler +// --------------------------------------------------------------------------- #[panic_handler] fn panic(info: &PanicInfo) -> ! { @@ -35,12 +24,12 @@ fn panic(info: &PanicInfo) -> ! { if rsnix::sched::is_ready() && !rsnix::sched::in_isr() && rsnix::sched::current() >= 2 { let mut buf = [0u8; 192]; let n = { - let mut w = PanicBuf { b: &mut buf, n: 0 }; + let mut w = rsnix::strbuf::FmtBuf::new(&mut buf); let _ = write!(&mut w, "{}", info.message()); if let Some(loc) = info.location() { let _ = write!(&mut w, " at {}:{}", loc.file(), loc.line()); } - w.n + w.len() }; let msg = core::str::from_utf8(&buf[..n]).unwrap_or("task panic"); rsnix::sched::abort_current(msg); diff --git a/rsnix/src/ahci.rs b/rsnix/src/ahci.rs index 72da07d..a77b341 100644 --- a/rsnix/src/ahci.rs +++ b/rsnix/src/ahci.rs @@ -510,22 +510,24 @@ fn active() -> Option<&'static Disk> { disks().get(unsafe { ACTIVE }).and_then(|d| d.as_ref()) } -/// DMA `count` sectors starting at `lba` into the selected disk's buffer. -pub fn read_sectors(lba: u64, count: u32) -> Result<(), ()> { +/// Bounds-checked DMA of `count` sectors starting at `lba` into/out of the +/// selected disk's buffer (single code path for both directions). +fn dma_sectors(lba: u64, count: u32, cmd: u8, write: bool) -> Result<(), ()> { let d = active().ok_or(())?; if lba + count as u64 > d.sectors { return Err(()); } - issue(d, ATA_READ_DMA_EXT, lba, count, false) + issue(d, cmd, lba, count, write) +} + +/// DMA `count` sectors starting at `lba` into the selected disk's buffer. +pub fn read_sectors(lba: u64, count: u32) -> Result<(), ()> { + dma_sectors(lba, count, ATA_READ_DMA_EXT, false) } /// DMA `count` sectors starting at `lba` out of the selected disk's buffer. pub fn write_sectors(lba: u64, count: u32) -> Result<(), ()> { - let d = active().ok_or(())?; - if lba + count as u64 > d.sectors { - return Err(()); - } - issue(d, ATA_WRITE_DMA_EXT, lba, count, true) + dma_sectors(lba, count, ATA_WRITE_DMA_EXT, true) } /// Pointer to the selected disk's 4 KiB DMA buffer. diff --git a/rsnix/src/fat.rs b/rsnix/src/fat.rs index bad01cd..eda81ed 100644 --- a/rsnix/src/fat.rs +++ b/rsnix/src/fat.rs @@ -167,8 +167,18 @@ fn write_sector(lba: u64, data: &[u8; SECTOR]) -> Result<(), ()> { // --- cluster math ---------------------------------------------------------- +/// Current volume's metadata — only ever reached with a selected volume +/// (the fs layer calls `select_vol` first), so a missing slot is a kernel +/// invariant violation, not a recoverable error. fn v() -> &'static mut Volume { - vols().get_mut(*active_vol()).unwrap() + let i = *active_vol(); + match vols().get_mut(i) { + Some(v) => v, + None => { + crate::serial_println!("fat: internal error — no volume at slot {}", i); + crate::interrupts::halt(); + } + } } fn spc() -> usize { diff --git a/rsnix/src/memory.rs b/rsnix/src/memory.rs index 7c3eda0..e03af8b 100644 --- a/rsnix/src/memory.rs +++ b/rsnix/src/memory.rs @@ -458,7 +458,9 @@ pub fn self_test() -> bool { seed = seed.wrapping_mul(6364136223846793005).wrapping_add(1); let align = 16usize << (seed as usize % 8); seed = seed.wrapping_mul(6364136223846793005).wrapping_add(1); - let layout = Layout::from_size_align(size, align).unwrap(); + let Ok(layout) = Layout::from_size_align(size, align) else { + return false; // impossible: size <= 2048, align <= 2048 + }; let p = ALLOCATOR.alloc(layout); if p.is_null() { break; // heap exhausted — stop @@ -493,7 +495,9 @@ pub fn self_test() -> bool { seed = seed.wrapping_mul(6364136223846793005).wrapping_add(1); let align = 16usize << (seed as usize % 8); seed = seed.wrapping_mul(6364136223846793005).wrapping_add(1); - let layout = Layout::from_size_align(size, align).unwrap(); + let Ok(layout) = Layout::from_size_align(size, align) else { + return false; // impossible: size <= 2048, align <= 2048 + }; let p = ALLOCATOR.alloc(layout); if p.is_null() { return false; // failed to reuse freed space @@ -510,7 +514,9 @@ pub fn self_test() -> bool { if *ptr == 0 { continue; } - let layout = Layout::from_size_align(*size, *align).unwrap(); + let Ok(layout) = Layout::from_size_align(*size, *align) else { + return false; // impossible: sizes/aligns came from the loop above + }; ALLOCATOR.dealloc(*ptr as *mut u8, layout); } let stats = heap_stats(); diff --git a/rsnix/src/pci.rs b/rsnix/src/pci.rs index 21d7536..b9c2265 100644 --- a/rsnix/src/pci.rs +++ b/rsnix/src/pci.rs @@ -46,25 +46,25 @@ fn inl(port: u16) -> u32 { val } -/// Read a dword from a device's config space (`reg` must be dword-aligned). -pub fn config_read(bus: u8, device: u8, func: u8, reg: u8) -> u32 { - let addr = 0x8000_0000 +/// Encode the PCI config-space port address for (bus, dev, fn, reg); +/// `reg` must be dword-aligned. +fn config_addr(bus: u8, device: u8, func: u8, reg: u8) -> u32 { + 0x8000_0000 | ((bus as u32) << 16) | ((device as u32) << 11) | ((func as u32) << 8) - | ((reg as u32) & 0xFC); - outl(CONFIG_ADDRESS, addr); + | ((reg as u32) & 0xFC) +} + +/// Read a dword from a device's config space (`reg` must be dword-aligned). +pub fn config_read(bus: u8, device: u8, func: u8, reg: u8) -> u32 { + outl(CONFIG_ADDRESS, config_addr(bus, device, func, reg)); inl(CONFIG_DATA) } /// Write a dword to a device's config space (`reg` must be dword-aligned). pub fn config_write(bus: u8, device: u8, func: u8, reg: u8, val: u32) { - let addr = 0x8000_0000 - | ((bus as u32) << 16) - | ((device as u32) << 11) - | ((func as u32) << 8) - | ((reg as u32) & 0xFC); - outl(CONFIG_ADDRESS, addr); + outl(CONFIG_ADDRESS, config_addr(bus, device, func, reg)); outl(CONFIG_DATA, val) } @@ -83,7 +83,11 @@ pub fn bar64(bus: u8, device: u8, func: u8, bar_index: u8) -> Option { 0 }; let base = ((hi as u64) << 32) | (lo as u64 & 0xFFFF_FFF0); - if base == 0 { None } else { Some(base) } + if base == 0 { + None + } else { + Some(base) + } } /// Enumerate every device/function present on all buses. diff --git a/rsnix/src/sched.rs b/rsnix/src/sched.rs index 4b10503..6ffa745 100644 --- a/rsnix/src/sched.rs +++ b/rsnix/src/sched.rs @@ -372,7 +372,7 @@ pub fn page_fault(cr2: u64, frame_ptr: usize) -> usize { let rip = unsafe { (frame_ptr as *const IntFrame).read().rip }; let mut buf = [0u8; 96]; let n = { - let mut f = FmtBuf { b: &mut buf, n: 0 }; + let mut f = crate::strbuf::FmtBuf::new(&mut buf); let _ = fmt::write( &mut f, core::format_args!( @@ -386,7 +386,7 @@ pub fn page_fault(cr2: u64, frame_ptr: usize) -> usize { rip ), ); - f.n + f.len() }; abort_current(core::str::from_utf8(&buf[..n]).unwrap_or("task fault")) } @@ -528,17 +528,3 @@ fn idle_main() { } } } - -struct FmtBuf<'a> { - b: &'a mut [u8], - n: usize, -} - -impl fmt::Write for FmtBuf<'_> { - fn write_str(&mut self, s: &str) -> fmt::Result { - let m = s.len().min(self.b.len() - self.n); - self.b[self.n..self.n + m].copy_from_slice(&s.as_bytes()[..m]); - self.n += m; - Ok(()) - } -} diff --git a/rsnix/src/strbuf.rs b/rsnix/src/strbuf.rs index 262720d..0e3ea56 100644 --- a/rsnix/src/strbuf.rs +++ b/rsnix/src/strbuf.rs @@ -69,3 +69,31 @@ impl fmt::Debug for String { write!(f, "{:?}", self.as_str()) } } + +/// Small `fmt::Write` over a byte slice — no allocation, bounded. +/// Shared by the panic handler (task-panic message) and the scheduler +/// (page-fault abort message) so they don't each reinvent it. +pub struct FmtBuf<'a> { + b: &'a mut [u8], + n: usize, +} + +impl<'a> FmtBuf<'a> { + pub fn new(b: &'a mut [u8]) -> Self { + FmtBuf { b, n: 0 } + } + + /// Bytes written so far (clamped at the buffer length). + pub fn len(&self) -> usize { + self.n + } +} + +impl fmt::Write for FmtBuf<'_> { + fn write_str(&mut self, s: &str) -> fmt::Result { + let m = s.len().min(self.b.len() - self.n); + self.b[self.n..self.n + m].copy_from_slice(&s.as_bytes()[..m]); + self.n += m; + Ok(()) + } +} diff --git a/scripts/boot-image.sh b/scripts/boot-image.sh index 913e5cb..e8c093c 100755 --- a/scripts/boot-image.sh +++ b/scripts/boot-image.sh @@ -34,29 +34,8 @@ GUI="" DEBUG="" # ---- args ---- -while [ $# -gt 0 ]; do - case "$1" in - --gui | -g) GUI="-vga std" ;; - --debug | -d) DEBUG="-s -S" ;; - --mem) - shift - MEM="$1" - ;; - --smp) - shift - SMP="$1" - ;; - --help | -h) - head -20 "$0" | grep "^#" | sed 's/^# //' - exit 0 - ;; - *) - echo "unknown: $1" - exit 1 - ;; - esac - shift -done +source "$SCRIPT_DIR/lib-boot-args.sh" +parse_boot_args "$@" # ---- build image if missing ---- if [ ! -f "$IMAGE" ]; then diff --git a/scripts/boot.sh b/scripts/boot.sh index 99fd103..5b8e6fa 100755 --- a/scripts/boot.sh +++ b/scripts/boot.sh @@ -45,33 +45,21 @@ GUI="" DEBUG="" # ---- args ---- -while [ $# -gt 0 ]; do +source "$SCRIPT_DIR/lib-boot-args.sh" +# boot.sh-specific flags live here; everything else is shared. +handle_arg() { case "$1" in - --gui | -g) GUI="-vga std" ;; - --debug | -d) DEBUG="-s -S" ;; --release | -r) PROFILE="release" CARGO_FLAGS="--release" ;; - --mem) - shift - MEM="$1" - ;; - --smp) - shift - SMP="$1" - ;; - --help | -h) - head -20 "$0" | grep "^#" | sed 's/^# //' - exit 0 - ;; *) echo "unknown: $1" exit 1 ;; esac - shift -done +} +parse_boot_args "$@" OUT_DIR="target/x86_64-unknown-uefi/$PROFILE" KERNEL="$OUT_DIR/rsdos.efi" diff --git a/scripts/lib-boot-args.sh b/scripts/lib-boot-args.sh new file mode 100644 index 0000000..ec97500 --- /dev/null +++ b/scripts/lib-boot-args.sh @@ -0,0 +1,38 @@ +#!/usr/bin/env bash +# +# scripts/lib-boot-args.sh — shared CLI parsing for the QEMU boot scripts. +# Sourced, not executed. The caller sets default values for GUI/DEBUG/ +# MEM/SMP first, then calls `parse_boot_args "$@"`. A caller with extra +# flags defines its own `handle_arg()` to intercept them before the +# shared cases run (unknown args still fail loudly). +set -euo pipefail + +parse_boot_args() { + while [ $# -gt 0 ]; do + case "$1" in + --gui | -g) GUI="-vga std" ;; + --debug | -d) DEBUG="-s -S" ;; + --mem) + shift + MEM="$1" + ;; + --smp) + shift + SMP="$1" + ;; + --help | -h) + head -20 "$0" | grep "^#" | sed 's/^# //' + exit 0 + ;; + *) handle_arg "$1" ;; + esac + shift + done +} + +# Default: no extra flags. A script with more defines its own before +# calling parse_boot_args. +handle_arg() { + echo "unknown: $1" + exit 1 +} diff --git a/scripts/test-disk.sh b/scripts/test-disk.sh index 11a9b05..0268fff 100755 --- a/scripts/test-disk.sh +++ b/scripts/test-disk.sh @@ -72,7 +72,11 @@ cargo build --package rsdos --target x86_64-unknown-uefi echo "==> 2/6 deploy to ESP + ensure disk image" mkdir -p "$ESP_DIR/EFI/BOOT" cp target/x86_64-unknown-uefi/debug/rsdos.efi "$ESP_DIR/EFI/BOOT/BOOTX64.EFI" -"$SCRIPT_DIR/mkdisk.sh" $([ "$FRESH" -eq 1 ] && echo --fresh) "$IMG" +if [ "$FRESH" -eq 1 ]; then + "$SCRIPT_DIR/mkdisk.sh" --fresh "$IMG" +else + "$SCRIPT_DIR/mkdisk.sh" "$IMG" +fi echo "==> 3/6 boot QEMU (headless)" kill "$(cat "$QPID" 2>/dev/null)" 2>/dev/null || true diff --git a/shell/src/lib.rs b/shell/src/lib.rs index 617e5a4..4640697 100644 --- a/shell/src/lib.rs +++ b/shell/src/lib.rs @@ -515,7 +515,12 @@ impl Pipeline { } } Filter::Pause => { - let StageState::Pause { shown, screen, cols } = st else { + let StageState::Pause { + shown, + screen, + cols, + } = st + else { unreachable!("stage state does not match filter") }; for l in cur.drain(..) { @@ -871,7 +876,7 @@ fn cmd_cat(p: &Parsed) { if p.args.is_empty() { // Path-less `cat` with no input keyword reads the keyboard // until ^D. - cat_stdin(&mut pipe); + cat_stdin(&mut pipe, None); } else { for path in &p.args { if !cat_file(&mut pipe, path) { @@ -885,7 +890,7 @@ fn cmd_cat(p: &Parsed) { } Input::Til(marker) => { if p.args.is_empty() { - cat_stdin_until(&mut pipe, marker); + cat_stdin(&mut pipe, Some(marker)); } else { // `cat TIL ` — the paths are the input // source, truncated at the first marker line. @@ -904,34 +909,21 @@ fn cmd_cat(p: &Parsed) { pipe.end(); } -fn cat_stdin(pipe: &mut Pipeline) -> bool { +/// Read lines from the tty until ^C/^D (or an exact marker line, when +/// `marker` is Some — the marker itself is not part of the input). +fn cat_stdin(pipe: &mut Pipeline, marker: Option<&str>) -> bool { loop { match tty::read_line() { LineInput::Line(l) => { - if !pipe.line(l.as_str()) { - return false; - } - } - LineInput::Interrupted => return false, // ^C aborts the stream - LineInput::Eof => return true, // ^D = EOF - } - } -} - -fn cat_stdin_until(pipe: &mut Pipeline, marker: &str) -> bool { - loop { - match tty::read_line() { - LineInput::Line(l) => { - // Marker line is not part of the input. - if l.as_str() == marker { + if marker.is_some() && l.as_str() == marker.unwrap() { return true; } if !pipe.line(l.as_str()) { return false; } } - LineInput::Interrupted => return false, - LineInput::Eof => return true, + LineInput::Interrupted => return false, // ^C aborts the stream + LineInput::Eof => return true, // ^D = EOF } } } @@ -1030,25 +1022,6 @@ struct HexCtx<'a> { alive: bool, } -fn format_row(addr: u64, row: &[u8; 16]) -> String { - let mut out = String::new(); - use core::fmt::Write as _; - let _ = core::write!(out, "{:08x} ", addr); - for &b in row { - let _ = core::write!(out, "{:02x} ", b); - } - out.push_str(" |"); - for &b in row { - out.push(if b.is_ascii_graphic() || b == b' ' { - b as char - } else { - '.' - }); - } - out.push('|'); - out -} - fn format_partial(addr: u64, row: &[u8]) -> String { let mut out = String::new(); use core::fmt::Write as _; @@ -1081,7 +1054,7 @@ impl HexCtx<'_> { self.row[self.row_len] = b; self.row_len += 1; if self.row_len == 16 { - let line = format_row(self.pos, &self.row); + let line = format_partial(self.pos, &self.row); self.alive = self.pipe.line(&line); self.pos += 16; self.row_len = 0; @@ -1106,7 +1079,7 @@ fn cmd_hexdump(p: &Parsed) { match &p.input { Input::None => { if p.args.is_empty() { - hexdump_stdin(&mut pipe); + hexdump_stdin(&mut pipe, None); } else { for path in &p.args { if !hexdump_file(&mut pipe, path) { @@ -1120,7 +1093,7 @@ fn cmd_hexdump(p: &Parsed) { } Input::Til(marker) => { if p.args.is_empty() { - hexdump_stdin_until(&mut pipe, marker); + hexdump_stdin(&mut pipe, Some(marker)); } else { // `hexdump TIL ` — the paths are the input // source, truncated at the first marker line. @@ -1176,34 +1149,9 @@ fn hexdump_file(pipe: &mut Pipeline, path: &str) -> bool { ctx.alive } -fn hexdump_stdin(pipe: &mut Pipeline) { - let mut ctx = HexCtx { - pipe, - pos: 0, - row: [0u8; 16], - row_len: 0, - alive: true, - }; - loop { - match tty::read_line() { - LineInput::Line(l) => { - ctx.feed(l.as_str().as_bytes()); - if !ctx.alive { - break; - } - ctx.feed(b"\n"); - if !ctx.alive { - break; - } - } - LineInput::Interrupted => break, - LineInput::Eof => break, - } - } - ctx.flush(); -} - -fn hexdump_stdin_until(pipe: &mut Pipeline, marker: &str) { +/// Hex-dump lines from the tty until ^C/^D (or an exact marker line, +/// when `marker` is Some — the marker itself is not dumped). +fn hexdump_stdin(pipe: &mut Pipeline, marker: Option<&str>) { let mut ctx = HexCtx { pipe, pos: 0, @@ -1214,7 +1162,7 @@ fn hexdump_stdin_until(pipe: &mut Pipeline, marker: &str) { loop { match tty::read_line() { LineInput::Line(l) => { - if l.as_str() == marker { + if marker.is_some() && l.as_str() == marker.unwrap() { break; } ctx.feed(l.as_str().as_bytes()); -- cgit v1.2.3