From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 87634932BE for ; Fri, 17 Feb 2023 18:15:32 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 695A892B2 for ; Fri, 17 Feb 2023 18:15:32 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 17 Feb 2023 18:15:30 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 58787477F2 for ; Fri, 17 Feb 2023 18:15:30 +0100 (CET) Message-ID: Date: Fri, 17 Feb 2023 18:15:29 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:110.0) Gecko/20100101 Thunderbird/110.0 Content-Language: en-GB, de-AT To: Proxmox VE development discussion , Max Carrara References: <20230217152517.84874-1-m.carrara@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20230217152517.84874-1-m.carrara@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.076 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.256 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH http-server 0/2] refactor HTTP request processing X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Feb 2023 17:15:32 -0000 Am 17/02/2023 um 16:25 schrieb Max Carrara: > This patch series refactors the request processing algorithm by > separating its data and logic from one another in order to make future > changes regarding requests more transparent and explicit. > > Patch 1: Introduce the refactored algorithm > Patch 2: Replace and remove the existing algorithm with the new one See not much benefit in having this split into two patches in this way, constructing (one or ideally more) patches such that the refactored (out) code is directly put to use in(stead of) it's old place has normally more benefits, even if just allows one to see what actually moved (e.g., using the `--color-moved=zebra --color-moved-ws=ignore-all-space` git switches) But no need to resend now, I can squash locally for doing a more in-depth review first. > > This refactor is conducted very carefully to ensure that none of the > existing behaviours change in any way. Careful as in? ^^ Doesn't seem to be a very conservative "just split up", but more involved; and there isn't any testing added to ensure the changes don't mess with semantics - which tbf might not be the easiest thing to do in a sensible manner, that is actually covering a lot, here. But especially header parsing with some edge cases could be quite nicely done with the respective request methods mocked. > One exception however would be > the reading of the HTTP header itself: Instead of reading the > header by recursively appending / prepending callbacks to the handle's > and reading the header line-by-line, the entire header is read at > once, reducing the number of callbacks needed to parse the header from > the number of lines to one. This affects when the header line count > and size limits are checked - instead of being checked *while* the > header is being read, these limits are verified *after* the header > was read from the read buffer. If this change in behaviour is > undesirable, I'll gladly provide a v2. that seems to make those limits mostly (i.e., besides maybe proxied requests) irrelevant though? If one spent all of the resources to process a huge header fully it doesn't makes sense to only then die if it's deemed to big. But again, I'd wait out for a more in-depth review before going on a v2. > > Otherwise, the behaviour is identical; the logic is restructured, > but entirely preserved. > > > To give a more concrete example of what these changes would make more > transparent, the enhancement requested in #4494[1] would become just > another step (-> subroutine) in the refactored algorithm. That way it > becomes much clearer where the TLS handshake is being verified > and which responses (e.g. a `301`) are sent in case the handshake > never happened or was aborted. A patch, even if just POC, would be a more concrete example ;-) that could then _actually_ show that the changes actually provide a nicer interface to extend. > > [0] https://bugzilla.proxmox.com/show_bug.cgi?id=4494 > > > pve-http-server: > > Max Carrara (2): > http-server: anyevent: add new subroutine for processing requests > http-server: anyevent: replace request processing subroutine > > src/PVE/APIServer/AnyEvent.pm | 671 ++++++++++++++++++++++------------ > 1 file changed, 445 insertions(+), 226 deletions(-) > puh, almost doubles SLOC...