[pbs-devel] [PATCH v6 pxar 04/14] encoder: add optional output writer for file payloads

[Christian Ebner]

> On 21.05.2024 12:06 CEST Dominik Csapak <d.csapak at proxmox.com> wrote:
> 
>  
> one small comment inline
> 
> On 5/14/24 12:33, Christian Ebner wrote:
> [snip]
> > diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs
> > index da41733..99c3758 100644
> > --- a/src/encoder/mod.rs
> > +++ b/src/encoder/mod.rs
> > @@ -17,7 +17,7 @@ use endian_trait::Endian;
> >   
> >   use crate::binary_tree_array;
> >   use crate::decoder::{self, SeqRead};
> > -use crate::format::{self, GoodbyeItem};
> > +use crate::format::{self, GoodbyeItem, PayloadRef};
> >   use crate::Metadata;
> >   
> >   pub mod aio;
> > @@ -221,6 +221,9 @@ struct EncoderState {
> >   
> >       /// We need to keep track how much we have written to get offsets.
> >       write_position: u64,
> > +
> > +    /// Track the bytes written to the payload writer
> > +    payload_write_position: u64,
> >   }
> >   
> >   impl EncoderState {
> > @@ -278,6 +281,7 @@ impl<'a, T> std::convert::From<&'a mut T> for EncoderOutput<'a, T> {
> >   /// synchronous or `async` I/O objects in as output.
> >   pub(crate) struct EncoderImpl<'a, T: SeqWrite + 'a> {
> >       output: EncoderOutput<'a, T>,
> > +    payload_output: EncoderOutput<'a, Option<T>>,
> >       state: EncoderState,
> >       parent: Option<&'a mut EncoderState>,
> >       finished: bool,
> > @@ -306,12 +310,14 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> {
> >       pub async fn new(
> >           output: EncoderOutput<'a, T>,
> >           metadata: &Metadata,
> > +        payload_output: Option<T>,
> >       ) -> io::Result<EncoderImpl<'a, T>> {
> >           if !metadata.is_dir() {
> >               io_bail!("directory metadata must contain the directory mode flag");
> >           }
> >           let mut this = Self {
> >               output,
> > +            payload_output: EncoderOutput::Owned(None),
> >               state: EncoderState::default(),
> >               parent: None,
> >               finished: false,
> > @@ -323,6 +329,10 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> {
> >           this.encode_metadata(metadata).await?;
> >           this.state.files_offset = this.position();
> >   
> > +        if let Some(payload_output) = payload_output {
> > +            this.payload_output = EncoderOutput::Owned(Some(payload_output));
> > +        }
> > +
> 
> you could do that above directly in the payload_output property
> since you do change it to that anyway in patch 12/14
> 
> and should be semantically the same

true, inlined this into the `EncoderImpl` instantiation, thanks!

> 
> >           Ok(this)
> >       }
> >