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 03ECB9127B for ; Wed, 3 Apr 2024 14:02:28 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D171B17243 for ; Wed, 3 Apr 2024 14:01:57 +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 14:01:54 +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 3719444D32 for ; Wed, 3 Apr 2024 14:01:54 +0200 (CEST) Date: Wed, 03 Apr 2024 14:01:46 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240328123707.336951-1-c.ebner@proxmox.com> <20240328123707.336951-15-c.ebner@proxmox.com> In-Reply-To: <20240328123707.336951-15-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1712144650.ypugljzopo.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.059 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_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. [mk-format-hashes.rs, sync.rs, mod.rs, self.data, proxmox.com, lib.rs, aio.rs] Subject: Re: [pbs-devel] [PATCH v3 pxar 14/58] format/encoder/decoder: add entry type cli params 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 12:02:28 -0000 On March 28, 2024 1:36 pm, Christian Ebner wrote: > Add an additional entrt type PXAR_CLI_PARAMS which is used to store > additional metadata passed by the cli arguments such as the pxar cli > exclude patterns. >=20 > The content is encoded as an arbitrary byte slice. The entry must be > encoded right after the pxar format version entry, it is not possible to > encode this with the previous format version 1. since (from pxar's perspective) this is just an opaque blob of data, isn't PXAR_CLI_PARAMS a bit of a misnomer? do we want a single blob, or multiple delineated ones (might be more handy for the client using it)? >=20 > Signed-off-by: Christian Ebner > --- > changes since version 2: > - not present in previous version >=20 > examples/mk-format-hashes.rs | 1 + > src/accessor/mod.rs | 9 +++----- > src/decoder/mod.rs | 18 +++++++++++++++- > src/encoder/aio.rs | 19 ++++++++++++----- > src/encoder/mod.rs | 40 +++++++++++++++++++++++++++++------- > src/encoder/sync.rs | 11 ++++++++-- > src/format/mod.rs | 26 +++++++++++++++++++++++ > src/lib.rs | 3 +++ > 8 files changed, 106 insertions(+), 21 deletions(-) >=20 > diff --git a/examples/mk-format-hashes.rs b/examples/mk-format-hashes.rs > index e5d69b1..12394f3 100644 > --- a/examples/mk-format-hashes.rs > +++ b/examples/mk-format-hashes.rs > @@ -16,6 +16,7 @@ const CONSTANTS: &[(&str, &str, &str)] =3D &[ > "PXAR_ENTRY_V1", > "__PROXMOX_FORMAT_ENTRY__", > ), > + ("", "PXAR_CLI_PARAMS", "__PROXMOX_FORMAT_CLI_PARAMS__"), > ("", "PXAR_FILENAME", "__PROXMOX_FORMAT_FILENAME__"), > ("", "PXAR_SYMLINK", "__PROXMOX_FORMAT_SYMLINK__"), > ("", "PXAR_DEVICE", "__PROXMOX_FORMAT_DEVICE__"), > diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs > index 4789595..3b6ae44 100644 > --- a/src/accessor/mod.rs > +++ b/src/accessor/mod.rs > @@ -345,12 +345,9 @@ impl AccessorImpl { > =20 > let link_offset =3D entry_file_offset - link_offset; > =20 > - let (mut decoder, entry_offset) =3D get_decoder_at_filename( > - self.input.clone(), > - link_offset..self.size, > - PathBuf::new(), > - ) > - .await?; > + let (mut decoder, entry_offset) =3D > + get_decoder_at_filename(self.input.clone(), link_offset..sel= f.size, PathBuf::new()) > + .await?; > =20 > let entry =3D decoder > .next() this whole hunk just reverts a change done earlier in the same series (forgotten `cargo fmt` for the first patch maybe? ;)) > diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs > index 5b2fafb..4170b2f 100644 > --- a/src/decoder/mod.rs > +++ b/src/decoder/mod.rs > @@ -266,7 +266,13 @@ impl DecoderImpl { > if let Ok(Some(ref entry)) =3D entry { > if let EntryKind::Version(version) =3D entry.kin= d() { > self.version =3D version.clone(); > - return self.read_next_entry().await.map(Some= ); > + let entry =3D self.read_next_entry().await.m= ap(Some); > + if let Ok(Some(ref entry)) =3D entry { > + if let EntryKind::CliParams(_) =3D entry= .kind() { > + return self.read_next_entry().await.= map(Some); > + } > + } > + return entry; so maybe we want a new State::Prelude or something that we transition to from Begin if we encounter a FormatVersion, then we can match this and future "special" entries before proceeding with the regular archive? > } > } > return entry; > @@ -429,6 +435,11 @@ impl DecoderImpl { > self.current_header =3D header; > self.entry.kind =3D EntryKind::Version(self.read_format_vers= ion().await?); > =20 > + Ok(Some(self.entry.take())) > + } else if header.htype =3D=3D format::PXAR_CLI_PARAMS { > + self.current_header =3D header; > + self.entry.kind =3D EntryKind::CliParams(self.read_cli_param= s().await?); > + and here (well, not here, at the start of read_next_entry_or_eof ;)) we should maybe save the previous state before setting it to Default, so that we can then check it for some header types like FormatVersion or CliParams to ensure a misconstructed input cannot confuse our state machine/decoder? > Ok(Some(self.entry.take())) > } else if header.htype =3D=3D format::PXAR_ENTRY || header.htype= =3D=3D format::PXAR_ENTRY_V1 { > if header.htype =3D=3D format::PXAR_ENTRY { > @@ -802,6 +813,11 @@ impl DecoderImpl { > _ =3D> io_bail!("unexpected pxar format version"), > } > } > + > + async fn read_cli_params(&mut self) -> io::Result= { > + let data =3D self.read_entry_as_bytes().await?; > + Ok(format::CliParams { data }) > + } > } > =20 > /// Reader for file contents inside a pxar archive. > diff --git a/src/encoder/aio.rs b/src/encoder/aio.rs > index 6da32bd..956b2a3 100644 > --- a/src/encoder/aio.rs > +++ b/src/encoder/aio.rs > @@ -25,11 +25,13 @@ impl<'a, T: tokio::io::AsyncWrite + 'a> Encoder<'a, T= okioWriter> { > output: T, > metadata: &Metadata, > payload_output: Option, > + cli_params: Option<&[u8]>, > ) -> io::Result>> { > Encoder::new( > TokioWriter::new(output), > metadata, > payload_output.map(|payload_output| TokioWriter::new(payload= _output)), > + cli_params, > ) > .await > } > @@ -46,6 +48,7 @@ impl<'a> Encoder<'a, TokioWriter> { > TokioWriter::new(tokio::fs::File::create(path.as_ref()).awai= t?), > metadata, > None, > + None, > ) > .await > } > @@ -57,9 +60,11 @@ impl<'a, T: SeqWrite + 'a> Encoder<'a, T> { > output: T, > metadata: &Metadata, > payload_output: Option, > + cli_params: Option<&[u8]>, > ) -> io::Result> { > Ok(Self { > - inner: encoder::EncoderImpl::new(output.into(), metadata, pa= yload_output).await?, > + inner: encoder::EncoderImpl::new(output.into(), metadata, pa= yload_output, cli_params) > + .await?, > }) > } > =20 > @@ -331,10 +336,14 @@ mod test { > /// Assert that `Encoder` is `Send` > fn send_test() { > let test =3D async { > - let mut encoder =3D > - Encoder::new(DummyOutput, &Metadata::dir_builder(0o700).= build(), None) > - .await > - .unwrap(); > + let mut encoder =3D Encoder::new( > + DummyOutput, > + &Metadata::dir_builder(0o700).build(), > + None, > + None, > + ) > + .await > + .unwrap(); > { > encoder > .create_directory("baba", &Metadata::dir_builder(0o7= 00).build()) > diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs > index 9270153..b0ec877 100644 > --- a/src/encoder/mod.rs > +++ b/src/encoder/mod.rs > @@ -316,6 +316,7 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { > output: EncoderOutput<'a, T>, > metadata: &Metadata, > mut payload_output: Option, > + cli_params: Option<&[u8]>, > ) -> io::Result> { > if !metadata.is_dir() { > io_bail!("directory metadata must contain the directory mode= flag"); > @@ -343,6 +344,9 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { > }; > =20 > this.encode_format_version().await?; > + if let Some(params) =3D cli_params { > + this.encode_cli_params(params).await?; > + } > this.encode_metadata(metadata).await?; > let state =3D this.state_mut()?; > state.files_offset =3D state.position(); > @@ -740,16 +744,38 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> { > Ok(()) > } > =20 > + async fn encode_cli_params(&mut self, params: &[u8]) -> io::Result<(= )> { > + if self.version =3D=3D FormatVersion::Version1 { > + io_bail!("encoding cli params not supported pxar format vers= ion 1"); nit: missing "in" or "for" or "with" > + } > + > + let (output, state) =3D self.output_state()?; > + if state.write_position !=3D (size_of::() + size_of::()) as u64 { this seems brittle, shouldn't it explicitly use the size of a FormatVersion entry? this and the similar check for the version introduced in the previous patch smell a bit like "we actually have a state machine here but pretend not to" :) for the payload archive, we also have a very simple one: start_marker (1) -> payload entry (0..N) -> tail_marker (1) that is not enforced atm (as in, nothing stops a bug from writing other entry types, or additonal start/tail markers, or .. to the payload output). > + io_bail!( > + "cli params must be encoded following the version header= , current position {}", > + state.write_position, > + ); > + } > + > + seq_write_pxar_entry( > + output, > + format::PXAR_CLI_PARAMS, > + params, > + &mut state.write_position, > + ) > + .await > + } > + > async fn encode_format_version(&mut self) -> io::Result<()> { > - let version_bytes =3D match self.version { > - format::FormatVersion::Version1 =3D> return Ok(()), > - format::FormatVersion::Version2 =3D> 2u64.to_le_bytes(), > - }; > + let version_bytes =3D match self.version { > + format::FormatVersion::Version1 =3D> return Ok(()), > + format::FormatVersion::Version2 =3D> 2u64.to_le_bytes(), > + }; cargo fmt? > =20 > let (output, state) =3D self.output_state()?; > - if state.write_position !=3D 0 { > - io_bail!("pxar format version must be encoded at the beginning of an = archive"); > - } > + if state.write_position !=3D 0 { > + io_bail!("pxar format version must be encoded at the beginni= ng of an archive"); > + } cargo fmt? > =20 > seq_write_pxar_entry( > output, > diff --git a/src/encoder/sync.rs b/src/encoder/sync.rs > index a6e16f4..3f706c1 100644 > --- a/src/encoder/sync.rs > +++ b/src/encoder/sync.rs > @@ -28,7 +28,7 @@ impl<'a, T: io::Write + 'a> Encoder<'a, StandardWriter<= T>> { > /// Encode a `pxar` archive into a regular `std::io::Write` output. > #[inline] > pub fn from_std(output: T, metadata: &Metadata) -> io::Result>> { > - Encoder::new(StandardWriter::new(output), metadata, None) > + Encoder::new(StandardWriter::new(output), metadata, None, None) > } > } > =20 > @@ -42,6 +42,7 @@ impl<'a> Encoder<'a, StandardWriter> { > StandardWriter::new(std::fs::File::create(path.as_ref())?), > metadata, > None, > + None, > ) > } > } > @@ -53,12 +54,18 @@ impl<'a, T: SeqWrite + 'a> Encoder<'a, T> { > /// not allowed to use the `Waker`, as this will cause a `panic!`. > // Optionally attach a dedicated writer to redirect the payloads of = regular files to a separate > // output. > - pub fn new(output: T, metadata: &Metadata, payload_output: Option= ) -> io::Result { > + pub fn new( > + output: T, > + metadata: &Metadata, > + payload_output: Option, > + cli_params: Option<&[u8]>, > + ) -> io::Result { > Ok(Self { > inner: poll_result_once(encoder::EncoderImpl::new( > output.into(), > metadata, > payload_output, > + cli_params, > ))?, > }) > } > diff --git a/src/format/mod.rs b/src/format/mod.rs > index 2bf33c9..82ef196 100644 > --- a/src/format/mod.rs > +++ b/src/format/mod.rs > @@ -87,6 +87,7 @@ pub const PXAR_FORMAT_VERSION: u64 =3D 0x730f6c75df16a4= 0d; > pub const PXAR_ENTRY: u64 =3D 0xd5956474e588acef; > /// Previous version of the entry struct > pub const PXAR_ENTRY_V1: u64 =3D 0x11da850a1c1cceff; > +pub const PXAR_CLI_PARAMS: u64 =3D 0xcf58b7dd627f604a; > pub const PXAR_FILENAME: u64 =3D 0x16701121063917b3; > pub const PXAR_SYMLINK: u64 =3D 0x27f971e7dbf5dc5f; > pub const PXAR_DEVICE: u64 =3D 0x9fc9e906586d5ce9; > @@ -147,6 +148,7 @@ impl Header { > #[inline] > pub fn max_content_size(&self) -> u64 { > match self.htype { > + PXAR_CLI_PARAMS =3D> u64::MAX - (size_of::() as u64), > // + null-termination > PXAR_FILENAME =3D> crate::util::MAX_FILENAME_LEN + 1, > // + null-termination > @@ -190,6 +192,7 @@ impl Display for Header { > fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > let readable =3D match self.htype { > PXAR_FORMAT_VERSION =3D> "FORMAT_VERSION", > + PXAR_CLI_PARAMS =3D> "CLI_PARAMS", > PXAR_FILENAME =3D> "FILENAME", > PXAR_SYMLINK =3D> "SYMLINK", > PXAR_HARDLINK =3D> "HARDLINK", > @@ -694,6 +697,29 @@ impl Device { > } > } > =20 > +#[derive(Clone, Debug)] > +pub struct CliParams { > + pub data: Vec, > +} > + > +impl CliParams { > + pub fn as_os_str(&self) -> &OsStr { > + self.as_ref() > + } > +} > + > +impl AsRef<[u8]> for CliParams { > + fn as_ref(&self) -> &[u8] { > + &self.data > + } > +} > + > +impl AsRef for CliParams { > + fn as_ref(&self) -> &OsStr { > + OsStr::from_bytes(&self.data[..self.data.len().max(1) - 1]) > + } > +} > + > #[cfg(all(test, target_os =3D "linux"))] > #[test] > fn test_linux_devices() { > diff --git a/src/lib.rs b/src/lib.rs > index a87b5ac..cc85759 100644 > --- a/src/lib.rs > +++ b/src/lib.rs > @@ -345,6 +345,9 @@ pub enum EntryKind { > /// Pxar file format version > Version(format::FormatVersion), > =20 > + /// Cli parameter. > + CliParams(format::CliParams), > + > /// Symbolic links. > Symlink(format::Symlink), > =20 > --=20 > 2.39.2 >=20 >=20 >=20 > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >=20 >=20 >=20