From: Alan T. DeKok <aland@freeradius.org>
Date: Sun, 24 Dec 2023 01:22:09 +0000 (-0500)
Subject: clean up handling of := deletion
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8747ba2ac13406fd4b96b56bb45a98c72d85f8b0;p=thirdparty%2Ffreeradius-server.git

clean up handling of := deletion
---

diff --git a/src/lib/unlang/edit.c b/src/lib/unlang/edit.c
index 72fe938c298..211edb8a7f9 100644
--- a/src/lib/unlang/edit.c
+++ b/src/lib/unlang/edit.c
@@ -507,20 +507,35 @@ done:
 	return rcode;
 }
 
-static int edit_set_eq(request_t *request, edit_map_t *current, bool delete)
+static int edit_delete_lhs(request_t *request, edit_map_t *current, bool delete)
 {
 	tmpl_dcursor_ctx_t cc;
 	fr_dcursor_t cursor;
-	bool first = fr_type_is_structural(tmpl_attr_tail_da(current->lhs.vpt)->type);
+
+	/*
+	 *	These are magic.
+	 */
+	if (delete) {
+		fr_dict_attr_t const *da = tmpl_attr_tail_da(current->lhs.vpt);
+
+		if (fr_type_is_structural(da->type) &&
+		    ((da == request_attr_request) ||
+		     (da == request_attr_reply) ||
+		     (da == request_attr_control) ||
+		     (da == request_attr_state))) {
+			delete = false;
+		}
+	}
 
 	while (true) {
 		int err;
 		fr_pair_t *vp, *parent;
 
 		/*
-		 *	Reinitialize the cursor for every VP.  This is because
-		 *	fr_dcursor_remove() does not work with tmpl_dcursors, as the
-		 *	tmpl_dcursor code does not set the "remove" callback.
+		 *	Reinitialize the cursor for every VP.  This is because fr_dcursor_remove() does not
+		 *	work with tmpl_dcursors, as the tmpl_dcursor code does not set the "remove" callback.
+		 *	And the tmpl is NUM_UNSPEC, which means "the first one", whereas for T_OP_SET_EQ, we
+		 *	really mean "delete all except the first one".
 		 *
 		 *	Once that's implemented, we also need to update the edit list API to
 		 *	allow for "please delete children"?
@@ -528,51 +543,29 @@ static int edit_set_eq(request_t *request, edit_map_t *current, bool delete)
 		vp = tmpl_dcursor_init(&err, current->ctx, &cc, &cursor, request, current->lhs.vpt);
 		if (!vp) break;
 
-		/*
-		 *	For structural attributes, we leave the first one, and delete the subsequent
-		 *	ones.  That way we leave the main lists alone ("request", "reply", "control", etc.)
-		 *
-		 *	For leaf attributes, we just skip this step, as "first" is always "false".
-		 */
-		if (first) {
-			first = false;
-			if (fr_edit_list_free_pair_children(current->el, vp) < 0) return -1;
-			vp = fr_dcursor_next(&cursor);
-			if (!vp) goto clear;
-			continue;
-
-		} else if (fr_type_is_structural(tmpl_attr_tail_da(current->lhs.vpt)->type)) {
-			/*
-			 *	We skipped the first structural member, so keep skipping it for all of the next vps.
-			 */
-			vp = fr_dcursor_next(&cursor);
-			if (!vp) {
-			clear:
-				tmpl_dcursor_clear(&cc);
-				break;
-			}
-		}
-
 		parent = fr_pair_parent(vp);
 		fr_assert(parent != NULL);
 
-		/*
-		 *	We can't delete these ones.
-		 */
-		fr_assert(vp != request->pair_list.request);
-		fr_assert(vp != request->pair_list.reply);
-		fr_assert(vp != request->pair_list.control);
-		fr_assert(vp != request->pair_list.state);
+		if (!delete) {
+			if (fr_type_is_structural(vp->vp_type)) {
+				if (fr_edit_list_free_pair_children(current->el, vp) < 0) return -1;
+			} else {
+				/*
+				 *	No need to save value, as fr_edit_list_apply_pair_assignment() will do
+				 *	that for us.
+				 */
+			}
 
-		/*
-		 *	Delete if we're not over-riding a particular value.
-		 */
-		if (delete) {
-			if (fr_edit_list_pair_delete(current->el, &parent->vp_group, vp) < 0) return -1;
+			current->lhs.vp = vp;
 			tmpl_dcursor_clear(&cc);
-		} else {
-			goto clear;
+			return 0;
 		}
+
+		/*
+		 *	Delete all of them.  We'll create one later for the SET operation.
+		 */
+		if (fr_edit_list_pair_delete(current->el, &parent->vp_group, vp) < 0) return -1;
+		tmpl_dcursor_clear(&cc);
 	}
 
 	return 0;
@@ -623,7 +616,8 @@ static int apply_edits_to_leaf(request_t *request, unlang_frame_state_edit_t *st
 
 	if (!current->rhs.vpt) {
 		/*
-		 *	There's no RHS tmpl, so the result must be in in the parent RHS tmpl as data, OR in the RHS result list.
+		 *	There's no RHS tmpl, so the result must be in in the parent RHS tmpl as data, OR in
+		 *	the RHS result list.
 		 */
 		if (tmpl_is_data(map->rhs)) {
 			box = tmpl_value(map->rhs);
@@ -672,7 +666,7 @@ static int apply_edits_to_leaf(request_t *request, unlang_frame_state_edit_t *st
 		fr_assert(tmpl_is_attr(current->rhs.vpt));
 
 		/*
-		 *	Get a cursor over the pairs.  If there are no matching pairs, then we do nothing.
+		 *	Get a cursor over the RHS pairs.
 		 */
 		vp = tmpl_dcursor_init(&err, request, &cc, &pair_cursor, request, current->rhs.vpt);
 		if (!vp) {
@@ -680,7 +674,10 @@ static int apply_edits_to_leaf(request_t *request, unlang_frame_state_edit_t *st
 
 			if (map->op != T_OP_SET) return 0;
 
-			return edit_set_eq(request, current, true);
+			/*
+			 *	No RHS pairs means we can finally delete all of the LHS.
+			 */
+			return edit_delete_lhs(request, current, true);
 		}
 
 		box = fr_pair_dcursor_nested_init(&cursor, &pair_cursor); // the list is unused
@@ -958,7 +955,7 @@ static int check_rhs(request_t *request, unlang_frame_state_edit_t *state, edit_
 	if ((map->op == T_OP_SET) &&
 	    ((tmpl_attr_tail_num(current->lhs.vpt) == NUM_UNSPEC) || (tmpl_attr_tail_num(current->lhs.vpt) > 0) ||
 	     !current->map->rhs)) {
-		if (edit_set_eq(request, current,
+		if (edit_delete_lhs(request, current,
 				(tmpl_attr_tail_num(current->lhs.vpt) == NUM_UNSPEC) || !current->map->rhs) < 0) return -1;
 	}