Re: [pbs-devel] [PATCH v3 proxmox-backup 30/58] catalog: shell: redirect payload reader for split streams

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

On 4/4/24 11:49, Fabian Grünbichler wrote:
> On March 28, 2024 1:36 pm, Christian Ebner wrote:
>> Allows to attach to pxar archives with split metadata and payload
>> streams, by redirecting the payload input to a dedicated reader
>> accessing the payload index.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 2:
>> - use mpxar and ppxar file extensions
>> - use pxar fuse reader helper
>>
>>   proxmox-backup-client/src/catalog.rs | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/proxmox-backup-client/src/catalog.rs b/proxmox-backup-client/src/catalog.rs
>> index 2073e058d..3e52880b9 100644
>> --- a/proxmox-backup-client/src/catalog.rs
>> +++ b/proxmox-backup-client/src/catalog.rs
>> @@ -181,7 +181,10 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
>>           }
>>       };
>>   
>> -    let server_archive_name = if archive_name.ends_with(".pxar") {
>> +    let server_archive_name = if archive_name.ends_with(".pxar")
>> +        || archive_name.ends_with(".mpxar")
>> +        || archive_name.ends_with(".ppxar")
>> +    {
> 
> as with mount - there is a call to get_pxar_archive_names betwee this
> hunk and the next one (introduced by the previous patch), shouldn't that
> just be moved up here?

But this just adds the `.didx` postfix to the filename, not creating the 
possible split archive names just yet. I left this in place as is.

> 
>>           format!("{}.didx", archive_name)
>>       } else {
>>           bail!("Can only mount pxar archives.");
>> @@ -216,7 +219,18 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
>>       )
>>       .await?;
>>   
>> -    let decoder = pbs_pxar_fuse::Accessor::new(reader, archive_size).await?;
>> +    let decoder = if let Some(payload_archive_name) = payload_archive_name {
>> +        let (payload_reader, _) = helper::get_pxar_fuse_reader(
>> +            &payload_archive_name,
>> +            client.clone(),
>> +            &manifest,
>> +            crypt_config.clone(),
>> +        )
>> +        .await?;
>> +        pbs_pxar_fuse::Accessor::new(reader, archive_size, Some(payload_reader)).await?
>> +    } else {
>> +        pbs_pxar_fuse::Accessor::new(reader, archive_size, None).await?
>> +    };
> 
> we have this exact pattern twice at least as well, once for mount, once
> for the catalog.. in fact, all four calls to helpers::get_pxar_fuse_reader
> are in those two call sites, so probably the helper should just be that
> whole sequence instead (or the existing helper made internal to a new
> helper for this sequence)? :)

Yes, I moved this and the same code for the mount part into a helper 
function and merged this now rather unspectacular patch with the 
previous one also touching the catalog shell restore functionality.
Didn't make sense anymore to keep them separate.

> 
>>   
>>       client.download(CATALOG_NAME, &mut tmpfile).await?;
>>       let index = DynamicIndexReader::new(tmpfile)
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> 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
> 
>