From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 9C64A1FF142 for ; Fri, 03 Jul 2026 14:21:27 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A50BB21255; Fri, 03 Jul 2026 14:21:26 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 03 Jul 2026 14:21:20 +0200 Message-Id: To: "David Riley" , Subject: Re: [PATCH pve-access-control v4 1/5] fix: #7520: sdn: prune orphaned ACLs on resource deletion From: "Daniel Kral" X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260626105258.56914-1-d.riley@proxmox.com> <20260626105258.56914-2-d.riley@proxmox.com> <8d7a699c-f820-49c0-939b-e4a91668b50d@proxmox.com> In-Reply-To: <8d7a699c-f820-49c0-939b-e4a91668b50d@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783081273192 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.037 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: QAGRQLRIF2M2B7GRHVVZBWPK6UYR4KWE X-Message-ID-Hash: QAGRQLRIF2M2B7GRHVVZBWPK6UYR4KWE X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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) =3D @_; >>> + >>> + my $current =3D $root; >>> + my $parent =3D undef; >>> + my $last_key =3D 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 her= e >> (if this is still needed with the comment below) > > > Good catch. Will adapt this to keep it consistent. > >>> + if (defined($current->{children}) && defined($current->{childr= en}->{$step})) { >>> + $parent =3D $current; >>> + $last_key =3D $step; >>> + $current =3D $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) =3D find_acl_tree_node_full($root, $path); >> >> return $node; >> } >> >> No hard feelings here though. > > > I did have a look at the=C2=A0find_acl_tree_node=C2=A0subroutine. > > 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 =3D { >>> u2f =3D> 1, >>> oath =3D> 1,