From a7ef2c4f12ed643fb593ffeb1b4f60d9617a7d05 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Wed, 23 Aug 2023 11:02:18 +0200 Subject: [PATCH] Rewrote parsing of extra config-files to work recursively over structured data Fixed that modified config variables were not properly overwritten in XML Added `pretty` parameter to clicon_options_dump Added testcases for recursive and structure extra config files --- CHANGELOG.md | 2 + apps/backend/backend_main.c | 2 +- apps/cli/cli_main.c | 2 +- apps/cli/cli_show.c | 2 + apps/netconf/netconf_main.c | 2 +- apps/restconf/restconf_main_fcgi.c | 2 +- apps/restconf/restconf_main_native.c | 2 +- apps/snmp/snmp_main.c | 2 +- lib/clixon/clixon_options.h | 2 +- lib/src/clixon_options.c | 143 ++++++++++++++++++--------- test/test_config_dir.sh | 39 +++++--- 11 files changed, 135 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90476e8c..2fd3fb27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,8 @@ Developers may need to change their code ### Corrected Bugs +* Fixed several issues with extra-config files, including overwriting of structured sub-configs + * including ``and m̀ ` * [YANG error when poking on EOS configuration](https://github.com/clicon/clixon-controller/issues/26) * [CLICON_CONFIGDIR with external subsystems causes endless looping](https://github.com/clicon/clixon/issues/439) * Fixed: ["show configuration devices" and "show configuration devices | display cli" differs](https://github.com/clicon/clixon-controller/issues/24) diff --git a/apps/backend/backend_main.c b/apps/backend/backend_main.c index f1f55164..25680a66 100644 --- a/apps/backend/backend_main.c +++ b/apps/backend/backend_main.c @@ -1014,7 +1014,7 @@ main(int argc, goto done; /* Explicit dump of config (also debug dump below). */ if (config_dump){ - if (clicon_option_dump1(h, stdout, config_dump_format) < 0) + if (clicon_option_dump1(h, stdout, config_dump_format, 1) < 0) goto done; } diff --git a/apps/cli/cli_main.c b/apps/cli/cli_main.c index 3600aa8e..b6bb00a6 100644 --- a/apps/cli/cli_main.c +++ b/apps/cli/cli_main.c @@ -862,7 +862,7 @@ main(int argc, * (there is also debug dump below). */ if (config_dump){ - if (clicon_option_dump1(h, stdout, config_dump_format) < 0) + if (clicon_option_dump1(h, stdout, config_dump_format, 1) < 0) goto done; } /* Debug dump of config options */ diff --git a/apps/cli/cli_show.c b/apps/cli/cli_show.c index ade8fa1d..f5e7383c 100644 --- a/apps/cli/cli_show.c +++ b/apps/cli/cli_show.c @@ -1138,6 +1138,8 @@ cli_show_auto_mode(clicon_handle h, } /*! Show clixon configuration options as loaded + * +'* @see clicon_option_dump clicon_option_dump1 */ int cli_show_options(clicon_handle h, diff --git a/apps/netconf/netconf_main.c b/apps/netconf/netconf_main.c index da027c2c..8f51bf6e 100644 --- a/apps/netconf/netconf_main.c +++ b/apps/netconf/netconf_main.c @@ -879,7 +879,7 @@ main(int argc, /* Explicit dump of config (also debug dump below). */ if (config_dump){ - if (clicon_option_dump1(h, stdout, config_dump_format) < 0) + if (clicon_option_dump1(h, stdout, config_dump_format, 1) < 0) goto done; goto ok; } diff --git a/apps/restconf/restconf_main_fcgi.c b/apps/restconf/restconf_main_fcgi.c index 6d60d07e..7d37934c 100644 --- a/apps/restconf/restconf_main_fcgi.c +++ b/apps/restconf/restconf_main_fcgi.c @@ -539,7 +539,7 @@ main(int argc, /* Explicit dump of config (also debug dump below). */ if (config_dump){ - if (clicon_option_dump1(h, stdout, config_dump_format) < 0) + if (clicon_option_dump1(h, stdout, config_dump_format, 1) < 0) goto done; goto ok; } diff --git a/apps/restconf/restconf_main_native.c b/apps/restconf/restconf_main_native.c index 64090fa7..588c4662 100644 --- a/apps/restconf/restconf_main_native.c +++ b/apps/restconf/restconf_main_native.c @@ -1297,7 +1297,7 @@ main(int argc, /* Explicit dump of config (also debug dump below). */ if (config_dump){ - if (clicon_option_dump1(h, stdout, config_dump_format) < 0) + if (clicon_option_dump1(h, stdout, config_dump_format, 1) < 0) goto done; goto ok; } diff --git a/apps/snmp/snmp_main.c b/apps/snmp/snmp_main.c index ea2175dd..81530d24 100644 --- a/apps/snmp/snmp_main.c +++ b/apps/snmp/snmp_main.c @@ -523,7 +523,7 @@ main(int argc, goto done; if (config_dump){ - if (clicon_option_dump1(h, stdout, config_dump_format) < 0) + if (clicon_option_dump1(h, stdout, config_dump_format, 1) < 0) goto done; goto ok; } diff --git a/lib/clixon/clixon_options.h b/lib/clixon/clixon_options.h index f2aab440..222c5d41 100644 --- a/lib/clixon/clixon_options.h +++ b/lib/clixon/clixon_options.h @@ -109,7 +109,7 @@ enum regexp_mode{ int clicon_option_dump(clicon_handle h, int dblevel); /*! Dump config options on stdio using output format */ -int clicon_option_dump1(clicon_handle h, FILE *f, int format); +int clicon_option_dump1(clicon_handle h, FILE *f, int format, int pretty); /* Add a clicon options overriding file setting */ int clicon_option_add(clicon_handle h, const char *name, char *value); diff --git a/lib/src/clixon_options.c b/lib/src/clixon_options.c index 8c68a738..202499a6 100644 --- a/lib/src/clixon_options.c +++ b/lib/src/clixon_options.c @@ -71,6 +71,7 @@ #include "clixon_log.h" #include "clixon_yang.h" #include "clixon_xml.h" +#include "clixon_xml_sort.h" #include "clixon_json.h" #include "clixon_text_syntax.h" #include "clixon_proto.h" @@ -141,6 +142,7 @@ static const map_str2int yang_regexp_map[] = { * @note CLICON_FEATURE, CLICON_YANG_DIR and CLICON_SNMP_MIB are treated specially since they are lists * @note sub-config structs not shown: eg autocli/restconf * @see clicon_option_dump1 different formats + * @see cli_show_options */ int clicon_option_dump(clicon_handle h, @@ -201,6 +203,7 @@ clicon_option_dump(clicon_handle h, * @param[in] h Clicon handle * @param[in] f Output file * @param[in] format Dump output format + * @param[in] pretty Set if pretty-print xml and json * @retval 0 OK * @retval -1 Error * @see clicon_option_dump for debug log @@ -208,7 +211,8 @@ clicon_option_dump(clicon_handle h, int clicon_option_dump1(clicon_handle h, FILE *f, - int format) + int format, + int pretty) { int retval = -1; cxobj *xc; @@ -216,11 +220,11 @@ clicon_option_dump1(clicon_handle h, xc = clicon_conf_xml(h); switch (format){ case FORMAT_XML: - if (clixon_xml2file(f, xc, 0, 1, "", cligen_output, 0, 0) < 0) + if (clixon_xml2file(f, xc, 0, pretty, "", cligen_output, 0, 0) < 0) goto done; break; case FORMAT_JSON: - if (clixon_json2file(f, xc, 1, cligen_output, 0, 0) < 0) + if (clixon_json2file(f, xc, pretty, cligen_output, 0, 0) < 0) goto done; break; case FORMAT_TEXT: @@ -304,6 +308,68 @@ parse_configfile_one(const char *filename, return retval; } +/*! Merge XML sub config file into main config-file + */ +static int +merge_control_xml(clicon_handle h, + cxobj *xt, + cxobj *xe) +{ + int retval = -1; + char *name; + char *body; + cxobj *xec; + cxobj *xtc; + cxobj *x; + + /* One could have used xml_merge, but there are several special conditions */ + xec = NULL; + while ((xec = xml_child_each(xe, xec, CX_ELMNT)) != NULL){ + if ((name = xml_name(xec)) == NULL) + continue; + if ((body = xml_body(xec)) == NULL){ + if ((xtc = xml_find_type(xt, NULL, name, CX_ELMNT)) != NULL){ + if (merge_control_xml(h, xtc, xec) < 0) + goto done; + } + else{ + /* Copy and add to master */ + if ((x = xml_dup(xec)) == NULL) + goto done; + if (xml_addsub(xt, x) < 0) + goto done; + } + continue; + } + /* Ignored from file due to bootstrapping */ + if (strcmp(name,"CLICON_CONFIGFILE")==0) { + continue; + } + /* List options for configure options that are lists or leaf-lists: append to main */ + if (strcmp(name,"CLICON_FEATURE")==0 || + strcmp(name,"CLICON_YANG_DIR")==0 || + strcmp(name,"CLICON_SNMP_MIB")==0){ + if ((x = xml_dup(xec)) == NULL) + goto done; + if (xml_addsub(xt, x) < 0) + goto done; + continue; + } + /* Overwrite: remove existing in master if any */ + if ((x = xml_find_type(xt, NULL, name, CX_ELMNT)) != NULL) + xml_purge(x); + /* Copy and add to master */ + if ((x = xml_dup(xec)) == NULL) + goto done; + if (xml_addsub(xt, x) < 0) + goto done; + } + + retval = 0; + done: + return retval; +} + /*! Read filename and set values to global options registry. XML variant. * * @param[in] h Clixon handle @@ -340,7 +406,6 @@ parse_configfile(clicon_handle h, char filename1[MAXPATHLEN]; char *extraconfdir = NULL; cxobj *xe = NULL; - cxobj *xec; DIR *dirp; if (filename == NULL || !strlen(filename)){ @@ -380,35 +445,9 @@ parse_configfile(clicon_handle h, snprintf(filename1, sizeof(filename1), "%s/%s", extraconfdir, dp[i].d_name); if (parse_configfile_one(filename1, yspec, &xe) < 0) goto done; - /* Drain objects from extrafile and replace/append to main */ - while ((xec = xml_child_i_type(xe, 0, CX_ELMNT)) != NULL) { - name = xml_name(xec); - body = xml_body(xec); - /* Ignore non-leafs */ - if (name == NULL || body == NULL) { - xml_purge(xec); - continue; - } - /* Ignored from file due to bootstrapping */ - if (strcmp(name,"CLICON_CONFIGFILE")==0) { - xml_purge(xec); - continue; - } - /* List options for configure options that are lists or leaf-lists: append to main */ - if (strcmp(name,"CLICON_FEATURE")==0 || - strcmp(name,"CLICON_YANG_DIR")==0 || - strcmp(name,"CLICON_SNMP_MIB")==0){ - if (xml_addsub(xt, xec) < 0) - goto done; - continue; - } - /* Remove existing in master if any */ - if ((x = xml_find_type(xt, NULL, name, CX_ELMNT)) != NULL) - xml_purge(x); - /* Append to master (removed from xe) */ - if (xml_addsub(xt, xec) < 0) - goto done; - } + /* Merge objects from extrafile into main, xml_merge cannot be used due to special cases */ + if (merge_control_xml(h, xt, xe) < 0) + goto done; if (xe) xml_free(xe); xe = NULL; @@ -439,6 +478,9 @@ parse_configfile(clicon_handle h, clicon_err(OE_CFG, 0, "Config file validation: %s", cbuf_get(cbret)); goto done; } + /* Add top-level hash options. + * Hashed options are historical and could be replaced with xml, see eg clicon_option_str + */ x = NULL; while ((x = xml_child_each(xt, x, CX_ELMNT)) != NULL) { name = xml_name(x); @@ -464,6 +506,7 @@ parse_configfile(clicon_handle h, strlen(body)+1) == NULL) goto done; } + xml_sort(xt); retval = 0; *xconfig = xt; xt = NULL; @@ -500,24 +543,35 @@ clicon_option_add(clicon_handle h, { int retval = -1; clicon_hash_t *copt = clicon_options(h); - cxobj *x; + cxobj *xconfig; + cxobj *xopt; + if ((xconfig = clicon_conf_xml(h)) == NULL){ + clicon_err(OE_UNIX, ENOENT, "option %s not found (clicon_conf_xml_set has not been called?)", name); + goto done; + } if (strcmp(name, "CLICON_FEATURE")==0 || strcmp(name, "CLICON_YANG_DIR")==0 || strcmp(name, "CLICON_SNMP_MIB")==0){ - if ((x = clicon_conf_xml(h)) == NULL){ - clicon_err(OE_UNIX, ENOENT, "option %s not found (clicon_conf_xml_set has not been called?)", name); - goto done; - } - if (clixon_xml_parse_va(YB_NONE, NULL, &x, NULL, "<%s>%s", + if (clixon_xml_parse_va(YB_NONE, NULL, &xconfig, NULL, "<%s>%s", name, value, name) < 0) goto done; } - if (clicon_hash_add(copt, - name, - value, - strlen(value)+1) == NULL) - goto done; + else{ + /* Add/change hash */ + if (clicon_hash_add(copt, + name, + value, + strlen(value)+1) == NULL) + goto done; + /* Add/change in clicon_conf_xml */ + if ((xopt = xpath_first(xconfig, 0, "%s", name)) != NULL) + xml_purge(xopt); + if (clixon_xml_parse_va(YB_NONE, NULL, &xconfig, NULL, "<%s>%s", + name, value, name) < 0) + goto done; + } + xml_sort(xconfig); retval = 0; done: return retval; @@ -624,6 +678,7 @@ clicon_options_main(clicon_handle h) goto done; yspec = NULL; /* Set clixon_conf pointer to handle */ + xml_sort(xconfig); if (clicon_conf_xml_set(h, xconfig) < 0) goto done; diff --git a/test/test_config_dir.sh b/test/test_config_dir.sh index 88b89685..e3563d51 100755 --- a/test/test_config_dir.sh +++ b/test/test_config_dir.sh @@ -10,6 +10,7 @@ # Two options are used for testing: # CLICON_MODULE_SET_ID is a single var (replaced) # CLICON_FEATURE is a list var (append) +# Check subconfigs, ie /restconf/server-cert-path used since it does not have default # # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -29,14 +30,15 @@ cat < $cfg /usr/local/var/$APPNAME/$APPNAME.pidfile /usr/local/var/$APPNAME ${YANG_INSTALLDIR} - /usr/local/lib/$APPNAME/clispec - /usr/local/lib/$APPNAME/cli - $APPNAME + $dir 1 test1 EOF +# dummy +touch $dir/spec.cli + new "Start without configdir as baseline" cat < $cfile1 @@ -45,7 +47,7 @@ cat < $cfile1 EOF -expectpart "$($clixon_cli -1 -f $cfg show options)" 0 'CLICON_MODULE_SET_ID: "1"' 'CLICON_FEATURE: "test1"' --not-- 'CLICON_FEATURE: "test2"' +expectpart "$($clixon_cli -1 -f $cfg -C xml)" 0 "1" "test1" --not-- "test2" new "Start with wrong configdir" cat < $cfg @@ -63,7 +65,7 @@ cat < $cfg EOF -expectpart "$($clixon_cli -1 -f $cfg -l o show options)" 255 "UNIX error: CLICON_CONFIGDIR:" "opendir: No such file or directory" +expectpart "$($clixon_cli -1 -f $cfg -l o -C xml)" 255 "UNIX error: CLICON_CONFIGDIR:" "opendir: No such file or directory" new "Start with wrong configdir -E override" rm -f $cfile1 @@ -82,7 +84,7 @@ cat < $cfg EOF -expectpart "$($clixon_cli -1 -f $cfg -E $cdir show options)" 0 'CLICON_MODULE_SET_ID: "1"' 'CLICON_FEATURE: "test1"' --not-- 'CLICON_FEATURE: "test2"' +expectpart "$($clixon_cli -1 -f $cfg -E $cdir -C xml)" 0 "1" "test1" --not-- "test2" new "Start with empty configdir" cat < $cfg @@ -97,23 +99,32 @@ cat < $cfg $APPNAME 1 test1 + + foo + EOF -expectpart "$($clixon_cli -1 -f $cfg -l o show options)" 0 'CLICON_MODULE_SET_ID: "1"' 'CLICON_FEATURE: "test1"' --not-- 'CLICON_FEATURE: "test2"' +expectpart "$($clixon_cli -1 -f $cfg -C xml)" 0 "1" "test1" --not-- "test2" + +new "Check subconfig" +expectpart "$($clixon_cli -1 -f $cfg -C xml)" 0 "" "foo" new "Start with 1 extra configfile" cat < $cfile1 2 test2 - - false - + + bar + EOF -expectpart "$($clixon_cli -1 -f $cfg -l o show options)" 0 'CLICON_MODULE_SET_ID: "2"' 'CLICON_FEATURE: "test1"' 'CLICON_FEATURE: "test2"' +expectpart "$($clixon_cli -1 -f $cfg -C xml)" 0 "2" "test1" "test2" + +new "Check subconfig override" +expectpart "$($clixon_cli -1 -f $cfg -C xml)" 0 "" "bar" new "Start with 2 extra configfiles" cat < $cfile2 @@ -123,10 +134,10 @@ cat < $cfile2 EOF -expectpart "$($clixon_cli -1 -f $cfg -l o show options)" 0 'CLICON_MODULE_SET_ID: "3"' 'CLICON_FEATURE: "test1"' 'CLICON_FEATURE: "test2"' 'CLICON_FEATURE: "test3"' +expectpart "$($clixon_cli -1 -f $cfg -C xml)" 0 "3" "test1" "test2" "test3" -new "Start with 2 extra configfiles + command-line" -expectpart "$($clixon_cli -1 -f $cfg -o CLICON_MODULE_SET_ID=4 -o CLICON_FEATURE=test4 -l o show options)" 0 'CLICON_MODULE_SET_ID: "4"' 'CLICON_FEATURE: "test1"' 'CLICON_FEATURE: "test2"' 'CLICON_FEATURE: "test3"' 'CLICON_FEATURE: "test4"' +new "Start with 2 extra configfiles + command-line -C xml" +expectpart "$($clixon_cli -1 -f $cfg -o CLICON_MODULE_SET_ID=4 -o CLICON_FEATURE=test4 -C xml)" 0 "4" "test1" "test2" "test3" "test4" rm -rf $dir