tape: improve EOT error handling

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(_reader) => bail!("media is not empty (format it first)"),
-                Ok(None) => { /* EOF mark at BOT, assume tape is empty */ },
+                Err(BlockReadError::EndOfFile) => { /* EOF mark at BOT, assume tape is empty */ },
+                Err(BlockReadError::EndOfStream) => { /* tape is empty */ },
                 Err(err) => {
-                    println!("TEST {:?}", err);
+                    bail!("media read error - {}", 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);
-                    }
                 }
             }
 
@@ -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(BlockReadError::EndOfFile) => { /* EOF mark at BOT, assume tape is empty */ },
-            Err(err) => {
+            Err(BlockReadError::EndOfStream) => { /* tape is empty */ },
-                if err.is_errno(nix::errno::Errno::ENOSPC) || err.is_errno(nix::errno::Errno::EIO) {
+            Err(_err) => {
-                    /* assume tape is empty */
+                worker.warn(format!("media '{}' read error (maybe not empty - format it first)", label_text));
-                } else {
+                continue;
-                    worker.warn(format!("media '{}' read error (maybe not empty - format it first)", label_text));
-                    continue;
-                }
             }
         }
 

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()? {
+        let reader = match drive.read_next_file() {
-            None => {
+            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()? {
+            let mut reader = match drive.read_next_file() {
-                None => {
+                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()? };

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()? {
+        match drive.read_next_file() {
-            None => {
+            Err(BlockReadError::EndOfFile) => {
-                println!("EOD");
+                println!("filemark number {}", file_number);
                 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() };

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 {
+        let handle: Box<dyn TapeRead> = Box::new(reader);
-            Some(reader) => {
-                let reader: Box<dyn TapeRead> = Box::new(reader);
-                Some(reader)
-            }
-            None => None,
-        };
-
         Ok(handle)
     }
 

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)? {
+        BlockedReader::open(reader)
-            Some(reader) => Ok(Some(reader)),
-            None => Ok(None),
-        }
     }
 
     /// 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)
     }
 }

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()? {
+            let mut reader = match self.read_next_file() {
-                None => return Ok((None, None)), // tape is empty
+                Err(BlockReadError::EndOfStream) => {
-                Some(reader) => reader,
+                    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()? {
+        let mut reader = match self.read_next_file() {
-            None => return Ok((Some(media_id), None)),
+            Err(BlockReadError::EndOfStream) => {
-            Some(reader) => reader,
+                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()? };

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);
-
+                let reader = BlockedReader::open(reader)?;
-                match BlockedReader::open(reader)? {
+                Ok(Box::new(reader))
-                    Some(reader) => Ok(Some(Box::new(reader))),
+            }
-                    None => Ok(None),
+            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)."),
         }
     }
 

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
+    /// This tries to read the first block. Please inspect the error
-    /// at EOT.
+    /// to detect EOF and EOT.
-    pub fn open(mut reader: R) -> Result<Option<Self>, std::io::Error> {
+    pub fn open(mut reader: R) -> Result<Self, BlockReadError> {
 
         let mut buffer = BlockHeader::new();
 
-        if !Self::read_block_frame(&mut buffer, &mut reader)? {
+        Self::read_block_frame(&mut buffer, &mut reader)?;
-            return Ok(None);
-        }
 
         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) {
+        let bytes = reader.read_block(data)?;
-            Ok(BlockReadStatus::Ok(bytes)) => {
+
-                if bytes != BlockHeader::SIZE {
+        if bytes != BlockHeader::SIZE {
-                    proxmox::io_bail!("got wrong block size");
+            return Err(proxmox::io_format_err!("got wrong block size").into());
-                }
-                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)
-            }
         }
+
+        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)? {
+        match Self::read_block_frame(&mut self.buffer, &mut self.reader) {
-            self.got_eod = true;
+            Ok(()) => { /* ok */ }
-            self.read_pos = self.buffer.payload.len();
+            Err(BlockReadError::EndOfFile) => {
-            if !self.found_end_marker {
+                self.got_eod = true;
-                proxmox::io_bail!("detected tape stream without end marker");
+                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)?;

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 ({} != {})",
+                    return Err(BlockReadError::Error(
-                                      buffer.len(), PROXMOX_TAPE_BLOCK_SIZE);
+                        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())
             }
         }
     }

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 {
+#[derive(thiserror::Error, Debug)]
-    Ok(usize),
+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>;
 }