[pbs-devel] [PATCH v3 pxar 14/58] format/encoder/decoder: add entry type cli params
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Apr 3 14:01:46 CEST 2024
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.
>
> 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)?
>
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> changes since version 2:
> - not present in previous version
>
> 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(-)
>
> 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)] = &[
> "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<T: Clone + ReadAt> AccessorImpl<T> {
>
> let link_offset = entry_file_offset - link_offset;
>
> - let (mut decoder, entry_offset) = get_decoder_at_filename(
> - self.input.clone(),
> - link_offset..self.size,
> - PathBuf::new(),
> - )
> - .await?;
> + let (mut decoder, entry_offset) =
> + get_decoder_at_filename(self.input.clone(), link_offset..self.size, PathBuf::new())
> + .await?;
>
> let entry = 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<I: SeqRead> DecoderImpl<I> {
> 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);
> + let entry = self.read_next_entry().await.map(Some);
> + if let Ok(Some(ref entry)) = entry {
> + if let EntryKind::CliParams(_) = 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<I: SeqRead> DecoderImpl<I> {
> self.current_header = header;
> self.entry.kind = EntryKind::Version(self.read_format_version().await?);
>
> + Ok(Some(self.entry.take()))
> + } else if header.htype == format::PXAR_CLI_PARAMS {
> + self.current_header = header;
> + self.entry.kind = EntryKind::CliParams(self.read_cli_params().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 == format::PXAR_ENTRY || header.htype == format::PXAR_ENTRY_V1 {
> if header.htype == format::PXAR_ENTRY {
> @@ -802,6 +813,11 @@ impl<I: SeqRead> DecoderImpl<I> {
> _ => io_bail!("unexpected pxar format version"),
> }
> }
> +
> + async fn read_cli_params(&mut self) -> io::Result<format::CliParams> {
> + let data = self.read_entry_as_bytes().await?;
> + Ok(format::CliParams { data })
> + }
> }
>
> /// 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, TokioWriter<T>> {
> output: T,
> metadata: &Metadata,
> payload_output: Option<T>,
> + cli_params: Option<&[u8]>,
> ) -> io::Result<Encoder<'a, TokioWriter<T>>> {
> 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<tokio::fs::File>> {
> TokioWriter::new(tokio::fs::File::create(path.as_ref()).await?),
> metadata,
> None,
> + None,
> )
> .await
> }
> @@ -57,9 +60,11 @@ impl<'a, T: SeqWrite + 'a> Encoder<'a, T> {
> output: T,
> metadata: &Metadata,
> payload_output: Option<T>,
> + cli_params: Option<&[u8]>,
> ) -> io::Result<Encoder<'a, T>> {
> Ok(Self {
> - inner: encoder::EncoderImpl::new(output.into(), metadata, payload_output).await?,
> + inner: encoder::EncoderImpl::new(output.into(), metadata, payload_output, cli_params)
> + .await?,
> })
> }
>
> @@ -331,10 +336,14 @@ mod test {
> /// Assert that `Encoder` is `Send`
> fn send_test() {
> let test = async {
> - let mut encoder =
> - Encoder::new(DummyOutput, &Metadata::dir_builder(0o700).build(), None)
> - .await
> - .unwrap();
> + let mut encoder = Encoder::new(
> + DummyOutput,
> + &Metadata::dir_builder(0o700).build(),
> + None,
> + None,
> + )
> + .await
> + .unwrap();
> {
> encoder
> .create_directory("baba", &Metadata::dir_builder(0o700).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<T>,
> + cli_params: Option<&[u8]>,
> ) -> io::Result<EncoderImpl<'a, T>> {
> 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> {
> };
>
> this.encode_format_version().await?;
> + if let Some(params) = cli_params {
> + this.encode_cli_params(params).await?;
> + }
> this.encode_metadata(metadata).await?;
> let state = this.state_mut()?;
> state.files_offset = state.position();
> @@ -740,16 +744,38 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> {
> Ok(())
> }
>
> + async fn encode_cli_params(&mut self, params: &[u8]) -> io::Result<()> {
> + if self.version == FormatVersion::Version1 {
> + io_bail!("encoding cli params not supported pxar format version 1");
nit: missing "in" or "for" or "with"
> + }
> +
> + let (output, state) = self.output_state()?;
> + if state.write_position != (size_of::<u64>() + size_of::<format::Header>()) 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 = match self.version {
> - format::FormatVersion::Version1 => return Ok(()),
> - format::FormatVersion::Version2 => 2u64.to_le_bytes(),
> - };
> + let version_bytes = match self.version {
> + format::FormatVersion::Version1 => return Ok(()),
> + format::FormatVersion::Version2 => 2u64.to_le_bytes(),
> + };
cargo fmt?
>
> 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");
> - }
> + if state.write_position != 0 {
> + io_bail!("pxar format version must be encoded at the beginning of an archive");
> + }
cargo fmt?
>
> 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<'a, StandardWriter<T>>> {
> - Encoder::new(StandardWriter::new(output), metadata, None)
> + Encoder::new(StandardWriter::new(output), metadata, None, None)
> }
> }
>
> @@ -42,6 +42,7 @@ impl<'a> Encoder<'a, StandardWriter<std::fs::File>> {
> 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<T>) -> io::Result<Self> {
> + pub fn new(
> + output: T,
> + metadata: &Metadata,
> + payload_output: Option<T>,
> + cli_params: Option<&[u8]>,
> + ) -> io::Result<Self> {
> 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 = 0x730f6c75df16a40d;
> pub const PXAR_ENTRY: u64 = 0xd5956474e588acef;
> /// Previous version of the entry struct
> pub const PXAR_ENTRY_V1: u64 = 0x11da850a1c1cceff;
> +pub const PXAR_CLI_PARAMS: u64 = 0xcf58b7dd627f604a;
> pub const PXAR_FILENAME: u64 = 0x16701121063917b3;
> pub const PXAR_SYMLINK: u64 = 0x27f971e7dbf5dc5f;
> pub const PXAR_DEVICE: u64 = 0x9fc9e906586d5ce9;
> @@ -147,6 +148,7 @@ impl Header {
> #[inline]
> pub fn max_content_size(&self) -> u64 {
> match self.htype {
> + PXAR_CLI_PARAMS => u64::MAX - (size_of::<Self>() as u64),
> // + null-termination
> PXAR_FILENAME => 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 = match self.htype {
> PXAR_FORMAT_VERSION => "FORMAT_VERSION",
> + PXAR_CLI_PARAMS => "CLI_PARAMS",
> PXAR_FILENAME => "FILENAME",
> PXAR_SYMLINK => "SYMLINK",
> PXAR_HARDLINK => "HARDLINK",
> @@ -694,6 +697,29 @@ impl Device {
> }
> }
>
> +#[derive(Clone, Debug)]
> +pub struct CliParams {
> + pub data: Vec<u8>,
> +}
> +
> +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<OsStr> for CliParams {
> + fn as_ref(&self) -> &OsStr {
> + OsStr::from_bytes(&self.data[..self.data.len().max(1) - 1])
> + }
> +}
> +
> #[cfg(all(test, target_os = "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),
>
> + /// Cli parameter.
> + CliParams(format::CliParams),
> +
> /// Symbolic links.
> Symlink(format::Symlink),
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
More information about the pbs-devel
mailing list