[pbs-devel] [PATCH 02/11] tape: introduce trait BlockWrite

[Dietmar Maurer <dietmar@proxmox.com>]

---
 src/tape/drive/linux_tape.rs            | 126 +++++++++++++++---------
 src/tape/drive/virtual_tape.rs          |   3 +-
 src/tape/file_formats/blocked_writer.rs |  37 +++++--
 src/tape/helpers/emulate_tape_writer.rs |  37 +++----
 src/tape/tape_write.rs                  |  61 ++----------
 5 files changed, 141 insertions(+), 123 deletions(-)

diff --git a/src/tape/drive/linux_tape.rs b/src/tape/drive/linux_tape.rs
index 9e86d0a3..d556f0f5 100644
--- a/src/tape/drive/linux_tape.rs
+++ b/src/tape/drive/linux_tape.rs
@@ -1,7 +1,7 @@
 //! Driver for Linux SCSI tapes
 
 use std::fs::{OpenOptions, File};
-use std::io::Read;
+use std::io::{Read, Write};
 use std::os::unix::fs::OpenOptionsExt;
 use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 use std::convert::TryFrom;
@@ -9,8 +9,10 @@ use std::convert::TryFrom;
 use anyhow::{bail, format_err, Error};
 use nix::fcntl::{fcntl, FcntlArg, OFlag};
 
