add section / pair / data in alloc functions, not elsewhere

on review, most of the calls to cf_section_add() were wrong.
the child section was already added to the parent section by
_cf_section_alloc().  There were many, many, calls to
cf_item_add() or cf_section_add() which resulted in the
section being added twice.

However, cf_pair_alloc() and cf_data_alloc() did not add the item
to the parent.

In the interest of less code and fewer errors, we've removed
cf_section_add() and cf_pair_add().  The calls to cf_item_add()
are now done only from the alloc functions.

We still have many calls to cf_data_add(), because that function
is in fact allocating *and* adding the CONF_DATA items.

diff --git a/src/bin/unit_test_module.c b/src/bin/unit_test_module.c
index 350e1382f6dd4512e81c9ba818f8fd2bee352fd2..25f702ec55ab9d2bba736fb603ed3f6df138ed26 100644 (file)
--- a/src/bin/unit_test_module.c
+++ b/src/bin/unit_test_module.c
@@ -106,25 +106,12 @@ static RADCLIENT *client_alloc(TALLOC_CTX *ctx, char const *ip, char const *name
 
        cs = cf_section_alloc(ctx, NULL, "client", name);
        cp = cf_pair_alloc(cs, "ipaddr", ip, T_OP_EQ, T_BARE_WORD, T_BARE_WORD);
-       cf_pair_add(cs, cp);
-
        cp = cf_pair_alloc(cs, "secret", "supersecret", T_OP_EQ, T_BARE_WORD, T_DOUBLE_QUOTED_STRING);
-       cf_pair_add(cs, cp);
-
        cp = cf_pair_alloc(cs, "nas_type", "test", T_OP_EQ, T_BARE_WORD, T_DOUBLE_QUOTED_STRING);
-       cf_pair_add(cs, cp);
-
        cp = cf_pair_alloc(cs, "shortname", "test", T_OP_EQ, T_BARE_WORD, T_DOUBLE_QUOTED_STRING);
-       cf_pair_add(cs, cp);
-
        cp = cf_pair_alloc(cs, "groups", "foo", T_OP_EQ, T_BARE_WORD, T_DOUBLE_QUOTED_STRING);
-       cf_pair_add(cs, cp);
-
        cp = cf_pair_alloc(cs, "groups", "bar", T_OP_EQ, T_BARE_WORD, T_DOUBLE_QUOTED_STRING);
-       cf_pair_add(cs, cp);
-
        cp = cf_pair_alloc(cs, "groups", "baz", T_OP_EQ, T_BARE_WORD, T_DOUBLE_QUOTED_STRING);
-       cf_pair_add(cs, cp);
 
        client = client_afrom_cs(ctx, cs, NULL);
        if (!client) {

diff --git a/src/lib/server/cf_file.c b/src/lib/server/cf_file.c
index 8131469ea73d63738a82cc5b5c5ff80658d5be18..73596b1ce2ea1431a1ee57b438f7ce84b4e053c7 100644 (file)
--- a/src/lib/server/cf_file.c
+++ b/src/lib/server/cf_file.c
@@ -321,7 +321,6 @@ char const *cf_expand_variables(char const *cf, int lineno,
 
                                cf_filename_set(subcs, ci->filename);
                                cf_lineno_set(subcs, ci->lineno);
-                               cf_item_add(outer_cs, &(subcs->item));
 
                                ptr = next + 1;
 
@@ -436,7 +435,6 @@ static bool cf_template_merge(CONF_SECTION *cs, CONF_SECTION const *template)
 
                        cf_filename_set(cp2, cp1->item.filename);
                        cf_lineno_set(cp2, cp1->item.lineno);
-                       cf_item_add(cs, &(cp2->item));
                        continue;
                }
 
@@ -469,7 +467,6 @@ static bool cf_template_merge(CONF_SECTION *cs, CONF_SECTION const *template)
 
                        cf_filename_set(subcs2, subcs1->item.filename);
                        cf_lineno_set(subcs2, subcs1->item.lineno);
-                       cf_item_add(cs, &(subcs2->item));
                        continue;
                }
 
@@ -1565,7 +1562,6 @@ static int add_pair(CONF_SECTION *parent, char const *attr, char const *value,
        cf_filename_set(cp, filename);
        cf_lineno_set(cp, lineno);
        cp->pass2 = pass2;
-       cf_item_add(parent, &(cp->item));
 
        cd = cf_data_find(CF_TO_ITEM(parent), CONF_PARSER, attr);
        if (!cd) return 0;
@@ -2326,8 +2322,6 @@ int cf_file_read(CONF_SECTION *cs, char const *filename)
        p = strrchr(cp->value, FR_DIR_SEP);
        if (p) *p = '\0';
 
-       cf_item_add(cs, &(cp->item));
-
        MEM(tree = rbtree_talloc_alloc(cs, cf_file_t, node, _inode_cmp, NULL, 0));
 
        cf_data_add(cs, tree, "filename", false);

diff --git a/src/lib/server/cf_parse.c b/src/lib/server/cf_parse.c
index cd53539518aa668f97c9314c02201de70b862df9..f903ee5f8206ae84acf113a003b5649f92848d6c 100644 (file)
--- a/src/lib/server/cf_parse.c
+++ b/src/lib/server/cf_parse.c
@@ -831,7 +831,6 @@ static int CC_HINT(nonnull(4,5)) cf_pair_parse_internal(TALLOC_CTX *ctx, void *o
                        ret = func(value_ctx, entry, base, cf_pair_to_item(cp), rule);
                        if (ret < 0) {
                                talloc_free(array);
-                               talloc_free(dflt_cp);
                                return -1;
                        }
                        cp->parsed = true;
@@ -880,23 +879,10 @@ static int CC_HINT(nonnull(4,5)) cf_pair_parse_internal(TALLOC_CTX *ctx, void *o
                }
 
                ret = func(ctx, out, base, cf_pair_to_item(cp), rule);
-               if (ret < 0) {
-                       talloc_free(dflt_cp);
-                       return -1;
-               }
+               if (ret < 0) return -1;
                cp->parsed = true;
        }
 
-       /*
-        *      If we created a default cp and succeeded
-        *      in parsing the dflt value, add the new
-        *      cp to the enclosing section.
-        */
-       if (dflt_cp) {
-               cf_item_add(cs, &(dflt_cp->item));
-               return 1;
-       }
-
        return 0;
 }
 

diff --git a/src/lib/server/cf_util.c b/src/lib/server/cf_util.c
index 1f7717d82c799eacd32ad915e790382dc6fcc400..9ab8e077102b85bed0ce5fd1d590797786cb64bf 100644 (file)
--- a/src/lib/server/cf_util.c
+++ b/src/lib/server/cf_util.c
@@ -854,7 +854,6 @@ CONF_SECTION *cf_section_dup(TALLOC_CTX *ctx, CONF_SECTION *parent, CONF_SECTION
                                talloc_free(new);
                                return NULL;
                        }
-                       cf_section_add(new, subcs);
                        break;
 
                case CONF_ITEM_PAIR:
@@ -863,7 +862,6 @@ CONF_SECTION *cf_section_dup(TALLOC_CTX *ctx, CONF_SECTION *parent, CONF_SECTION
                                talloc_free(new);
                                return NULL;
                        }
-                       cf_pair_add(new, cp);
                        break;
 
                case CONF_ITEM_DATA: /* Skip data */
@@ -877,17 +875,6 @@ CONF_SECTION *cf_section_dup(TALLOC_CTX *ctx, CONF_SECTION *parent, CONF_SECTION
        return new;
 }
 
-/** Add a section as a child of another section
- *
- * @param[in] parent   section we're adding to.
- * @param[in] cs       we're adding.
- *
- * @hidecallergraph
- */
-void cf_section_add(CONF_SECTION *parent, CONF_SECTION *cs)
-{
-       cf_item_add(parent, &(cs->item));
-}
 
 /** Return the next child that's a #CONF_SECTION
  *
@@ -1122,6 +1109,7 @@ CONF_PAIR *cf_pair_alloc(CONF_SECTION *parent, char const *attr, char const *val
                if (!cp->value) goto error;
        }
 
+       cf_item_add(parent, &(cp->item));
        return cp;
 }
 
@@ -1185,15 +1173,6 @@ int cf_pair_replace(CONF_SECTION *cs, CONF_PAIR *cp, char const *value)
        return 0;
 }
 
-/** Add a configuration pair to a section
- *
- * @param[in] parent section to add pair to.
- * @param[in] cp to add.
- */
-void cf_pair_add(CONF_SECTION *parent, CONF_PAIR *cp)
-{
-       cf_item_add(parent, cf_pair_to_item(cp));
-}
 
 /** Mark a pair as parsed
  *
@@ -1430,6 +1409,7 @@ static CONF_DATA *cf_data_alloc(CONF_ITEM *parent, void const *data, char const
                talloc_set_destructor(cd, _cd_free);
        }
 
+       cf_item_add(parent, cd);
        return cd;
 }
 
@@ -1547,7 +1527,6 @@ CONF_DATA const *_cf_data_add(CONF_ITEM *ci, void const *data, char const *name,
        cd->is_talloced = true;
        cf_filename_set(cd, filename);
        cf_lineno_set(cd, lineno);
-       cf_item_add(ci, cd);
 
        return cd;
 }
@@ -1592,7 +1571,6 @@ CONF_DATA const *_cf_data_add_static(CONF_ITEM *ci, void const *data, char const
        cd->is_talloced = false;
        cf_filename_set(cd, filename);
        cf_lineno_set(cd, lineno);
-       cf_item_add(ci, cd);
 
        return cd;
 }

diff --git a/src/lib/server/cf_util.h b/src/lib/server/cf_util.h
index efd580358d6918f75bf030752c37a80fc7e0a3ad..6ca8e2f3f1ba94068dd4ce765fea2bdfa161aa15 100644 (file)
--- a/src/lib/server/cf_util.h
+++ b/src/lib/server/cf_util.h
@@ -140,7 +140,6 @@ CONF_SECTION        *_cf_section_alloc(TALLOC_CTX *ctx, CONF_SECTION *parent,
                                   char const *filename, int lineno);
 CONF_SECTION   *cf_section_dup(TALLOC_CTX *ctx, CONF_SECTION *parent, CONF_SECTION const *cs,
                                char const *name1, char const *name2, bool copy_meta);
-void           cf_section_add(CONF_SECTION *parent, CONF_SECTION *cs);
 /** @hidecallergraph */
 CONF_SECTION   *cf_section_next(CONF_SECTION const *cs, CONF_SECTION const *prev);
 /** @hidecallergraph */
@@ -170,7 +169,6 @@ CONF_PAIR   *cf_pair_alloc(CONF_SECTION *parent, char const *attr, char const *val
                               fr_token_t op, fr_token_t lhs_type, fr_token_t rhs_type);
 CONF_PAIR      *cf_pair_dup(CONF_SECTION *parent, CONF_PAIR *cp);
 int            cf_pair_replace(CONF_SECTION *cs, CONF_PAIR *cp, char const *value);
-void           cf_pair_add(CONF_SECTION *parent, CONF_PAIR *cp);
 void           cf_pair_mark_parsed(CONF_PAIR *cp);
 CONF_PAIR      *cf_pair_next(CONF_SECTION const *cs, CONF_PAIR const *prev);
 CONF_PAIR      *cf_pair_find(CONF_SECTION const *cs, char const *name);

diff --git a/src/lib/server/client.c b/src/lib/server/client.c
index 0119c44eb76e88b2a5f83f46b7c26b0369f9af03..c17a98c525d3ff7ebf534265154c2cc962027fb9 100644 (file)
--- a/src/lib/server/client.c
+++ b/src/lib/server/client.c
@@ -634,7 +634,6 @@ int client_map_section(CONF_SECTION *out, CONF_SECTION const *map, client_value_
                        cc = cf_section_find(out, cf_section_name1(cs), cf_section_name2(cs));
                        if (!cc) {
                                cc = cf_section_alloc(out, out, cf_section_name1(cs), cf_section_name2(cs));
-                               cf_section_add(out, cc);
                                if (!cc) return -1;
                        }
 
@@ -1025,7 +1024,6 @@ RADCLIENT *client_afrom_request(TALLOC_CTX *ctx, request_t *request)
                }
 
                RDEBUG2("%s = %s", cf_pair_attr(cp), cf_pair_value(cp));
-               cf_pair_add(cs, cp);
        }
 
        REXDENT();

