[pbs-devel] [PATCH v8 proxmox-backup 22/69] client: helper: add method for split archive name mapping

[Christian Ebner]

On 6/4/24 10:17, Fabian Grünbichler wrote:
> On May 28, 2024 11:42 am, Christian Ebner wrote:
>> Helper method that takes the meta or payload archive name as input
>> and maps it to the correct archive names for metadata and payload
>> archive.
>>
>> If neither is matched, fallback to returning the passed in archive
>> name as target archive and `None` for the payload archive name.
>>
>> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
>> ---
>> changes since version 7:
>> - no changes
>>
>> changes since version 6:
>> - extend mapping to also include `.pxar` as allowed extension, mapping
>>    to `.mpxar`
>>
>>   proxmox-backup-client/src/helper.rs | 42 +++++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>
>> diff --git a/proxmox-backup-client/src/helper.rs b/proxmox-backup-client/src/helper.rs
>> index 5b21b6720..5589aa5b1 100644
>> --- a/proxmox-backup-client/src/helper.rs
>> +++ b/proxmox-backup-client/src/helper.rs
>> @@ -70,3 +70,45 @@ pub(crate) async fn get_buffered_pxar_reader(
>>   
>>       Ok(BufferedDynamicReader::new(index, chunk_reader))
>>   }
>> +
>> +pub(crate) fn get_pxar_archive_names(
>> +    archive_name: &str,
>> +    manifest: &BackupManifest,
>> +) -> (String, Option<String>) {
>> +    let filename = archive_name.strip_suffix(".didx").unwrap_or(archive_name);
>> +
>> +    if let Some(base) = filename
>> +        .strip_suffix(".mpxar")
>> +        .or_else(|| filename.strip_suffix(".ppxar"))
>> +    {
>> +        if archive_name.ends_with(".didx") {
>> +            return (
>> +                format!("{base}.mpxar.didx"),
>> +                Some(format!("{base}.ppxar.didx")),
>> +            );
>> +        } else {
>> +            return (format!("{base}.mpxar"), Some(format!("{base}.ppxar")));
>> +        }
>> +    }
>> +
>> +    if let Some(base) = filename.strip_suffix(".pxar") {
>> +        // Check if pxar is present, otherwise fallback to split archive naming
>> +        if manifest
>> +            .files()
>> +            .iter()
>> +            .find(|fileinfo| fileinfo.filename == filename)
>> +            .is_none()
>> +        {
> 
> I know why this is here, but the behaviour of this helper is now in a
> weird half-way state of checking but not really.
> 
> currently it's called for various restore-type operations (where passing
> in an archive that is not found in any variation is an error anyway) and
> for preparing the reference for an incremental backup, where the result
> is an option.
> 
> so could this helper not be changed to check whether the desired archive
> is referenced by the manifest (including the fallback to split archive
> when given a regular pxar archive name) and return None otherwise? or a
> Result, and the incremental case would catch and log it, but (obviously)
> not treat it as fatal, while the others would?
> 

Yes, you are right... Now that also the manifest is required it would 
indeed make sense to not just perform a mapping, but rather verify the 
archive being contained in the manifest. Will check if this is fine to 
adapt without hurting the ergonomics on the caller side because of the 
then required additional Option/Result handling to much. But this should 
be fine in most cases.