From: Alan T. DeKok Date: Mon, 18 Jul 2022 15:45:00 +0000 (-0400) Subject: do less copying when editing lists X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=76ec55120b2f3cfe33f7872ca42f6f25c251b7ff;p=thirdparty%2Ffreeradius-server.git do less copying when editing lists --- diff --git a/src/lib/unlang/edit.c b/src/lib/unlang/edit.c index 88a05448741..7027a16e556 100644 --- a/src/lib/unlang/edit.c +++ b/src/lib/unlang/edit.c @@ -277,9 +277,11 @@ static int remove_vps(request_t *request, unlang_frame_state_edit_t *state, edit */ static int apply_edits(request_t *request, unlang_frame_state_edit_t *state, edit_map_t *current, map_t const *map) { - fr_pair_t *vp, *vp_to_free = NULL; + fr_pair_t *vp; fr_pair_list_t *children; fr_value_box_t const *rhs_box = NULL; + bool copy_vps = true; + int rcode; fr_assert(current->lhs.vp != NULL); @@ -289,6 +291,7 @@ static int apply_edits(request_t *request, unlang_frame_state_edit_t *state, edi if (!current->rhs.vpt) { children = ¤t->rhs.pair_list; + copy_vps = false; goto apply_list; } @@ -314,6 +317,7 @@ static int apply_edits(request_t *request, unlang_frame_state_edit_t *state, edi if (fr_type_is_group(da->type)) da = fr_dict_root(request->dict); children = ¤t->rhs.pair_list; + copy_vps = false; switch (rhs_box->type) { case FR_TYPE_STRING: @@ -414,15 +418,16 @@ static int apply_edits(request_t *request, unlang_frame_state_edit_t *state, edi * treats the RHS as a one-member list. */ if (fr_type_is_leaf(vp->vp_type)) { - vp_to_free = fr_pair_copy(request, vp); - if (!vp_to_free) return -1; + fr_pair_t *vp_copy; + + vp_copy = fr_pair_copy(request, vp); + if (!vp_copy) return -1; fr_assert(fr_pair_list_empty(¤t->rhs.pair_list)); - fr_pair_append(¤t->rhs.pair_list, vp_to_free); + fr_pair_append(¤t->rhs.pair_list, vp_copy); children = ¤t->rhs.pair_list; - - vp_to_free = NULL; /* it's not in the pair list, and will be freed there */ + copy_vps = false; } else { /* @@ -440,7 +445,7 @@ static int apply_edits(request_t *request, unlang_frame_state_edit_t *state, edi return -1; } - children = &vp->children; + children = &vp->vp_group; /* and copy_vps for any VP we edit */ } /* @@ -465,17 +470,14 @@ apply_list: RDEBUG2("}"); } - if (fr_edit_list_apply_list_assignment(state->el, - current->lhs.vp, - map->op, - children) < 0) { - RPERROR("Failed performing list %s operation", fr_tokens[map->op]); - talloc_free(vp_to_free); - return -1; - } + rcode = fr_edit_list_apply_list_assignment(state->el, current->lhs.vp, map->op, children, copy_vps); + if (rcode < 0) RPERROR("Failed performing list %s operation", fr_tokens[map->op]); - talloc_free(vp_to_free); - return 0; + /* + * If the child list wasn't copied, then we just created it, and we need to free it. + */ + if (!copy_vps) fr_pair_list_free(children); + return rcode; leaf: /* diff --git a/src/lib/util/edit.c b/src/lib/util/edit.c index 4c78e389c5f..bc21cb6b7ac 100644 --- a/src/lib/util/edit.c +++ b/src/lib/util/edit.c @@ -747,13 +747,21 @@ int fr_edit_list_apply_pair_assignment(fr_edit_list_t *el, fr_pair_t *vp, fr_tok return fr_value_calc_assignment_op(vp, &vp->data, op, in); } +#undef COPY +#define COPY(_x) do { if (copy) { \ + c = fr_pair_copy(dst, _x); \ + if (!c) return -1; \ + } else { \ + c = talloc_steal(dst, _x); \ + } \ + } while (0) /** A UNION B * */ -static int list_union(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) +static int list_union(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src, bool copy) { - fr_pair_t *a, *b; + fr_pair_t *a, *b, *c; fr_dcursor_t cursor1, cursor2; /* @@ -791,7 +799,9 @@ static int list_union(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) * A is done, so we always union in B at the end of A. */ if (!a) { - if (fr_edit_list_insert_pair_tail(el, &dst->children, fr_pair_copy(dst, b)) < 0) { + COPY(b); + + if (fr_edit_list_insert_pair_tail(el, &dst->children, c) < 0) { return -1; } @@ -821,7 +831,9 @@ static int list_union(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) * to the destination list, before A. */ if (rcode > 0) { - if (fr_edit_list_insert_pair_before(el, &dst->children, a, fr_pair_copy(dst, b)) < 0) { + COPY(b); + + if (fr_edit_list_insert_pair_before(el, &dst->children, a, c) < 0) { return -1; } @@ -849,7 +861,7 @@ static int list_union(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) * been added in. */ if (fr_type_is_structural(a->vp_type)) { - rcode = list_union(el, a, &b->children); + rcode = list_union(el, a, &b->children, copy); if (rcode < 0) return rcode; fr_dcursor_next(&cursor1); @@ -857,10 +869,8 @@ static int list_union(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) continue; } - /* - * - */ - if (fr_edit_list_insert_pair_after(el, &dst->children, a, fr_pair_copy(dst, b)) < 0) { + COPY(b); + if (fr_edit_list_insert_pair_after(el, &dst->children, a, c) < 0) { return -1; } @@ -877,9 +887,9 @@ static int list_union(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) * * with priority to A */ -static int list_merge_lhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) +static int list_merge_lhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src, bool copy) { - fr_pair_t *a, *b; + fr_pair_t *a, *b, *c; fr_dcursor_t cursor1, cursor2; fr_pair_list_sort(&dst->children, fr_pair_cmp_by_parent_num); @@ -903,7 +913,9 @@ static int list_merge_lhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *sr * A is done, so we always merge in B at the end of A. */ if (!a) { - if (fr_edit_list_insert_pair_tail(el, &dst->children, fr_pair_copy(dst, b)) < 0) { + COPY(b); + + if (fr_edit_list_insert_pair_tail(el, &dst->children, c) < 0) { return -1; } @@ -933,7 +945,9 @@ static int list_merge_lhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *sr * to before A. */ if (rcode > 0) { - if (fr_edit_list_insert_pair_before(el, &dst->children, a, fr_pair_copy(dst, b)) < 0) { + COPY(b); + + if (fr_edit_list_insert_pair_before(el, &dst->children, a, c) < 0) { return -1; } @@ -955,7 +969,7 @@ static int list_merge_lhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *sr fr_assert(a->da == b->da); if (fr_type_is_structural(a->vp_type)) { - rcode = list_merge_lhs(el, a, &b->children); + rcode = list_merge_lhs(el, a, &b->children, copy); if (rcode < 0) return rcode; } @@ -973,9 +987,9 @@ static int list_merge_lhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *sr * * with priority to B */ -static int list_merge_rhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src) +static int list_merge_rhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *src, bool copy) { - fr_pair_t *a, *b; + fr_pair_t *a, *b, *c; fr_dcursor_t cursor1, cursor2; fr_pair_list_sort(&dst->children, fr_pair_cmp_by_parent_num); @@ -999,7 +1013,9 @@ static int list_merge_rhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *sr * A is done, so we always merge in B at the end of A. */ if (!a) { - if (fr_edit_list_insert_pair_tail(el, &dst->children, fr_pair_copy(dst, b)) < 0) { + COPY(b); + + if (fr_edit_list_insert_pair_tail(el, &dst->children, c) < 0) { return -1; } @@ -1029,7 +1045,9 @@ static int list_merge_rhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *sr * to before A. */ if (rcode < 0) { - if (fr_edit_list_insert_pair_before(el, &dst->children, a, fr_pair_copy(dst, b)) < 0) { + COPY(b); + + if (fr_edit_list_insert_pair_before(el, &dst->children, a, c) < 0) { return -1; } @@ -1051,7 +1069,7 @@ static int list_merge_rhs(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t *sr fr_assert(a->da == b->da); if (fr_type_is_structural(a->vp_type)) { - rcode = list_merge_rhs(el, a, &b->children); + rcode = list_merge_rhs(el, a, &b->children, copy); if (rcode < 0) return rcode; } @@ -1182,9 +1200,9 @@ static int list_intersection(fr_edit_list_t *el, fr_pair_t *dst, fr_pair_list_t * * The src list is sorted, but is otherwise not modified. */ -int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_token_t op, fr_pair_list_t *src) +int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_token_t op, fr_pair_list_t *src, bool copy) { - fr_pair_list_t copy; + fr_pair_list_t list; if (!fr_type_is_structural(dst->vp_type)) { fr_strerror_printf("Cannot perform list assignment to non-structural type '%s'", @@ -1192,9 +1210,14 @@ int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_to return -1; } -#define COPY do { \ - fr_pair_list_init(©); \ - if (fr_pair_list_copy(dst, ©, src) < 0) return -1; \ +#undef COPY +#define COPY do { if (copy) { \ + fr_pair_list_init(&list); \ + if (fr_pair_list_copy(dst, &list, src) < 0) return -1;\ + src = &list; \ + } else { \ + fr_pair_list_steal(dst, src); \ + } \ } while (0) @@ -1216,7 +1239,7 @@ int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_to } COPY; - return fr_edit_list_insert_list_tail(el, &dst->children, ©); + return fr_edit_list_insert_list_tail(el, &dst->children, src); case T_OP_PREPEND: if (&dst->children == src) { @@ -1225,7 +1248,7 @@ int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_to } COPY; - return fr_edit_list_insert_list_head(el, &dst->children, ©); + return fr_edit_list_insert_list_head(el, &dst->children, src); case T_OP_AND_EQ: if (&dst->children == src) return 0; /* A INTERSECTION A == A */ @@ -1235,17 +1258,17 @@ int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_to case T_OP_OR_EQ: if (&dst->children == src) return 0; /* A UNION A == A */ - return list_union(el, dst, src); + return list_union(el, dst, src, copy); case T_OP_GE: if (&dst->children == src) return 0; /* A MERGE A == A */ - return list_merge_lhs(el, dst, src); + return list_merge_lhs(el, dst, src, copy); case T_OP_LE: if (&dst->children == src) return 0; /* A MERGE A == A */ - return list_merge_rhs(el, dst, src); + return list_merge_rhs(el, dst, src, copy); default: break; diff --git a/src/lib/util/edit.h b/src/lib/util/edit.h index 835ae7bc0e2..f3ef05a2876 100644 --- a/src/lib/util/edit.h +++ b/src/lib/util/edit.h @@ -71,7 +71,7 @@ int fr_edit_list_insert_list_after(fr_edit_list_t *el, fr_pair_list_t *list, fr_ #define fr_edit_list_insert_list_tail(_el, _list, _to_insert) fr_edit_list_insert_list_after(_el, _list, fr_pair_list_tail(_list), _to_insert) -int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_token_t op, fr_pair_list_t *src) CC_HINT(nonnull(1,2,4)); +int fr_edit_list_apply_list_assignment(fr_edit_list_t *el, fr_pair_t *dst, fr_token_t op, fr_pair_list_t *src, bool copy) CC_HINT(nonnull(1,2,4)); #ifdef __cplusplus }