diff --git a/src/lib/server/dependency.c b/src/lib/server/dependency.c
index cf87f6fe618a02ef815abe94f3fe2648abb8ba9d..56adb3e200294493df0cbef1b7938a981208184d 100644 (file)
--- a/src/lib/server/dependency.c
+++ b/src/lib/server/dependency.c
@@ -290,7 +290,6 @@ int dependency_feature_add(CONF_SECTION *cs, char const *name, bool enabled)
                cp = cf_pair_alloc(cs, name, enabled ? "yes" : "no",
                                   T_OP_EQ, T_BARE_WORD, T_BARE_WORD);
                if (!cp) return -1;
-               cf_pair_add(cs, cp);
        }
 
        return 0;
@@ -333,7 +332,6 @@ int dependency_version_number_add(CONF_SECTION *cs, char const *name, char const
                cp = cf_pair_alloc(cs, name, version, T_OP_EQ, T_BARE_WORD, T_SINGLE_QUOTED_STRING);
                if (!cp) return -1;
 
-               cf_pair_add(cs, cp);
        } else {
                WARN("Replacing user version.%s (%s) with %s", name, cf_pair_value(old), version);
 

diff --git a/src/lib/server/main_config.c b/src/lib/server/main_config.c
index 056a4dcf21f3fb7c8c5a404846ef742479b5bf4f..3858e8d577e87d81cffcee8259c12a74a4ba7d1d 100644 (file)
--- a/src/lib/server/main_config.c
+++ b/src/lib/server/main_config.c
@@ -1005,7 +1005,6 @@ do {\
                        }
                } else {
                        MEM(cp = cf_pair_alloc(cs, "name", config->name, T_OP_EQ, T_BARE_WORD, T_DOUBLE_QUOTED_STRING));
-                       cf_pair_add(cs, cp);
                }
        }
 

