Re: [pbs-devel] [PATCH v3 proxmox-backup 2/5] api types: introduce `BackupArchiveName` type

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On October 24, 2024 10:01 am, Christian Ebner wrote:
> Introduces a dedicated wrapper type to be used for backup archive
> names instead of plain strings and associated helper methods for
> archive type checks and archive name mappings.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - reworded commit message
> 
>  pbs-api-types/src/datastore.rs | 107 ++++++++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index dfa6bb259..62e4f7345 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -1,5 +1,7 @@
> +use std::convert::{AsRef, TryFrom};
>  use std::fmt;
>  use std::path::{Path, PathBuf};
> +use std::str::FromStr;
>  
>  use anyhow::{bail, format_err, Error};
>  use const_format::concatcp;
> @@ -1570,7 +1572,7 @@ pub fn print_store_and_ns(store: &str, ns: &BackupNamespace) -> String {
>      }
>  }
>  
> -#[derive(PartialEq, Eq)]
> +#[derive(Clone, PartialEq, Eq)]
>  /// Allowed variants of backup archives to be contained in a snapshot's manifest
>  pub enum ArchiveType {
>      FixedIndex,
> @@ -1590,3 +1592,106 @@ impl ArchiveType {
>          Ok(archive_type)
>      }
>  }
> +
> +#[derive(Clone, PartialEq, Eq)]
> +/// Name of archive files contained in snapshot's manifest
> +pub struct BackupArchiveName {
> +    // archive name including the `.fidx`, `.didx` or `.blob` extension
> +    name: String,
> +    // type parsed based on given extension
> +    ty: ArchiveType,
> +}
> +
> +impl fmt::Display for BackupArchiveName {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(f, "{name}", name = self.name)
> +    }
> +}
> +
> +serde_plain::derive_deserialize_from_fromstr!(BackupArchiveName, "archive name");
> +
> +impl FromStr for BackupArchiveName {
> +    type Err = Error;
> +
> +    fn from_str(name: &str) -> Result<Self, Self::Err> {
> +        Self::try_from(name)
> +    }
> +}
> +
> +serde_plain::derive_serialize_from_display!(BackupArchiveName);
> +
> +impl TryFrom<&str> for BackupArchiveName {
> +    type Error = anyhow::Error;
> +
> +    fn try_from(value: &str) -> Result<Self, Self::Error> {
> +        let (name, ty) = Self::parse_archive_type(value)?;
> +        Ok(Self { name, ty })
> +    }
> +}
> +
> +impl AsRef<str> for BackupArchiveName {
> +    fn as_ref(&self) -> &str {
> +        &self.name
> +    }
> +}
> +
> +impl BackupArchiveName {
> +    pub fn from_path(path: impl AsRef<Path>) -> Result<Self, Error> {
> +        let path = path.as_ref();
> +        if path.as_os_str().as_encoded_bytes().last() == Some(&b'/') {
> +            bail!("invalid archive name, got directory");
> +        }
> +        let file_name = path
> +            .file_name()
> +            .ok_or_else(|| format_err!("invalid archive name"))?;
> +        let name = file_name
> +            .to_str()
> +            .ok_or_else(|| format_err!("archive name not valid UTF-8"))?;
> +
> +        Self::try_from(name)
> +    }
> +
> +    pub fn archive_type(&self) -> ArchiveType {
> +        self.ty.clone()
> +    }
> +
> +    pub fn ends_with(&self, postfix: &str) -> bool {
> +        self.name.ends_with(postfix)
> +    }
> +
> +    pub fn has_pxar_filename_extension(&self) -> bool {
> +        self.name.ends_with(".pxar.didx")
> +            || self.name.ends_with(".mpxar.didx")
> +            || self.name.ends_with(".ppxar.didx")
> +    }
> +
> +    pub fn without_type_extension(&self) -> String {
> +        match self.ty {
> +            ArchiveType::DynamicIndex => self.name.strip_suffix(".didx").unwrap().into(),
> +            ArchiveType::FixedIndex => self.name.strip_suffix(".fidx").unwrap().into(),
> +            ArchiveType::Blob => self.name.strip_suffix(".blob").unwrap().into(),

if ArchiveType would have a getter for the corresponding extension, then
this could just become

self.name.strip_suffix(self.ty.extension()).unwrap().into()

> +        }
> +    }
> +
> +    fn parse_archive_type(archive_name: &str) -> Result<(String, ArchiveType), Error> {
> +        if archive_name.ends_with(".didx")
> +            || archive_name.ends_with(".fidx")
> +            || archive_name.ends_with(".blob")
> +        {
> +            Ok((archive_name.into(), ArchiveType::from_path(archive_name)?))

and this here could maybe also be turned around -> get the extension
from archive_name:

if let Ok(ty) = ArchiveType::from_path(..) {
  Ok((archive_name.into(), ty))
} else if ..


> +        } else if archive_name.ends_with(".pxar")
> +            || archive_name.ends_with(".mpxar")
> +            || archive_name.ends_with(".ppxar")
> +        {
> +            Ok((format!("{archive_name}.didx"), ArchiveType::DynamicIndex))
> +        } else if archive_name.ends_with(".img") {
> +            Ok((format!("{archive_name}.fidx"), ArchiveType::FixedIndex))

not sure whether we want these associations (between ArchiveType and
contained files) to live somewhere more declarative?

> +        } else {
> +            Ok((format!("{archive_name}.blob"), ArchiveType::Blob))

this last catchall here might be a bit dangerous? it basically makes the
introduction of a new archive type collide with any existing blobs that
happen to have a file name that ends with that new archive type..

e.g., qemu-server.conf.blob

with this patch, qemu-server.conf will automatically expand to
qemu-server.conf.blob

if we then at some point introduce an archive type `conf`, then it
wouldn't anymore, but instead be interpreted as archive of that type..

similarly, all of the above "inner" extensions are burned for archive
type usage already..

this was basically the only reason I didn't apply this right now leaving
the rest above as follow-up material ;)

> +        }
> +    }
> +}
> +
> +impl ApiType for BackupArchiveName {
> +    const API_SCHEMA: Schema = BACKUP_ARCHIVE_NAME_SCHEMA;
> +}
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> 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