Re: [pbs-devel] [PATCH v6 proxmox-backup 51/65] client: pxar: add helper to handle optional preludes

[Dominik Csapak <d.csapak@proxmox.com>]

comment inline

On 5/14/24 12:34, Christian Ebner wrote:
> Pxar archives with format version 2 allows to store optional
> information file format version and prelude entries.
> 
> Cover the case for these entries, the file format version entry being
> introduced to distinguish between different file formats used for
> encoding as well as the prelude entry used to store optional metadata
> such as the pxar cli exlude parameters.
> 
> Add the logic to accept and decode these prelude entries when
> accessing the archive via a decoder instance.
> 
> For now simply ignore them.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>   pbs-client/src/pxar/create.rs             |  1 +
>   pbs-client/src/pxar/extract.rs            |  7 +++---
>   pbs-client/src/pxar/tools.rs              |  7 ++++++
>   pbs-client/src/tools/mod.rs               | 27 +++++++++++++++++++++++
>   src/api2/tape/restore.rs                  | 17 +++++---------
>   src/tape/file_formats/snapshot_archive.rs |  8 +++++--
>   6 files changed, 51 insertions(+), 16 deletions(-)
> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 6dd0f3106..153c71349 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -241,6 +241,7 @@ where
>           &mut writers.writer,
>           &metadata,
>           writers.payload_writer.as_mut(),
> +        None,
>       )
>       .await?;
>   
> diff --git a/pbs-client/src/pxar/extract.rs b/pbs-client/src/pxar/extract.rs
> index 5f5ac6188..23b2f6ba5 100644
> --- a/pbs-client/src/pxar/extract.rs
> +++ b/pbs-client/src/pxar/extract.rs
> @@ -29,6 +29,7 @@ use proxmox_compression::zip::{ZipEncoder, ZipEntry};
>   use crate::pxar::dir_stack::PxarDirStack;
>   use crate::pxar::metadata;
>   use crate::pxar::Flags;
> +use crate::tools::handle_root_with_optional_format_version_prelude;
>   
>   pub struct PxarExtractOptions<'a> {
>       pub match_list: &'a [MatchEntry],
> @@ -124,9 +125,7 @@ where
>           // we use this to keep track of our directory-traversal
>           decoder.enable_goodbye_entries(true);
>   
> -        let root = decoder
> -            .next()
> -            .context("found empty pxar archive")?
> +        let (root, _, _) = handle_root_with_optional_format_version_prelude(&mut decoder)
>               .context("error reading pxar archive")?;
>   
>           if !root.is_dir() {
> @@ -267,6 +266,8 @@ where
>           };
>   
>           let extract_res = match (did_match, entry.kind()) {
> +            (_, EntryKind::Version(_version)) => Ok(()),
> +            (_, EntryKind::Prelude(_prelude)) => Ok(()),
>               (_, EntryKind::Directory) => {
>                   self.callback(entry.path());
>   
> diff --git a/pbs-client/src/pxar/tools.rs b/pbs-client/src/pxar/tools.rs
> index 459951d50..27e5185a3 100644
> --- a/pbs-client/src/pxar/tools.rs
> +++ b/pbs-client/src/pxar/tools.rs
> @@ -172,6 +172,13 @@ pub fn format_multi_line_entry(entry: &Entry) -> String {
>       let meta = entry.metadata();
>   
>       let (size, link, type_name, payload_offset) = match entry.kind() {
> +        EntryKind::Version(version) => (format!("{version:?}"), String::new(), "version", None),
> +        EntryKind::Prelude(prelude) => (
> +            "0".to_string(),
> +            format!("raw data: {:?} bytes", prelude.data.len()),
> +            "prelude",
> +            None,
> +        ),
>           EntryKind::File {
>               size,
>               payload_offset,
> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index f8d3102d1..e6cf066e4 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs
> @@ -529,3 +529,30 @@ pub fn place_xdg_file(
>           .and_then(|base| base.place_config_file(file_name).map_err(Error::from))
>           .with_context(|| format!("failed to place {} in xdg home", description))
>   }
> +
> +pub fn handle_root_with_optional_format_version_prelude<R: pxar::decoder::SeqRead>(
> +    decoder: &mut pxar::decoder::sync::Decoder<R>,
> +) -> Result<(pxar::Entry, Option<pxar::Entry>, Option<pxar::Entry>), Error> {

why return 3 values here? only the first one is ever used in this patch
and later you'll use the third, but never the second one

i know it makes refactoring/rebasing more cumbersome but i'd really
like that patches introduce only things they'll need

at least please omit the second return value for now, if we'll need it we can
add it later at any time

> +    let first = decoder
> +        .next()
> +        .ok_or_else(|| format_err!("missing root entry"))??;
> +    match first.kind() {
> +        pxar::EntryKind::Directory => Ok((first, None, None)),
> +        pxar::EntryKind::Version(_version) => {
> +            let second = decoder
> +                .next()
> +                .ok_or_else(|| format_err!("missing root entry"))??;
> +            match second.kind() {
> +                pxar::EntryKind::Directory => Ok((second, Some(first), None)),
> +                pxar::EntryKind::Prelude(_prelude) => {
> +                    let third = decoder
> +                        .next()
> +                        .ok_or_else(|| format_err!("missing root entry"))??;
> +                    Ok((third, Some(first), Some(second)))
> +                }
> +                _ => bail!("unexpected entry kind {:?}", second.kind()),
> +            }
> +        }
> +        _ => bail!("unexpected entry kind {:?}", first.kind()),
> +    }
> +}
> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
> index 11fb2b4cd..46093c7b0 100644
> --- a/src/api2/tape/restore.rs
> +++ b/src/api2/tape/restore.rs
> @@ -23,6 +23,7 @@ use pbs_api_types::{
>       PRIV_DATASTORE_MODIFY, PRIV_TAPE_READ, TAPE_RESTORE_NAMESPACE_SCHEMA,
>       TAPE_RESTORE_SNAPSHOT_SCHEMA, UPID_SCHEMA,
>   };
> +use pbs_client::tools::handle_root_with_optional_format_version_prelude;
>   use pbs_config::CachedUserInfo;
>   use pbs_datastore::dynamic_index::DynamicIndexReader;
>   use pbs_datastore::fixed_index::FixedIndexReader;
> @@ -1712,17 +1713,11 @@ fn try_restore_snapshot_archive<R: pxar::decoder::SeqRead>(
>       decoder: &mut pxar::decoder::sync::Decoder<R>,
>       snapshot_path: &Path,
>   ) -> Result<BackupManifest, Error> {
> -    let _root = match decoder.next() {
> -        None => bail!("missing root entry"),
> -        Some(root) => {
> -            let root = root?;
> -            match root.kind() {
> -                pxar::EntryKind::Directory => { /* Ok */ }
> -                _ => bail!("wrong root entry type"),
> -            }
> -            root
> -        }
> -    };
> +    let (root, _, _) = handle_root_with_optional_format_version_prelude(decoder)?;
> +    match root.kind() {
> +        pxar::EntryKind::Directory => { /* Ok */ }
> +        _ => bail!("wrong root entry type"),
> +    }
>   
>       let root_path = Path::new("/");
>       let manifest_file_name = OsStr::new(MANIFEST_BLOB_NAME);
> diff --git a/src/tape/file_formats/snapshot_archive.rs b/src/tape/file_formats/snapshot_archive.rs
> index 43d1cf9c3..7e052919b 100644
> --- a/src/tape/file_formats/snapshot_archive.rs
> +++ b/src/tape/file_formats/snapshot_archive.rs
> @@ -58,8 +58,12 @@ pub fn tape_write_snapshot_archive<'a>(
>               ));
>           }
>   
> -        let mut encoder =
> -            pxar::encoder::sync::Encoder::new(PxarTapeWriter::new(writer), &root_metadata, None)?;
> +        let mut encoder = pxar::encoder::sync::Encoder::new(
> +            PxarTapeWriter::new(writer),
> +            &root_metadata,
> +            None,
> +            None,
> +        )?;
>   
>           for filename in file_list.iter() {
>               let mut file = snapshot_reader.open_file(filename).map_err(|err| {



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel