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

["Max R. Carrara" <m.carrara@proxmox.com>]

On Wed Sep 2, 2026 at 6:17 PM CEST, Robert Obkircher wrote:
> > ... 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).

Ah, I think this was meant to be a response to patch #04?

Anyway, I agree; will correct this in v2. Thanks!