On 9/26/24 08:37, Thomas Lamprecht wrote: > I'd prefer a _hook suffix for such a method for slightly added clarity. > > And FWIW, if all you do now is check privileges, and there's nothing you > already > know that's gonna get added here soon, you could just name it after what it > does > and avoid being all to generic, i.e. something like > > sub assert_privs_for_method
Thats's probably the better approach for naming this function, since I don't have any future additions in mind. >> +my $option_properties = $PVE::Firewall::vnet_option_properties; > > might need a clone to avoid modifying the original reference I think Yes, I think we never write to it - but accidents happen and better safe than sorry. Might make sense to also do this in the other API modules as well in a separate patch then. > Hmm, I'm wondering might it make sense to add a SDN.Firewall privilege to > separate > allowing VNet allocation and allowing to configure a VNet's firewall? > While adding privs is a bit tricky, this one might be dooable later one too > though. > > But whatever gets chosen should then be also addressed in a commit message > with some > background reasoning (if it's already then I might have overlooked, I did not > checked > every all too closely yet). Initial reasoning was that there's not really a privilege like that for DC / Host / Guests, where it is always tied to the networking permissions. Maybe it would make sense to create a completely separate Firewall permission that is then also used for DC / Host / Guests? Of course this could only happen in the next major release. Thanks for the review! _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
