From 2dc503e07e6d2d40a71be498afb0202c7bc7dc07 Mon Sep 17 00:00:00 2001 From: Gabriel Kaszewski Date: Sun, 18 Oct 2026 03:41:14 +0200 Subject: [PATCH] refactor(storage-server): add SectorId, FileName, PackedPath newtypes, eliminate raw primitives and DRY violations --- .../storage-server/src/adapters/mock_block.rs | 10 ++-- .../storage-server/src/adapters/mock_fs.rs | 38 +++++------- .../storage-server/src/block_device.rs | 17 +++++- .../servers/storage-server/src/filesystem.rs | 48 ++++++++++++--- crates/servers/storage-server/src/protocol.rs | 59 +++++++++++++++---- crates/servers/storage-server/src/server.rs | 49 ++++++--------- .../storage-server/src/tests/protocol.rs | 11 +++- .../storage-server/src/tests/server.rs | 41 ++++--------- 8 files changed, 162 insertions(+), 111 deletions(-) diff --git a/crates/servers/storage-server/src/adapters/mock_block.rs b/crates/servers/storage-server/src/adapters/mock_block.rs index 15fb976..3ccde2a 100644 --- a/crates/servers/storage-server/src/adapters/mock_block.rs +++ b/crates/servers/storage-server/src/adapters/mock_block.rs @@ -1,4 +1,4 @@ -use crate::block_device::BlockDevice; +use crate::block_device::{BlockDevice, SectorId}; const SECTOR_SIZE: usize = 512; const MAX_SECTORS: usize = 64; @@ -26,8 +26,8 @@ impl BlockDevice for MockBlockDevice { self.sectors } - fn read_sector(&mut self, sector: u32, buf: &mut [u8]) -> bool { - let idx = sector as usize; + fn read_sector(&mut self, sector: SectorId, buf: &mut [u8]) -> bool { + let idx = sector.raw() as usize; if idx >= self.sectors || buf.len() < SECTOR_SIZE { return false; } @@ -35,8 +35,8 @@ impl BlockDevice for MockBlockDevice { true } - fn write_sector(&mut self, sector: u32, buf: &[u8]) -> bool { - let idx = sector as usize; + fn write_sector(&mut self, sector: SectorId, buf: &[u8]) -> bool { + let idx = sector.raw() as usize; if idx >= self.sectors || buf.len() < SECTOR_SIZE { return false; } diff --git a/crates/servers/storage-server/src/adapters/mock_fs.rs b/crates/servers/storage-server/src/adapters/mock_fs.rs index cec7134..6019942 100644 --- a/crates/servers/storage-server/src/adapters/mock_fs.rs +++ b/crates/servers/storage-server/src/adapters/mock_fs.rs @@ -1,24 +1,22 @@ -use crate::filesystem::{DirEntry, FileHandle, FileSystem}; +use crate::filesystem::{DirEntry, FileHandle, FileName, FileSystem}; const MAX_FILES: usize = 16; const MAX_FILE_SIZE: usize = 256; struct FileEntry { - name: [u8; 32], - name_len: u8, + name: FileName, data: [u8; MAX_FILE_SIZE], size: usize, open: bool, } impl FileEntry { - fn empty() -> Self { + fn new(name: FileName) -> Self { Self { - name: [0; 32], - name_len: 0, + name, data: [0; MAX_FILE_SIZE], size: 0, - open: false, + open: true, } } } @@ -37,9 +35,9 @@ impl MockFileSystem { } fn find_by_name(&self, path: &[u8]) -> Option { - for i in 0..self.count { + for i in 0..MAX_FILES { if let Some(entry) = &self.files[i] { - if &entry.name[..entry.name_len as usize] == path { + if entry.name == *path { return Some(i); } } @@ -82,16 +80,14 @@ impl FileSystem for MockFileSystem { fn list_dir(&mut self, _path: &[u8], entries: &mut [DirEntry]) -> Option { let mut count = 0; - for i in 0..self.count { + for i in 0..MAX_FILES { if count >= entries.len() { break; } if let Some(file) = &self.files[i] { - let mut entry = DirEntry::empty(); - let len = file.name_len as usize; - entry.name[..len].copy_from_slice(&file.name[..len]); - entry.name_len = file.name_len; - entry.size = file.size as u32; - entry.is_dir = false; - entries[count] = entry; + entries[count] = DirEntry { + name: file.name, + size: file.size, + is_dir: false, + }; count += 1; } } @@ -99,15 +95,11 @@ impl FileSystem for MockFileSystem { } fn create(&mut self, path: &[u8]) -> Option { - if self.count >= MAX_FILES || path.len() > 32 { + if self.count >= MAX_FILES { return None; } let idx = self.count; - let mut entry = FileEntry::empty(); - entry.name[..path.len()].copy_from_slice(path); - entry.name_len = path.len() as u8; - entry.open = true; - self.files[idx] = Some(entry); + self.files[idx] = Some(FileEntry::new(FileName::from_bytes(path))); self.count += 1; Some(FileHandle::new(idx as u16)) } diff --git a/crates/servers/storage-server/src/block_device.rs b/crates/servers/storage-server/src/block_device.rs index adbbfb7..9e5fb21 100644 --- a/crates/servers/storage-server/src/block_device.rs +++ b/crates/servers/storage-server/src/block_device.rs @@ -1,6 +1,19 @@ +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct SectorId(u32); + +impl SectorId { + pub const fn new(id: u32) -> Self { + Self(id) + } + + pub const fn raw(&self) -> u32 { + self.0 + } +} + pub trait BlockDevice { fn sector_size(&self) -> usize; fn sector_count(&self) -> usize; - fn read_sector(&mut self, sector: u32, buf: &mut [u8]) -> bool; - fn write_sector(&mut self, sector: u32, buf: &[u8]) -> bool; + fn read_sector(&mut self, sector: SectorId, buf: &mut [u8]) -> bool; + fn write_sector(&mut self, sector: SectorId, buf: &[u8]) -> bool; } diff --git a/crates/servers/storage-server/src/filesystem.rs b/crates/servers/storage-server/src/filesystem.rs index 773f96d..ae2896a 100644 --- a/crates/servers/storage-server/src/filesystem.rs +++ b/crates/servers/storage-server/src/filesystem.rs @@ -11,27 +11,57 @@ impl FileHandle { } } +pub const MAX_FILE_NAME: usize = 32; + +#[derive(Debug, Clone, Copy)] +pub struct FileName { + bytes: [u8; MAX_FILE_NAME], + len: u8, +} + +impl FileName { + pub fn empty() -> Self { + Self { bytes: [0; MAX_FILE_NAME], len: 0 } + } + + pub fn from_bytes(src: &[u8]) -> Self { + let mut name = Self::empty(); + let len = src.len().min(MAX_FILE_NAME); + name.bytes[..len].copy_from_slice(&src[..len]); + name.len = len as u8; + name + } + + pub fn as_bytes(&self) -> &[u8] { + &self.bytes[..self.len as usize] + } + + pub fn len(&self) -> usize { + self.len as usize + } +} + +impl PartialEq<[u8]> for FileName { + fn eq(&self, other: &[u8]) -> bool { + self.as_bytes() == other + } +} + #[derive(Debug, Clone, Copy)] pub struct DirEntry { - pub name: [u8; 32], - pub name_len: u8, - pub size: u32, + pub name: FileName, + pub size: usize, pub is_dir: bool, } impl DirEntry { pub fn empty() -> Self { Self { - name: [0; 32], - name_len: 0, + name: FileName::empty(), size: 0, is_dir: false, } } - - pub fn name_bytes(&self) -> &[u8] { - &self.name[..self.name_len as usize] - } } pub trait FileSystem { diff --git a/crates/servers/storage-server/src/protocol.rs b/crates/servers/storage-server/src/protocol.rs index 267de0b..87a3be1 100644 --- a/crates/servers/storage-server/src/protocol.rs +++ b/crates/servers/storage-server/src/protocol.rs @@ -1,15 +1,50 @@ use server_framework::{Decode, Encode}; use crate::filesystem::FileHandle; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct PackedPath { + word0: usize, + word1: usize, +} + +impl PackedPath { + pub fn from_bytes(src: &[u8]) -> Self { + let mut w0 = [0u8; 8]; + let mut w1 = [0u8; 8]; + for (i, &b) in src.iter().enumerate() { + if i < 8 { w0[i] = b; } + else if i < 16 { w1[i - 8] = b; } + } + Self { + word0: usize::from_le_bytes(w0), + word1: usize::from_le_bytes(w1), + } + } + + pub fn from_words(word0: usize, word1: usize) -> Self { + Self { word0, word1 } + } + + pub fn to_bytes(&self) -> [u8; 16] { + let mut buf = [0u8; 16]; + buf[..8].copy_from_slice(&self.word0.to_le_bytes()); + buf[8..16].copy_from_slice(&self.word1.to_le_bytes()); + buf + } + + pub fn word0(&self) -> usize { self.word0 } + pub fn word1(&self) -> usize { self.word1 } +} + #[derive(Debug, PartialEq)] pub enum StorageRequest { - Open { path_word0: usize, path_word1: usize }, + Open { path: PackedPath }, Read { handle: FileHandle, max_bytes: usize }, Write { handle: FileHandle, data_word: usize }, Close { handle: FileHandle }, - ListDir { path_word0: usize, path_word1: usize }, - Create { path_word0: usize, path_word1: usize }, - Delete { path_word0: usize, path_word1: usize }, + ListDir { path: PackedPath }, + Create { path: PackedPath }, + Delete { path: PackedPath }, FileSize { handle: FileHandle }, } @@ -28,13 +63,13 @@ pub enum StorageResponse { impl Encode for StorageRequest { fn encode(&self) -> [usize; 4] { match self { - StorageRequest::Open { path_word0, path_word1 } => [0, *path_word0, *path_word1, 0], + StorageRequest::Open { path } => [0, path.word0(), path.word1(), 0], StorageRequest::Read { handle, max_bytes } => [1, handle.raw() as usize, *max_bytes, 0], StorageRequest::Write { handle, data_word } => [2, handle.raw() as usize, *data_word, 0], StorageRequest::Close { handle } => [3, handle.raw() as usize, 0, 0], - StorageRequest::ListDir { path_word0, path_word1 } => [4, *path_word0, *path_word1, 0], - StorageRequest::Create { path_word0, path_word1 } => [5, *path_word0, *path_word1, 0], - StorageRequest::Delete { path_word0, path_word1 } => [6, *path_word0, *path_word1, 0], + StorageRequest::ListDir { path } => [4, path.word0(), path.word1(), 0], + StorageRequest::Create { path } => [5, path.word0(), path.word1(), 0], + StorageRequest::Delete { path } => [6, path.word0(), path.word1(), 0], StorageRequest::FileSize { handle } => [7, handle.raw() as usize, 0, 0], } } @@ -43,13 +78,13 @@ impl Encode for StorageRequest { impl Decode for StorageRequest { fn decode(words: [usize; 4]) -> Option { match words[0] { - 0 => Some(StorageRequest::Open { path_word0: words[1], path_word1: words[2] }), + 0 => Some(StorageRequest::Open { path: PackedPath::from_words(words[1], words[2]) }), 1 => Some(StorageRequest::Read { handle: FileHandle::new(words[1] as u16), max_bytes: words[2] }), 2 => Some(StorageRequest::Write { handle: FileHandle::new(words[1] as u16), data_word: words[2] }), 3 => Some(StorageRequest::Close { handle: FileHandle::new(words[1] as u16) }), - 4 => Some(StorageRequest::ListDir { path_word0: words[1], path_word1: words[2] }), - 5 => Some(StorageRequest::Create { path_word0: words[1], path_word1: words[2] }), - 6 => Some(StorageRequest::Delete { path_word0: words[1], path_word1: words[2] }), + 4 => Some(StorageRequest::ListDir { path: PackedPath::from_words(words[1], words[2]) }), + 5 => Some(StorageRequest::Create { path: PackedPath::from_words(words[1], words[2]) }), + 6 => Some(StorageRequest::Delete { path: PackedPath::from_words(words[1], words[2]) }), 7 => Some(StorageRequest::FileSize { handle: FileHandle::new(words[1] as u16) }), _ => None, } diff --git a/crates/servers/storage-server/src/server.rs b/crates/servers/storage-server/src/server.rs index 85039f9..38bf9c5 100644 --- a/crates/servers/storage-server/src/server.rs +++ b/crates/servers/storage-server/src/server.rs @@ -1,6 +1,6 @@ use server_framework::Server; -use crate::filesystem::{FileHandle, FileSystem}; +use crate::filesystem::{FileSystem, DirEntry}; use crate::protocol::{StorageRequest, StorageResponse}; pub struct StorageServer { @@ -15,19 +15,6 @@ impl StorageServer { pub fn filesystem(&self) -> &F { &self.fs } - - fn path_from_words(w0: usize, w1: usize) -> [u8; 16] { - let mut path = [0u8; 16]; - let b0 = w0.to_le_bytes(); - let b1 = w1.to_le_bytes(); - path[..8].copy_from_slice(&b0); - path[8..16].copy_from_slice(&b1); - path - } - - fn path_len(path: &[u8]) -> usize { - path.iter().position(|&b| b == 0).unwrap_or(path.len()) - } } impl Server for StorageServer { @@ -36,10 +23,10 @@ impl Server for StorageServer { fn handle(&mut self, request: StorageRequest) -> StorageResponse { match request { - StorageRequest::Open { path_word0, path_word1 } => { - let path = Self::path_from_words(path_word0, path_word1); - let len = Self::path_len(&path); - match self.fs.open(&path[..len]) { + StorageRequest::Open { path } => { + let buf = path.to_bytes(); + let len = buf.iter().position(|&b| b == 0).unwrap_or(16); + match self.fs.open(&buf[..len]) { Some(h) => StorageResponse::Handle(h), None => StorageResponse::NotFound, } @@ -63,27 +50,27 @@ impl Server for StorageServer { self.fs.close(handle); StorageResponse::Ok } - StorageRequest::ListDir { path_word0, path_word1 } => { - let path = Self::path_from_words(path_word0, path_word1); - let len = Self::path_len(&path); - let mut entries = [crate::filesystem::DirEntry::empty(); 16]; - match self.fs.list_dir(&path[..len], &mut entries) { + StorageRequest::ListDir { path } => { + let buf = path.to_bytes(); + let len = buf.iter().position(|&b| b == 0).unwrap_or(16); + let mut entries = [DirEntry::empty(); 16]; + match self.fs.list_dir(&buf[..len], &mut entries) { Some(count) => StorageResponse::DirEntries { count }, None => StorageResponse::NotFound, } } - StorageRequest::Create { path_word0, path_word1 } => { - let path = Self::path_from_words(path_word0, path_word1); - let len = Self::path_len(&path); - match self.fs.create(&path[..len]) { + StorageRequest::Create { path } => { + let buf = path.to_bytes(); + let len = buf.iter().position(|&b| b == 0).unwrap_or(16); + match self.fs.create(&buf[..len]) { Some(h) => StorageResponse::Handle(h), None => StorageResponse::Error, } } - StorageRequest::Delete { path_word0, path_word1 } => { - let path = Self::path_from_words(path_word0, path_word1); - let len = Self::path_len(&path); - if self.fs.delete(&path[..len]) { + StorageRequest::Delete { path } => { + let buf = path.to_bytes(); + let len = buf.iter().position(|&b| b == 0).unwrap_or(16); + if self.fs.delete(&buf[..len]) { StorageResponse::Ok } else { StorageResponse::NotFound diff --git a/crates/servers/storage-server/src/tests/protocol.rs b/crates/servers/storage-server/src/tests/protocol.rs index 93c3467..8874597 100644 --- a/crates/servers/storage-server/src/tests/protocol.rs +++ b/crates/servers/storage-server/src/tests/protocol.rs @@ -4,7 +4,8 @@ use server_framework::{Decode, Encode}; #[test] fn open_roundtrip() { - let req = StorageRequest::Open { path_word0: 0x61626364, path_word1: 0 }; + let path = PackedPath::from_bytes(b"test.txt"); + let req = StorageRequest::Open { path }; assert_eq!(StorageRequest::decode(req.encode()), Some(req)); } @@ -62,3 +63,11 @@ fn all_response_variants() { fn invalid_request_returns_none() { assert!(StorageRequest::decode([99, 0, 0, 0]).is_none()); } + +#[test] +fn packed_path_roundtrip() { + let path = PackedPath::from_bytes(b"hello"); + let buf = path.to_bytes(); + assert_eq!(&buf[..5], b"hello"); + assert_eq!(buf[5], 0); +} diff --git a/crates/servers/storage-server/src/tests/server.rs b/crates/servers/storage-server/src/tests/server.rs index 845e397..f144dd6 100644 --- a/crates/servers/storage-server/src/tests/server.rs +++ b/crates/servers/storage-server/src/tests/server.rs @@ -1,5 +1,4 @@ use crate::adapters::mock_fs::MockFileSystem; -use crate::filesystem::FileHandle; use crate::protocol::*; use crate::server::StorageServer; use server_framework::Server; @@ -8,42 +7,33 @@ fn test_server() -> StorageServer { StorageServer::new(MockFileSystem::new()) } -fn path_words(s: &[u8]) -> (usize, usize) { - let mut w0 = [0u8; 8]; - let mut w1 = [0u8; 8]; - for (i, &b) in s.iter().enumerate() { - if i < 8 { w0[i] = b; } - else if i < 16 { w1[i - 8] = b; } - } - (usize::from_le_bytes(w0), usize::from_le_bytes(w1)) +fn path(s: &[u8]) -> PackedPath { + PackedPath::from_bytes(s) } #[test] fn create_and_open() { let mut server = test_server(); - let (w0, w1) = path_words(b"test.txt"); - let resp = server.handle(StorageRequest::Create { path_word0: w0, path_word1: w1 }); + let resp = server.handle(StorageRequest::Create { path: path(b"test.txt") }); assert!(matches!(resp, StorageResponse::Handle(_))); - let resp = server.handle(StorageRequest::Open { path_word0: w0, path_word1: w1 }); + let resp = server.handle(StorageRequest::Open { path: path(b"test.txt") }); assert!(matches!(resp, StorageResponse::Handle(_))); } #[test] fn open_nonexistent_returns_not_found() { let mut server = test_server(); - let (w0, w1) = path_words(b"nope.txt"); - let resp = server.handle(StorageRequest::Open { path_word0: w0, path_word1: w1 }); + let resp = server.handle(StorageRequest::Open { path: path(b"nope.txt") }); assert_eq!(resp, StorageResponse::NotFound); } #[test] fn write_then_read() { let mut server = test_server(); - let (w0, w1) = path_words(b"data"); - let handle = match server.handle(StorageRequest::Create { path_word0: w0, path_word1: w1 }) { + let handle = match server.handle(StorageRequest::Create { path: path(b"data") }) { StorageResponse::Handle(h) => h, _ => panic!("expected handle"), }; @@ -58,35 +48,30 @@ fn write_then_read() { #[test] fn delete_file() { let mut server = test_server(); - let (w0, w1) = path_words(b"del.txt"); - server.handle(StorageRequest::Create { path_word0: w0, path_word1: w1 }); - let resp = server.handle(StorageRequest::Delete { path_word0: w0, path_word1: w1 }); + server.handle(StorageRequest::Create { path: path(b"del.txt") }); + let resp = server.handle(StorageRequest::Delete { path: path(b"del.txt") }); assert_eq!(resp, StorageResponse::Ok); - let resp = server.handle(StorageRequest::Open { path_word0: w0, path_word1: w1 }); + let resp = server.handle(StorageRequest::Open { path: path(b"del.txt") }); assert_eq!(resp, StorageResponse::NotFound); } #[test] fn list_dir() { let mut server = test_server(); - let (w0a, w1a) = path_words(b"a.txt"); - let (w0b, w1b) = path_words(b"b.txt"); - server.handle(StorageRequest::Create { path_word0: w0a, path_word1: w1a }); - server.handle(StorageRequest::Create { path_word0: w0b, path_word1: w1b }); + server.handle(StorageRequest::Create { path: path(b"a.txt") }); + server.handle(StorageRequest::Create { path: path(b"b.txt") }); - let (w0r, w1r) = path_words(b"/"); - let resp = server.handle(StorageRequest::ListDir { path_word0: w0r, path_word1: w1r }); + let resp = server.handle(StorageRequest::ListDir { path: path(b"/") }); assert_eq!(resp, StorageResponse::DirEntries { count: 2 }); } #[test] fn close_succeeds() { let mut server = test_server(); - let (w0, w1) = path_words(b"c.txt"); - let handle = match server.handle(StorageRequest::Create { path_word0: w0, path_word1: w1 }) { + let handle = match server.handle(StorageRequest::Create { path: path(b"c.txt") }) { StorageResponse::Handle(h) => h, _ => panic!("expected handle"), };