clean up fr_pair_list_move_op() and uses

some users didn't free the source list.  But it should really have
been freed.  So we move the free into fr_pair_list_move_op()

and do other minor cleanups of the function and users

diff --git a/src/lib/server/map.c b/src/lib/server/map.c
index f50233be8fb0db4e58c8c0e52ba2cf6209d83486..356ef6d98fafa1978387aa7a132bf79806d1263e 100644 (file)
--- a/src/lib/server/map.c
+++ b/src/lib/server/map.c
@@ -1917,14 +1917,12 @@ int map_to_request(request_t *request, map_t const *map, radius_map_getvalue_t f
                                FALL_THROUGH;
 
                case T_OP_ADD_EQ:
-                               fr_pair_list_move_op(list, &src_list, T_OP_ADD_EQ);
-                               fr_pair_list_free(&src_list);
+                       fr_pair_list_move_op(list, &src_list, T_OP_ADD_EQ);
                        }
                        goto update;
 
                case T_OP_PREPEND:
-                       fr_pair_list_move_op(list, &src_list, map->op);
-                       fr_pair_list_free(&src_list);
+                       fr_pair_list_move_op(list, &src_list, T_OP_PREPEND);
                        goto update;
 
                default:

diff --git a/src/lib/server/pairmove.c b/src/lib/server/pairmove.c
index 1f42a114320d9cbb6ce0927bba641441deee173a..d0a575ec960b992860753f09ed9188a23aa8831e 100644 (file)
--- a/src/lib/server/pairmove.c
+++ b/src/lib/server/pairmove.c
@@ -35,10 +35,7 @@ RCSID("$Id$")
 #include <ctype.h>
 
 /*
- *     The fr_pair_list_move_op() function in src/lib/valuepair.c does all sorts of
- *     extra magic that we don't want here.
- *
- *     FIXME: integrate this with the code calling it, so that we
+ *     @fixme - integrate this with the code calling it, so that we
  *     only fr_pair_list_copy() those attributes that we're really going to
  *     use.
  */
