From 99b7a1fe5bf21a985928f4e9fc079bf6af9b4a5e Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Thu, 9 May 2019 14:25:16 +0200 Subject: [PATCH 1/4] Clixon config option `CLICON_XMLDB_CACHE` renamed to `CLICON_DATASTORE_CACHE` and changed type from `boolean` to `datastore_cache` --- CHANGELOG.md | 4 + apps/backend/backend_client.c | 13 +- apps/backend/backend_commit.c | 46 +++---- apps/backend/backend_startup.c | 8 +- lib/clixon/clixon_datastore.h | 6 +- lib/clixon/clixon_options.h | 10 ++ lib/src/clixon_datastore.c | 7 +- lib/src/clixon_datastore_read.c | 146 ++++++++++++---------- lib/src/clixon_datastore_write.c | 10 +- lib/src/clixon_nacm.c | 2 +- lib/src/clixon_options.c | 49 +++++--- util/clixon_util_datastore.c | 15 ++- yang/clixon/clixon-config@2019-03-05.yang | 41 ++++-- 13 files changed, 207 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57ab07c6..974201f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,10 @@ ### API changes on existing features (you may need to change your code) +* Clixon config option `CLICON_XMLDB_CACHE` renamed to `CLICON_DATASTORE_CACHE` and changed type from `boolean` to `datastore_cache` + * Change code from: `clicon_option_bool(h, "CLICON_XMLDB_CACHE")` to `clicon_datastore_cache(h) == DATASTORE_CACHE` + * Type `datastore_cache` have values: nocache, cache, or cache-zerocopy + * `xmldb_get1` removed (functionality merged with `xmldb_get`) * Non-key list now not accepted in edit-config (before only on validation) * Changed return values in internal functions * These functions are affected: `netconf_trymerge`, `startup_module_state`, `yang_modules_state_get` diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index c5a208ff..9a059027 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -336,7 +336,10 @@ from_client_get_config(clicon_handle h, if ((xfilter = xml_find(xe, "filter")) != NULL) if ((xpath = xml_find_value(xfilter, "select"))==NULL) xpath="/"; - if (xmldb_get(h, db, xpath, &xret, NULL) < 0){ + /* Note xret can be pruned by nacm below (and change name), + * so zero-copy cant be used + */ + if (xmldb_get(h, db, xpath, 1, &xret, NULL) < 0){ if (netconf_operation_failed(cbret, "application", "read registry")< 0) goto done; goto ok; @@ -800,14 +803,16 @@ from_client_get(clicon_handle h, if ((xfilter = xml_find(xe, "filter")) != NULL) if ((xpath = xml_find_value(xfilter, "select"))==NULL) xpath="/"; - /* Get config */ - if (xmldb_get(h, "running", xpath, &xret, NULL) < 0){ + /* Get config + * Note xret can be pruned by nacm below and change name and + * metrged with state data, so zero-copy cant be used + */ + if (xmldb_get(h, "running", xpath, 1, &xret, NULL) < 0){ if (netconf_operation_failed(cbret, "application", "read registry")< 0) goto done; goto ok; } /* Get state data from plugins as defined by plugin_statedata(), if any */ - assert(xret); clicon_err_reset(); if ((ret = client_statedata(h, xpath, &xret)) < 0) goto done; diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index 6587b581..198fe16d 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -182,7 +182,7 @@ startup_common(clicon_handle h, if ((msd = modstate_diff_new()) == NULL) goto done; clicon_debug(1, "Reading startup config from %s", db); - if (xmldb_get1(h, db, "/", &xt, msd) < 0) + if (xmldb_get(h, db, "/", 0, &xt, msd) < 0) goto done; /* Clear flags xpath for get */ xml_apply0(xt, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, @@ -276,7 +276,7 @@ startup_validate(clicon_handle h, if (ret == 0) goto fail; /* Clear cached trees from default values and marking */ - if (xmldb_get1_clear(h, db) < 0) + if (xmldb_get_clear(h, td->td_target) < 0) goto done; if (xtr){ *xtr = td->td_target; @@ -285,11 +285,7 @@ startup_validate(clicon_handle h, retval = 1; done: if (td){ - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ - /* xmldb_get1 requires free only if not cache */ - td->td_target = NULL; - td->td_src = NULL; - } + xmldb_get_free(h, &td->td_target); transaction_free(td); } return retval; @@ -332,7 +328,7 @@ startup_commit(clicon_handle h, if (plugin_transaction_commit(h, td) < 0) goto done; /* Clear cached trees from default values and marking */ - if (xmldb_get1_clear(h, db) < 0) + if (xmldb_get_clear(h, td->td_target) < 0) goto done; /* [Delete and] create running db */ @@ -356,11 +352,7 @@ startup_commit(clicon_handle h, retval = 1; done: if (td){ - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ - /* xmldb_get1 requires free only if not cache */ - td->td_target = NULL; - td->td_src = NULL; - } + xmldb_get_free(h, &td->td_target); transaction_free(td); } return retval; @@ -398,7 +390,7 @@ from_validate_common(clicon_handle h, goto done; } /* This is the state we are going to */ - if (xmldb_get1(h, candidate, "/", &td->td_target, NULL) < 0) + if (xmldb_get(h, candidate, "/", 0, &td->td_target, NULL) < 0) goto done; /* Clear flags xpath for get */ @@ -416,7 +408,7 @@ from_validate_common(clicon_handle h, /* 2. Parse xml trees * This is the state we are going from */ - if (xmldb_get1(h, "running", "/", &td->td_src, NULL) < 0) + if (xmldb_get(h, "running", "/", 0, &td->td_src, NULL) < 0) goto done; /* Clear flags xpath for get */ xml_apply0(td->td_src, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, @@ -521,9 +513,9 @@ candidate_commit(clicon_handle h, goto done; /* Clear cached trees from default values and marking */ - if (xmldb_get1_clear(h, candidate) < 0) + if (xmldb_get_clear(h, td->td_target) < 0) goto done; - if (xmldb_get1_clear(h, "running") < 0) + if (xmldb_get_clear(h, td->td_src) < 0) goto done; /* Optionally write (potentially modified) tree back to candidate @@ -559,11 +551,8 @@ candidate_commit(clicon_handle h, if (retval < 1 && td) plugin_transaction_abort(h, td); if (td){ - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ - /* xmldb_get1 requires free only if not cache */ - td->td_target = NULL; - td->td_src = NULL; - } + xmldb_get_free(h, &td->td_target); + xmldb_get_free(h, &td->td_src); transaction_free(td); } return retval; @@ -749,9 +738,9 @@ from_client_validate(clicon_handle h, goto ok; } /* Clear cached trees from default values and marking */ - if (xmldb_get1_clear(h, db) < 0) + if (xmldb_get_clear(h, td->td_target) < 0) goto done; - if (xmldb_get1_clear(h, "running") < 0) + if (xmldb_get_clear(h, td->td_src) < 0) goto done; /* Optionally write (potentially modified) tree back to candidate */ @@ -768,12 +757,9 @@ from_client_validate(clicon_handle h, if (retval < 0 && td) plugin_transaction_abort(h, td); if (td){ - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ - /* xmldb_get1 requires free only if not cache */ - td->td_target = NULL; - td->td_src = NULL; - } - transaction_free(td); + xmldb_get_free(h, &td->td_target); + xmldb_get_free(h, &td->td_src); + transaction_free(td); } return retval; } /* from_client_validate */ diff --git a/apps/backend/backend_startup.c b/apps/backend/backend_startup.c index f2270644..8f5d94a8 100644 --- a/apps/backend/backend_startup.c +++ b/apps/backend/backend_startup.c @@ -102,13 +102,12 @@ db_merge(clicon_handle h, cxobj *xt = NULL; /* Get data as xml from db1 */ - if (xmldb_get(h, (char*)db1, NULL, &xt, NULL) < 0) + if (xmldb_get(h, (char*)db1, NULL, 0, &xt, NULL) < 0) goto done; /* Merge xml into db2. Without commit */ retval = xmldb_put(h, (char*)db2, OP_MERGE, xt, clicon_username_get(h), cbret); done: - if (xt) - xml_free(xt); + xmldb_get_free(h, &xt); return retval; } @@ -270,8 +269,7 @@ startup_extraxml(clicon_handle h, ok: retval = 1; done: - if (xt && !clicon_option_bool(h, "CLICON_XMLDB_CACHE")) - xml_free(xt); + xmldb_get_free(h, &xt); if (xmldb_delete(h, db) != 0 && errno != ENOENT) return -1; return retval; diff --git a/lib/clixon/clixon_datastore.h b/lib/clixon/clixon_datastore.h index 61ec86a2..5089d0df 100644 --- a/lib/clixon/clixon_datastore.h +++ b/lib/clixon/clixon_datastore.h @@ -48,9 +48,9 @@ int xmldb_db2file(clicon_handle h, const char *db, char **filename); int xmldb_validate_db(const char *db); int xmldb_connect(clicon_handle h); int xmldb_disconnect(clicon_handle h); -int xmldb_get(clicon_handle h, const char *db, char *xpath, cxobj **xtop, modstate_diff_t *msd); /* in clixon_datastore_read.[ch] */ -int xmldb_get1(clicon_handle h, const char *db, char *xpath, cxobj **xtop, modstate_diff_t *msd); /* in clixon_datastore_read.[ch] */ -int xmldb_get1_clear(clicon_handle h, const char *db); +int xmldb_get(clicon_handle h, const char *db, char *xpath, int copy, cxobj **xtop, modstate_diff_t *msd); /* in clixon_datastore_read.[ch] */ +int xmldb_get_clear(clicon_handle h, cxobj *x); +int xmldb_get_free(clicon_handle h, cxobj **xp); int xmldb_put(clicon_handle h, const char *db, enum operation_type op, cxobj *xt, char *username, cbuf *cbret); /* in clixon_datastore_write.[ch] */ int xmldb_copy(clicon_handle h, const char *from, const char *to); int xmldb_lock(clicon_handle h, const char *db, int pid); diff --git a/lib/clixon/clixon_options.h b/lib/clixon/clixon_options.h index d0f60bab..5f183d14 100644 --- a/lib/clixon/clixon_options.h +++ b/lib/clixon/clixon_options.h @@ -73,6 +73,15 @@ enum startup_mode_t{ SM_INIT }; +/*! Datastore cache behaviour, see clixon_datastore.[ch] + * See config option type datastore_cache in clixon-config.yang + */ +enum datastore_cache{ + DATASTORE_NOCACHE, + DATASTORE_CACHE, + DATASTORE_CACHE_ZEROCOPY +}; + /* * Prototypes */ @@ -163,6 +172,7 @@ int clicon_sock_family(clicon_handle h); int clicon_sock_port(clicon_handle h); int clicon_autocommit(clicon_handle h); int clicon_startup_mode(clicon_handle h); +enum datastore_cache clicon_datastore_cache(clicon_handle h); /*-- Specific option access functions for non-yang options --*/ int clicon_quiet_mode(clicon_handle h); diff --git a/lib/src/clixon_datastore.c b/lib/src/clixon_datastore.c index d5b918c7..b27ce1b4 100644 --- a/lib/src/clixon_datastore.c +++ b/lib/src/clixon_datastore.c @@ -176,7 +176,6 @@ xmldb_disconnect(clicon_handle h) return retval; } - /*! Copy database from db1 to db2 * @param[in] h Clicon handle * @param[in] from Source database @@ -199,7 +198,7 @@ xmldb_copy(clicon_handle h, cxobj *x2 = NULL; /* to */ /* XXX lock */ - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ + if (clicon_datastore_cache(h) != DATASTORE_NOCACHE){ /* Copy in-memory cache */ /* 1. "to" xml tree in x1 */ if ((de1 = clicon_db_elmnt_get(h, from)) != NULL) @@ -387,7 +386,7 @@ xmldb_delete(clicon_handle h, cxobj *xt = NULL; struct stat sb; - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ + if (clicon_datastore_cache(h) != DATASTORE_NOCACHE){ if ((de = clicon_db_elmnt_get(h, db)) != NULL){ if ((xt = de->de_xml) != NULL){ xml_free(xt); @@ -425,7 +424,7 @@ xmldb_create(clicon_handle h, db_elmnt *de = NULL; cxobj *xt = NULL; - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ /* XXX This should not really happen? */ + if (clicon_datastore_cache(h) != DATASTORE_NOCACHE){ if ((de = clicon_db_elmnt_get(h, db)) != NULL){ if ((xt = de->de_xml) != NULL){ xml_free(xt); diff --git a/lib/src/clixon_datastore_read.c b/lib/src/clixon_datastore_read.c index e92f6fdb..80bf71a6 100644 --- a/lib/src/clixon_datastore_read.c +++ b/lib/src/clixon_datastore_read.c @@ -572,11 +572,11 @@ xmldb_get_cache(clicon_handle h, * @retval -1 Error */ static int -xmldb_get1_cache(clicon_handle h, - const char *db, - char *xpath, - cxobj **xtop, - modstate_diff_t *msd) +xmldb_get_zerocopy(clicon_handle h, + const char *db, + char *xpath, + cxobj **xtop, + modstate_diff_t *msd) { int retval = -1; yang_stmt *yspec; @@ -641,16 +641,29 @@ xmldb_get1_cache(clicon_handle h, * @param[in] h Clicon handle * @param[in] db Name of database to search in (filename including dir path * @param[in] xpath String with XPATH syntax. or NULL for all + * @param[in] copy Force copy. Overrides cache_zerocopy -> cache * @param[out] xret Single return XML tree. Free with xml_free() * @param[out] msd If set, return modules-state differences * @retval 0 OK * @retval -1 Error + * There are two variants of the call: * @code * cxobj *xt; - * if (xmldb_get(xh, "running", "/interfaces/interface[name="eth"]", &xt, NULL) < 0) + * if (xmldb_get(xh, "running", "/interfaces/interface[name="eth"]", 0, &xt, NULL) < 0) + * err; + * # Code accessing and setting XML flags on xt, sorting, populate w yang spec,... + * xmldb_get_clear(h, xt); # Clear tree from default values and flags + * # (only if cache+zerocopy + * xmldb_get_free(h, &xt); # Free tree (only if not zero-copy) + * @endcode + * The second variant only applies to zerocopy cases where you want to force a copy + * since it may be too difficult to handle marked subtrees. + * @code + * if (xmldb_get(xh, "running", "/interfaces/interface[name="eth"]", 1, &xt, NULL) < 0) * err; * xml_free(xt); * @endcode + * * @note if xvec is given, then purge tree, if not return whole tree. * @see xpath_vec */ @@ -658,84 +671,83 @@ int xmldb_get(clicon_handle h, const char *db, char *xpath, + int copy, cxobj **xret, modstate_diff_t *msd) { int retval = -1; - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")) + switch (clicon_datastore_cache(h)){ + case DATASTORE_NOCACHE: + /* Read from file into created/copy tree, prune non-matching xpath + * Add default values in copy + * Copy deleted by xmldb_free + */ + retval = xmldb_get_nocache(h, db, xpath, xret, msd); + break; + case DATASTORE_CACHE_ZEROCOPY: + /* Get cache (file if empty) mark xpath match in original tree + * add default values in original tree and return that. + * Default values and markings removed in xmldb_clear + */ + if (!copy){ + retval = xmldb_get_zerocopy(h, db, xpath, xret, msd); + break; + } + /* fall through */ + case DATASTORE_CACHE: + /* Get cache (file if empty) mark xpath match and copy marked into copy + * Add default values in copy, return copy + * Copy deleted by xmldb_free + */ retval = xmldb_get_cache(h, db, xpath, xret, msd); - else - retval = xmldb_get_nocache(h, db, xpath, xret, msd); + break; + } return retval; } -/*! Get content of database using xpath. return a set of matching sub-trees - * The function returns a minimal tree that includes all sub-trees that match - * xpath. - * @param[in] h Clicon handle - * @param[in] db Name of database to search in (filename including dir path - * @param[in] xpath String with XPATH syntax. or NULL for all - * @param[out] xret Single return XML tree. see note - * @param[out] msd If set, return modules-state differences - * @retval 0 OK - * @retval -1 Error - * @code - * cxobj *xt; - * if (xmldb_get(xh, "running", "/interfaces/interface[name="eth"]", &xt, NULL) < 0) - * err; - * xml_free(xt); - * @endcode - * @note if xvec is given, then purge tree, if not return whole tree. - * @see xmldb_get This version uses direct cache access and needs to be - * cleanued up after use - * @see xmldb_get1_clear Must call after use - * @note If !CLICON_XMLDB_CACHE you need to free xret after use - * @note If CLICON_XMLDB_CACHE mark|change flags set, need to clear after call - */ -int -xmldb_get1(clicon_handle h, - const char *db, - char *xpath, - cxobj **xret, - modstate_diff_t *msd) -{ - int retval = -1; - - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")) - retval = xmldb_get1_cache(h, db, xpath, xret, msd); - else - retval = xmldb_get_nocache(h, db, xpath, xret, msd); - return retval; -} - -/*! Clear cached tree after accessed by xmldb_get1 +/*! Clear cached xml tree obtained with xmldb_get, if zerocopy * - * @param[in] h Clicon handle - * @param[in] dbname Name of database to search in (filename including dir path - * @see xmldb_get1 + * @param[in] h Clicon handle + * @param[in] db Name of datastore + * "Clear" an xml tree means removing default values and resetting all flags. + * @see xmldb_get */ int -xmldb_get1_clear(clicon_handle h, - const char *db) +xmldb_get_clear(clicon_handle h, + cxobj *x) { int retval = -1; - db_elmnt *de = NULL; - if (!clicon_option_bool(h, "CLICON_XMLDB_CACHE")) - goto ok; /* dont bother, tree is a copy */ - de = clicon_db_elmnt_get(h, db); - if (de != NULL && de->de_xml != NULL){ - /* clear XML tree of defaults */ - if (xml_tree_prune_flagged(de->de_xml, XML_FLAG_DEFAULT, 1) < 0) - goto done; - /* clear mark and change */ - xml_apply0(de->de_xml, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, - (void*)(0xff)); - } + if (clicon_datastore_cache(h) != DATASTORE_CACHE_ZEROCOPY) + goto ok; + if (x == NULL) + goto ok; + /* clear XML tree of defaults */ + if (xml_tree_prune_flagged(x, XML_FLAG_DEFAULT, 1) < 0) + goto done; + /* clear mark and change */ + xml_apply0(x, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset, + (void*)(0xff)); ok: retval = 0; done: return retval; - +} + +/*! Free xml tree obtained with xmldb_get, unless zerocopy + * @param[in] h Clixon handle + * @param[in,out] xp Pointer to XML cache. + * @see xmldb_get + */ +int +xmldb_get_free(clicon_handle h, + cxobj **xp) +{ + if (*xp == NULL || + clicon_datastore_cache(h) == DATASTORE_CACHE_ZEROCOPY) + return 0; + xml_free(*xp); + *xp = NULL; + return 0; } diff --git a/lib/src/clixon_datastore_write.c b/lib/src/clixon_datastore_write.c index cb0407ee..c8a01526 100644 --- a/lib/src/clixon_datastore_write.c +++ b/lib/src/clixon_datastore_write.c @@ -652,8 +652,9 @@ xmldb_put(clicon_handle h, xml_name(x1)); goto done; } - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ - if ((de = clicon_db_elmnt_get(h, db)) != NULL) + + if ((de = clicon_db_elmnt_get(h, db)) != NULL){ + if (clicon_datastore_cache(h) != DATASTORE_NOCACHE) x0 = de->de_xml; } /* If there is no xml x0 tree (in cache), then read it from file */ @@ -713,8 +714,9 @@ xmldb_put(clicon_handle h, if (xml_apply0(x0, -1, xml_sort_verify, NULL) < 0) clicon_log(LOG_NOTICE, "%s: verify failed #3", __FUNCTION__); #endif + /* Write back to datastore cache if first time */ - if (clicon_option_bool(h, "CLICON_XMLDB_CACHE")){ + if (clicon_datastore_cache(h) != DATASTORE_NOCACHE){ db_elmnt de0 = {0,}; if (de != NULL) de0 = *de; @@ -770,7 +772,7 @@ xmldb_put(clicon_handle h, free(dbfile); if (cb) cbuf_free(cb); - if (!clicon_option_bool(h, "CLICON_XMLDB_CACHE") && x0) + if (x0 && clicon_datastore_cache(h) == DATASTORE_NOCACHE) xml_free(x0); return retval; fail: diff --git a/lib/src/clixon_nacm.c b/lib/src/clixon_nacm.c index 2c57da27..02fdf22e 100644 --- a/lib/src/clixon_nacm.c +++ b/lib/src/clixon_nacm.c @@ -890,7 +890,7 @@ nacm_access_pre(clicon_handle h, goto done; } else if (strcmp(mode, "internal")==0){ - if (xmldb_get(h, "running", "nacm", &xnacm0, NULL) < 0) + if (xmldb_get(h, "running", "nacm", 1, &xnacm0, NULL) < 0) goto done; } } diff --git a/lib/src/clixon_options.c b/lib/src/clixon_options.c index bf815fd4..b42373b0 100644 --- a/lib/src/clixon_options.c +++ b/lib/src/clixon_options.c @@ -543,30 +543,25 @@ clicon_cli_genmodel_completion(clicon_handle h) return 0; } +static const map_str2int cli_genmodel_map[] = { + {"NONE", GT_NONE}, + {"VARS", GT_VARS}, + {"ALL", GT_ALL}, + {NULL, -1} +}; + /*! How to generate and show CLI syntax: VARS|ALL * @see clixon-config@.yang CLICON_CLI_GENMODEL_TYPE */ enum genmodel_type clicon_cli_genmodel_type(clicon_handle h) { - char *s; - enum genmodel_type gt = GT_ERR; + char *str; - if (!clicon_option_exists(h, "CLICON_CLI_GENMODEL_TYPE")){ - gt = GT_VARS; - goto done; - } - s = clicon_option_str(h, "CLICON_CLI_GENMODEL_TYPE"); - if (strcmp(s, "NONE")==0) - gt = GT_NONE; + if ((str = clicon_option_str(h, "CLICON_CLI_GENMODEL_TYPE")) == NULL) + return GT_VARS; else - if (strcmp(s, "VARS")==0) - gt = GT_VARS; - else - if (strcmp(s, "ALL")==0) - gt = GT_ALL; - done: - return gt; + return clicon_str2int(cli_genmodel_map, str); } /*! Get Dont include keys in cvec in cli vars callbacks @@ -638,6 +633,28 @@ clicon_startup_mode(clicon_handle h) return clicon_str2int(startup_mode_map, mode); } +static const map_str2int datastore_cache_map[] = { + {"nocache", DATASTORE_NOCACHE}, + {"cache", DATASTORE_CACHE}, + {"cache-zerocopy", DATASTORE_CACHE_ZEROCOPY}, + {NULL, -1} +}; + +/*! How to generate and show CLI syntax: VARS|ALL + * @see clixon-config@.yang CLICON_CLI_GENMODEL_TYPE + */ +enum datastore_cache +clicon_datastore_cache(clicon_handle h) +{ + char *str; + + if ((str = clicon_option_str(h, "CLICON_DATASTORE_CACHE")) == NULL) + return DATASTORE_CACHE; + else + return clicon_str2int(datastore_cache_map, str); +} + + /*--------------------------------------------------------------------- * Specific option access functions for non-yang options * Typically dynamic values and more complex datatypes, diff --git a/util/clixon_util_datastore.c b/util/clixon_util_datastore.c index 3b0e5911..0138d6d5 100644 --- a/util/clixon_util_datastore.c +++ b/util/clixon_util_datastore.c @@ -198,11 +198,14 @@ main(int argc, char **argv) xpath = argv[1]; else xpath = "/"; - if (xmldb_get(h, db, xpath, &xt, NULL) < 0) + if (xmldb_get(h, db, xpath, 1, &xt, NULL) < 0) goto done; clicon_xml2file(stdout, xt, 0, 0); - fprintf(stdout, "\n"); + if (xt){ + xml_free(xt); + xt = NULL; + } } else if (strcmp(cmd, "mget")==0){ int nr; @@ -214,15 +217,17 @@ main(int argc, char **argv) else xpath = "/"; for (i=0;i Date: Fri, 10 May 2019 14:35:34 +0200 Subject: [PATCH 2/4] * Clixon transaction mechanism has changed which may affect your backend plugin callbacks: * Validate-only transactions are terminated by an `end` or `abort` callback. * If a commit user callback fails, a new `revert` callback will be made to plugins that have made a succesful commit. --- CHANGELOG.md | 5 + apps/backend/backend_commit.c | 38 +++-- apps/backend/backend_plugin.c | 20 +-- doc/FAQ.md | 40 ++++- example/main/README.md | 2 +- example/main/example_backend.c | 28 ++-- example/main/example_backend_nacm.c | 143 +++++++++++++++-- lib/clixon/clixon_plugin.h | 2 + test/test_transaction.sh | 231 ++++++++++++++++++++++++---- 9 files changed, 422 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 974201f8..489199d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,11 @@ ### API changes on existing features (you may need to change your code) +* Clixon transaction mechanism has changed which may affect your backend plugin callbacks: + * Validate-only transactions are terminated by an `end` or `abort` callback. Now all started transactions are terminated either by an `end` or `abort` without exceptions + * Validate-only transactions used to be terminated by `complete` + * If a commit user callback fails, a new `revert` callback will be made to plugins that have made a succesful commit. + * Clixon used to play the (already made) commit callbacks in reverse order * Clixon config option `CLICON_XMLDB_CACHE` renamed to `CLICON_DATASTORE_CACHE` and changed type from `boolean` to `datastore_cache` * Change code from: `clicon_option_bool(h, "CLICON_XMLDB_CACHE")` to `clicon_datastore_cache(h) == DATASTORE_CACHE` * Type `datastore_cache` have values: nocache, cache, or cache-zerocopy diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index 198fe16d..e7cf93e9 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -271,15 +271,20 @@ startup_validate(clicon_handle h, /* Handcraft a transition with only target and add trees */ if ((td = transaction_new()) == NULL) goto done; - if ((ret = startup_common(h, db, td, cbret)) < 0) + if ((ret = startup_common(h, db, td, cbret)) < 0){ + plugin_transaction_abort(h, td); goto done; - if (ret == 0) + } + if (ret == 0){ + plugin_transaction_abort(h, td); goto fail; + } + plugin_transaction_end(h, td); /* Clear cached trees from default values and marking */ if (xmldb_get_clear(h, td->td_target) < 0) goto done; if (xtr){ - *xtr = td->td_target; + *xtr = td->td_target; td->td_target = NULL; } retval = 1; @@ -348,10 +353,11 @@ startup_commit(clicon_handle h, goto fail; /* 10. Call plugin transaction end callbacks */ plugin_transaction_end(h, td); - retval = 1; done: if (td){ + if (retval < 1) + plugin_transaction_abort(h, td); xmldb_get_free(h, &td->td_target); transaction_free(td); } @@ -548,9 +554,9 @@ candidate_commit(clicon_handle h, retval = 1; done: /* In case of failure (or error), call plugin transaction termination callbacks */ - if (retval < 1 && td) - plugin_transaction_abort(h, td); if (td){ + if (retval < 1) + plugin_transaction_abort(h, td); xmldb_get_free(h, &td->td_target); xmldb_get_free(h, &td->td_src); transaction_free(td); @@ -730,33 +736,37 @@ from_client_validate(clicon_handle h, goto done; /* Common steps (with commit) */ if ((ret = from_validate_common(h, db, td, cbret)) < 1){ - clicon_debug(1, "Validate %s failed", db); - if (ret < 0){ - if (netconf_operation_failed(cbret, "application", clicon_err_reason)< 0) - goto done; - } + plugin_transaction_abort(h, td); + if (netconf_operation_failed(cbret, "application", clicon_err_reason)< 0) + goto done; goto ok; } /* Clear cached trees from default values and marking */ if (xmldb_get_clear(h, td->td_target) < 0) goto done; - if (xmldb_get_clear(h, td->td_src) < 0) + if (xmldb_get_clear(h, td->td_src) < 0 || + xmldb_get_clear(h, td->td_target) < 0){ + plugin_transaction_abort(h, td); goto done; + } /* Optionally write (potentially modified) tree back to candidate */ if (clicon_option_bool(h, "CLICON_TRANSACTION_MOD")){ + plugin_transaction_abort(h, td); if ((ret = xmldb_put(h, "candidate", OP_REPLACE, td->td_target, clicon_username_get(h), cbret)) < 0) goto done; goto ok; } cprintf(cbret, ""); + /* Call plugin transaction end callbacks */ + plugin_transaction_end(h, td); ok: retval = 0; done: - if (retval < 0 && td) - plugin_transaction_abort(h, td); if (td){ + if (retval < 0) + plugin_transaction_abort(h, td); xmldb_get_free(h, &td->td_target); xmldb_get_free(h, &td->td_src); transaction_free(td); diff --git a/apps/backend/backend_plugin.c b/apps/backend/backend_plugin.c index de04d161..4a64b6f9 100644 --- a/apps/backend/backend_plugin.c +++ b/apps/backend/backend_plugin.c @@ -295,28 +295,14 @@ plugin_transaction_revert(clicon_handle h, int nr) { int retval = 0; - transaction_data_t tr; /* revert transaction */ clixon_plugin *cp = NULL; trans_cb_t *fn; - /* Create a new reversed transaction from the original where src and target - are swapped */ - memcpy(&tr, td, sizeof(tr)); - tr.td_src = td->td_target; - tr.td_target = td->td_src; - tr.td_dlen = td->td_alen; - tr.td_dvec = td->td_avec; - tr.td_alen = td->td_dlen; - tr.td_avec = td->td_dvec; - tr.td_clen = td->td_clen; - tr.td_scvec = td->td_tcvec; - tr.td_tcvec = td->td_scvec; - while ((cp = clixon_plugin_each_revert(h, cp, nr)) != NULL) { - if ((fn = cp->cp_api.ca_trans_commit) == NULL) + if ((fn = cp->cp_api.ca_trans_revert) == NULL) continue; - if ((retval = fn(h, (transaction_data)&tr)) < 0){ - clicon_log(LOG_NOTICE, "%s: Plugin '%s' trans_commit revert callback failed", + if ((retval = fn(h, (transaction_data)td)) < 0){ + clicon_log(LOG_NOTICE, "%s: Plugin '%s' trans_revert callback failed", __FUNCTION__, cp->cp_name); break; } diff --git a/doc/FAQ.md b/doc/FAQ.md index 24aaf048..9cf184fa 100644 --- a/doc/FAQ.md +++ b/doc/FAQ.md @@ -15,7 +15,8 @@ * [How do I use restconf?](#how-do-i-use-restconf) * [What about reference documentation?](#what-about-reference-documentation) * [How is configuration data stored?](#how-is-configuration-data-stored) - * [What is validate and commit?](#what-is-validate-and-commit) + * [What is validate and commit?](#what-is-validate-and-commi)t + * [Does Clixon support transactions?](#does-clixon-support-transactions) * [What is a Clixon configuration file?](#what-is-a-clixon-configuration-file) * [How are Clixon configuration files found?](#how-are-clixon-configuration-files-found) * [Can I modify clixon options at runtime?](#can-i-modify-clixon-options-at-runtime) @@ -227,6 +228,42 @@ A clixon developer writes commit functions to incrementaly upgrade a system state based on configuration changes. Writing commit callbacks is the core functionality of a clixon system. +## Does Clixon support transactions? + +Yes. The netconf validation and commit operation is implemented in +Clixon by a transaction mechanism, which ensures that user-written +plugin callbacks are invoked atomically and revert on error. If you +have two plugins, for example, a transaction sequence looks like the +following: +``` +Backend Plugin1 Plugin2 + | | | + +--------->+--------->+ begin + | | | + +--------->+--------->+ validate + | | | + +--------->+--------->+ commit + | | | + +--------->+--------->+ end +``` + +If an error occurs in the commit call of Plugin2, for example, +the transaction is aborted and the commit reverted: +``` +Backend Plugin1 Plugin2 + | | | + +--------->+--------->+ begin + | | | + +--------->+--------->+ validate + | | | + +--------->+---->X + commit error + | | | + +--------->+ + revert + | | | + +--------->+--------->+ abort +``` + + ## What is a Clixon configuration file? Clixon options are stored in an XML configuration file. The default @@ -430,7 +467,6 @@ Each plugin is initiated with an API struct followed by a plugin init function a ``` For more info see [../example/main/README.md] - ## How do I write a commit function? In the example, you write a commit function in example_backend.c. Every time a commit is made, transaction_commit() is called in the diff --git a/example/main/README.md b/example/main/README.md index 16215237..2d00dec2 100644 --- a/example/main/README.md +++ b/example/main/README.md @@ -14,7 +14,7 @@ ## Content -This directory contains a Clixon example which includes a simple example. It contains the following files: +This directory contains a Clixon example used primarily for testing. It can be used as a basis for making new Clixon applications. But please consider also the minimal [hello](../hello) example as well. It contains the following files: * `example.xml` The configuration file. See [yang/clixon-config@.yang](../../yang/clixon-config@2019-03-05.yang) for the documentation of all available fields. * `clixon-example@2019-01-13.yang` The yang spec of the example. * `example_cli.cli` CLIgen specification. diff --git a/example/main/example_backend.c b/example/main/example_backend.c index fe891335..e132b278 100644 --- a/example/main/example_backend.c +++ b/example/main/example_backend.c @@ -85,8 +85,7 @@ main_begin(clicon_handle h, transaction_data td) { if (_transaction_log) - transaction_log(h, td, LOG_NOTICE, "begin"); - + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); return 0; } /*! This is called on validate (and commit). Check validity of candidate @@ -96,7 +95,7 @@ main_validate(clicon_handle h, transaction_data td) { if (_transaction_log) - transaction_log(h, td, LOG_NOTICE, "validate"); + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); return 0; } @@ -105,7 +104,7 @@ main_complete(clicon_handle h, transaction_data td) { if (_transaction_log) - transaction_log(h, td, LOG_NOTICE, "complete"); + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); return 0; } @@ -113,7 +112,7 @@ main_complete(clicon_handle h, */ int main_commit(clicon_handle h, - transaction_data td) + transaction_data td) { cxobj *target = transaction_target(td); /* wanted XML tree */ cxobj **vec = NULL; @@ -121,7 +120,8 @@ main_commit(clicon_handle h, size_t len; if (_transaction_log) - transaction_log(h, td, LOG_NOTICE, "commit"); + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + /* Get all added i/fs */ if (xpath_vec_flag(target, "//interface", XML_FLAG_ADD, &vec, &len) < 0) return -1; @@ -134,12 +134,21 @@ main_commit(clicon_handle h, return 0; } +int +main_revert(clicon_handle h, + transaction_data td) +{ + if (_transaction_log) + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + return 0; +} + int main_end(clicon_handle h, transaction_data td) { if (_transaction_log) - transaction_log(h, td, LOG_NOTICE, "end"); + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); return 0; } @@ -148,7 +157,7 @@ main_abort(clicon_handle h, transaction_data td) { if (_transaction_log) - transaction_log(h, td, LOG_NOTICE, "abort"); + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); return 0; } @@ -568,6 +577,7 @@ static clixon_plugin_api api = { .ca_trans_validate=main_validate, /* trans validate */ .ca_trans_complete=main_complete, /* trans complete */ .ca_trans_commit=main_commit, /* trans commit */ + .ca_trans_revert=main_revert, /* trans revert */ .ca_trans_end=main_end, /* trans end */ .ca_trans_abort=main_abort /* trans abort */ }; @@ -594,7 +604,7 @@ clixon_plugin_init(clicon_handle h) goto done; opterr = 0; optind = 1; - while ((c = getopt(argc, argv, "rsut")) != -1) + while ((c = getopt(argc, argv, "rsut:")) != -1) switch (c) { case 'r': _reset = 1; diff --git a/example/main/example_backend_nacm.c b/example/main/example_backend_nacm.c index b2058817..1614f985 100644 --- a/example/main/example_backend_nacm.c +++ b/example/main/example_backend_nacm.c @@ -32,11 +32,16 @@ ***** END LICENSE BLOCK ***** * - * IETF yang routing example - * Secondary backend for testing more than one backend plugin + * Secondary backend for testing more than one backend plugin, with the following + * features: + * - nacm + * - transaction test + * The transaction test is test/test_transaction.sh where a user-error is triggered + * by this plugin if started with -- -t . _transaction_xpath is then set + * and triggers a validation error if it matches. The error also toggles between + * validation and commit errors. */ - #include #include #include @@ -55,6 +60,97 @@ /* These include signatures for plugin and transaction callbacks. */ #include +/*! Variable to control transaction logging (for debug) + * If set, call syslog for every transaction callback + */ +static int _transaction_log = 0; + +/*! Variable for failing validate and commit, if set, fail on validate vs commit + */ +static char * _transaction_xpath = NULL; +static int _transaction_error_toggle = 0; /* fail at validate vs commit */ + +int +nacm_begin(clicon_handle h, + transaction_data td) +{ + if (_transaction_log) + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + return 0; +} +/*! This is called on validate (and commit). Check validity of candidate + */ +int +nacm_validate(clicon_handle h, + transaction_data td) +{ + if (_transaction_log){ + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + if (_transaction_error_toggle==0 && + xpath_first(transaction_target(td), "%s", _transaction_xpath)){ + _transaction_error_toggle=1; /* toggle if triggered */ + clicon_err(OE_XML, 0, "User error"); + return -1; /* induce fail */ + } + } + return 0; +} + +int +nacm_complete(clicon_handle h, + transaction_data td) +{ + if (_transaction_log) + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + return 0; +} + +/*! This is called on commit. Identify modifications and adjust machine state + */ +int +nacm_commit(clicon_handle h, + transaction_data td) +{ + cxobj *target = transaction_target(td); /* wanted XML tree */ + + if (_transaction_log){ + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + if (_transaction_error_toggle==1 && + xpath_first(target, "%s", _transaction_xpath)){ + _transaction_error_toggle=0; /* toggle if triggered */ + clicon_err(OE_XML, 0, "User error"); + return -1; /* induce fail */ + } + } + return 0; +} + +int +nacm_revert(clicon_handle h, + transaction_data td) +{ + if (_transaction_log) + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + return 0; +} + +int +nacm_end(clicon_handle h, + transaction_data td) +{ + if (_transaction_log) + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + return 0; +} + +int +nacm_abort(clicon_handle h, + transaction_data td) +{ + if (_transaction_log) + transaction_log(h, td, LOG_NOTICE, __FUNCTION__); + return 0; +} /*! Called to get NACM state data * @param[in] h Clicon handle @@ -88,21 +184,21 @@ nacm_statedata(clicon_handle h, return retval; } -int -plugin_start(clicon_handle h) -{ - return 0; -} - clixon_plugin_api *clixon_plugin_init(clicon_handle h); static clixon_plugin_api api = { "nacm", /* name */ /*--- Common fields. ---*/ clixon_plugin_init, /* init */ - plugin_start, /* start */ + NULL, /* start */ NULL, /* exit */ - .ca_reset=NULL, /* reset */ .ca_statedata=nacm_statedata, /* statedata */ + .ca_trans_begin=nacm_begin, /* trans begin */ + .ca_trans_validate=nacm_validate, /* trans validate */ + .ca_trans_complete=nacm_complete, /* trans complete */ + .ca_trans_commit=nacm_commit, /* trans commit */ + .ca_trans_revert=nacm_revert, /* trans revert */ + .ca_trans_end=nacm_end, /* trans end */ + .ca_trans_abort=nacm_abort /* trans abort */ }; /*! Backend plugin initialization @@ -113,13 +209,34 @@ static clixon_plugin_api api = { clixon_plugin_api * clixon_plugin_init(clicon_handle h) { - char *nacm_mode; + char *nacm_mode; + int argc; /* command-line options (after --) */ + char **argv; + int c; clicon_debug(1, "%s backend nacm", __FUNCTION__); + /* Get user command-line options (after --) */ + if (clicon_argv_get(h, &argc, &argv) < 0) + goto done; + opterr = 0; + optind = 1; + while ((c = getopt(argc, argv, "rsut:")) != -1) + switch (c) { + case 't': /* transaction log */ + _transaction_log = 1; + _transaction_xpath = optarg; + break; + } + nacm_mode = clicon_option_str(h, "CLICON_NACM_MODE"); if (nacm_mode==NULL || strcmp(nacm_mode, "disabled") == 0){ clicon_log(LOG_DEBUG, "%s CLICON_NACM_MODE not enabled: example nacm module disabled", __FUNCTION__); - return NULL; + /* Skip nacm module if not enabled _unless_ we use transaction tests */ + if (_transaction_log == 0) + return NULL; } + /* Return plugin API */ return &api; + done: + return NULL; } diff --git a/lib/clixon/clixon_plugin.h b/lib/clixon/clixon_plugin.h index 9a84d601..0ac4aae6 100644 --- a/lib/clixon/clixon_plugin.h +++ b/lib/clixon/clixon_plugin.h @@ -177,6 +177,7 @@ struct clixon_plugin_api{ trans_cb_t *cb_trans_validate; /* Transaction validation */ trans_cb_t *cb_trans_complete; /* Transaction validation complete */ trans_cb_t *cb_trans_commit; /* Transaction commit */ + trans_cb_t *cb_trans_revert; /* Transaction revert */ trans_cb_t *cb_trans_end; /* Transaction completed */ trans_cb_t *cb_trans_abort; /* Transaction aborted */ @@ -195,6 +196,7 @@ struct clixon_plugin_api{ #define ca_trans_validate u.cau_backend.cb_trans_validate #define ca_trans_complete u.cau_backend.cb_trans_complete #define ca_trans_commit u.cau_backend.cb_trans_commit +#define ca_trans_revert u.cau_backend.cb_trans_revert #define ca_trans_end u.cau_backend.cb_trans_end #define ca_trans_abort u.cau_backend.cb_trans_abort diff --git a/test/test_transaction.sh b/test/test_transaction.sh index 7eb1f7ad..3699a9b5 100755 --- a/test/test_transaction.sh +++ b/test/test_transaction.sh @@ -1,10 +1,20 @@ #!/bin/bash # Transaction functionality -# The test uses a backend that logs to a file and a netconf client to push -# changes. The main example backend plugin logs to the file, and the test -# verifies the logs. -# The yang is a list with three members, so that you can do add/delete/change -# in a single go. +# The test uses two backend plugins (main and nacm) that logs to a file and a +# netconf client to push operation. The tests then look at the log. +# The tests are as follows (first five only callbacks per se; then data vector tests) +# 1. Validate-only transaction +# 2. Commit transaction +# 3. Validate system-error (invalid type detected by system) +# 4. Validate user-error (invalidation by user callback) +# 5. Commit user-error (invalidation by user callback) +# -- to here only basic callback tests (that they occur). Below transaction data +# 6. Detailed transaction vector add/del/change tests +# For the last test, the yang is a list with three members, so that you can do +# add/delete/change in a single go. +# The user-error uses a trick feature in the example nacm plugin which is started +# with an "error-trigger" xpath which triggers an error. This also toggles between +# validation and commit errors # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -19,6 +29,9 @@ fyang=$dir/trans.yang flog=$dir/backend.log touch $flog +# Used as a trigger for user-validittion errors, eg $errnr is invalid +errnr=42 + cat < $fyang module trans{ yang-version 1.1; @@ -31,8 +44,10 @@ module trans{ type int32; } leaf b { - description "change this"; - type int32; + description "change this (also use to check invalid)"; + type int32{ + range "0..100"; + } } leaf c { description "del this"; @@ -54,7 +69,7 @@ cat < $cfg $fyang /usr/local/lib/$APPNAME/clispec /usr/local/lib/$APPNAME/backend - example_backend.so$ + /usr/local/lib/$APPNAME/netconf /usr/local/lib/$APPNAME/restconf /usr/local/lib/$APPNAME/cli @@ -70,8 +85,9 @@ EOF # Check statements in log checklog(){ s=$1 # statement + l0=$2 # linenr new "Check $s in log" - t=$(grep "$s" $flog) + t=$(grep -n "transaction_log $s" $flog) if [ -z "$t" ]; then echo -e "\e[31m\nError in Test$testnr [$testname]:" if [ $# -gt 0 ]; then @@ -81,9 +97,19 @@ checklog(){ echo -e "\e[0m" exit -1 fi + l1=$(echo "$t" | awk -F ":" '{print $1}') + if [ $l1 -ne $l0 ]; then + echo -e "\e[31m\nError in Test$testnr [$testname]:" + if [ $# -gt 0 ]; then + echo "Expected match on line $l0, found on $l1" + echo + fi + echo -e "\e[0m" + exit -1 + fi } -new "test params: -f $cfg -l f$flog -- -t" +new "test params: -f $cfg -l f$flog -- -t /x/y[a=$errnr]" # Fail on this # Bring your own backend if [ $BE -ne 0 ]; then # kill old backend (if any) @@ -92,47 +118,190 @@ if [ $BE -ne 0 ]; then if [ $? -ne 0 ]; then err fi - new "start backend -s init -f $cfg -l f$flog -- -t" - start_backend -s init -f $cfg -l f$flog -- -t # -t means transaction logging + new "start backend -s init -f $cfg -l f$flog -- -t /x/y[a=$errnr]" + start_backend -s init -f $cfg -l f$flog -- -t /x/y[a=$errnr] # -t means transaction logging new "waiting" sleep $RCWAIT fi -new "netconf base config (0) a,b,c" -expecteof "$clixon_netconf -qf $cfg" 0 '000]]>]]>' '^]]>]]>$' +let nr=1 + +new "Basic transaction to add top-level x" +expecteof "$clixon_netconf -qf $cfg" 0 "$nr]]>]]>" '^]]>]]>$' + +new "Commit base" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' + +let line=12 # Skipping basic transaction + +# 1. validate(-only) transaction +let nr++ +let line +new "1. Validate-only transaction" +expecteof "$clixon_netconf -qf $cfg" 0 "$nr]]>]]>" '^]]>]]>$' + +new "Validate-only validate" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' + +xml="$nr" +for op in begin validate complete end; do + checklog "$nr main_$op add: $xml" $line + let line++ + checklog "$nr nacm_$op add: $xml" $line + let line++ +done + +new "Validate-only discard-changes" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +# 2. Commit transaction +let nr++ +new "2. Commit transaction config" +expecteof "$clixon_netconf -qf $cfg" 0 "$nr]]>]]>" '^]]>]]>$' + +new "Commit transaction: commit" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' + +xml="$nr" +for op in begin validate complete commit end; do + checklog "$nr main_$op add: $xml" $line + let line++ + checklog "$nr nacm_$op add: $xml" $line + let line++ +done + +# 3. Validate only system-error (invalid type detected by system) +let nr++ +new "3. Validate system-error config (9999 not in range)" +expecteof "$clixon_netconf -qf $cfg" 0 "$nr9999]]>]]>" '^]]>]]>$' + +new "Validate system-error validate (should fail)" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationbad-elementberrorNumber out of range: 9999]]>]]>$' + +new "Validate system-error discard-changes" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +for op in begin abort; do + checklog "$nr main_$op add: $nr9999" $line + let line++ + checklog "$nr nacm_$op add: $nr9999" $line + let line++ +done + +# 4. Validate only user-error (invalidation by user callback) +let nr++ +new "4. Validate user-error config ($errnr is invalid)" +expecteof "$clixon_netconf -qf $cfg" 0 "$errnr]]>]]>" '^]]>]]>$' + +new "Validate user-error validate (should fail)" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationoperation-failederrorUser error]]>]]>$' + +new "Validate user-error discard-changes" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +for op in begin validate; do + checklog "$nr main_$op add: $errnr" $line + let line++ + checklog "$nr nacm_$op add: $errnr" $line + let line++ +done +let line++ # error message +for op in abort; do + checklog "$nr main_$op add: $errnr" $line + let line++ + checklog "$nr nacm_$op add: $errnr" $line + let line++ +done + +# 5. Commit user-error (invalidation by user callback) +# XXX Note Validate-only user-error must immediately preceede this due to toggling +# in nacm/transaction example test module +let nr++ +new "5. Commit user-error ($errnr is invalid)" +expecteof "$clixon_netconf -qf $cfg" 0 "$errnr]]>]]>" '^]]>]]>$' + +new "Commit user-error commit" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^applicationoperation-failederrorUser error]]>]]>$' + +new "Commit user-error discard-changes" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +for op in begin validate complete commit; do + checklog "$nr main_$op add: $errnr" $line + let line++ + checklog "$nr nacm_$op add: $errnr" $line + let line++ +done +let line++ # error message +checklog "$nr main_revert add: $errnr" $line +let line++ +for op in abort; do + checklog "$nr main_$op add: $errnr" $line + let line++ + checklog "$nr nacm_$op add: $errnr" $line + let line++ +done + +# 6. Detailed transaction vector add/del/change tests +let nr++ +let base=nr +new "Add base $base entry" +expecteof "$clixon_netconf -qf $cfg" 0 "$base00]]>]]>" '^]]>]]>$' new "netconf commit base" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' #Ignore +let line+=10 -new "netconf mixed change: change b, del c, add d" -expecteof "$clixon_netconf -qf $cfg" 0 '0420]]>]]>' '^]]>]]>$' +let nr++ +new "6. netconf mixed change: change b, del c, add d" +expecteof "$clixon_netconf -qf $cfg" 0 "$base420]]>]]>" '^]]>]]>$' new "netconf commit change" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' -# Check complete transaction 2: +# Check complete transaction $nr: for op in begin validate complete commit; do - checklog "transaction_log 2 $op add: 0" - checklog "transaction_log 2 $op change: 042" + checklog "$nr main_$op add: 0" $line + let line++ + checklog "$nr main_$op change: 042" $line + let line++ + checklog "$nr nacm_$op add: 0" $line + let line++ + checklog "$nr nacm_$op change: 042" $line + let line++ done -# End is special -checklog "transaction_log 2 end add: 0" -checklog "transaction_log 2 end change: 42" -new "netconf config (1) end-points a,d " -expecteof "$clixon_netconf -qf $cfg" 0 '11]]>]]>' '^]]>]]>$' +# End is special because change does not haveold element +checklog "$nr main_end add: 0" $line +let line++ +checklog "$nr main_end change: 42" $line +let line+=3 # skip nacm + +let nr++ +let base=nr +new "Add base $base entry" +expecteof "$clixon_netconf -qf $cfg" 0 "$base1]]>]]>" '^]]>]]>$' + +new "netconf commit base" +expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' +let line+=10 + +# Variant check that only b,c +let nr++ +new "7. netconf insert b,c between end-points" +expecteof "$clixon_netconf -qf $cfg" 0 "$base11]]>]]>" '^]]>]]>$' new "netconf commit base" expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' -new "netconf insert b,c between end-points" -expecteof "$clixon_netconf -qf $cfg" 0 '111]]>]]>' '^]]>]]>$' - -new "netconf commit base" -expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>' '^]]>]]>$' - -checklog "transaction_log 4 validate add: 11" +# check complete +for op in begin validate complete commit end; do + checklog "$nr main_$op add: 11" $line + let line++ + checklog "$nr nacm_$op add: 11" $line + let line++ +done if [ $BE -eq 0 ]; then exit # BE From c38a648ca4d0d3073be1d84c10746a7be364cd5b Mon Sep 17 00:00:00 2001 From: Olof Hagsand Date: Fri, 10 May 2019 14:44:21 +0000 Subject: [PATCH 3/4] wrong fread -1 check --- lib/src/clixon_datastore_tree.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/src/clixon_datastore_tree.c b/lib/src/clixon_datastore_tree.c index 6fc61dd8..179318f6 100644 --- a/lib/src/clixon_datastore_tree.c +++ b/lib/src/clixon_datastore_tree.c @@ -206,6 +206,7 @@ buf2xml(FILE *f, int i; uint32_t ind; cxobj *xc; + size_t ret; /* Read hdr * +-----+-----+-----+-----+-----+-----+-----+-----+ @@ -216,7 +217,7 @@ buf2xml(FILE *f, clicon_err(OE_UNIX, errno, "fseek"); goto done; } - if (fread(hdr, sizeof(char), sizeof(hdr), f) < 0){ + if ((ret = fread(hdr, sizeof(char), sizeof(hdr), f)) != sizeof(hdr)){ clicon_err(OE_XML, errno, "fread"); goto done; } @@ -250,7 +251,7 @@ buf2xml(FILE *f, clicon_err(OE_UNIX, errno, "malloc"); goto done; } - if (fread(buf, sizeof(char), len-sizeof(hdr), f) < 0){ + if ((ret = fread(buf, sizeof(char), len-sizeof(hdr), f)) != len-sizeof(hdr)){ clicon_err(OE_XML, errno, "fread"); goto done; } @@ -318,7 +319,7 @@ xml2buf(FILE *f, int len1; int ptr; int i; - char *buf0; + char *buf0 = NULL; char *buf; uint32_t myindex; From e68a5ffa97e39ffec05747676b90fab37e87ef5c Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Fri, 10 May 2019 16:49:00 +0200 Subject: [PATCH 4/4] cli history file long string test --- test/test_cli_history.sh | 41 ++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/test/test_cli_history.sh b/test/test_cli_history.sh index c6700496..f6c50382 100755 --- a/test/test_cli_history.sh +++ b/test/test_cli_history.sh @@ -36,7 +36,7 @@ cat < $histfile first line EOF -# NOTE Backend is not really use here +# NOTE Backend is not really used here new "test params: -f $cfg" if [ $BE -ne 0 ]; then new "kill old backend" @@ -59,22 +59,22 @@ if [ ! -f $histfile ]; then err "$histfile" "not found" fi -new "Check it has two entries" -lines=$(cat $histfile | wc -l) -if [ $lines -ne 2 ]; then - err "Line:$lines" "2" +new "Check histfile has two entries" +nr=$(cat $histfile | wc -l) +if [ $nr -ne 2 ]; then + err "2" "$nr" fi -new "check it contains first line" +new "Check histfile contains first line" nr=$(grep -c "example 42" $histfile) if [ $nr -ne 1 ]; then - err "Contains: example 42" "1" + err "Contains: example 42" "$nr" fi -new "Check it contains example 42" +new "Check histfile contains example 42" nr=$(grep -c "example 42" $histfile) if [ $nr -ne 1 ]; then - err "Contains: example 42" "1" + err "1" "$nr" fi new "cli add entry and create newhist file" @@ -88,7 +88,28 @@ fi new "check it contains example 43" nr=$(grep -c "example 43" $dir/newhist) if [ $nr -ne 1 ]; then - err "Contains: example 43" "1" + err "1" "$nr" +fi + +# Add a long (128 chars) string and see it survives +str128="1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567" +new "cli add long line" +expecteof "$clixon_cli -f $cfg" 0 "$str128" "" 2> /dev/null # ignore error output + +new "Check histfile contains long string" +nr=$(grep -c "$str128" $histfile) +if [ $nr -ne 1 ]; then + err "1" "$nr" +fi + +new "cli load arrow-up save -> create two copies of long string" +expecteof "$clixon_cli -f $cfg" 0 "q" "" +expecteof "$clixon_cli -f $cfg" 0 "" "" 2> /dev/null + +new "Check histfile contains two copies of long string" +nr=$(grep -c "$str128" $histfile) +if [ $nr -ne 2 ]; then + err "2" "$nr" fi if [ $BE -eq 0 ]; then