0.18 round 1 polish: address libfreemkv code-review findings

Applies must-fix + in-scope should-fix items from the round-1 code
review:

- M1: FileSectorSource::open takes &Path (was &str — non-UTF-8 panic)
- M2: drop FileSectorSource's BufReader (defeated by absolute seeks)
- M3: WritebackFile Drop impl finalises the writeback pipeline
- M4: Pipeline::finish preserves panic payload in error message
- M5: pes::Stream is left without a : Send supertrait — concrete
  in-tree impls (MkvStream, M2tsStream) hold Box<dyn Read> /
  Box<dyn Write> trait objects that aren't Send, so the simple
  trait tightening would cascade into a wider Send audit. Per the
  review's escape clause the FrameSource blanket impl keeps its
  T: Send bound and the constraint is documented loudly there.
- S6: document Pipeline::send post-Flow::Stop semantics
- S9: truncate stale Stream docs (E9001/E9000 was runtime-only)
- S10: document WritebackPipeline.fd lifetime invariant
- S11: pub use pes::Stream as PesStream to disambiguate from
  disc::Stream codec enum at crate root
- S12: rename DEFAULT_DEPTH → DEFAULT_PIPELINE_DEPTH; add
  WRITE_THROUGH_DEPTH constant
- N14: drop Halt's Default derive (redundant with Halt::new)
- N17: Pipeline::spawn propagates thread-spawn error instead of expect
- N19: deprecation since = "0.18.0" (was "0.18.0-dev", non-conventional)
- N21: rename Apply enum to Flow

Deferred to follow-up commits: SectorReader/SectorSource competition
(migration commit), WritebackFile::create/open orphans (migration
commit), AACS round-trip test (design doc defers), various nits.

See (internal)/memory/0_18_redesign.md.

Single contributor: MattJackson.
This commit is contained in:
MattJackson
2026-05-09 09:52:25 -07:00
parent 8d67790d2e
commit 925c30686b
9 changed files with 182 additions and 110 deletions
+14 -13
View File
@@ -8,7 +8,7 @@
//! mux.
use std::fs::{File, OpenOptions};
use std::io::{BufReader, Read, Seek, SeekFrom, Write};
use std::io::{Read, Seek, SeekFrom, Write};
use std::path::Path;
use crate::error::{Error, Result};
@@ -18,10 +18,14 @@ use super::{SectorReader, SectorSink};
/// SectorSource backed by a file (ISO image).
///
/// Seeks to `lba * 2048`, reads `count * 2048` bytes per call. The
/// underlying file is wrapped in a 4 MiB `BufReader` so adjacent
/// small reads coalesce into single syscalls.
/// file is held directly: every `read_sectors` call performs an
/// absolute seek, so a wrapping `BufReader` would have its buffer
/// invalidated on every call (its internal cursor moves with the
/// `Seek` impl) — pure overhead. Callers that benefit from buffered
/// reads should compose their own `BufReader` at the `read_sectors`
/// granularity they care about.
pub struct FileSectorSource {
file: BufReader<File>,
file: File,
capacity: u32,
}
@@ -30,21 +34,18 @@ impl FileSectorSource {
/// from `metadata().len() / 2048`. Returns
/// [`Error::IsoTooLarge`] if the file would exceed the 32-bit
/// LBA address space (~8 TB).
pub fn open(path: &str) -> std::io::Result<Self> {
pub fn open(path: &Path) -> std::io::Result<Self> {
let file = File::open(path)?;
let len = file.metadata()?.len();
let sectors = len / 2048;
if sectors > u32::MAX as u64 {
return Err(Error::IsoTooLarge {
path: path.to_string(),
path: path.to_string_lossy().into_owned(),
}
.into());
}
let capacity = sectors as u32;
Ok(Self {
file: BufReader::with_capacity(4 * 1024 * 1024, file),
capacity,
})
Ok(Self { file, capacity })
}
}
@@ -173,7 +174,7 @@ mod tests {
sink.write_sectors(2, &payload).unwrap();
Box::new(sink).finish().unwrap();
let mut src = FileSectorSource::open(path.to_str().unwrap()).unwrap();
let mut src = FileSectorSource::open(&path).unwrap();
assert_eq!(src.capacity_sectors(), 4);
let mut got = [0u8; 2048];
@@ -200,7 +201,7 @@ mod tests {
sink.write_sectors(0, &payload).unwrap();
Box::new(sink).finish().unwrap();
let mut src = FileSectorSource::open(path.to_str().unwrap()).unwrap();
let mut src = FileSectorSource::open(&path).unwrap();
assert_eq!(src.capacity_sectors(), 8);
let mut got = vec![0u8; 8 * 2048];
@@ -226,7 +227,7 @@ mod tests {
sink.write_sectors(1, &pat_b).unwrap();
Box::new(sink).finish().unwrap();
let mut src = FileSectorSource::open(path.to_str().unwrap()).unwrap();
let mut src = FileSectorSource::open(&path).unwrap();
assert_eq!(src.capacity_sectors(), 4);
let mut got = [0u8; 2048];