From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 245AE1FF0A7 for ; Wed, 02 Sep 2026 18:17:25 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id C90DE215CF; Wed, 02 Sep 2026 18:17:22 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH proxmox v2 02/10] proxmox-alloc: document undefined behavior regarding custom allocs From: Robert Obkircher To: "Max R. Carrara" In-Reply-To: <20260821140238.615302-3-m.carrara@proxmox.com> References: <20260821140238.615302-1-m.carrara@proxmox.com> <20260821140238.615302-3-m.carrara@proxmox.com> Date: Wed, 02 Sep 2026 18:17:14 +0200 Message-Id: <178836583424.294173.6954096291117410170.b4-review@b4> X-Mailer: b4 0.16-dev X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788365833674 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.619 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: RQOQ7VLSJU22W3YRS3ABXU6P3TAAFDN5 X-Message-ID-Hash: RQOQ7VLSJU22W3YRS3ABXU6P3TAAFDN5 X-MailFrom: r.obkircher@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: > ... 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 > > 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 { > reader: R, > - buffer: Box, > + buffer: Box, > seq_nr: u32, > found_end_marker: bool, > incomplete: bool, > @@ -33,7 +33,7 @@ impl BlockedReader { > /// 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(); > + let mut buffer = TapeBlock::new(); > > Self::read_block_frame(&mut buffer, &mut reader)?; > > @@ -43,7 +43,7 @@ impl BlockedReader { > 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 BlockedReader { > }) > } > > - 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 BlockedReader { > } > > 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 BlockedReader { > 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 BlockedReader { > 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 { > writer: W, > - buffer: Box, > + buffer: Box, > buffer_pos: usize, > seq_nr: u32, > logical_end_of_media: bool, > @@ -36,7 +36,7 @@ impl BlockedWriter { > 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 BlockedWriter { > } > } > > - fn write_block(buffer: &BlockHeader, writer: &mut W) -> Result { > + fn write_block(buffer: &TapeBlock, writer: &mut W) -> Result { > 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 BlockedWriter { > 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 BlockedWriter { > 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 TapeWrite for BlockedWriter { > /// END_OF_STREAM flag. > fn finish(&mut self, incomplete: bool) -> Result { > 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