From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 689B31FF2C5
for <inbox@lore.proxmox.com>; Mon, 8 Jul 2024 09:33:01 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 87821369E6;
Mon, 8 Jul 2024 09:33:22 +0200 (CEST)
Date: Mon, 08 Jul 2024 09:32:48 +0200
Message-Id: <D2JZECCPNK1R.KFQA68WEQPPX@proxmox.com>
To: "Proxmox Backup Server development discussion"
<pbs-devel@lists.proxmox.com>
From: "Max Carrara" <m.carrara@proxmox.com>
Mime-Version: 1.0
X-Mailer: aerc 0.17.0-72-g6a84f1331f1c
References: <20240705130432.360361-1-m.sandoval@proxmox.com>
<20240705130432.360361-2-m.sandoval@proxmox.com>
In-Reply-To: <20240705130432.360361-2-m.sandoval@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.029 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pbs-devel] [PATCH proxmox v5 1/5] compression: deflate: move
encoder into a mod
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.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