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 B6D47934C7 for ; Tue, 9 Apr 2024 10:55:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 95C1518F40 for ; Tue, 9 Apr 2024 10:55:23 +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 ; Tue, 9 Apr 2024 10:55:22 +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 C709F42DE6 for ; Tue, 9 Apr 2024 10:55:21 +0200 (CEST) Message-ID: <4fe30e73-7184-4c66-875d-671c9285a78c@proxmox.com> Date: Tue, 9 Apr 2024 10:55:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: pve-devel@lists.proxmox.com References: <20240402171629.536804-1-s.hanreich@proxmox.com> <20240402171629.536804-14-s.hanreich@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.570 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH proxmox-firewall 13/37] config: firewall: add host specific config + option types 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: Tue, 09 Apr 2024 08:55:23 -0000 On 4/3/24 12:47, Max Carrara wrote: > Should maybe document such defaults in the docstring of the `pub` > function above? > >> + >> + pub fn synflood_burst(&self) -> i64 { >> + self.config >> + .options >> + .protection_synflood_burst >> + .unwrap_or(1000) >> + } > > Same here. > > Also, numeric defaults like those could maaaaaybe be declared as a > `const` upfront (and documented). Technically, doing this for the > boolean defaults here in this patch wouldn't hurt either - I realize > that it's clear from the context of the code what's meant, but in this > case it would be solely for documentation purposes. > > E.g. if the question "Does the firewall enable NDP by default?" arises, > one could just check the (docstrings of the) constants declared at the > top of the file, or even better, browse the docs generated by cargo if > they're not a developer. Those defaults are already documented quite well in the Firewall documentation [1], but having it explicitly in the source code as well wouldn't hurt in any case I'd say. Certainly something for a v2. Generally I wasn't sure how to best implement this, since another possibility would be implementing Default for the Options and then just overwriting the default values if they occur in the configuration. Thinking about it more, this might be the better way to go about this, but I think there was a reason why I opted against it, I just cannot remember it atm. I'll definitely look into it! [1] https://pve.proxmox.com/wiki/Firewall