Re: [pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Mar 24, 2021 at 11:56:38AM +0100, Dominik Csapak wrote:
> some writers may need to be flushed to write out all data
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> for now, it is not needed because in all code paths we use it
> the writers do not need to be flushed, but there is no guarantee
> it will stay that way
> 
>  src/encoder/mod.rs | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs
> index 1004efa..c114657 100644
> --- a/src/encoder/mod.rs
> +++ b/src/encoder/mod.rs
> @@ -83,6 +83,13 @@ async fn seq_write<T: SeqWrite + ?Sized>(
>      Ok(put)
>  }
>  
> +/// awaitable version of 'poll_flush'.
> +async fn flush<T: SeqWrite + ?Sized>(
> +    output: &mut T,
> +) -> io::Result<()> {
> +    poll_fn(|cx| unsafe { Pin::new_unchecked(&mut *output).poll_flush(cx) }).await
> +}
> +
>  /// Write the entire contents of a buffer, handling short writes.
>  async fn seq_write_all<T: SeqWrite + ?Sized>(
>      output: &mut T,
> @@ -715,6 +722,8 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> {
>          )
>          .await?;
>  
> +        flush(self.output.as_mut()).await?;

According to the patch comment this hasn't broken anywhere at the time,
but was there any test-code that did need this?

I'd like to make this at least conditional on the writer being
`EncoderOutput::Owned` to not cause additional flushes for every single
level of directory nesting.
That said, I'm not even convinced an `Owned` writer would really need
this? You don't need to explicitly call `flush()` on a `std::fs::File`
or even a `std::io::BufWriter` explicitly (`BufWriter` explicitly
flushes in its `Drop` handler), unless you *explicitly* want to handle
its error, but then you should keep ownership of the writer you pass to
the encoder anyway and flush manually, not leave that up to the pxar
code.