[pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation

[Stefan Hanreich]

On 4/3/24 12:46, Max Carrara wrote:
> Four overall things I want to mention:
> 
>   1. IMO a lot of the `pub` items should eventually be documented,
>      preferably once the actual series is out. I don't think we need to
>      be as thorough as e.g. the Rust STL's documentation, but I don't
>      think it would hurt if the overall functionality of things was
>      documented. (Of course, e.g. saying that `pub fn hostname()`
>      "gets the hostname" isn't necessary; but you get what I mean :P )

As already mentioned in-line I am currently working on this.

>   2. Constants and defaults should also be documented, simply because
>      it makes it easier to refer to those defaults if necessary. On top
>      of that, it's also more obvious if those constants / defaults ever
>      have to be changed for some reason. That way we would avoid
>      accidental semver-breakage. There's a more specific example inline.

see my in-line comments in the specific patch.

>   3. Would it perhaps actually make sense to use `thiserror` instead of
>      `anyhow`? I know we've speculated a little off list about this
>      already - I still am not 100% convinced that `thiserror` is
>      necessary, but then again, it would be quite nice in the library
>      crates, as you don't really need to propagate any `anyhow::Context`
>      anyways ...
> 
>      There's already `NftError` in proxmox-nftables that *could perhaps*
>      just be implemented via `thiserror`, I think.

Yes, error handling is probably the one big thing that needs some
overhauling. Since this was a monolithic crate that I've then extracted
into 3 different crates, anyhow was used throughout the whole codebase.

Not sure if thiserror is really necessary here, just like you, probably
just custom error types would suffice imo.

>   4. Some of the types (in particular in `proxmox-ve-config` and
>      `proxmox-nftables`) could use some more trait-deriving - a lot of
>      the structs and enums could benefit from deriving `PartialOrd`,
>      `Ord` and `Hash` for interoperability's sake [0]. While it's
>      probably unlikely that some types will ever be used as keys in a
>      hashmap, deriving the trait IMO doesn't hurt.
> 
>      A lot of types also implement `PartialEq` and `Eq` only for tests,
>      but IMO those traits could theoretically just always be implemented
>      for most of them.
> 
>      As this affects a lot of types I've decided to just sum this up
>      here by the way; if you need more concrete examples, please let me
>      know and I'll add respective comments inline.

Good point, I will review the structs/enums and add additional
derivations where applicable.