From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 1268D913C4 for ; Wed, 3 Apr 2024 15:32:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DF39018E41 for ; Wed, 3 Apr 2024 15:32:02 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 3 Apr 2024 15:32:02 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B6E5244B02 for ; Wed, 3 Apr 2024 15:32:01 +0200 (CEST) Message-ID: <23fbf7c2-bcd8-44a3-8dda-902404607171@proxmox.com> Date: Wed, 3 Apr 2024 15:31:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= References: <20240328123707.336951-1-c.ebner@proxmox.com> <20240328123707.336951-14-c.ebner@proxmox.com> <1712143636.hs6xlwzexi.astroid@yuna.none> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <1712143636.hs6xlwzexi.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.371 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, lib.rs, mod.rs] Subject: Re: [pbs-devel] [PATCH v3 pxar 13/58] format: add pxar format version entry X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Apr 2024 13:32:33 -0000 On 4/3/24 13:41, Fabian Grünbichler wrote: >> @@ -258,7 +261,16 @@ impl DecoderImpl { >> loop { >> match self.state { >> State::Eof => return Ok(None), >> - State::Begin => return self.read_next_entry().await.map(Some), >> + State::Begin => { >> + let entry = self.read_next_entry().await.map(Some); >> + if let Ok(Some(ref entry)) = entry { >> + if let EntryKind::Version(version) = entry.kind() { >> + self.version = version.clone(); >> + return self.read_next_entry().await.map(Some); >> + } >> + } >> + return entry; > > a bit unsure here, if we want to enforce the order, wouldn't it be more > clean to transition to a new state here rather than adding more nested > ifs over time? ;) Okay, agreed, will try to de-clutter this a bit by adding a Prelude state as suggested in your other reply. >> + 1u64 => Ok(format::FormatVersion::Version1), > > this should never happen though, right? No, right... Will remove it. > >> + 2u64 => Ok(format::FormatVersion::Version2), > > also this (cted below) > >> + _ => io_bail!("unexpected pxar format version"), > > this should maybe include the value? ;) Okay, will add that. > >> + } >> + } >> } >> >> /// Reader for file contents inside a pxar archive. >> diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs >> index 88c0ed5..9270153 100644 >> --- a/src/encoder/mod.rs >> +++ b/src/encoder/mod.rs >> @@ -17,7 +17,7 @@ use endian_trait::Endian; >> >> use crate::binary_tree_array; >> use crate::decoder::{self, SeqRead}; >> -use crate::format::{self, GoodbyeItem, PayloadRef}; >> +use crate::format::{self, FormatVersion, GoodbyeItem, PayloadRef}; >> use crate::Metadata; >> >> pub mod aio; >> @@ -307,6 +307,8 @@ pub(crate) struct EncoderImpl<'a, T: SeqWrite + 'a> { >> /// Since only the "current" entry can be actively writing files, we share the file copy >> /// buffer. >> file_copy_buffer: Arc>>, >> + /// Pxar format version to encode >> + version: format::FormatVersion, >> } >> >> impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { >> @@ -320,11 +322,14 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { >> } >> >> let mut state = EncoderState::default(); >> - if let Some(payload_output) = payload_output.as_mut() { >> + let version = if let Some(payload_output) = payload_output.as_mut() { >> let header = format::Header::with_content_size(format::PXAR_PAYLOAD_START_MARKER, 0); >> header.check_header_size()?; >> seq_write_struct(payload_output, header, &mut state.payload_write_position).await?; >> - } >> + format::FormatVersion::Version2 >> + } else { >> + format::FormatVersion::default() > > shouldn't this be Version1 instead of default()? they are the same > *now*, but that might not be the case forever? Okay, will set this to the new version. > >> + }; >> >> let mut this = Self { >> output, >> @@ -334,8 +339,10 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { >> file_copy_buffer: Arc::new(Mutex::new(unsafe { >> crate::util::vec_new_uninitialized(1024 * 1024) >> })), >> + version, >> }; >> >> + this.encode_format_version().await?; >> this.encode_metadata(metadata).await?; >> let state = this.state_mut()?; >> state.files_offset = state.position(); >> @@ -522,6 +529,10 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { >> file_size: u64, >> payload_offset: PayloadOffset, >> ) -> io::Result<()> { >> + if self.version == FormatVersion::Version1 { >> + io_bail!("payload references not supported pxar format version 1"); >> + } >> + >> if self.payload_output.as_mut().is_none() { >> io_bail!("unable to add payload reference"); >> } >> @@ -729,6 +740,26 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { >> Ok(()) >> } >> >> + async fn encode_format_version(&mut self) -> io::Result<()> { >> + let version_bytes = match self.version { >> + format::FormatVersion::Version1 => return Ok(()), >> + format::FormatVersion::Version2 => 2u64.to_le_bytes(), > > (cted from above) and this here should maybe go together? Not sure I understand, above is in the decoder, this in the encoder... What was your intention? Move the encoding/decoding to the FormatVersion type? > >> + }; >> + >> + let (output, state) = self.output_state()?; >> + if state.write_position != 0 { >> + io_bail!("pxar format version must be encoded at the beginning of an archive"); > > should this also be enforced while decoding? Okay, will see to add a check for that as well. > > should we also encode a/the version of the payload archive? The payload archive has the PAYLOAD_START_MARKER, which can be changed just like the pxar entries v2? > >> + } >> + >> + seq_write_pxar_entry( >> + output, >> + format::PXAR_FORMAT_VERSION, >> + &version_bytes, >> + &mut state.write_position, >> + ) >> + .await >> + } >> + >> async fn encode_metadata(&mut self, metadata: &Metadata) -> io::Result<()> { >> let (output, state) = self.output_state()?; >> seq_write_pxar_struct_entry( >> diff --git a/src/format/mod.rs b/src/format/mod.rs >> index a672d19..2bf33c9 100644 >> --- a/src/format/mod.rs >> +++ b/src/format/mod.rs >> @@ -6,6 +6,7 @@ >> //! item data. >> //! >> //! An archive contains items in the following order: >> +//! * `FORMAT_VERSION` -- (optional for v1), version of encoding format >> //! * `ENTRY` -- containing general stat() data and related bits >> //! * `XATTR` -- one extended attribute >> //! * ... -- more of these when there are multiple defined >> @@ -80,6 +81,8 @@ pub mod mode { >> } >> >> // Generated by `cargo run --example mk-format-hashes` >> +/// Pxar format version entry, fallback to version 1 if not present >> +pub const PXAR_FORMAT_VERSION: u64 = 0x730f6c75df16a40d; >> /// Beginning of an entry (current version). >> pub const PXAR_ENTRY: u64 = 0xd5956474e588acef; >> /// Previous version of the entry struct >> @@ -186,6 +189,7 @@ impl Header { >> impl Display for Header { >> fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { >> let readable = match self.htype { >> + PXAR_FORMAT_VERSION => "FORMAT_VERSION", >> PXAR_FILENAME => "FILENAME", >> PXAR_SYMLINK => "SYMLINK", >> PXAR_HARDLINK => "HARDLINK", >> @@ -551,6 +555,13 @@ impl From<&std::fs::Metadata> for Stat { >> } >> } >> >> +#[derive(Clone, Debug, Default, PartialEq)] >> +pub enum FormatVersion { >> + #[default] >> + Version1, >> + Version2, >> +} >> + >> #[derive(Clone, Debug)] >> pub struct Filename { >> pub name: Vec, >> diff --git a/src/lib.rs b/src/lib.rs >> index ef81a85..a87b5ac 100644 >> --- a/src/lib.rs >> +++ b/src/lib.rs >> @@ -342,6 +342,9 @@ impl Acl { >> /// Identifies whether the entry is a file, symlink, directory, etc. >> #[derive(Clone, Debug)] >> pub enum EntryKind { >> + /// Pxar file format version >> + Version(format::FormatVersion), >> + >> /// Symbolic links. >> Symlink(format::Symlink), >> >> -- >> 2.39.2 >> >> >> >> _______________________________________________ >> pbs-devel mailing list >> pbs-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >> >> >> > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > >