From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 89BBE1FF0A7 for ; Wed, 02 Sep 2026 18:17:23 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 088FA215BC; 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-backup v2 06/10] tape: tape block: represent tape block header with its own struct From: Robert Obkircher To: "Max R. Carrara" In-Reply-To: <20260821140238.615302-7-m.carrara@proxmox.com> References: <20260821140238.615302-1-m.carrara@proxmox.com> <20260821140238.615302-7-m.carrara@proxmox.com> Date: Wed, 02 Sep 2026 18:17:14 +0200 Message-Id: <178836583425.294173.2400215559678666219.b4-review@b4> X-Mailer: b4 0.16-dev X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788365833793 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.625 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: WYJ7IUF5R4XIG3CWXZ4RI6HSKUMLVH4Q X-Message-ID-Hash: WYJ7IUF5R4XIG3CWXZ4RI6HSKUMLVH4Q 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: > Both the `BlockedReader` and `BlockedWriter` structs use private > helpers that cast their `TapeBlock` to a (mutable) byte slice using an > `unsafe` block for reading and writing a tape block, respectively. > > Neither `unsafe` block is prefixed with a "// SAFETY: ..." comment, > nor should these casts be done inline in the first place. > > Instead, implement these casts as methods on `TapeBlock` directly, > with either `unsafe` block being preceded with a SAFETY comment. > > Signed-off-by: Max R. Carrara > > diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs > index 6b1e3c692..6f5f87aa7 100644 > --- a/pbs-tape/src/blocked_reader.rs > +++ b/pbs-tape/src/blocked_reader.rs > @@ -95,14 +95,7 @@ impl BlockedReader { > } > > fn read_block_frame(tape_block: &mut TapeBlock, reader: &mut R) -> Result<(), BlockReadError> { > - let data = unsafe { > - std::slice::from_raw_parts_mut( > - (tape_block as *mut TapeBlock) as *mut u8, > - TapeBlock::SIZE, > - ) > - }; > - > - let bytes = reader.read_block(data)?; > + let bytes = reader.read_block(tape_block.as_bytes_mut())?; > > if bytes != TapeBlock::SIZE { > return Err(proxmox_lang::io_format_err!("got wrong block size").into()); > diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs > index 0d5d10147..44ff15ae0 100644 > --- a/pbs-tape/src/blocked_writer.rs > +++ b/pbs-tape/src/blocked_writer.rs > @@ -46,13 +46,7 @@ impl BlockedWriter { > } > > fn write_block(tape_block: &TapeBlock, writer: &mut W) -> Result { > - let data = unsafe { > - std::slice::from_raw_parts( > - (tape_block as *const TapeBlock) as *const u8, > - TapeBlock::SIZE, > - ) > - }; > - writer.write_block(data) > + writer.write_block(tape_block.as_bytes()) > } > > fn write_eof(&mut self) -> Result<(), std::io::Error> { > diff --git a/pbs-tape/src/tape_block.rs b/pbs-tape/src/tape_block.rs > index c73efafa7..c6ff98395 100644 > --- a/pbs-tape/src/tape_block.rs > +++ b/pbs-tape/src/tape_block.rs > @@ -117,4 +117,35 @@ impl TapeBlock { > pub fn payload_mut(&mut self) -> &mut [u8] { > &mut self.payload > } > + > + /// Returns the entirety of the tape block, meaning both its header and data > + /// payload, as a byte slice. > + pub fn as_bytes(&self) -> &[u8] { > + // SAFETY: > + // - Since `self` is a reference, we can convert it to a pointer without > + // any concerns. The resulting pointer is always valid and non-null. This point doesn't seem worth mentioning, especially if you leave out the more important ones like the fact that the struct doesn't contain any (uninitialized) padding bytes or interior mutability. > + // - The pointer used here is not used or aliased anywhere else. This is neither true nor relevant. e.g. if you call as_bytes twice it will be aliased. > + // - We allocated `*self` with a total size of `Self::SIZE` earlier, > + // meaning that the resulting slice never goes out of bounds. Relying on the fact that no other constructor exists seems like a bad idea. Just use `size_of_val(self)`. > + // - The resulting slice never outlives `self`. > + unsafe { std::slice::from_raw_parts((self as *const _) as *const u8, Self::SIZE) } I'd prefer `ptr::from_ref(self).cast()` to avoid the _ -- Robert Obkircher