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 3B0FD9125B for ; Wed, 3 Apr 2024 13:47:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 233B616EF5 for ; Wed, 3 Apr 2024 13:47:25 +0200 (CEST) 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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 3 Apr 2024 13:47:24 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5467644D69 for ; Wed, 3 Apr 2024 13:47:24 +0200 (CEST) Message-ID: <90ef036d-5873-4247-a295-17bced916b6c@proxmox.com> Date: Wed, 3 Apr 2024 13:47:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= References: <20240328123707.336951-1-c.ebner@proxmox.com> <20240328123707.336951-8-c.ebner@proxmox.com> <1712138653.sp8y5k19rp.astroid@yuna.none> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <1712138653.sp8y5k19rp.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 v3 pxar 07/58] decoder/accessor: add optional payload input stream 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: , X-List-Received-Date: Wed, 03 Apr 2024 11:47:25 -0000 On 4/3/24 12:38, Fabian Grünbichler wrote: > style nit: for those fns that take input and payload_input, it might > make sense to order them next to eachother? IMHO it makes the call sites > more readable, especially in those cases where it's mostly passed along > ;) for the constructors, I am a bit torn on which variant is nicer.\ Maybe I can do that as a followup, as this already touches so much stuff it feels wrong to also move implementations around? > > nit: would be easier to parse if it were > > let range = .. > if let Some(ref payload_input) = .. { > .. > } else { > .. > } > > and it would also mesh better with `open_contents_at_range` above. Ah yes, will change that! > > I am a bit confused by this here - shouldn't all payloads be encoded via > refs now if we have a split archive? and vice versa? why the second > condition? and what if I pass a bogus payload input for an archive that > doesn't contain any references? Got me on this one, sorry for confusing you here: this was added with the intention to not encode files with zero payload size in the payload stream, but rather in the metadata stream for space optimization. However this caused issues so was removed again on the proxmox-backup side, but I forgot to remove it here as well. Will remove this for the next version. > > similar here.. > > e.g., something like this: > > let input = if *payload_ref { > if let Some(payload_input) = self.payload_input.as_mut() { > payload_input > } else { > return None; > } > } else { > &mut self.input > }; > Some(Contents::new(input, offset, *size)) > > although technically we do have an invariant there that we could check - > we shouldn't encounter a non-ref payload when we have a payload_input.. yes same as above, this was added for the zero payload files.