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

[Christian Ebner <c.ebner@proxmox.com>]

On 10/25/24 14:15, Fabian Grünbichler wrote:
> 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()

Acked, will adapt for version 4 of the patch series.

> 
>> +        }
>> +    }
>> +
>> +    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 ..

Agreed, will also incorporate this.

> 
>> +        } 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?

Could be moved to the `ArchiveType`, as `archive_extension` mimicking 
the `extension` getter as suggested above.

> 
>> +        } 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..

This is true, but we already have that exact same mapping currently in 
use (see patch 4, which drops the pre-existing helper). But that was 
arguably more limited in scope.

So maybe we might keep the pre-existing helper here instead of 
implementing `parse_archive_type` for the API type directly?

> 
> 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
> 
> 



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