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 E6D7A1FF0AB for ; Mon, 07 Sep 2026 13:44:23 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6B6F021474; Mon, 07 Sep 2026 13:44:23 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 07 Sep 2026 13:44:19 +0200 Message-Id: From: "Max R. Carrara" Subject: Re: [PATCH proxmox-backup v2 06/10] tape: tape block: represent tape block header with its own struct To: "Robert Obkircher" X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20260821140238.615302-1-m.carrara@proxmox.com> <20260821140238.615302-7-m.carrara@proxmox.com> <178836583425.294173.2400215559678666219.b4-review@b4> In-Reply-To: <178836583425.294173.2400215559678666219.b4-review@b4> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788781452979 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.631 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: 4NBNVGMASKTKU325IEFQHMZ3LWYCCS42 X-Message-ID-Hash: 4NBNVGMASKTKU325IEFQHMZ3LWYCCS42 X-MailFrom: m.carrara@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: On Wed Sep 2, 2026 at 6:17 PM CEST, Robert Obkircher wrote: > > 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_read= er.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 =3D unsafe { > > - std::slice::from_raw_parts_mut( > > - (tape_block as *mut TapeBlock) as *mut u8, > > - TapeBlock::SIZE, > > - ) > > - }; > > - > > - let bytes =3D reader.read_block(data)?; > > + let bytes =3D reader.read_block(tape_block.as_bytes_mut())?; > > > > if bytes !=3D TapeBlock::SIZE { > > return Err(proxmox_lang::io_format_err!("got wrong block s= ize").into()); > > diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writ= er.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 =3D 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 heade= r 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 point= er without > > + // any concerns. The resulting pointer is always valid and n= on-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. Good point, will add in v2. Thanks! > > + // - The pointer used here is not used or aliased anywhere els= e. > This is neither true nor relevant. e.g. if you call as_bytes twice it > will be aliased. Right, but I was referring to the pointer that is `self as *const _` (or `self as *mut _`) -- that pointer specifically is not aliased anywhere else, meaning that the returned references are always valid and respect Rust's ownership model. All that being said, I should probably rephrase this to be more precise. Thanks a lot, will fix in v2! > > + // - We allocated `*self` with a total size of `Self::SIZE` ea= rlier, > > + // 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)`. Good point actually, since we just return a byte slice. Didn't think of that. Will fix in v2, thanks! > > + // - The resulting slice never outlives `self`. > > + unsafe { std::slice::from_raw_parts((self as *const _) as *con= st u8, Self::SIZE) } > I'd prefer `ptr::from_ref(self).cast()` to avoid the _ ACK, will fix in v2. Thanks!