From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <s.hanreich@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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>
<D0AFEVNE6NLP.3DTHAPFITTC2B@proxmox.com>
Content-Language: en-US
From: Stefan Hanreich <s.hanreich@proxmox.com>
In-Reply-To: <D0AFEVNE6NLP.3DTHAPFITTC2B@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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