From: Alan T. DeKok Date: Tue, 12 Dec 2023 21:15:56 +0000 (-0500) Subject: allow for nested edit lists X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cee0d0e4ac2190b532f4f35d30ccc9f01c41a941;p=thirdparty%2Ffreeradius-server.git allow for nested edit lists --- diff --git a/src/lib/unlang/edit.c b/src/lib/unlang/edit.c index 36f288d6c07..cf456cbdf02 100644 --- a/src/lib/unlang/edit.c +++ b/src/lib/unlang/edit.c @@ -1587,7 +1587,7 @@ static void edit_state_init_internal(request_t *request, unlang_frame_state_edit * generally be large enough for most edits. */ if (!el) { - MEM(state->el = fr_edit_list_alloc(state, map_list_num_elements(map_list))); + MEM(state->el = fr_edit_list_alloc(state, map_list_num_elements(map_list), NULL)); } current->ctx = state; diff --git a/src/lib/util/edit.c b/src/lib/util/edit.c index 783b8a8cee6..132ed051cd7 100644 --- a/src/lib/util/edit.c +++ b/src/lib/util/edit.c @@ -58,6 +58,7 @@ typedef enum { FR_EDIT_VALUE, //!< edit a VP in place FR_EDIT_CLEAR, //!< clear the children of a structural entry. FR_EDIT_INSERT, //!< insert a VP into a list, after another one. + FR_EDIT_CHILD, //!< child edit list } fr_edit_op_t; #if 0 @@ -73,24 +74,6 @@ static const char *edit_names[4] = { }; #endif -/** Track a series of edits. - * - */ -struct fr_edit_list_s { - /* - * List of undo changes to be made, in order. - */ - fr_dlist_head_t undo; - - fr_dlist_head_t ignore; //!< lists to ignore - - /* - * VPs which were inserted, and then over-written by a - * later edit. - */ - fr_pair_list_t deleted_pairs; -}; - /** Track one particular edit. */ typedef struct { @@ -103,6 +86,7 @@ typedef struct { union { fr_value_box_t data; //!< original data fr_pair_list_t children; //!< original child list, for "clear" + fr_edit_list_t *child_edit; }; struct { @@ -112,6 +96,27 @@ typedef struct { }; } fr_edit_t; +/** Track a series of edits. + * + */ +struct fr_edit_list_s { + /* + * List of undo changes to be made, in order. + */ + fr_dlist_head_t undo; + + fr_dlist_head_t ignore; //!< lists to ignore + + /* + * VPs which were inserted, and then over-written by a + * later edit. + */ + fr_pair_list_t deleted_pairs; + + fr_edit_list_t *parent; //!< for nested transactions + fr_edit_t *e; //!< so we don't have to loop over parent edits on abort +}; + typedef struct { fr_dlist_t entry; fr_pair_list_t *list; //!< list to ignore (never dereferenced) @@ -170,6 +175,10 @@ static int edit_undo(fr_edit_t *e) */ fr_pair_delete(e->list, vp); break; + + case FR_EDIT_CHILD: + fr_edit_list_abort(e->child_edit); + break; } return 0; @@ -210,6 +219,14 @@ void fr_edit_list_abort(fr_edit_list_t *el) */ } + /* + * There's a parent, we remove ourselves from the parent undo list. + */ + if (el->parent) { + fr_dlist_remove(&el->parent->undo, el->e); + talloc_free(el->e); + } + talloc_free(el); } @@ -234,6 +251,8 @@ static int edit_record(fr_edit_list_t *el, fr_edit_op_t op, fr_pair_t *vp, fr_pa fr_assert(el != NULL); fr_assert(vp != NULL); + fr_assert(op != FR_EDIT_CHILD); /* only used by fr_edit_alloc() */ + /* * When we insert a structural type, we also want to * not track edits to it's children. The "ignore list" @@ -449,6 +468,10 @@ static int edit_record(fr_edit_list_t *el, fr_edit_op_t op, fr_pair_t *vp, fr_pa e->op = FR_EDIT_DELETE; fr_dlist_remove(&el->undo, e); goto delete; + + case FR_EDIT_CHILD: /* e->vp==NULL, so this should never happen */ + fr_assert(0); + return -1; } } /* loop over existing edits */ @@ -464,6 +487,7 @@ static int edit_record(fr_edit_list_t *el, fr_edit_op_t op, fr_pair_t *vp, fr_pa switch (op) { case FR_EDIT_INVALID: + case FR_EDIT_CHILD: fail: talloc_free(e); return -1; @@ -739,6 +763,10 @@ static int _edit_list_destructor(fr_edit_list_t *el) fr_assert(fr_type_is_leaf(e->vp->vp_type)); fr_value_box_clear(&e->data); break; + + case FR_EDIT_CHILD: + talloc_free(e->child_edit); + break; } } @@ -749,9 +777,27 @@ static int _edit_list_destructor(fr_edit_list_t *el) return 0; } -fr_edit_list_t *fr_edit_list_alloc(TALLOC_CTX *ctx, int hint) +/** Allocate an edit list. + * + * Edit lists can be nested. If the child list commits, then it does nothing + * until the parent list commits. On the other hand, if the child list aborts, + * then the parent list might continue. + * + * @param ctx talloc context. Ignored if parent!=NULL + * @param hint how many edits we are likely to allocate + * @param parent a parent edit list + */ +fr_edit_list_t *fr_edit_list_alloc(TALLOC_CTX *ctx, int hint, fr_edit_list_t *parent) { fr_edit_list_t *el; + fr_edit_t *e; + + /* + * If we have nested transactions, then allocate this + * list in the context of the parent. Otherwise it will + * be freed too soon. + */ + if (parent) ctx = parent; el = talloc_zero_pooled_object(ctx, fr_edit_list_t, hint, hint * sizeof(fr_edit_t)); if (!el) return NULL; @@ -763,9 +809,42 @@ fr_edit_list_t *fr_edit_list_alloc(TALLOC_CTX *ctx, int hint) talloc_set_destructor(el, _edit_list_destructor); + el->parent = parent; + + if (!parent) return el; + + e = talloc_zero(parent, fr_edit_t); + if (!e) { + talloc_free(el); + return NULL; + } + + /* + * Insert the child into the tail of the current edit list. + */ + e->op = FR_EDIT_CHILD; + e->vp = NULL; + e->child_edit = el; + + fr_dlist_insert_tail(&parent->undo, e); + el->e = e; + return el; } +/** Commit an edit list. + * + * If there are nested transactions, then this transaction is + * committed only when the parent transaction has been commited. + * + */ +void fr_edit_list_commit(fr_edit_list_t *el) +{ + if (el->parent) return; + + talloc_free(el); +} + /** Notes * * Unlike "update" sections, edits are _not_ hierarchical. If we're diff --git a/src/lib/util/edit.h b/src/lib/util/edit.h index 3cc0d66cf34..2a5fd474dec 100644 --- a/src/lib/util/edit.h +++ b/src/lib/util/edit.h @@ -33,11 +33,11 @@ extern "C" { typedef struct fr_edit_list_s fr_edit_list_t; -fr_edit_list_t *fr_edit_list_alloc(TALLOC_CTX *ctx, int hint); +fr_edit_list_t *fr_edit_list_alloc(TALLOC_CTX *ctx, int hint, fr_edit_list_t *parent); void fr_edit_list_abort(fr_edit_list_t *el); -#define fr_edit_list_commit(_x) talloc_free(_x) +void fr_edit_list_commit(fr_edit_list_t *el); /* * Functions to modify #fr_pair_t diff --git a/src/lib/util/edit_tests.c b/src/lib/util/edit_tests.c index c992fe7a153..a16fac18c39 100644 --- a/src/lib/util/edit_tests.c +++ b/src/lib/util/edit_tests.c @@ -141,7 +141,7 @@ static void test_pair_delete_head(void) vp = fr_pair_list_head(&local_pairs); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_pair_delete(el, &local_pairs, vp); @@ -174,7 +174,7 @@ static void test_pair_delete_head_abort(void) vp = fr_pair_list_head(&local_pairs); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_pair_delete(el, &local_pairs, vp); @@ -208,7 +208,7 @@ static void test_pair_delete_middle(void) vp = fr_pair_list_next(&local_pairs, vp); fr_assert(vp != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_pair_delete(el, &local_pairs, vp); @@ -247,7 +247,7 @@ static void test_pair_delete_middle_abort(void) fr_assert(middle != NULL); TEST_CHECK(middle->da == fr_dict_attr_test_octets); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_pair_delete(el, &local_pairs, middle); @@ -287,7 +287,7 @@ static void test_pair_delete_multiple(void) vp = fr_pair_list_next(&local_pairs, vp); fr_assert(vp != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_pair_delete(el, &local_pairs, vp); /* middle */ @@ -332,7 +332,7 @@ static void test_pair_delete_multiple_abort(void) fr_assert(vp != NULL); TEST_CHECK(vp->da == fr_dict_attr_test_octets); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_pair_delete(el, &local_pairs, vp); /* middle */ @@ -377,7 +377,7 @@ static void test_pair_edit_value(void) vp = fr_pair_list_head(&local_pairs); fr_assert(vp != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_save_pair_value(el, vp); @@ -411,7 +411,7 @@ static void test_pair_edit_value_abort(void) vp = fr_pair_list_head(&local_pairs); fr_assert(vp != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_save_pair_value(el, vp); @@ -446,7 +446,7 @@ static void test_pair_insert_after_head(void) add_pairs(&local_pairs); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); TEST_CHECK((vp = fr_pair_afrom_da(autofree, fr_dict_attr_test_string)) != NULL); @@ -479,7 +479,7 @@ static void test_pair_insert_after_head_abort(void) add_pairs(&local_pairs); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); TEST_CHECK((vp = fr_pair_afrom_da(autofree, fr_dict_attr_test_string)) != NULL); @@ -515,7 +515,7 @@ static void test_pair_insert_after_middle(void) middle = fr_pair_list_next(&local_pairs, vp); fr_assert(middle != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); TEST_CHECK((vp = fr_pair_afrom_da(autofree, fr_dict_attr_test_string)) != NULL); @@ -548,7 +548,7 @@ static void test_pair_insert_after_middle_abort(void) middle = fr_pair_list_next(&local_pairs, vp); fr_assert(middle != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); TEST_CHECK((vp = fr_pair_afrom_da(autofree, fr_dict_attr_test_string)) != NULL); @@ -582,7 +582,7 @@ static void test_pair_edit_value_delete(void) vp = fr_pair_list_head(&local_pairs); fr_assert(vp != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_save_pair_value(el, vp); @@ -620,7 +620,7 @@ static void test_pair_edit_value_delete_abort(void) vp = fr_pair_list_head(&local_pairs); fr_assert(vp != NULL); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); rcode = fr_edit_list_save_pair_value(el, vp); @@ -658,7 +658,7 @@ static void test_pair_insert_after_head_delete(void) add_pairs(&local_pairs); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); TEST_CHECK((vp = fr_pair_afrom_da(autofree, fr_dict_attr_test_string)) != NULL); @@ -694,7 +694,7 @@ static void test_pair_insert_after_head_delete_abort(void) add_pairs(&local_pairs); - el = fr_edit_list_alloc(NULL, 5); + el = fr_edit_list_alloc(NULL, 5, NULL); fr_assert(el != NULL); TEST_CHECK((vp = fr_pair_afrom_da(autofree, fr_dict_attr_test_string)) != NULL); @@ -715,6 +715,104 @@ static void test_pair_insert_after_head_delete_abort(void) } +static void test_pair_edit_child_value(void) +{ + fr_pair_t *vp; + fr_pair_list_t local_pairs; + fr_edit_list_t *el, *child; + int rcode; + + TEST_CASE("Add 3 pairs and change the value of the first one in a child transaction"); + + add_pairs(&local_pairs); + + vp = fr_pair_list_head(&local_pairs); + fr_assert(vp != NULL); + + el = fr_edit_list_alloc(NULL, 5, NULL); + fr_assert(el != NULL); + + rcode = fr_edit_list_save_pair_value(el, vp); + TEST_CHECK(rcode == 0); + + TEST_CHECK(vp->vp_uint32 == 0); + + vp->vp_uint32 = 1; + TEST_CHECK(vp->vp_uint32 == 1); + + child = fr_edit_list_alloc(NULL, 5, el); + fr_assert(child != NULL); + + rcode = fr_edit_list_save_pair_value(child, vp); /* CHILD */ + TEST_CHECK(rcode == 0); + + TEST_CHECK(vp->vp_uint32 == 1); + + vp->vp_uint32 = 2; + TEST_CHECK(vp->vp_uint32 == 2); + + fr_edit_list_commit(child); /* should do nothing */ + + TEST_CHECK(vp->vp_uint32 == 2); + + fr_edit_list_commit(el); + + vp = fr_pair_list_head(&local_pairs); + TEST_CHECK(vp->da == fr_dict_attr_test_uint32); + TEST_CHECK(vp->vp_uint32 == 2); + + expect3(&local_pairs); +} + +static void test_pair_edit_child_value_abort(void) +{ + fr_pair_t *vp; + fr_pair_list_t local_pairs; + fr_edit_list_t *el, *child; + int rcode; + + TEST_CASE("Add 3 pairs and change the value of the first one, then abort"); + + add_pairs(&local_pairs); + + vp = fr_pair_list_head(&local_pairs); + fr_assert(vp != NULL); + + el = fr_edit_list_alloc(NULL, 5, NULL); + fr_assert(el != NULL); + + rcode = fr_edit_list_save_pair_value(el, vp); + TEST_CHECK(rcode == 0); + + TEST_CHECK(vp->vp_uint32 == 0); + + vp->vp_uint32 = 1; + TEST_CHECK(vp->vp_uint32 == 1); + + child = fr_edit_list_alloc(NULL, 5, el); + fr_assert(child != NULL); + + rcode = fr_edit_list_save_pair_value(child, vp); /* CHILD */ + TEST_CHECK(rcode == 0); + + TEST_CHECK(vp->vp_uint32 == 1); + + vp->vp_uint32 = 2; + TEST_CHECK(vp->vp_uint32 == 2); + + fr_edit_list_abort(child); /* CHILD */ + + TEST_CHECK(vp->vp_uint32 == 1); + + fr_edit_list_commit(el); + + vp = fr_pair_list_head(&local_pairs); + TEST_CHECK(vp->da == fr_dict_attr_test_uint32); + TEST_CHECK(vp->vp_uint32 == 1); + + expect3(&local_pairs); +} + TEST_LIST = { /* * Deletion. @@ -755,5 +853,11 @@ TEST_LIST = { { "pair_insert_after_head_delete", test_pair_insert_after_head_delete }, { "pair_insert_after_head_delete_abort", test_pair_insert_after_head_delete_abort }, + /* + * Value modification in child list + */ + { "pair_edit_child_value", test_pair_edit_child_value }, + { "pair_edit_child_value_abort", test_pair_edit_child_value_abort }, + { NULL } };