* [pbs-devel] [PATCH 2/2] tape: improve EOT error handling
2021-04-12 9:32 [pbs-devel] [PATCH 1/2] sgutils2: use thiserror to derive Error Dietmar Maurer
@ 2021-04-12 9:32 ` Dietmar Maurer
2021-04-12 10:04 ` [pbs-devel] [PATCH 1/2] sgutils2: use thiserror to derive Error Dominik Csapak
1 sibling, 0 replies; 3+ messages in thread
From: Dietmar Maurer @ 2021-04-12 9:32 UTC (permalink / raw)
To: pbs-devel
---
src/api2/tape/drive.rs | 29 ++++-------
src/api2/tape/restore.rs | 27 +++++++---
src/bin/proxmox-tape.rs | 18 +++++--
src/tape/drive/lto/mod.rs | 12 ++---
src/tape/drive/lto/sg_tape.rs | 31 ++++++-----
src/tape/drive/mod.rs | 31 ++++++++---
src/tape/drive/virtual_tape.rs | 22 ++++----
src/tape/file_formats/blocked_reader.rs | 69 ++++++++++++-------------
src/tape/helpers/emulate_tape_reader.rs | 19 ++++---
src/tape/tape_read.rs | 10 ++--
10 files changed, 152 insertions(+), 116 deletions(-)
diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs
index ca45bb69..c1fd72e1 100644
--- a/src/api2/tape/drive.rs
+++ b/src/api2/tape/drive.rs
@@ -10,7 +10,6 @@ use proxmox::{
identity,
list_subdirs_api_method,
tools::Uuid,
- sys::error::SysError,
api::{
api,
section_config::SectionConfigData,
@@ -60,6 +59,7 @@ use crate::{
Inventory,
MediaCatalog,
MediaId,
+ BlockReadError,
lock_media_set,
lock_media_pool,
lock_unassigned_media_pool,
@@ -528,15 +528,11 @@ pub fn label_media(
drive.rewind()?;
match drive.read_next_file() {
- Ok(Some(_file)) => bail!("media is not empty (format it first)"),
- Ok(None) => { /* EOF mark at BOT, assume tape is empty */ },
+ Ok(_reader) => bail!("media is not empty (format it first)"),
+ Err(BlockReadError::EndOfFile) => { /* EOF mark at BOT, assume tape is empty */ },
+ Err(BlockReadError::EndOfStream) => { /* tape is empty */ },
Err(err) => {
- println!("TEST {:?}", err);
- if err.is_errno(nix::errno::Errno::ENOSPC) || err.is_errno(nix::errno::Errno::EIO) {
- /* assume tape is empty */
- } else {
- bail!("media read error - {}", err);
- }
+ bail!("media read error - {}", err);
}
}
@@ -1091,18 +1087,15 @@ fn barcode_label_media_worker(
drive.rewind()?;
match drive.read_next_file() {
- Ok(Some(_file)) => {
+ Ok(_reader) => {
worker.log(format!("media '{}' is not empty (format it first)", label_text));
continue;
}
- Ok(None) => { /* EOF mark at BOT, assume tape is empty */ },
- Err(err) => {
- if err.is_errno(nix::errno::Errno::ENOSPC) || err.is_errno(nix::errno::Errno::EIO) {
- /* assume tape is empty */
- } else {
- worker.warn(format!("media '{}' read error (maybe not empty - format it first)", label_text));
- continue;
- }
+ Err(BlockReadError::EndOfFile) => { /* EOF mark at BOT, assume tape is empty */ },
+ Err(BlockReadError::EndOfStream) => { /* tape is empty */ },
+ Err(_err) => {
+ worker.warn(format!("media '{}' read error (maybe not empty - format it first)", label_text));
+ continue;
}
}
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 78aac73c..afce3449 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -70,6 +70,7 @@ use crate::{
tape::{
TAPE_STATUS_DIR,
TapeRead,
+ BlockReadError,
MediaId,
MediaSet,
MediaCatalog,
@@ -418,12 +419,19 @@ pub fn restore_media(
loop {
let current_file_number = drive.current_file_number()?;
- let reader = match drive.read_next_file()? {
- None => {
+ let reader = match drive.read_next_file() {
+ Err(BlockReadError::EndOfFile) => {
+ task_log!(worker, "skip unexpected filemark at pos {}", current_file_number);
+ continue;
+ }
+ Err(BlockReadError::EndOfStream) => {
task_log!(worker, "detected EOT after {} files", current_file_number);
break;
}
- Some(reader) => reader,
+ Err(BlockReadError::Error(err)) => {
+ return Err(err.into());
+ }
+ Ok(reader) => reader,
};
restore_archive(worker, reader, current_file_number, target, &mut catalog, verbose)?;
@@ -811,12 +819,19 @@ pub fn fast_catalog_restore(
let current_file_number = drive.current_file_number()?;
{ // limit reader scope
- let mut reader = match drive.read_next_file()? {
- None => {
+ let mut reader = match drive.read_next_file() {
+ Err(BlockReadError::EndOfFile) => {
+ task_log!(worker, "skip unexpected filemark at pos {}", current_file_number);
+ continue;
+ }
+ Err(BlockReadError::EndOfStream) => {
task_log!(worker, "detected EOT after {} files", current_file_number);
break;
}
- Some(reader) => reader,
+ Err(BlockReadError::Error(err)) => {
+ return Err(err.into());
+ }
+ Ok(reader) => reader,
};
let header: MediaContentHeader = unsafe { reader.read_le_value()? };
diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs
index 1d23b71c..3b3bbafa 100644
--- a/src/bin/proxmox-tape.rs
+++ b/src/bin/proxmox-tape.rs
@@ -43,6 +43,7 @@ use proxmox_backup::{
media_pool::complete_pool_name,
},
tape::{
+ BlockReadError,
drive::{
open_drive,
lock_tape_device,
@@ -587,12 +588,19 @@ fn debug_scan(mut param: Value) -> Result<(), Error> {
loop {
let file_number = drive.current_file_number()?;
- match drive.read_next_file()? {
- None => {
- println!("EOD");
+ match drive.read_next_file() {
+ Err(BlockReadError::EndOfFile) => {
+ println!("filemark");
continue;
- },
- Some(mut reader) => {
+ }
+ Err(BlockReadError::EndOfStream) => {
+ println!("got EOT");
+ return Ok(());
+ }
+ Err(BlockReadError::Error(err)) => {
+ return Err(err.into());
+ }
+ Ok(mut reader) => {
println!("got file number {}", file_number);
let header: Result<MediaContentHeader, _> = unsafe { reader.read_le_value() };
diff --git a/src/tape/drive/lto/mod.rs b/src/tape/drive/lto/mod.rs
index 9dc85ac9..642c3cc7 100644
--- a/src/tape/drive/lto/mod.rs
+++ b/src/tape/drive/lto/mod.rs
@@ -43,6 +43,7 @@ use crate::{
tape::{
TapeRead,
TapeWrite,
+ BlockReadError,
drive::{
TapeDriver,
},
@@ -320,16 +321,9 @@ impl TapeDriver for LtoTapeHandle {
self.sg_tape.format_media(fast)
}
- fn read_next_file<'a>(&'a mut self) -> Result<Option<Box<dyn TapeRead + 'a>>, std::io::Error> {
+ fn read_next_file<'a>(&'a mut self) -> Result<Box<dyn TapeRead + 'a>, BlockReadError> {
let reader = self.sg_tape.open_reader()?;
- let handle = match reader {
- Some(reader) => {
- let reader: Box<dyn TapeRead> = Box::new(reader);
- Some(reader)
- }
- None => None,
- };
-
+ let handle: Box<dyn TapeRead> = Box::new(reader);
Ok(handle)
}
diff --git a/src/tape/drive/lto/sg_tape.rs b/src/tape/drive/lto/sg_tape.rs
index 9f59b321..3f73e025 100644
--- a/src/tape/drive/lto/sg_tape.rs
+++ b/src/tape/drive/lto/sg_tape.rs
@@ -32,7 +32,7 @@ use crate::{
},
tape::{
BlockRead,
- BlockReadStatus,
+ BlockReadError,
BlockWrite,
file_formats::{
BlockedWriter,
@@ -526,11 +526,13 @@ impl SgTape {
}
}
- fn read_block(&mut self, buffer: &mut [u8]) -> Result<BlockReadStatus, std::io::Error> {
+ fn read_block(&mut self, buffer: &mut [u8]) -> Result<usize, BlockReadError> {
let transfer_len = buffer.len();
if transfer_len > 0xFFFFFF {
- proxmox::io_bail!("read failed - buffer too large");
+ return Err(BlockReadError::Error(
+ proxmox::io_format_err!("read failed - buffer too large")
+ ));
}
let mut sg_raw = SgRaw::new(&mut self.file, 0)
@@ -549,21 +551,25 @@ impl SgTape {
let data = match sg_raw.do_in_command(&cmd, buffer) {
Ok(data) => data,
Err(ScsiError::Sense(SenseInfo { sense_key: 0, asc: 0, ascq: 1 })) => {
- return Ok(BlockReadStatus::EndOfFile);
+ return Err(BlockReadError::EndOfFile);
}
Err(ScsiError::Sense(SenseInfo { sense_key: 8, asc: 0, ascq: 5 })) => {
- return Ok(BlockReadStatus::EndOfStream);
+ return Err(BlockReadError::EndOfStream);
}
Err(err) => {
- proxmox::io_bail!("read failed - {}", err);
+ return Err(BlockReadError::Error(
+ proxmox::io_format_err!("read failed - {}", err)
+ ));
}
};
if data.len() != transfer_len {
- proxmox::io_bail!("read failed - unexpected block len ({} != {})", data.len(), buffer.len())
+ return Err(BlockReadError::Error(
+ proxmox::io_format_err!("read failed - unexpected block len ({} != {})", data.len(), buffer.len())
+ ));
}
- Ok(BlockReadStatus::Ok(transfer_len))
+ Ok(transfer_len)
}
pub fn open_writer(&mut self) -> BlockedWriter<SgTapeWriter> {
@@ -571,12 +577,9 @@ impl SgTape {
BlockedWriter::new(writer)
}
- pub fn open_reader(&mut self) -> Result<Option<BlockedReader<SgTapeReader>>, std::io::Error> {
+ pub fn open_reader(&mut self) -> Result<BlockedReader<SgTapeReader>, BlockReadError> {
let reader = SgTapeReader::new(self);
- match BlockedReader::open(reader)? {
- Some(reader) => Ok(Some(reader)),
- None => Ok(None),
- }
+ BlockedReader::open(reader)
}
/// Set important drive options
@@ -702,7 +705,7 @@ impl <'a> SgTapeReader<'a> {
impl <'a> BlockRead for SgTapeReader<'a> {
- fn read_block(&mut self, buffer: &mut [u8]) -> Result<BlockReadStatus, std::io::Error> {
+ fn read_block(&mut self, buffer: &mut [u8]) -> Result<usize, BlockReadError> {
self.sg_tape.read_block(buffer)
}
}
diff --git a/src/tape/drive/mod.rs b/src/tape/drive/mod.rs
index 0d9e3db3..fd8f503d 100644
--- a/src/tape/drive/mod.rs
+++ b/src/tape/drive/mod.rs
@@ -44,6 +44,7 @@ use crate::{
tape::{
TapeWrite,
TapeRead,
+ BlockReadError,
MediaId,
drive::lto::TapeAlertFlags,
file_formats::{
@@ -86,7 +87,7 @@ pub trait TapeDriver {
fn format_media(&mut self, fast: bool) -> Result<(), Error>;
/// Read/Open the next file
- fn read_next_file<'a>(&'a mut self) -> Result<Option<Box<dyn TapeRead + 'a>>, std::io::Error>;
+ fn read_next_file<'a>(&'a mut self) -> Result<Box<dyn TapeRead + 'a>, BlockReadError>;
/// Write/Append a new file
fn write_file<'a>(&'a mut self) -> Result<Box<dyn TapeWrite + 'a>, std::io::Error>;
@@ -132,9 +133,17 @@ pub trait TapeDriver {
self.rewind()?;
let label = {
- let mut reader = match self.read_next_file()? {
- None => return Ok((None, None)), // tape is empty
- Some(reader) => reader,
+ let mut reader = match self.read_next_file() {
+ Err(BlockReadError::EndOfStream) => {
+ return Ok((None, None)); // tape is empty
+ }
+ Err(BlockReadError::EndOfFile) => {
+ bail!("got unexpected filemark at BOT");
+ }
+ Err(BlockReadError::Error(err)) => {
+ return Err(err.into());
+ }
+ Ok(reader) => reader,
};
let header: MediaContentHeader = unsafe { reader.read_le_value()? };
@@ -155,9 +164,17 @@ pub trait TapeDriver {
let mut media_id = MediaId { label, media_set_label: None };
// try to read MediaSet label
- let mut reader = match self.read_next_file()? {
- None => return Ok((Some(media_id), None)),
- Some(reader) => reader,
+ let mut reader = match self.read_next_file() {
+ Err(BlockReadError::EndOfStream) => {
+ return Ok((Some(media_id), None));
+ }
+ Err(BlockReadError::EndOfFile) => {
+ bail!("got unexpected filemark after label");
+ }
+ Err(BlockReadError::Error(err)) => {
+ return Err(err.into());
+ }
+ Ok(reader) => reader,
};
let header: MediaContentHeader = unsafe { reader.read_le_value()? };
diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
index a852056a..f7ebc0bb 100644
--- a/src/tape/drive/virtual_tape.rs
+++ b/src/tape/drive/virtual_tape.rs
@@ -15,6 +15,7 @@ use crate::{
tape::{
TapeWrite,
TapeRead,
+ BlockReadError,
changer::{
MediaChange,
MtxStatus,
@@ -261,18 +262,18 @@ impl TapeDriver for VirtualTapeHandle {
}
- fn read_next_file(&mut self) -> Result<Option<Box<dyn TapeRead>>, io::Error> {
+ fn read_next_file(&mut self) -> Result<Box<dyn TapeRead>, BlockReadError> {
let mut status = self.load_status()
- .map_err(|err| io::Error::new(io::ErrorKind::Other, err.to_string()))?;
+ .map_err(|err| BlockReadError::Error(io::Error::new(io::ErrorKind::Other, err.to_string())))?;
match status.current_tape {
Some(VirtualTapeStatus { ref name, ref mut pos }) => {
let index = self.load_tape_index(name)
- .map_err(|err| io::Error::new(io::ErrorKind::Other, err.to_string()))?;
+ .map_err(|err| BlockReadError::Error(io::Error::new(io::ErrorKind::Other, err.to_string())))?;
if *pos >= index.files {
- return Ok(None); // EOM
+ return Err(BlockReadError::EndOfStream);
}
let path = self.tape_file_path(name, *pos);
@@ -282,16 +283,15 @@ impl TapeDriver for VirtualTapeHandle {
*pos += 1;
self.store_status(&status)
- .map_err(|err| io::Error::new(io::ErrorKind::Other, err.to_string()))?;
+ .map_err(|err| BlockReadError::Error(io::Error::new(io::ErrorKind::Other, err.to_string())))?;
let reader = EmulateTapeReader::new(file);
-
- match BlockedReader::open(reader)? {
- Some(reader) => Ok(Some(Box::new(reader))),
- None => Ok(None),
- }
+ let reader = BlockedReader::open(reader)?;
+ Ok(Box::new(reader))
+ }
+ None => {
+ return Err(BlockReadError::Error(proxmox::io_format_err!("drive is empty (no tape loaded).")));
}
- None => proxmox::io_bail!("drive is empty (no tape loaded)."),
}
}
diff --git a/src/tape/file_formats/blocked_reader.rs b/src/tape/file_formats/blocked_reader.rs
index 3df84a1b..936eb24b 100644
--- a/src/tape/file_formats/blocked_reader.rs
+++ b/src/tape/file_formats/blocked_reader.rs
@@ -3,7 +3,7 @@ use std::io::Read;
use crate::tape::{
TapeRead,
BlockRead,
- BlockReadStatus,
+ BlockReadError,
file_formats::{
PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0,
BlockHeader,
@@ -37,15 +37,13 @@ impl <R: BlockRead> BlockedReader<R> {
/// Create a new BlockedReader instance.
///
- /// This tries to read the first block, and returns None if we are
- /// at EOT.
- pub fn open(mut reader: R) -> Result<Option<Self>, std::io::Error> {
+ /// This tries to read the first block. Please inspect the error
+ /// to detect EOF and EOT.
+ pub fn open(mut reader: R) -> Result<Self, BlockReadError> {
let mut buffer = BlockHeader::new();
- if !Self::read_block_frame(&mut buffer, &mut reader)? {
- return Ok(None);
- }
+ Self::read_block_frame(&mut buffer, &mut reader)?;
let (_size, found_end_marker) = Self::check_buffer(&buffer, 0)?;
@@ -58,7 +56,7 @@ impl <R: BlockRead> BlockedReader<R> {
got_eod = true;
}
- Ok(Some(Self {
+ Ok(Self {
reader,
buffer,
found_end_marker,
@@ -67,7 +65,7 @@ impl <R: BlockRead> BlockedReader<R> {
seq_nr: 1,
read_error: false,
read_pos: 0,
- }))
+ })
}
fn check_buffer(buffer: &BlockHeader, seq_nr: u32) -> Result<(usize, bool), std::io::Error> {
@@ -95,7 +93,7 @@ impl <R: BlockRead> BlockedReader<R> {
Ok((size, found_end_marker))
}
- fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result<bool, std::io::Error> {
+ fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result<(), BlockReadError> {
let data = unsafe {
std::slice::from_raw_parts_mut(
@@ -104,38 +102,28 @@ impl <R: BlockRead> BlockedReader<R> {
)
};
- match reader.read_block(data) {
- Ok(BlockReadStatus::Ok(bytes)) => {
- if bytes != BlockHeader::SIZE {
- proxmox::io_bail!("got wrong block size");
- }
- Ok(true)
- }
- Ok(BlockReadStatus::EndOfFile) => {
- Ok(false)
- }
- Ok(BlockReadStatus::EndOfStream) => {
- return Err(std::io::Error::from_raw_os_error(nix::errno::Errno::ENOSPC as i32));
- }
- Err(err) => {
- Err(err)
- }
+ let bytes = reader.read_block(data)?;
+
+ if bytes != BlockHeader::SIZE {
+ return Err(proxmox::io_format_err!("got wrong block size").into());
}
+
+ Ok(())
}
fn consume_eof_marker(reader: &mut R) -> Result<(), std::io::Error> {
let mut tmp_buf = [0u8; 512]; // use a small buffer for testing EOF
match reader.read_block(&mut tmp_buf) {
- Ok(BlockReadStatus::Ok(_)) => {
+ Ok(_) => {
proxmox::io_bail!("detected tape block after block-stream end marker");
}
- Ok(BlockReadStatus::EndOfFile) => {
+ Err(BlockReadError::EndOfFile) => {
return Ok(());
}
- Ok(BlockReadStatus::EndOfStream) => {
+ Err(BlockReadError::EndOfStream) => {
proxmox::io_bail!("got unexpected end of tape");
}
- Err(err) => {
+ Err(BlockReadError::Error(err)) => {
return Err(err);
}
}
@@ -143,13 +131,22 @@ impl <R: BlockRead> BlockedReader<R> {
fn read_block(&mut self) -> Result<usize, std::io::Error> {
- if !Self::read_block_frame(&mut self.buffer, &mut self.reader)? {
- self.got_eod = true;
- self.read_pos = self.buffer.payload.len();
- if !self.found_end_marker {
- proxmox::io_bail!("detected tape stream without end marker");
+ match Self::read_block_frame(&mut self.buffer, &mut self.reader) {
+ Ok(()) => { /* ok */ }
+ Err(BlockReadError::EndOfFile) => {
+ self.got_eod = true;
+ self.read_pos = self.buffer.payload.len();
+ if !self.found_end_marker {
+ proxmox::io_bail!("detected tape stream without end marker");
+ }
+ return Ok(0); // EOD
+ }
+ Err(BlockReadError::EndOfStream) => {
+ proxmox::io_bail!("got unexpected end of tape");
+ }
+ Err(BlockReadError::Error(err)) => {
+ return Err(err);
}
- return Ok(0); // EOD
}
let (size, found_end_marker) = Self::check_buffer(&self.buffer, self.seq_nr)?;
diff --git a/src/tape/helpers/emulate_tape_reader.rs b/src/tape/helpers/emulate_tape_reader.rs
index fbb19028..92c975f9 100644
--- a/src/tape/helpers/emulate_tape_reader.rs
+++ b/src/tape/helpers/emulate_tape_reader.rs
@@ -4,7 +4,7 @@ use proxmox::tools::io::ReadExt;
use crate::tape::{
BlockRead,
- BlockReadStatus,
+ BlockReadError,
file_formats::PROXMOX_TAPE_BLOCK_SIZE,
};
@@ -24,22 +24,27 @@ impl <R: Read> EmulateTapeReader<R> {
}
impl <R: Read> BlockRead for EmulateTapeReader<R> {
- fn read_block(&mut self, buffer: &mut [u8]) -> Result<BlockReadStatus, std::io::Error> {
+ fn read_block(&mut self, buffer: &mut [u8]) -> Result<usize, BlockReadError> {
if self.got_eof {
- proxmox::io_bail!("detected read after EOF!");
+ return Err(BlockReadError::Error(proxmox::io_format_err!("detected read after EOF!")));
}
match self.reader.read_exact_or_eof(buffer)? {
false => {
self.got_eof = true;
- Ok(BlockReadStatus::EndOfFile)
+ Err(BlockReadError::EndOfFile)
}
true => {
// test buffer len after EOF test (to allow EOF test with small buffers in BufferedReader)
if buffer.len() != PROXMOX_TAPE_BLOCK_SIZE {
- proxmox::io_bail!("EmulateTapeReader: read_block with wrong block size ({} != {})",
- buffer.len(), PROXMOX_TAPE_BLOCK_SIZE);
+ return Err(BlockReadError::Error(
+ proxmox::io_format_err!(
+ "EmulateTapeReader: read_block with wrong block size ({} != {})",
+ buffer.len(),
+ PROXMOX_TAPE_BLOCK_SIZE,
+ )
+ ));
}
- Ok(BlockReadStatus::Ok(buffer.len()))
+ Ok(buffer.len())
}
}
}
diff --git a/src/tape/tape_read.rs b/src/tape/tape_read.rs
index 7cbe2b8b..c617ec61 100644
--- a/src/tape/tape_read.rs
+++ b/src/tape/tape_read.rs
@@ -15,14 +15,18 @@ pub trait TapeRead: Read {
fn has_end_marker(&self) -> Result<bool, std::io::Error>;
}
-pub enum BlockReadStatus {
- Ok(usize),
+#[derive(thiserror::Error, Debug)]
+pub enum BlockReadError {
+ #[error("{0}")]
+ Error(#[from] std::io::Error),
+ #[error("end of file")]
EndOfFile,
+ #[error("end of data stream")]
EndOfStream,
}
/// Read streams of blocks
pub trait BlockRead {
/// Read the next block (whole buffer)
- fn read_block(&mut self, buffer: &mut [u8]) -> Result<BlockReadStatus, std::io::Error>;
+ fn read_block(&mut self, buffer: &mut [u8]) -> Result<usize, BlockReadError>;
}
--
2.20.1
^ permalink raw reply [flat|nested] 3+ messages in thread