From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 689B31FF2C5 for ; 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: To: "Proxmox Backup Server development discussion" From: "Max Carrara" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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 DeflateEncoder { > 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 DeflateEncoder { > 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