Fixed bug that deletes running on startup if backup started with -m running

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## 3.4.0 (Upcoming)
 
+* Fixed bug that deletes running on startup if backup started with -m running.
+  The error was that the running (or startup) configuration may fail when
+  clixon backend starts. 
+  The fix now makes a copy of running and copies it back on failure.
+  
 * experimental netconf yang rpc 
 
 * datastore/keyvalue/Makefile is left behind on make distclean. Fixed by conditional configure. Thanks renato@netgate.com.

apps/backend/backend_commit.c

@@ -130,7 +130,8 @@ generic_validate(yang_spec          *yspec,
  * @param[in] h         Clicon handle
  * @param[in] candidate The candidate database. The wanted backend state
  * @retval    0         OK  
- * @retval   -1         Fatal error or netconf error XXX Differentiate
+ * @retval   -1         Fatal error or validation fail
+ * @note Need to differentiate between error and validation fail
  */
 static int
 validate_common(clicon_handle       h, 
@@ -215,7 +216,10 @@ validate_common(clicon_handle       h,
  * do something more drastic?
  * @param[in]  h         Clicon handle
  * @param[in]  candidate A candidate database, not necessarily "candidate"
-*/
+ * @retval    0         OK  
+ * @retval   -1         Fatal error or validation fail
+ * @note Need to differentiate between error and validation fail
+ */
 int
 candidate_commit(clicon_handle h, 
 		 char         *candidate)
@@ -285,7 +289,7 @@ from_client_commit(clicon_handle h,
 		piddb);
 	goto ok;
     }
-    if (candidate_commit(h, "candidate") < 0){
+    if (candidate_commit(h, "candidate") < 0){ /* Assume validation fail, nofatal */
 	clicon_debug(1, "Commit candidate failed");
 	cprintf(cbret, "<rpc-reply><rpc-error>"
 		"<error-tag>invalid-value</error-tag>"
@@ -295,8 +299,6 @@ from_client_commit(clicon_handle h,
 		"</rpc-error></rpc-reply>",
 		clicon_err_reason);
         goto ok;
-
-	goto ok;
     }
     cprintf(cbret, "<rpc-reply><ok/></rpc-reply>");
  ok:

apps/backend/backend_main.c

@@ -164,7 +164,7 @@ static int
 db_reset(clicon_handle h, 
 	 char         *db)
 {
-    if (xmldb_delete(h, db) != 0 && errno != ENOENT) 
+    if (xmldb_exists(h, db) == 1 && xmldb_delete(h, db) != 0 && errno != ENOENT) 
 	return -1;
     if (xmldb_create(h, db) < 0)
 	return -1;
@@ -502,13 +502,22 @@ startup_mode_init(clicon_handle h)
 
 /*! Clixon running startup mode: Commit running db configuration into running 
  *
-        copy   reset              commit merge
+OK:
-running----+   |--------------------+-----+------>
+        copy   reset              commit   merge
-            \                      /     /
+running----+   |--------------------+--------+------>
-candidate    +--------------------+     /
+            \                      /        /
-                                       /
+candidate    +--------------------+        /
-tmp           |-------+-----+---------+
+                                          /
+tmp           |-------+-----+------------+---|
              reset   extra  file
+
+COMMIT ERROR:
+        copy   reset              copy 
+running----+   |--------------------+------> EXIT
+            \                      /       
+candidate    +--------------------+        
+
+ * @note: if commit fails, copy candidate to running and exit
  */
 static int
 startup_mode_running(clicon_handle h,
@@ -522,9 +531,6 @@ startup_mode_running(clicon_handle h,
     /* Load plugins and call plugin_init() */
     if (plugin_initiate(h) != 0) 
 	goto done;
-    /* Clear running db */
-    if (db_reset(h, "running") < 0)
-	goto done;
     /* Clear tmp db */
     if (db_reset(h, "tmp") < 0)
 	goto done;
@@ -534,25 +540,53 @@ startup_mode_running(clicon_handle h,
     /* Get application extra xml from file */
     if (load_extraxml(h, extraxml_file, "tmp") < 0)   
 	goto done;	    
-    /* Commit original running */
+    /* Clear running db */
-    if (candidate_commit(h, "candidate") < 0)
+    if (db_reset(h, "running") < 0)
 	goto done;
+    /* Commit original running. Assume -1 is validate fail */
+    if (candidate_commit(h, "candidate") < 0){
+	/*  (1) We cannot differentiate between fatal errors and validation
+	 *      failures
+	 *  (2) If fatal error, we should exit
+	 *  (3) If validation fails we cannot continue. How could we?
+	 *  (4) Need to restore the running db since we destroyed it above
+	 */
+	clicon_log(LOG_NOTICE, "%s: Commit of saved running failed, exiting.", __FUNCTION__);
+	/* Reinstate original */
+	if (xmldb_copy(h, "candidate", "running") < 0)
+	    goto done;
+	goto done;
+    }
     /* Merge user reset state and extra xml file (no commit) */
     if (db_merge(h, "tmp", "running") < 0)
 	goto done;
     retval = 0;
  done:
+    if (xmldb_delete(h, "tmp") < 0)
+	goto done;
     return retval;
 }
 
 /*! Clixon startup startup mode: Commit startup configuration into running state
-               reset              commit merge
+
-running        |--------------------+-----+------>
+
-                                   /     /
+backup         +--------------------|
-startup       --------------------+     /
+         copy / reset              commit merge
-                                       /
+running   |-+----|--------------------+-----+------>
-tmp           |-------+-----+---------+
+                                     /     /
+startup    -------------------------+-->  /
+                                         /
+tmp        -----|-------+-----+---------+--|
              reset   extra  file
+
+COMMIT ERROR:
+backup         +------------------------+--|
+         copy / reset               copy \
+running   |-+----|--------------------+---+------->EXIT
+                               error / 
+startup    -------------------------+--|    
+
+ * @note: if commit fails, copy backup to commit and exit
  */
 static int
 startup_mode_startup(clicon_handle h,
@@ -560,6 +594,9 @@ startup_mode_startup(clicon_handle h,
 {
     int     retval = -1;
 
+    /* Stash original running to backup */
+    if (xmldb_copy(h, "running", "backup") < 0)
+	goto done;
     /* If startup does not exist, clear it */
     if (xmldb_exists(h, "startup") != 1) /* diff */
 	if (xmldb_create(h, "startup") < 0) /* diff */
@@ -567,9 +604,6 @@ startup_mode_startup(clicon_handle h,
     /* Load plugins and call plugin_init() */
     if (plugin_initiate(h) != 0) 
 	goto done;
-    /* Clear running db */
-    if (db_reset(h, "running") < 0)
-	goto done;
     /* Clear tmp db */
     if (db_reset(h, "tmp") < 0)
 	goto done;
@@ -579,14 +613,29 @@ startup_mode_startup(clicon_handle h,
     /* Get application extra xml from file */
     if (load_extraxml(h, extraxml_file, "tmp") < 0)   
 	goto done;	    
-    /* Commit startup */
+    /* Clear running db */
-    if (candidate_commit(h, "startup") < 0) /* diff */
+    if (db_reset(h, "running") < 0)
 	goto done;
+    /* Commit startup */
+    if (candidate_commit(h, "startup") < 0){ /* diff */
+	/*  We cannot differentiate between fatal errors and validation
+	 *  failures
+	 *  In both cases we copy back the original running and quit
+	 */
+	clicon_log(LOG_NOTICE, "%s: Commit of startup failed, exiting.", __FUNCTION__);
+	if (xmldb_copy(h, "backup", "running") < 0)
+	    goto done;
+	goto done;
+    }
     /* Merge user reset state and extra xml file (no commit) */
     if (db_merge(h, "tmp", "running") < 0)
 	goto done;
     retval = 0;
  done:
+    if (xmldb_delete(h, "backup") < 0)
+	goto done;
+    if (xmldb_delete(h, "tmp") < 0)
+	goto done;
     return retval;
 }
 

apps/backend/backend_plugin.h

@@ -40,10 +40,15 @@
  * Types
  */
 
-/*! Transaction data
+/*! Transaction data describing a system transition from a src to target state
  * Clicon internal, presented as void* to app's callback in the 'transaction_data'
  * type in clicon_backend_api.h
- * XXX: move to .c file?
+ * The struct contains source and target XML tree (e.g. candidate/running)
+ * But primarily a set of XML tree vectors (dvec, avec, cvec) and associated lengths
+ * These contain the difference between src and target XML, ie "what has changed".
+ * It is up to the validate callbacks to ensure that these changes are OK
+ * It is up to the commit callbacks to enforce these changes in the "state" of 
+ *the system.
  */
 typedef struct {
     uint64_t   td_id;       /* Transaction id */

datastore/text/clixon_xmldb_text.c

@@ -975,7 +975,7 @@ text_put(xmldb_handle        xh,
 
 /*! Copy database from db1 to db2
  * @param[in]  xh  XMLDB handle
- * @param[in]  from  Source database copy
+ * @param[in]  from  Source database
  * @param[in]  to    Destination database
  * @retval -1  Error
  * @retval  0  OK
@@ -995,7 +995,6 @@ text_copy(xmldb_handle xh,
     /* XXX lock */
     if (xmltree_cache){
 	/* 1. Free xml tree in "to"
-
 	 */
 	if ((de = hash_value(th->th_dbs, to, NULL)) != NULL){
 	    if (de->de_xml != NULL){

lib/src/clixon_xml_db.c

@@ -432,7 +432,7 @@ xmldb_put(clicon_handle       h,
 
 /*! Copy database from db1 to db2
  * @param[in]  h     Clicon handle
- * @param[in]  from  Source database copy
+ * @param[in]  from  Source database
  * @param[in]  to    Destination database
  * @retval -1  Error
  * @retval  0  OK