Re: [PATCH proxmox v2 02/10] proxmox-alloc: document undefined behavior regarding custom allocs

[Robert Obkircher <r.obkircher@proxmox.com>]

> ... to `TapeBlock` and `TapeBlockFlags`, since `BlockHeader` is a bit
> of a misnomer -- each tape block has a header followed by a data
> payload, so it makes sense to just name it after what it is.
> 
> Also adapt the docstring for `BlockHeader`.
> 
> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
>
> diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs
> index 22803371c..3e3f7095c 100644
> --- a/pbs-tape/src/blocked_reader.rs
> +++ b/pbs-tape/src/blocked_reader.rs
> @@ -1,7 +1,7 @@
>  use std::io::Read;
>  
>  use crate::{
> -    BlockHeader, BlockHeaderFlags, BlockRead, BlockReadError, PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0,
> +    BlockRead, BlockReadError, PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0, TapeBlock, TapeBlockFlags,
>      TapeRead,
>  };
>  
> @@ -18,7 +18,7 @@ use crate::{
>  /// the end of the stream).
>  pub struct BlockedReader<R> {
>      reader: R,
> -    buffer: Box<BlockHeader>,
> +    buffer: Box<TapeBlock>,
>      seq_nr: u32,
>      found_end_marker: bool,
>      incomplete: bool,
> @@ -33,7 +33,7 @@ impl<R: BlockRead> BlockedReader<R> {
>      /// 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();
> +        let mut buffer = TapeBlock::new();
>  
>          Self::read_block_frame(&mut buffer, &mut reader)?;
>  
> @@ -43,7 +43,7 @@ impl<R: BlockRead> BlockedReader<R> {
>          let mut got_eod = false;
>  
>          if found_end_marker {
> -            incomplete = buffer.flags.contains(BlockHeaderFlags::INCOMPLETE);
> +            incomplete = buffer.flags.contains(TapeBlockFlags::INCOMPLETE);
>              Self::consume_eof_marker(&mut reader)?;
>              got_eod = true;
>          }
> @@ -60,7 +60,7 @@ impl<R: BlockRead> BlockedReader<R> {
>          })
>      }
>  
> -    fn check_buffer(buffer: &BlockHeader, seq_nr: u32) -> Result<(usize, bool), std::io::Error> {
> +    fn check_buffer(buffer: &TapeBlock, seq_nr: u32) -> Result<(usize, bool), std::io::Error> {
>          if buffer.magic != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 {
>              proxmox_lang::io_bail!(
>                  "got tape block with unknown magic number - not written by PBS or incompatible LTO version"
> @@ -76,7 +76,7 @@ impl<R: BlockRead> BlockedReader<R> {
>          }
>  
>          let size = buffer.size();
> -        let found_end_marker = buffer.flags.contains(BlockHeaderFlags::END_OF_STREAM);
> +        let found_end_marker = buffer.flags.contains(TapeBlockFlags::END_OF_STREAM);
>  
>          if size > buffer.payload.len() {
>              proxmox_lang::io_bail!(
> @@ -91,17 +91,14 @@ impl<R: BlockRead> BlockedReader<R> {
>          Ok((size, found_end_marker))
>      }
>  
> -    fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result<(), BlockReadError> {
> +    fn read_block_frame(buffer: &mut TapeBlock, reader: &mut R) -> Result<(), BlockReadError> {
>          let data = unsafe {
> -            std::slice::from_raw_parts_mut(
> -                (buffer as *mut BlockHeader) as *mut u8,
> -                BlockHeader::SIZE,
> -            )
> +            std::slice::from_raw_parts_mut((buffer as *mut TapeBlock) as *mut u8, TapeBlock::SIZE)
>          };
>  
>          let bytes = reader.read_block(data)?;
>  
> -        if bytes != BlockHeader::SIZE {
> +        if bytes != TapeBlock::SIZE {
>              return Err(proxmox_lang::io_format_err!("got wrong block size").into());
>          }
>  
> @@ -147,7 +144,7 @@ impl<R: BlockRead> BlockedReader<R> {
>          if found_end_marker {
>              // consume EOF mark
>              self.found_end_marker = true;
> -            self.incomplete = self.buffer.flags.contains(BlockHeaderFlags::INCOMPLETE);
> +            self.incomplete = self.buffer.flags.contains(TapeBlockFlags::INCOMPLETE);
>              Self::consume_eof_marker(&mut self.reader)?;
>              self.got_eod = true;
>          }
> diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs
> index 7380af243..9aba7b832 100644
> --- a/pbs-tape/src/blocked_writer.rs
> +++ b/pbs-tape/src/blocked_writer.rs
> @@ -1,6 +1,6 @@
>  use proxmox_io::vec;
>  
> -use crate::{BlockHeader, BlockHeaderFlags, BlockWrite, TapeWrite};
> +use crate::{BlockWrite, TapeBlock, TapeBlockFlags, TapeWrite};
>  
>  /// Assemble and write blocks of data
>  ///
> @@ -9,7 +9,7 @@ use crate::{BlockHeader, BlockHeaderFlags, BlockWrite, TapeWrite};
>  /// to the underlying writer.
>  pub struct BlockedWriter<W: BlockWrite> {
>      writer: W,
> -    buffer: Box<BlockHeader>,
> +    buffer: Box<TapeBlock>,
>      buffer_pos: usize,
>      seq_nr: u32,
>      logical_end_of_media: bool,
> @@ -36,7 +36,7 @@ impl<W: BlockWrite> BlockedWriter<W> {
>      pub fn new(writer: W) -> Self {
>          Self {
>              writer,
> -            buffer: BlockHeader::new(),
> +            buffer: TapeBlock::new(),
>              buffer_pos: 0,
>              seq_nr: 0,
>              logical_end_of_media: false,
> @@ -45,12 +45,9 @@ impl<W: BlockWrite> BlockedWriter<W> {
>          }
>      }
>  
> -    fn write_block(buffer: &BlockHeader, writer: &mut W) -> Result<bool, std::io::Error> {
> +    fn write_block(buffer: &TapeBlock, writer: &mut W) -> Result<bool, std::io::Error> {
>          let data = unsafe {
> -            std::slice::from_raw_parts(
> -                (buffer as *const BlockHeader) as *const u8,
> -                BlockHeader::SIZE,
> -            )
> +            std::slice::from_raw_parts((buffer as *const TapeBlock) as *const u8, TapeBlock::SIZE)
>          };
>          writer.write_block(data)
>      }
> @@ -77,7 +74,7 @@ impl<W: BlockWrite> BlockedWriter<W> {
>          let rest = rest - bytes;
>  
>          if rest == 0 {
> -            self.buffer.flags = BlockHeaderFlags::empty();
> +            self.buffer.flags = TapeBlockFlags::empty();
>              self.buffer.set_size(self.buffer.payload.len());
>              self.buffer.set_seq_nr(self.seq_nr);
>              self.seq_nr += 1;
> @@ -86,7 +83,7 @@ impl<W: BlockWrite> BlockedWriter<W> {
>                  self.logical_end_of_media = true;
>              }
>              self.buffer_pos = 0;
> -            self.bytes_written += BlockHeader::SIZE;
> +            self.bytes_written += TapeBlock::SIZE;
>          } else {
>              self.buffer_pos += bytes;
>          }
> @@ -116,14 +113,14 @@ impl<W: BlockWrite> TapeWrite for BlockedWriter<W> {
>      /// END_OF_STREAM flag.
>      fn finish(&mut self, incomplete: bool) -> Result<bool, std::io::Error> {
>          vec::clear(&mut self.buffer.payload[self.buffer_pos..]);
> -        self.buffer.flags = BlockHeaderFlags::END_OF_STREAM;
> +        self.buffer.flags = TapeBlockFlags::END_OF_STREAM;
>          if incomplete {
> -            self.buffer.flags |= BlockHeaderFlags::INCOMPLETE;
> +            self.buffer.flags |= TapeBlockFlags::INCOMPLETE;
>          }
>          self.buffer.set_size(self.buffer_pos);
>          self.buffer.set_seq_nr(self.seq_nr);
>          self.seq_nr += 1;
> -        self.bytes_written += BlockHeader::SIZE;
> +        self.bytes_written += TapeBlock::SIZE;
>          let leom = Self::write_block(&self.buffer, &mut self.writer)?;
>          self.write_eof()?;
>          Ok(leom)
> diff --git a/pbs-tape/src/lib.rs b/pbs-tape/src/lib.rs
> index 6cff175dd..1b6dc94cc 100644
> --- a/pbs-tape/src/lib.rs
> +++ b/pbs-tape/src/lib.rs
> @@ -20,7 +20,7 @@ mod blocked_writer;
>  pub use blocked_writer::BlockedWriter;
>  
>  mod tape_block;
> -pub(crate) use tape_block::{BlockHeader, BlockHeaderFlags};
> +pub(crate) use tape_block::{TapeBlock, TapeBlockFlags};
>  
>  mod tape_write;
>  pub use tape_write::*;
> diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs
> index 4f9c2192f..4ab1f5ce9 100644
> --- a/pbs-tape/src/tape_block.rs
> +++ b/pbs-tape/src/tape_block.rs
> @@ -3,23 +3,22 @@ use bitflags::bitflags;
>  use crate::PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0;
>  use crate::PROXMOX_TAPE_BLOCK_SIZE;
>  
> -/// Tape Block Header with data payload
> +/// A [`TapeBlock`] consists of a tape header followed by a data payload.
>  ///
>  /// All tape files are written as sequence of blocks.
>  ///
> -/// Note: this struct is large, never put this on the stack!
> -/// so we use an unsized type to avoid that.
> +/// Note: This struct is a dynamically sized type and can therefore only ever
> +/// exist as a heap-allocated value.
nit: the important part was that it is large. You can definitely have
DSTs on the stack via unsize coercsion (or alloca).

-- 
Robert Obkircher <r.obkircher@proxmox.com>