@@ -57,12 +54,7 @@ void radius_pairmove(request_t *request, fr_pair_list_t *to, fr_pair_list_t *fro
         *      deletion.
         *
         *      It also means that the operators apply ONLY to the
-        *      attributes in the original list.  With the previous
-        *      implementation of fr_pair_list_move_op(), adding two attributes
-        *      via "+=" and then "=" would mean that the second one
-        *      wasn't added, because of the existence of the first
-        *      one in the "to" list.  This implementation doesn't
-        *      have that bug.
+        *      attributes in the original list.
         *
         *      Also, the previous implementation did NOT implement
         *      "-=" correctly.  If two of the same attributes existed

diff --git a/src/lib/util/pair_legacy.c b/src/lib/util/pair_legacy.c
index 259228a5aeaf0abcb516614d5011cc42341cedc0..5901ebf4c59647ec1b2cae819b8170e211783ac0 100644 (file)
--- a/src/lib/util/pair_legacy.c
+++ b/src/lib/util/pair_legacy.c
@@ -519,8 +519,8 @@ int fr_pair_list_afrom_file(TALLOC_CTX *ctx, fr_dict_t const *dict, fr_pair_list
  */
 void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t op)
 {
-       fr_pair_t *i, *found;
-       fr_pair_list_t head_new, head_prepend;
+       fr_pair_t *vp, *next, *found;
+       fr_pair_list_t head_append, head_prepend;
 
        if (!to || fr_pair_list_empty(from)) return;
 
@@ -530,7 +530,7 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
         *      be edited, so we create an intermediate list to hold
         *      them during the editing process.
         */
-       fr_pair_list_init(&head_new);
+       fr_pair_list_init(&head_append);
 
        /*
         *      Any attributes that are requested to be prepended
@@ -542,16 +542,15 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
         *      We're looping over the "from" list, moving some
         *      attributes out, but leaving others in place.
         */
-       for (i = fr_pair_list_head(from); i; ) {
-               fr_pair_t *j;
-
-               PAIR_VERIFY(i);
+       for (vp = fr_pair_list_head(from); vp != NULL; vp = next) {
+               PAIR_VERIFY(vp);
+               next = fr_pair_list_next(from, vp);
 
                /*
                 *      We never move Fall-Through.
                 */
-               if (fr_dict_attr_is_top_level(i->da) && (i->da->attr == FR_FALL_THROUGH)) {
-                       i = fr_pair_list_next(from, i);
+               if (fr_dict_attr_is_top_level(vp->da) && (vp->da->attr == FR_FALL_THROUGH) &&
+                   (fr_dict_by_da(vp->da) == fr_dict_internal())) {
                        continue;
                }
 
@@ -561,14 +560,13 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
                 *      treatment for passwords or Hint.
                 */
 
-               switch (i->op) {
+               switch (vp->op) {
                /*
                 *      Anything else are operators which
                 *      shouldn't occur.  We ignore them, and
                 *      leave them in place.
                 */
                default:
-                       i = fr_pair_list_next(from, i);
                        continue;
 
                /*
@@ -576,10 +574,8 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
                 *      it doesn't already exist.
                 */
                case T_OP_EQ:
-                       found = fr_pair_find_by_da(to, NULL, i->da);
+                       found = fr_pair_find_by_da(to, NULL, vp->da);
                        if (!found) goto do_add;
-
-                       i = fr_pair_list_next(from, i);
                        continue;
 
                /*
@@ -587,12 +583,11 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
                 *      of the same vendor/attr which already exists.
                 */
                case T_OP_SET:
-                       found = fr_pair_find_by_da(to, NULL, i->da);
+                       found = fr_pair_find_by_da(to, NULL, vp->da);
                        if (!found) goto do_add;
 
                        /*
-                        *      Delete *all* of the attributes
-                        *      of the same number.
+                        *      Delete *all* matching attribues.
                         */
                        fr_pair_delete_by_da(to, found->da);
                        goto do_add;
@@ -603,16 +598,13 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
                 */
                case T_OP_ADD_EQ:
        do_add:
-                       j = fr_pair_list_next(from, i);
-                       fr_pair_remove(from, i);
-                       fr_pair_append(&head_new, i);
-                       i = j;
+                       fr_pair_remove(from, vp);
+                       fr_pair_append(&head_append, vp);
                        continue;
+
                case T_OP_PREPEND:
-                       j = fr_pair_list_next(from, i);
-                       fr_pair_remove(from, i);
-                       fr_pair_prepend(&head_prepend, i);
-                       i = j;
+                       fr_pair_remove(from, vp);
+                       fr_pair_prepend(&head_prepend, vp);
                        continue;
                }
        } /* loop over the "from" list. */
@@ -622,7 +614,7 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
         *      attributes first as those whose individual operator
         *      is prepend should be prepended to the resulting list
         */
-       if (op == T_OP_PREPEND) fr_pair_list_prepend(to, &head_new);
+       if (op == T_OP_PREPEND) fr_pair_list_prepend(to, &head_append);
 
        /*
         *      If there are any items in the prepend list prepend
@@ -634,6 +626,7 @@ void fr_pair_list_move_op(fr_pair_list_t *to, fr_pair_list_t *from, fr_token_t o
         *      If the op parameter was not prepend, take the "new"
         *      list, and append it to the "to" list.
         */
-       if (op != T_OP_PREPEND) fr_pair_list_append(to, &head_new);
+       if (op != T_OP_PREPEND) fr_pair_list_append(to, &head_append);
 
+       fr_pair_list_free(from);
 }

diff --git a/src/lib/util/pair_legacy_tests.c b/src/lib/util/pair_legacy_tests.c
index e4c1626f10348e99a3a9db9310df3528a19e4134..4be133e7df3b49eb4ba931d0583641f2652509f2 100644 (file)
--- a/src/lib/util/pair_legacy_tests.c
+++ b/src/lib/util/pair_legacy_tests.c
@@ -200,7 +200,6 @@ static void test_fr_pair_list_move_op(void)
        TEST_CASE("Checking if (Test-String-0 == 'Testing123')");
        TEST_CHECK(vp && strcmp(vp->vp_strvalue, "Testing123") == 0);
 
-       fr_pair_list_free(&old_list);
        fr_pair_list_free(&new_list);
 
        fclose(fp);

diff --git a/src/modules/rlm_files/rlm_files.c b/src/modules/rlm_files/rlm_files.c
index 235ea5381280e3d3676adcbcca0ce06998bad367..e351d4c785adb1364a68045e6824dcc9c621399d 100644 (file)
--- a/src/modules/rlm_files/rlm_files.c
+++ b/src/modules/rlm_files/rlm_files.c
@@ -511,7 +511,6 @@ redo:
                 *      Move the control items over, too.
                 */
                fr_pair_list_move_op(&request->control_pairs, &list, T_OP_ADD_EQ);
-               fr_pair_list_free(&list);
 
                /* ctx may be reply */
                if (!map_list_empty(&pl->reply)) {