From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B55BE722F3 for ; Mon, 12 Apr 2021 11:32:57 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AA39A1C710 for ; Mon, 12 Apr 2021 11:32:57 +0200 (CEST) Received: from elsa.proxmox.com (unknown [94.136.29.99]) by firstgate.proxmox.com (Proxmox) with ESMTP id E7FEF1C6E6 for ; Mon, 12 Apr 2021 11:32:54 +0200 (CEST) Received: by elsa.proxmox.com (Postfix, from userid 0) id CD616AEAFB0; Mon, 12 Apr 2021 11:32:48 +0200 (CEST) From: Dietmar Maurer To: pbs-devel@lists.proxmox.com Date: Mon, 12 Apr 2021 11:32:47 +0200 Message-Id: <20210412093247.14191-2-dietmar@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20210412093247.14191-1-dietmar@proxmox.com> References: <20210412093247.14191-1-dietmar@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 1 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RDNS_NONE 1.274 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH 2/2] tape: improve EOT error handling X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Apr 2021 09:32:57 -0000 --- 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 = 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>, std::io::Error> { + fn read_next_file<'a>(&'a mut self) -> Result, BlockReadError> { let reader = self.sg_tape.open_reader()?; - let handle = match reader { - Some(reader) => { - let reader: Box = Box::new(reader); - Some(reader) - } - None => None, - }; - + let handle: Box = 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 { + fn read_block(&mut self, buffer: &mut [u8]) -> Result { 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 { @@ -571,12 +577,9 @@ impl SgTape { BlockedWriter::new(writer) } - pub fn open_reader(&mut self) -> Result>, std::io::Error> { + pub fn open_reader(&mut self) -> Result, 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 { + fn read_block(&mut self, buffer: &mut [u8]) -> Result { 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>, std::io::Error>; + fn read_next_file<'a>(&'a mut self) -> Result, BlockReadError>; /// Write/Append a new file fn write_file<'a>(&'a mut self) -> Result, 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>, io::Error> { + fn read_next_file(&mut self) -> Result, 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 BlockedReader { /// 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, 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 { 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 BlockedReader { got_eod = true; } - Ok(Some(Self { + Ok(Self { reader, buffer, found_end_marker, @@ -67,7 +65,7 @@ impl BlockedReader { 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 BlockedReader { Ok((size, found_end_marker)) } - fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result { + 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 BlockedReader { ) }; - 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 BlockedReader { fn read_block(&mut self) -> Result { - 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 EmulateTapeReader { } impl BlockRead for EmulateTapeReader { - fn read_block(&mut self, buffer: &mut [u8]) -> Result { + fn read_block(&mut self, buffer: &mut [u8]) -> Result { 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; } -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; + fn read_block(&mut self, buffer: &mut [u8]) -> Result; } -- 2.20.1