-use proxmox::sys::error::SysResult;
-use proxmox::tools::Uuid;
+use proxmox::{
+    tools::Uuid,
+    sys::error::{SysError, SysResult},
+};
 
 use crate::{
     config,
@@ -25,6 +27,7 @@ use crate::{
         LinuxDriveAndMediaStatus,
     },
     tape::{
+        BlockWrite,
         BlockRead,
         BlockReadStatus,
         TapeRead,
@@ -257,10 +260,9 @@ impl LinuxTapeHandle {
         Ok(())
     }
 
-    /// Write a single EOF mark
-    pub fn write_eof_mark(&self) -> Result<(), Error> {
-        tape_write_eof_mark(&self.file)?;
-        Ok(())
+    /// Write a single EOF mark without flushing buffers
+    pub fn write_eof_mark(&mut self) -> Result<(), std::io::Error> {
+        tape_write_eof_mark(&mut self.file)
     }
 
     /// Set the drive's block length to the value specified.
@@ -519,10 +521,8 @@ impl TapeDriver for LinuxTapeHandle {
 
     fn write_file<'a>(&'a mut self) -> Result<Box<dyn TapeWrite + 'a>, std::io::Error> {
 
-        let handle = TapeWriterHandle {
-            writer: BlockedWriter::new(&mut self.file),
-        };
-
+        let writer = LinuxTapeWriter::new(&mut self.file);
+        let handle = BlockedWriter::new(writer);
         Ok(Box::new(handle))
     }
 
@@ -545,27 +545,27 @@ impl TapeDriver for LinuxTapeHandle {
 
         self.set_encryption(None)?;
 
-        let mut handle = TapeWriterHandle {
-            writer: BlockedWriter::new(&mut self.file),
-        };
+        { // limit handle scope
+            let mut handle = self.write_file()?;
 
-        let mut value = serde_json::to_value(media_set_label)?;
-        if media_set_label.encryption_key_fingerprint.is_some() {
-            match key_config {
-                Some(key_config) => {
-                    value["key-config"] = serde_json::to_value(key_config)?;
-                }
-                None => {
-                    bail!("missing encryption key config");
+            let mut value = serde_json::to_value(media_set_label)?;
+            if media_set_label.encryption_key_fingerprint.is_some() {
+                match key_config {
+                    Some(key_config) => {
+                        value["key-config"] = serde_json::to_value(key_config)?;
+                    }
+                    None => {
+                        bail!("missing encryption key config");
+                    }
                 }
             }
-        }
 
-        let raw = serde_json::to_string_pretty(&value)?;
+            let raw = serde_json::to_string_pretty(&value)?;
 
-        let header = MediaContentHeader::new(PROXMOX_BACKUP_MEDIA_SET_LABEL_MAGIC_1_0, raw.len() as u32);
-        handle.write_header(&header, raw.as_bytes())?;
-        handle.finish(false)?;
+            let header = MediaContentHeader::new(PROXMOX_BACKUP_MEDIA_SET_LABEL_MAGIC_1_0, raw.len() as u32);
+            handle.write_header(&header, raw.as_bytes())?;
+            handle.finish(false)?;
+        }
 
         self.sync()?; // sync data to tape
 
@@ -655,7 +655,7 @@ impl TapeDriver for LinuxTapeHandle {
 }
 
 /// Write a single EOF mark without flushing buffers
-fn tape_write_eof_mark(file: &File) -> Result<(), std::io::Error> {
+fn tape_write_eof_mark(file: &mut File) -> Result<(), std::io::Error> {
 
     let cmd = mtop { mt_op: MTCmd::MTWEOFI, mt_count: 1 };
 
@@ -745,29 +745,67 @@ pub fn read_tapedev_options(file: &File) -> Result<SetDrvBufferOptions, Error> {
 }
 
 
-/// like BlockedWriter, but writes EOF mark on finish
-pub struct TapeWriterHandle<'a> {
-    writer: BlockedWriter<&'a mut File>,
+struct LinuxTapeWriter<'a> {
+    /// Assumes that 'file' is a linux tape device.
+    file: &'a mut File,
 }
 
-impl TapeWrite for TapeWriterHandle<'_> {
-
-    fn write_all(&mut self, data: &[u8]) -> Result<bool, std::io::Error> {
-        self.writer.write_all(data)
+impl <'a> LinuxTapeWriter<'a> {
+    pub fn new(file: &'a mut File) -> Self {
+        Self { file }
     }
+}
 
-    fn bytes_written(&self) -> usize {
-        self.writer.bytes_written()
-    }
+impl <'a> BlockWrite for LinuxTapeWriter<'a> {
 
-    fn finish(&mut self, incomplete: bool) -> Result<bool, std::io::Error> {
-        let leof = self.writer.finish(incomplete)?;
-        tape_write_eof_mark(self.writer.writer_ref_mut())?;
-        Ok(leof)
+    /// Write a single block to a linux tape device
+    ///
+    /// EOM Behaviour on Linux: When the end of medium early warning is
+    /// encountered, the current write is finished and the number of bytes
+    /// is returned. The next write returns -1 and errno is set to
+    /// ENOSPC. To enable writing a trailer, the next write is allowed to
+    /// proceed and, if successful, the number of bytes is returned. After
+    /// this, -1 and the number of bytes are alternately returned until
+    /// the physical end of medium (or some other error) is encountered.
+    ///
+    /// See: https://github.com/torvalds/linux/blob/master/Documentation/scsi/st.rst
+    ///
+    /// On success, this returns if we en countered a EOM condition.
+    fn write_block(&mut self, data: &[u8]) -> Result<bool, std::io::Error> {
+
+        let mut leof = false;
+
+        loop {
+            match self.file.write(data) {
+                Ok(count) if count == data.len() => return Ok(leof),
+                Ok(count) if count > 0 => {
+                    proxmox::io_bail!(
+                        "short block write ({} < {}). Tape drive uses wrong block size.",
+                        count, data.len());
+                }
+                Ok(_) => { // count is 0 here, assume EOT
+                    return Err(std::io::Error::from_raw_os_error(nix::errno::Errno::ENOSPC as i32));
+                }
+                // handle interrupted system call
+                Err(err) if err.kind() == std::io::ErrorKind::Interrupted => {
+                    continue;
+                }
+                // detect and handle LEOM (early warning)
+                Err(err) if err.is_errno(nix::errno::Errno::ENOSPC) => {
+                    if leof {
+                        return Err(err);
+                    } else {
+                        leof = true;
+                        continue; // next write will succeed
+                    }
+                }
+                Err(err) => return Err(err),
+            }
+        }
     }
 
-    fn logical_end_of_media(&self) -> bool {
-        self.writer.logical_end_of_media()
+    fn write_filemark(&mut self) -> Result<(), std::io::Error> {
+        tape_write_eof_mark(&mut self.file)
     }
 }
 
diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
index 2fc9fdea..54e0887f 100644
--- a/src/tape/drive/virtual_tape.rs
+++ b/src/tape/drive/virtual_tape.rs
@@ -276,8 +276,7 @@ impl TapeDriver for VirtualTapeHandle {
                     free_space = self.max_size - used_space;
                 }
 
-                let writer = Box::new(file);
-                let writer = Box::new(EmulateTapeWriter::new(writer, free_space));
+                let writer = EmulateTapeWriter::new(file, free_space);
                 let writer = Box::new(BlockedWriter::new(writer));
 
                 Ok(writer)
diff --git a/src/tape/file_formats/blocked_writer.rs b/src/tape/file_formats/blocked_writer.rs
index 961b2ed2..33fa2955 100644
--- a/src/tape/file_formats/blocked_writer.rs
+++ b/src/tape/file_formats/blocked_writer.rs
@@ -1,10 +1,8 @@
-use std::io::Write;
-
 use proxmox::tools::vec;
 
 use crate::tape::{
     TapeWrite,
-    tape_device_write_block,
+    BlockWrite,
     file_formats::{
         BlockHeader,
         BlockHeaderFlags,
@@ -16,16 +14,27 @@ use crate::tape::{
 /// This type implement 'TapeWrite'. Data written is assembled to
 /// equally sized blocks (see 'BlockHeader'), which are then written
 /// to the underlying writer.
-pub struct BlockedWriter<W> {
+pub struct BlockedWriter<W: BlockWrite> {
     writer: W,
     buffer: Box<BlockHeader>,
     buffer_pos: usize,
     seq_nr: u32,
     logical_end_of_media: bool,
     bytes_written: usize,
+    wrote_eof: bool,
+}
+
+impl <W: BlockWrite> Drop for BlockedWriter<W> {
+
+    // Try to make sure to end the file with a filemark
+    fn drop(&mut self) {
+        if !self.wrote_eof {
+            let _ = self.writer.write_filemark();
+        }
+    }
 }
 
-impl <W: Write> BlockedWriter<W> {
+impl <W: BlockWrite> BlockedWriter<W> {
 
     /// Allow access to underlying writer
     pub fn writer_ref_mut(&mut self) -> &mut W {
@@ -41,6 +50,7 @@ impl <W: Write> BlockedWriter<W> {
             seq_nr: 0,
             logical_end_of_media: false,
             bytes_written: 0,
+            wrote_eof: false,
         }
     }
 
@@ -52,7 +62,16 @@ impl <W: Write> BlockedWriter<W> {
                 BlockHeader::SIZE,
             )
         };
-        tape_device_write_block(writer, data)
+        writer.write_block(data)
+    }
+
+    fn write_eof(&mut self) -> Result<(), std::io::Error> {
+        if self.wrote_eof {
+            proxmox::io_bail!("BlockedWriter: detected multiple EOF writes");
+        }
+        self.wrote_eof = true;
+
+        self.writer.write_filemark()
     }
 
     fn write(&mut self, data: &[u8]) -> Result<usize, std::io::Error> {
@@ -85,7 +104,7 @@ impl <W: Write> BlockedWriter<W> {
 
 }
 
-impl <W: Write> TapeWrite for BlockedWriter<W> {
+impl <W: BlockWrite> TapeWrite for BlockedWriter<W> {
 
     fn write_all(&mut self, mut data: &[u8]) -> Result<bool, std::io::Error> {
         while !data.is_empty() {
@@ -113,7 +132,9 @@ impl <W: Write> TapeWrite for BlockedWriter<W> {
         self.buffer.set_seq_nr(self.seq_nr);
         self.seq_nr += 1;
         self.bytes_written += BlockHeader::SIZE;
-        Self::write_block(&self.buffer, &mut self.writer)
+        let leom = Self::write_block(&self.buffer, &mut self.writer)?;
+        self.write_eof()?;
+        Ok(leom)
     }
 
     /// Returns if the writer already detected the logical end of media
diff --git a/src/tape/helpers/emulate_tape_writer.rs b/src/tape/helpers/emulate_tape_writer.rs
index b385d6b3..eb4f29d7 100644
--- a/src/tape/helpers/emulate_tape_writer.rs
+++ b/src/tape/helpers/emulate_tape_writer.rs
@@ -1,6 +1,9 @@
 use std::io::{self, Write};
 
-use crate::tape::file_formats::PROXMOX_TAPE_BLOCK_SIZE;
+use crate::tape::{
+    BlockWrite,
+    file_formats::PROXMOX_TAPE_BLOCK_SIZE,
+};
 
 /// Emulate tape write behavior on a normal Writer
 ///
@@ -11,7 +14,7 @@ pub struct EmulateTapeWriter<W> {
     block_nr: usize,
     max_blocks: usize,
     writer: W,
-    leom_sent: bool,
+    wrote_eof: bool,
 }
 
 impl <W: Write> EmulateTapeWriter<W> {
@@ -27,16 +30,16 @@ impl <W: Write> EmulateTapeWriter<W> {
 
         Self {
             block_nr: 0,
-            leom_sent: false,
+            wrote_eof: false,
             writer,
             max_blocks,
         }
     }
 }
 
-impl <W: Write> Write for EmulateTapeWriter<W> {
+impl <W: Write> BlockWrite for EmulateTapeWriter<W> {
 
-    fn write(&mut self, buffer: &[u8]) -> Result<usize, io::Error> {
+    fn write_block(&mut self, buffer: &[u8]) -> Result<bool, io::Error> {
 
         if buffer.len() != PROXMOX_TAPE_BLOCK_SIZE {
             proxmox::io_bail!("EmulateTapeWriter: got write with wrong block size ({} != {}",
@@ -47,22 +50,22 @@ impl <W: Write> Write for EmulateTapeWriter<W> {
             return Err(io::Error::from_raw_os_error(nix::errno::Errno::ENOSPC as i32));
         }
 
-        if self.block_nr >= self.max_blocks {
-            if !self.leom_sent {
-                self.leom_sent = true;
-                return Err(io::Error::from_raw_os_error(nix::errno::Errno::ENOSPC as i32));
-            } else {
-                self.leom_sent = false;
-            }
-        }
-
         self.writer.write_all(buffer)?;
         self.block_nr += 1;
 
-        Ok(buffer.len())
+        if self.block_nr > self.max_blocks {
+            Ok(true)
+        } else {
+            Ok(false)
+        }
     }
 
-    fn flush(&mut self) -> Result<(), io::Error> {
-        proxmox::io_bail!("EmulateTapeWriter does not support flush");
+    fn write_filemark(&mut self) -> Result<(), std::io::Error> {
+        if self.wrote_eof {
+            proxmox::io_bail!("EmulateTapeWriter: detected multiple EOF writes");
+        }
+        // do nothing, just record the call
+        self.wrote_eof = true;
+        Ok(())
     }
 }
diff --git a/src/tape/tape_write.rs b/src/tape/tape_write.rs
index 8a3d4fd6..593d1a29 100644
--- a/src/tape/tape_write.rs
+++ b/src/tape/tape_write.rs
@@ -1,9 +1,5 @@
-use std::io::Write;
-
 use endian_trait::Endian;
 
-use proxmox::sys::error::SysError;
-
 use crate::tape::file_formats::MediaContentHeader;
 
 /// Write trait for tape devices
@@ -53,53 +49,14 @@ pub trait TapeWrite {
     }
 }
 
-/// Write a single block to a tape device
-///
-/// Assumes that 'writer' is a linux tape device.
-///
-/// EOM Behaviour on Linux: When the end of medium early warning is
-/// encountered, the current write is finished and the number of bytes
-/// is returned. The next write returns -1 and errno is set to
-/// ENOSPC. To enable writing a trailer, the next write is allowed to
-/// proceed and, if successful, the number of bytes is returned. After
-/// this, -1 and the number of bytes are alternately returned until
-/// the physical end of medium (or some other error) is encountered.
-///
-/// See: https://github.com/torvalds/linux/blob/master/Documentation/scsi/st.rst
-///
-/// On success, this returns if we en countered a EOM condition.
-pub fn tape_device_write_block<W: Write>(
-    writer: &mut W,
-    data: &[u8],
-) -> Result<bool, std::io::Error> {
-
-    let mut leof = false;
+/// Write streams of blocks
+pub trait BlockWrite {
+    /// Write a data block
+    ///
+    /// Returns true if the drive reached the Logical End Of Media
+    /// (early warning)
+    fn write_block(&mut self, buffer: &[u8]) -> Result<bool, std::io::Error>;
 
-    loop {
-        match writer.write(data) {
-            Ok(count) if count == data.len() => return Ok(leof),
-            Ok(count) if count > 0 => {
-                proxmox::io_bail!(
-                    "short block write ({} < {}). Tape drive uses wrong block size.",
-                    count, data.len());
-            }
-            Ok(_) => { // count is 0 here, assume EOT
-                return Err(std::io::Error::from_raw_os_error(nix::errno::Errno::ENOSPC as i32));
-            }
-            // handle interrupted system call
-            Err(err) if err.kind() == std::io::ErrorKind::Interrupted => {
-                continue;
-            }
-            // detect and handle LEOM (early warning)
-            Err(err) if err.is_errno(nix::errno::Errno::ENOSPC) => {
-                if leof {
-                    return Err(err);
-                } else {
-                    leof = true;
-                    continue; // next write will succeed
-                }
-            }
-            Err(err) => return Err(err),
-        }
-    }
+    /// Write a filemark
+    fn write_filemark(&mut self) -> Result<(), std::io::Error>;
 }
-- 
2.20.1