Re: [pbs-devel] [PATCH v6 proxmox-backup 17/65] client: pxar: combine writers into struct

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

On 5/21/24 12:29, Dominik Csapak wrote:
> nit inline:
> 
> On 5/14/24 12:33, Christian Ebner wrote:
>> Introduce a `PxarWriters` struct to bundle all writer instances
>> required for the pxar archive creation into a single object to limit
>> the number of function call parameters, allowing to extend this
>> further by e.g. the optional payload writer instance.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-client/src/pxar/create.rs                  | 18 ++++++++++++++----
>>   pbs-client/src/pxar/mod.rs                     |  2 +-
>>   pbs-client/src/pxar_backup_stream.rs           |  5 +++--
>>   .../src/proxmox_restore_daemon/api.rs          |  6 ++++--
>>   pxar-bin/src/main.rs                           |  6 +++---
>>   tests/catar.rs                                 |  3 +--
>>   6 files changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/pbs-client/src/pxar/create.rs 
>> b/pbs-client/src/pxar/create.rs
>> index c9bf6df85..82f05889b 100644
>> --- a/pbs-client/src/pxar/create.rs
>> +++ b/pbs-client/src/pxar/create.rs
>> @@ -135,12 +135,22 @@ struct Archiver {
>>   type Encoder<'a, T> = pxar::encoder::aio::Encoder<'a, T>;
>> +pub struct PxarWriters<T> {
>> +    writer: T,
> 
> nit: imho, i wouldn't call that 'writer' as we're already know that from 
> the struct name
> and often from the variable name (e.g. see below 'writers.writer' is 
> just confusing)
> 
> how about archive/main/pxar/... ?

Yes the `writer` is rather opaque and I just keep the current naming 
here. I will opt for renaming this to `archive_writer`, to be consistent 
with the `payload_writer`, although the `_writer` might be ambiguous 
because the struct already gives that context.

> 
> writers.pxar & writers.catalog is imho  more readable
> than writers.writer & writers.catalog
> 
> but no hard feelings
> 
>> +    catalog: Option<Arc<Mutex<dyn BackupCatalogWriter + Send>>>,
>> +}
>> +
>> +impl<T> PxarWriters<T> {
>> +    pub fn new(writer: T, catalog: Option<Arc<Mutex<dyn 
>> BackupCatalogWriter + Send>>>) -> Self {
>> +        Self { writer, catalog }
>> +    }
>> +}
>> +
>>   pub async fn create_archive<T, F>(
>>       source_dir: Dir,
>> -    mut writer: T,
>> +    mut writers: PxarWriters<T>,
>>       feature_flags: Flags,
>>       callback: F,
>> -    catalog: Option<Arc<Mutex<dyn BackupCatalogWriter + Send>>>,
>>       options: PxarCreateOptions,
>>   ) -> Result<(), Error>
>>   where
>> @@ -170,7 +180,7 @@ where
>>           set.insert(stat.st_dev);
>>       }
>> -    let mut encoder = Encoder::new(&mut writer, &metadata, None).await?;
>> +    let mut encoder = Encoder::new(&mut writers.writer, &metadata, 
>> None).await?;
>>       let mut patterns = options.patterns;
>> @@ -188,7 +198,7 @@ where
>>           fs_magic,
>>           callback: Box::new(callback),
>>           patterns,
>> -        catalog,
>> +        catalog: writers.catalog,
>>           path: PathBuf::new(),
>>           entry_counter: 0,
>>           entry_limit: options.entries_max,
>> diff --git a/pbs-client/src/pxar/mod.rs b/pbs-client/src/pxar/mod.rs
>> index 14674b9b9..b7dcf8362 100644
>> --- a/pbs-client/src/pxar/mod.rs
>> +++ b/pbs-client/src/pxar/mod.rs
>> @@ -56,7 +56,7 @@ pub(crate) mod tools;
>>   mod flags;
>>   pub use flags::Flags;
>> -pub use create::{create_archive, PxarCreateOptions};
>> +pub use create::{create_archive, PxarCreateOptions, PxarWriters};
>>   pub use extract::{
>>       create_tar, create_zip, extract_archive, extract_sub_dir, 
>> extract_sub_dir_seq, ErrorHandler,
>>       OverwriteFlags, PxarExtractContext, PxarExtractOptions,
>> diff --git a/pbs-client/src/pxar_backup_stream.rs 
>> b/pbs-client/src/pxar_backup_stream.rs
>> index 22a6ffdc2..bfa108a8b 100644
>> --- a/pbs-client/src/pxar_backup_stream.rs
>> +++ b/pbs-client/src/pxar_backup_stream.rs
>> @@ -17,6 +17,8 @@ use proxmox_io::StdChannelWriter;
>>   use pbs_datastore::catalog::CatalogWriter;
>> +use crate::pxar::create::PxarWriters;
>> +
>>   /// Stream implementation to encode and upload .pxar archives.
>>   ///
>>   /// The hyper client needs an async Stream for file upload, so we
>> @@ -56,13 +58,12 @@ impl PxarBackupStream {
>>               let writer = 
>> pxar::encoder::sync::StandardWriter::new(writer);
>>               if let Err(err) = crate::pxar::create_archive(
>>                   dir,
>> -                writer,
>> +                PxarWriters::new(writer, Some(catalog)),
>>                   crate::pxar::Flags::DEFAULT,
>>                   move |path| {
>>                       log::debug!("{:?}", path);
>>                       Ok(())
>>                   },
>> -                Some(catalog),
>>                   options,
>>               )
>>               .await
>> diff --git a/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs 
>> b/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
>> index c20552225..1ee200573 100644
>> --- a/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
>> +++ b/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
>> @@ -23,7 +23,9 @@ use proxmox_sortable_macro::sortable;
>>   use proxmox_sys::fs::read_subdir;
>>   use pbs_api_types::file_restore::{FileRestoreFormat, 
>> RestoreDaemonStatus};
>> -use pbs_client::pxar::{create_archive, Flags, PxarCreateOptions, 
>> ENCODER_MAX_ENTRIES};
>> +use pbs_client::pxar::{
>> +    create_archive, Flags, PxarCreateOptions, PxarWriters, 
>> ENCODER_MAX_ENTRIES,
>> +};
>>   use pbs_datastore::catalog::{ArchiveEntry, DirEntryAttribute};
>>   use pbs_tools::json::required_string_param;
>> @@ -356,7 +358,7 @@ fn extract(
>>                       };
>>                       let pxar_writer = TokioWriter::new(writer);
>> -                    create_archive(dir, pxar_writer, Flags::DEFAULT, 
>> |_| Ok(()), None, options)
>> +                    create_archive(dir, PxarWriters::new(pxar_writer, 
>> None), Flags::DEFAULT, |_| Ok(()), options)
>>                           .await
>>                   }
>>                   .await;
>> diff --git a/pxar-bin/src/main.rs b/pxar-bin/src/main.rs
>> index d475da4e3..ae2325078 100644
>> --- a/pxar-bin/src/main.rs
>> +++ b/pxar-bin/src/main.rs
>> @@ -13,7 +13,8 @@ use tokio::signal::unix::{signal, SignalKind};
>>   use pathpatterns::{MatchEntry, MatchType, PatternFlag};
>>   use pbs_client::pxar::{
>> -    format_single_line_entry, Flags, OverwriteFlags, 
>> PxarExtractOptions, ENCODER_MAX_ENTRIES,
>> +    format_single_line_entry, Flags, OverwriteFlags, 
>> PxarExtractOptions, PxarWriters,
>> +    ENCODER_MAX_ENTRIES,
>>   };
>>   use proxmox_router::cli::*;
>> @@ -376,13 +377,12 @@ async fn create_archive(
>>       let writer = pxar::encoder::sync::StandardWriter::new(writer);
>>       pbs_client::pxar::create_archive(
>>           dir,
>> -        writer,
>> +        PxarWriters::new(writer, None),
>>           feature_flags,
>>           move |path| {
>>               log::debug!("{:?}", path);
>>               Ok(())
>>           },
>> -        None,
>>           options,
>>       )
>>       .await?;
>> diff --git a/tests/catar.rs b/tests/catar.rs
>> index 36bb4f3bc..f414da8c9 100644
>> --- a/tests/catar.rs
>> +++ b/tests/catar.rs
>> @@ -35,10 +35,9 @@ fn run_test(dir_name: &str) -> Result<(), Error> {
>>       let rt = tokio::runtime::Runtime::new().unwrap();
>>       rt.block_on(create_archive(
>>           dir,
>> -        writer,
>> +        PxarWriters::new(writer, None),
>>           Flags::DEFAULT,
>>           |_| Ok(()),
>> -        None,
>>           options,
>>       ))?;
> 
> 



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