Re: [PATCH pve-access-control v4 1/5] fix: #7520: sdn: prune orphaned ACLs on resource deletion

["Daniel Kral" <d.kral@proxmox.com>]

On Fri Jul 3, 2026 at 2:10 PM CEST, David Riley wrote:
> Thanks for the feedback.
> Some comments inline.
>
> On 7/2/26 3:15 PM, Daniel Kral wrote:
>> On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:

[ snip ]

>>> +sub lookup_acl_node {
>>> +    my ($root, $path) = @_;
>>> +
>>> +    my $current = $root;
>>> +    my $parent = undef;
>>> +    my $last_key = undef;
>>> +
>>> +    for my $step (@$path) {
>> nit: another patch uses $segment (instead of $step) for the same
>>       concept, so it might also make more sense to use the same term here
>>       (if this is still needed with the comment below)
>
>
> Good catch. Will adapt this to keep it consistent.
>
>>> +        if (defined($current->{children}) && defined($current->{children}->{$step})) {
>>> +            $parent = $current;
>>> +            $last_key = $step;
>>> +            $current = $current->{children}->{$step};
>>> +        } else {
>>> +            return (undef, undef, undef);
>>> +        }
>>> +    }
>>> +
>>> +    return ($parent, $last_key, $current);
>>> +}
>> If $path is a string, then this could reuse the code from the existing
>> find_acl_tree_node() subroutine. Either adapt it with a
>>
>>      return wantarray ? ($node, $parent, $last_key) : $node;
>>
>> or a wrapper
>>
>>      sub find_acl_tree_node_full($root, $path) { ... }
>>
>>      sub find_acl_tree_node($root, $path) {
>>          my ($node) = find_acl_tree_node_full($root, $path);
>>
>>          return $node;
>>      }
>>
>> No hard feelings here though.
>
>
> I did have a look at the find_acl_tree_node subroutine.
>
> However, it automatically creates intermediate nodes if they do not
> exist. This felt somewhat counterintuitive, as the goal is to remove
> these paths and not create them.
>
> Additionally, I need the parent node so I can call |delete| on that
> specific child key. Adapting find_acl_tree_node to fit my use case
> without breaking existing callers would be quite tricky and not
> worth the regression risk, in my opinion.
>

Yeah, good point about it re-creating the path nodes and not
accidentally breaking existing callers!

If you keep it, then I'd add it next to find_acl_tree_node() (maybe
align with that name, e.g. find_acl_tree_node_with_parent()?) and add a
subroutine comment what it does (differently), so others are aware that
this exists as well.

>
>>> +
>>>   my $USER_CONTROLLED_TFA_TYPES = {
>>>       u2f => 1,
>>>       oath => 1,