diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index d6361492..ca3606fa 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -273,11 +273,10 @@ clixon_stats_get_db(clicon_handle h, cxobj *xt = NULL; uint64_t nr = 0; size_t sz = 0; - db_elmnt *de = NULL; /* This is the db cache */ - if ((de = clicon_db_elmnt_get(h, dbname)) == NULL || - (xt = de->de_xml) == NULL){ + if ((xt = xmldb_cache_get(h, dbname)) == NULL){ + cprintf(cb, "%s00", dbname); } else{ @@ -606,6 +605,7 @@ from_client_edit_config(clicon_handle h, goto done; goto ok; } + if ((x = xpath_first(xn, NULL, "default-operation")) != NULL){ if (xml_operation(xml_body(x), &operation) < 0){ if (netconf_invalid_value(cbret, "protocol", "Wrong operation")< 0) @@ -618,49 +618,48 @@ from_client_edit_config(clicon_handle h, goto done; goto ok; } - else{ - /* yang spec may be set to anyxml by ingress yang check,...*/ - if (xml_spec(xc) != NULL) - xml_spec_set(xc, NULL); - /* Populate XML with Yang spec (why not do this in parser?) - */ - if ((ret = xml_bind_yang(xc, YB_MODULE, yspec, &xret)) < 0) + /* yang spec may be set to anyxml by ingress yang check,...*/ + if (xml_spec(xc) != NULL) + xml_spec_set(xc, NULL); + /* Populate XML with Yang spec (why not do this in parser?) + */ + if ((ret = xml_bind_yang(xc, YB_MODULE, yspec, &xret)) < 0) + goto done; + if (ret == 0){ + if (clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) goto done; - if (ret == 0){ - if (clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) - goto done; - goto ok; - } - /* Maybe validate xml here as in text_modify_top? */ - if (xml_apply(xc, CX_ELMNT, xml_non_config_data, &non_config) < 0) - goto done; - if (non_config){ - if (netconf_invalid_value(cbret, "protocol", "State data not allowed")< 0) - goto done; - goto ok; - } - /* xmldb_put (difflist handling) requires list keys */ - if ((ret = xml_yang_validate_list_key_only(h, xc, &xret)) < 0) - goto done; - if (ret == 0){ - if (clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) - goto done; - goto ok; - } - /* Cant do this earlier since we dont have a yang spec to - * the upper part of the tree, until we get the "config" tree. - */ - if (xml_sort_recurse(xc) < 0) - goto done; - if ((ret = xmldb_put(h, target, operation, xc, username, cbret)) < 0){ - clicon_debug(1, "%s ERROR PUT", __FUNCTION__); - if (netconf_operation_failed(cbret, "protocol", clicon_err_reason)< 0) - goto done; - goto ok; - } - if (ret == 0) - goto ok; + goto ok; } + /* Maybe validate xml here as in text_modify_top? */ + if (xml_apply(xc, CX_ELMNT, xml_non_config_data, &non_config) < 0) + goto done; + if (non_config){ + if (netconf_invalid_value(cbret, "protocol", "State data not allowed")< 0) + goto done; + goto ok; + } + /* xmldb_put (difflist handling) requires list keys */ + if ((ret = xml_yang_validate_list_key_only(h, xc, &xret)) < 0) + goto done; + if (ret == 0){ + if (clicon_xml2cbuf(cbret, xret, 0, 0, -1) < 0) + goto done; + goto ok; + } + /* Cant do this earlier since we dont have a yang spec to + * the upper part of the tree, until we get the "config" tree. + */ + if (xml_sort_recurse(xc) < 0) + goto done; + if ((ret = xmldb_put(h, target, operation, xc, username, cbret)) < 0){ + clicon_debug(1, "%s ERROR PUT", __FUNCTION__); + if (netconf_operation_failed(cbret, "protocol", clicon_err_reason)< 0) + goto done; + goto ok; + } + if (ret == 0) + goto ok; + xmldb_modified_set(h, target, 1); /* mark as dirty */ assert(cbuf_len(cbret) == 0); cprintf(cbret, ""); ok: @@ -742,6 +741,7 @@ from_client_copy_config(clicon_handle h, goto done; goto ok; } + xmldb_modified_set(h, target, 1); /* mark as dirty */ cprintf(cbret, ""); ok: retval = 0; @@ -808,6 +808,7 @@ from_client_delete_config(clicon_handle h, goto done; goto ok; } + xmldb_modified_set(h, target, 1); /* mark as dirty */ cprintf(cbret, ""); ok: retval = 0; @@ -859,21 +860,26 @@ from_client_lock(clicon_handle h, /* * A lock MUST not be granted if either of the following conditions is true: * 1) A lock is already held by any NETCONF session or another entity. - * 2) The target configuration is , it has already been modified, and - * these changes have not been committed or rolled back. */ - iddb = xmldb_islocked(h, db); - if (iddb != 0){ + if ((iddb = xmldb_islocked(h, db)) != 0){ cprintf(cbx, "%u", iddb); if (netconf_lock_denied(cbret, cbuf_get(cbx), "Operation failed, lock is already held") < 0) goto done; goto ok; } - else{ - if (xmldb_lock(h, db, id) < 0) + /* 2) The target configuration is , it has already been modified, and + * these changes have not been committed or rolled back. + */ + if (strcmp(db, "candidate") == 0 && + xmldb_modified_get(h, db)){ + if (netconf_lock_denied(cbret, "0", + "Operation failed, candidate has already been modified and the changes have not been committed or rolled back (RFC 6241 7.5)") < 0) goto done; - cprintf(cbret, ""); + goto ok; } + if (xmldb_lock(h, db, id) < 0) + goto done; + cprintf(cbret, ""); ok: retval = 0; done: @@ -926,12 +932,19 @@ from_client_unlock(clicon_handle h, * An unlock operation will not succeed if any of the following * conditions are true: * 1) the specified lock is not currently active - * 2) the session issuing the operation is not the same + */ + if (iddb == 0){ + cprintf(cbx, "0"); + if (netconf_lock_denied(cbret, cbuf_get(cbx), "Unlock failed, lock is not currently active") < 0) + goto done; + goto ok; + } + /* 2) the session issuing the operation is not the same * session that obtained the lock */ - if (iddb == 0 || iddb != id){ - cprintf(cbx, "id=%u iddb=%d", id, iddb); - if (netconf_lock_denied(cbret, cbuf_get(cbx), "Unlock failed, lock is already held") < 0) + else if (iddb != id){ + cprintf(cbx, "%u", iddb); + if (netconf_lock_denied(cbret, cbuf_get(cbx), "Unlock failed, lock held by other session") < 0) goto done; goto ok; } diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c index 6879424b..9a285c4d 100644 --- a/apps/backend/backend_commit.c +++ b/apps/backend/backend_commit.c @@ -572,6 +572,7 @@ candidate_commit(clicon_handle h, */ if (xmldb_copy(h, candidate, "running") < 0) goto done; + xmldb_modified_set(h, candidate, 0); /* reset dirty bit */ /* Here pointers to old (source) tree are obsolete */ if (td->td_dvec){ td->td_dlen = 0; @@ -640,7 +641,7 @@ from_client_commit(clicon_handle h, goto done; } cprintf(cbx, "%u", iddb); - if (netconf_lock_denied(cbret, cbuf_get(cbx), "Operation failed, lock is already held") < 0) + if (netconf_in_use(cbret, cbuf_get(cbx), "Operation failed, lock is already held") < 0) goto done; goto ok; } @@ -703,6 +704,7 @@ from_client_discard_changes(clicon_handle h, goto done; goto ok; } + xmldb_modified_set(h, "candidate", 0); /* reset dirty bit */ cprintf(cbret, ""); ok: retval = 0; diff --git a/apps/backend/backend_main.c b/apps/backend/backend_main.c index 7d304305..96b889fc 100644 --- a/apps/backend/backend_main.c +++ b/apps/backend/backend_main.c @@ -871,6 +871,8 @@ main(int argc, /* Initiate the shared candidate. */ if (xmldb_copy(h, "running", "candidate") < 0) goto done; + xmldb_modified_set(h, "candidate", 0); + /* Set startup status */ if (clicon_startup_status_set(h, status) < 0) goto done; diff --git a/lib/clixon/clixon_data.h b/lib/clixon/clixon_data.h index 154af55b..e71c63ae 100644 --- a/lib/clixon/clixon_data.h +++ b/lib/clixon/clixon_data.h @@ -46,10 +46,15 @@ /* * Types */ -/* Struct per database in hash */ +/* Struct per database in hash + * Semantics of de_modified is to implement this from RFC 6241 Sec 7.5: + * The target configuration is , it has already been + * modified, and these changes have not been committed or rolled back. + */ typedef struct { - uint32_t de_id; /* session id */ - cxobj *de_xml; /* cache */ + uint32_t de_id; /* session id */ + cxobj *de_xml; /* cache */ + int de_modified; } db_elmnt; /* diff --git a/lib/clixon/clixon_datastore.h b/lib/clixon/clixon_datastore.h index 478280b8..8b014dda 100644 --- a/lib/clixon/clixon_datastore.h +++ b/lib/clixon/clixon_datastore.h @@ -69,4 +69,10 @@ int xmldb_create(clicon_handle h, const char *db); /* utility functions */ int xmldb_db_reset(clicon_handle h, char *db); +cxobj *xmldb_cache_get(clicon_handle h, const char *db); + +int xmldb_modified_get(clicon_handle h, const char *db); +int xmldb_modified_set(clicon_handle h, const char *db, int value); + + #endif /* _CLIXON_DATASTORE_H */ diff --git a/lib/src/clixon_datastore.c b/lib/src/clixon_datastore.c index 63d6f7e7..b091c08d 100644 --- a/lib/src/clixon_datastore.c +++ b/lib/src/clixon_datastore.c @@ -256,7 +256,7 @@ xmldb_copy(clicon_handle h, * @param[in] id Session id * @retval -1 Error * @retval 0 OK - */ + */ int xmldb_lock(clicon_handle h, const char *db, @@ -325,11 +325,11 @@ xmldb_unlock_all(clicon_handle h, } /*! Check if database is locked - * @param[in] h Clicon handle - * @param[in] db Database - * @retval -1 Error - * @retval 0 Not locked - * @retval >0 Id of locker + * @param[in] h Clicon handle + * @param[in] db Database + * @retval -1 Error + * @retval 0 Not locked + * @retval >0 Session id of locker */ uint32_t xmldb_islocked(clicon_handle h, @@ -479,3 +479,65 @@ xmldb_db_reset(clicon_handle h, return -1; return 0; } + +/*! Get datastore XML cache + * @param[in] h Clicon handle + * @param[in] db Database name + * @retval xml XML cached tree or NULL + */ +cxobj * +xmldb_cache_get(clicon_handle h, + const char *db) +{ + db_elmnt *de; + + if ((de = clicon_db_elmnt_get(h, db)) == NULL) + return NULL; + return de->de_xml; +} + +/*! Get modified flag from datastore + * @param[in] h Clicon handle + * @param[in] db Database name + * @retval -1 Error (datastore does not exist) + * @retval 0 Db is not modified + * @retval 1 Db is modified + * @note This only makes sense for "candidate", see RFC 6241 Sec 7.5 + * @note This only works if db cache is used,... + */ +int +xmldb_modified_get(clicon_handle h, + const char *db) +{ + db_elmnt *de; + + if ((de = clicon_db_elmnt_get(h, db)) == NULL){ + clicon_err(OE_CFG, EFAULT, "datastore %s does not exist", db); + return -1; + } + return de->de_modified; +} + +/*! Get modified flag from datastore + * @param[in] h Clicon handle + * @param[in] db Database name + * @param[in] value 0 or 1 + * @retval -1 Error (datastore does not exist) + * @retval 0 OK + * @note This only makes sense for "candidate", see RFC 6241 Sec 7.5 + * @note This only works if db cache is used,... + */ +int +xmldb_modified_set(clicon_handle h, + const char *db, + int value) +{ + db_elmnt *de; + + if ((de = clicon_db_elmnt_get(h, db)) == NULL){ + clicon_err(OE_CFG, EFAULT, "datastore %s does not exist", db); + return -1; + } + de->de_modified = value; + return 0; +} diff --git a/test/test_netconf.sh b/test/test_netconf.sh index 94163507..f93418eb 100755 --- a/test/test_netconf.sh +++ b/test/test_netconf.sh @@ -164,14 +164,20 @@ expecteof "$clixon_netconf -qf $cfg" 0 ']]>]]>" '^eth1ex:ethtrueup42foo]]>]]>$' +new "netconf lock" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +new "netconf unlock" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^protocollock-denied0errorUnlock failed, lock is not currently active]]>]]>$" + new "netconf lock/unlock" expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>]]>]]>" "^]]>]]>]]>]]>$" new "netconf lock/lock" -expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>]]>]]>" "^]]>]]>protocollock-denied" -new "netconf lock" -expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" +new "netconf unlock/unlock" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>]]>]]>" "^protocollock-denied0errorUnlock failed, lock is not currently active]]>]]>protocollock-denied0errorUnlock failed, lock is not currently active]]>]]>$" new "close-session" expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" @@ -180,7 +186,20 @@ expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^44]]>]]>" "^]]>]]>$" -new "copy startup" +# modify candidate, then lock, should fail. +new "netconf edit config" +expecteof "$clixon_netconf -qf $cfg" 0 'a
]]>]]>' "^]]>]]>$" + +new "netconf lock: should fail" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^protocollock-denied0errorOperation failed, candidate has already been modified and the changes have not been committed or rolled back (RFC 6241 7.5)]]>]]>$" + +new "netconf discard-changes" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +new "netconf lock" +expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" + +new "copy startup to candidate" expecteof "$clixon_netconf -qf $cfg" 0 "]]>]]>" "^]]>]]>$" new "netconf get startup"