From: "Max Carrara" <m.carrara@proxmox.com>
To: "Proxmox Backup Server development discussion"
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox v5 0/5] Teach HTTP client how to decode deflate
Date: Mon, 08 Jul 2024 09:32:57 +0200 [thread overview]
Message-ID: <D2JZEGJIJEGN.2CA8SOQOPP3OR@proxmox.com> (raw)
In-Reply-To: <20240705130432.360361-1-m.sandoval@proxmox.com>
On Fri Jul 5, 2024 at 3:04 PM CEST, Maximiliano Sandoval wrote:
> This patch series adds support for decoding HTTP requests if they contain the
> Content-Encoding=deflate header. Deciding on a public API (and implementing it)
> to set this header is out-of-scope for this merge request a the moment of
> writing 🙈.
>
> Note that currently the proxmox-rest-server replies with deflated replies in the
> following functions:
>
> - handle_api_request: Called as part of Formated::handle_request and
> H2Service::handle_request. Always compresses if the client declares that
> supports deflate.
> - handle_unformatted_api_request: Called as part of Unformatted::handle_request.
> It decides whether to compress the contents same as before
> - simple_static_file_download: Called as part of handle_static_file_download
> which is called in ApiConfig::handle_request, this one decides whether to
> compress based on the fn extension_to_content_type which is a map
> format:should-be-compressed and whether the clients supports it.
> - chunked_static_file_download is called as part of handle_static_file_downloda
> same as before
>
> One thing that might be worth pointing out is that according to [1], when the
> client supports `Accept-Encoding=deflate`, the HTTP server can reply with
> `Content-Encoding=deflate` and encode the body using zlib, which means that it
> should be encoded with deflate with the zlib headers included. This is somewhat
> confusing but in short it means that one should use a zlib encoder rather than a
> deflate encoder. At the moment the server always compresses using deflate
> without the zlib headers, not a zlib encoder.
Good to know, thanks! My review is below.
Review: v5
==========
All in all looks pretty good to me! I don't really have anything to
object. There's something I wanted to mention regarding the public API
of the `proxmox-compression` crate; see my response to patch 01.
Nothing that's a blocker though. ;)
Leaving my trailers here so that they don't get lost:
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
Testing
-------
* ran `cargo test --all-features` in the `proxmox-compression` crate
* all tests passed
* ran `cargo test --all-features` in the `proxmox-http` crate
* all tests passed
* ran `cargo test --all --all-featuers` in the workspace
* all tests passed
* recompiled PBS with your changes
* noticed nothing out of the ordinary
Everything works just fine, nice job!
Code Review
-----------
Also nothing out of the ordinary, the patches are easy to follow and the
code looks fine to me. Again, nice job!
Summary / Conclusion
--------------------
This can be merged as is IMO, all looks fine. My comments regarding the
public API of the `proxmox-compression` crate on patch 01 can (and
probably should) be addressed in another series.
LGTM.
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
>
> [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding#deflate
>
> Differences from v4:
> - Rebased
>
> Differences from v3:
> - Make deflate module private instead of pub(crate)
>
> Differences from v2:
> - Split into multiple commits
> - More tests
> - Add builders for DeflateEndoder and DeflateDecoder
> - Both the deflate encoder and decoder where moved into a folder
>
> Maximiliano Sandoval (5):
> compression: deflate: move encoder into a mod
> compression: deflate: add builder pattern
> compression: deflate: add a decoder
> compression: deflate: add test module
> http: teach the Client how to decode deflate content
>
> proxmox-compression/Cargo.toml | 3 +-
> .../src/{ => deflate}/compression.rs | 59 +++++--
> .../src/deflate/decompression.rs | 141 +++++++++++++++
> proxmox-compression/src/deflate/mod.rs | 167 ++++++++++++++++++
> proxmox-compression/src/lib.rs | 4 +-
> proxmox-compression/src/zip.rs | 2 +-
> proxmox-http/Cargo.toml | 7 +
> proxmox-http/src/client/simple.rs | 65 ++++++-
> 8 files changed, 430 insertions(+), 18 deletions(-)
> rename proxmox-compression/src/{ => deflate}/compression.rs (83%)
> create mode 100644 proxmox-compression/src/deflate/decompression.rs
> create mode 100644 proxmox-compression/src/deflate/mod.rs
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
next prev parent reply other threads:[~2024-07-08 7:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-05 13:04 Maximiliano Sandoval
2024-07-05 13:04 ` [pbs-devel] [PATCH proxmox v5 1/5] compression: deflate: move encoder into a mod Maximiliano Sandoval
2024-07-08 7:32 ` Max Carrara
2024-07-05 13:04 ` [pbs-devel] [PATCH proxmox v5 2/5] compression: deflate: add builder pattern Maximiliano Sandoval
2024-07-05 13:04 ` [pbs-devel] [PATCH proxmox v5 3/5] compression: deflate: add a decoder Maximiliano Sandoval
2024-07-05 13:04 ` [pbs-devel] [PATCH proxmox v5 4/5] compression: deflate: add test module Maximiliano Sandoval
2024-07-05 13:04 ` [pbs-devel] [PATCH proxmox v5 5/5] http: teach the Client how to decode deflate content Maximiliano Sandoval
2024-07-08 7:32 ` Max Carrara [this message]
2024-07-10 10:18 ` [pbs-devel] applied-series: [PATCH proxmox v5 0/5] Teach HTTP client how to decode deflate Wolfgang Bumiller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D2JZEGJIJEGN.2CA8SOQOPP3OR@proxmox.com \
--to=m.carrara@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox