Re: [pbs-devel] [PATCH proxmox v5 1/5] compression: deflate: move encoder into a mod

["Max Carrara" <m.carrara@proxmox.com>]

On Fri Jul 5, 2024 at 3:04 PM CEST, Maximiliano Sandoval wrote:
> This allows to add a decompression mod inside the deflate mod. This does
> not touch the public API.
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  proxmox-compression/src/{ => deflate}/compression.rs | 6 ++----
>  proxmox-compression/src/deflate/mod.rs               | 5 +++++
>  proxmox-compression/src/lib.rs                       | 4 ++--
>  proxmox-compression/src/zip.rs                       | 2 +-
>  4 files changed, 10 insertions(+), 7 deletions(-)
>  rename proxmox-compression/src/{ => deflate}/compression.rs (97%)
>  create mode 100644 proxmox-compression/src/deflate/mod.rs
>
> diff --git a/proxmox-compression/src/compression.rs b/proxmox-compression/src/deflate/compression.rs
> similarity index 97%
> rename from proxmox-compression/src/compression.rs
> rename to proxmox-compression/src/deflate/compression.rs
> index 632a5991..6e6a151d 100644
> --- a/proxmox-compression/src/compression.rs
> +++ b/proxmox-compression/src/deflate/compression.rs
> @@ -12,8 +12,6 @@ use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
>  use proxmox_io::ByteBuffer;
>  use proxmox_lang::io_format_err;
>  
> -const BUFFER_SIZE: usize = 8192;
> -
>  pub enum Level {
>      Fastest,
>      Best,
> @@ -53,7 +51,7 @@ impl<T> DeflateEncoder<T> {
>          Self {
>              inner,
>              compressor: Compress::new(level, false),
> -            buffer: ByteBuffer::with_capacity(BUFFER_SIZE),
> +            buffer: ByteBuffer::with_capacity(super::BUFFER_SIZE),
>              input_buffer: Bytes::new(),
>              state: EncoderState::Reading,
>          }
> @@ -109,7 +107,7 @@ impl<T: AsyncWrite + Unpin> DeflateEncoder<T> {
>      where
>          R: AsyncRead + Unpin,
>      {
> -        let mut buffer = ByteBuffer::with_capacity(BUFFER_SIZE);
> +        let mut buffer = ByteBuffer::with_capacity(super::BUFFER_SIZE);
>          let mut eof = false;
>          loop {
>              if !eof && !buffer.is_full() {
> diff --git a/proxmox-compression/src/deflate/mod.rs b/proxmox-compression/src/deflate/mod.rs
> new file mode 100644
> index 00000000..514ccbdc
> --- /dev/null
> +++ b/proxmox-compression/src/deflate/mod.rs
> @@ -0,0 +1,5 @@
> +mod compression;
> +
> +pub use compression::{DeflateEncoder, Level};
> +
> +const BUFFER_SIZE: usize = 8192;
> diff --git a/proxmox-compression/src/lib.rs b/proxmox-compression/src/lib.rs
> index 7a9837e4..4be643c4 100644
> --- a/proxmox-compression/src/lib.rs
> +++ b/proxmox-compression/src/lib.rs
> @@ -1,8 +1,8 @@
>  #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]
>  
> -mod compression;
> -pub use compression::*;
> +pub use deflate::{DeflateEncoder, Level};
>  
> +mod deflate;

IMO this should be `pub` so it's structured similar to the other
modules. I get that the re-exports here are there so that the public API
isn't broken, but eventually we should adapt the public API (not in this
series though) so that the crate may be used like

  use proxmox_compression::deflate::{DeflateEncoder, DeflateLevel};

instead of

  use proxmox_compression::{DeflateEncoder, Level};

in the future.

An alias for `Level` named `DeflateLevel` could be introduced there as
well while the API is being adapted, so that it's easier to differ
between compression levels of different algorithms, should more enums be
added in that regard.

What I usually do in cases like that, is the following:
  1. Compare what the crate looks like *right now* vs. what it might
     look like in the future
  2. Identify the most common patterns of the structure public API
     and see if you can make the rest of the crate use the same patterns
  3. Once that is done, put yourself in the shoes of an absolute
     beginner (or someone who has no clue about the crate at all)
     and think about how they might use it
     - if it's simple and in no ways confusing, the API you just came
       up with is *probably* pretty good
     - if you notice that some things don't really make sense, or are
       inconsisent, awkward, painful etc., see if you can come up with
       a better API ;)
  4. If necessary, come up with a way to transition from the old API to
     the new one

Just wanted to share; that's what I usually do :)

NOTE: This is something that could (or rather, should) be done in a
different series IMO; just wanted to mention it here. Not a blocker or
anything!

>  pub mod tar;
>  pub mod zip;
>  pub mod zstd;
> diff --git a/proxmox-compression/src/zip.rs b/proxmox-compression/src/zip.rs
> index d2d3fd80..3ccece9b 100644
> --- a/proxmox-compression/src/zip.rs
> +++ b/proxmox-compression/src/zip.rs
> @@ -22,7 +22,7 @@ use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt, ReadBuf};
>  use crc32fast::Hasher;
>  use proxmox_time::gmtime;
>  
> -use crate::compression::{DeflateEncoder, Level};
> +use crate::deflate::{DeflateEncoder, Level};
>  
>  const LOCAL_FH_SIG: u32 = 0x04034B50;
>  const LOCAL_FF_SIG: u32 = 0x08074B50;



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