Re: [pve-devel] [PATCH proxmox-firewall 13/37] config: firewall: add host specific config + option types

[Stefan Hanreich <s.hanreich@proxmox.com>]

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