diff --git a/src/listen/radius/proto_radius.c b/src/listen/radius/proto_radius.c
index 8eeb318e64bb7c2523bd23e3c0c85c122ca7285a..838936be161f3341949f8d117f2c1a772b5ed433 100644 (file)
--- a/src/listen/radius/proto_radius.c
+++ b/src/listen/radius/proto_radius.c
@@ -175,7 +175,6 @@ static int transport_parse(TALLOC_CTX *ctx, void *out, UNUSED void *parent, CONF
         */
        if (!transport_cs) {
                transport_cs = cf_section_alloc(listen_cs, listen_cs, name, NULL);
-               cf_section_add(listen_cs, transport_cs);
                inst->io.app_io_conf = transport_cs;
        }
 

diff --git a/src/listen/tacacs/proto_tacacs.c b/src/listen/tacacs/proto_tacacs.c
index e22b3ca338f33d9b4fbdc2c7338c7e42e845ab6e..35e118cb52a917cc0cec7fa914e318a4b8f76638 100644 (file)
--- a/src/listen/tacacs/proto_tacacs.c
+++ b/src/listen/tacacs/proto_tacacs.c
@@ -151,7 +151,6 @@ static int transport_parse(TALLOC_CTX *ctx, void *out, UNUSED void *parent, CONF
         */
        if (!transport_cs) {
                transport_cs = cf_section_alloc(listen_cs, listen_cs, name, NULL);
-               cf_section_add(listen_cs, transport_cs);
                inst->io.app_io_conf = transport_cs;
        }
 

diff --git a/src/modules/proto_ldap_sync/proto_ldap_sync.c b/src/modules/proto_ldap_sync/proto_ldap_sync.c
index d84e39f6e2576eab22172576b2cbb94dd00d111c..4b0de34960ab481800c13106ad908cd194b395a6 100644 (file)
--- a/src/modules/proto_ldap_sync/proto_ldap_sync.c
+++ b/src/modules/proto_ldap_sync/proto_ldap_sync.c
@@ -785,9 +785,7 @@ static RADCLIENT *proto_ldap_fake_client_alloc(proto_ldap_inst_t *inst)
        cs = cf_section_alloc(NULL, NULL, "client", "ldap");
        cp = cf_pair_alloc(cs, "ipaddr", fr_inet_ntop(buffer, sizeof(buffer), &inst->dst_ipaddr),
                           T_OP_EQ, T_BARE_WORD, T_BARE_WORD);
-       cf_pair_add(cs, cp);
        cp = cf_pair_alloc(cs, "secret", "fake", T_OP_EQ, T_BARE_WORD, T_BARE_WORD);
-       cf_pair_add(cs, cp);
 
        client = client_afrom_cs(inst, cs, NULL);
        if (!client) {

diff --git a/src/modules/rlm_redis_ippool/rlm_redis_ippool_tool.c b/src/modules/rlm_redis_ippool/rlm_redis_ippool_tool.c
index e627ca4fd051fc896a0635a41449f7224b782581..b067b1207d72b6cd38e287f83c298e992bae164a 100644 (file)
--- a/src/modules/rlm_redis_ippool/rlm_redis_ippool_tool.c
+++ b/src/modules/rlm_redis_ippool/rlm_redis_ippool_tool.c
@@ -1340,7 +1340,6 @@ do { \
                ERROR("Failed creating server pair");
                fr_exit_now(EXIT_FAILURE);
        }
-       cf_pair_add(conf->cs, cp);
 
        /*
         *      Unescape sequences in the pool name
@@ -1386,17 +1385,14 @@ do { \
        cp = cf_pair_find(pool_cs, "start");
        if (!cp) {
                cp = cf_pair_alloc(pool_cs, "start", "0", T_OP_EQ, T_BARE_WORD, T_BARE_WORD);
-               cf_pair_add(pool_cs, cp);
        }
        cp = cf_pair_find(pool_cs, "spare");
        if (!cp) {
                cp = cf_pair_alloc(pool_cs, "spare", "0", T_OP_EQ, T_BARE_WORD, T_BARE_WORD);
-               cf_pair_add(pool_cs, cp);
        }
        cp = cf_pair_find(pool_cs, "min");
        if (!cp) {
                cp = cf_pair_alloc(pool_cs, "min", "0", T_OP_EQ, T_BARE_WORD, T_BARE_WORD);
-               cf_pair_add(pool_cs, cp);
        }
 
        if (driver_init(conf, conf->cs, &conf->driver) < 0) {