C-API: New no-copy xmldb_get_cache function for performance

Added SKIP flag to XML for skipping nodes in xml_diff

CHANGELOG.md

@@ -17,6 +17,7 @@ Expected: January 2025
 
 ### Features
 
+* C-API: New no-copy `xmldb_get_cache` function for performance
 * New: CLI generic pipe callbacks
   * Add scripts in `CLICON_CLI_PIPE_DIR`
 * New: [feature request: support xpath functions for strings](https://github.com/clicon/clixon/issues/556)

apps/backend/backend_commit.c

@@ -441,9 +441,8 @@ startup_validate(clixon_handle  h,
     }
     retval = 1;
  done:
-     if (td){
+     if (td)
-         transaction_free(td);
+         transaction_free1(td, 1);
-     }
     return retval;
  fail: /* cbret should be set */
     retval = 0;
@@ -662,7 +661,7 @@ candidate_validate(clixon_handle h,
      if (td){
          if (retval < 1)
              plugin_transaction_abort_all(h, td);
-         transaction_free(td);
+        transaction_free1(td, 1);
      }
     return retval;
  fail:
@@ -772,7 +771,7 @@ candidate_commit(clixon_handle  h,
     if (td){
         if (retval < 1)
             plugin_transaction_abort_all(h, td);
-        transaction_free(td);
+        transaction_free1(td, 1);
     }
     if (xret)
         xml_free(xret);
@@ -1081,9 +1080,8 @@ from_client_restart_one(clixon_handle    h,
         goto fail;
     retval = 1;
  done:
-    if (td){
+    if (td)
-         transaction_free(td);
+        transaction_free1(td, 1);
-     }
     return retval;
  fail:
     retval = 0;

apps/backend/backend_plugin.c

@@ -555,10 +555,32 @@ transaction_new(void)
 int
 transaction_free(transaction_data_t *td)
 {
-    if (td->td_src)
+    return transaction_free1(td, 1);
-        xml_free(td->td_src);
+}
-    if (td->td_target)
+
-        xml_free(td->td_target);
+/*! Free transaction structure
+ *
+ * @param[in]  td    Transaction data will be deallocated after the call
+ * @param[in]  copy  0: XML trees are no-copy, clear and dont free, 1: free XML trees
+ */
+int
+transaction_free1(transaction_data_t *td,
+                  int                 copy)
+{
+    if (td->td_src){
+        if (copy)
+            xml_free(td->td_src);
+        else
+            xml_apply(td->td_src, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset,
+                      (void*)(XML_FLAG_NONE|XML_FLAG_ADD|XML_FLAG_DEL|XML_FLAG_CHANGE|XML_FLAG_SKIP|XML_FLAG_MARK));
+    }
+    if (td->td_target){
+        if (copy)
+            xml_free(td->td_target);
+        else
+            xml_apply(td->td_target, CX_ELMNT, (xml_applyfn_t*)xml_flag_reset,
+                      (void*)(XML_FLAG_NONE|XML_FLAG_ADD|XML_FLAG_DEL|XML_FLAG_CHANGE|XML_FLAG_SKIP|XML_FLAG_MARK));
+    }
     if (td->td_dvec)
         free(td->td_dvec);
     if (td->td_avec)

apps/backend/clixon_backend_plugin.h

@@ -111,6 +111,7 @@ int clixon_pagination_free(clixon_handle h);
 
 transaction_data_t * transaction_new(void);
 int transaction_free(transaction_data_t *);
+int transaction_free1(transaction_data_t *, int copy);
 
 int plugin_transaction_begin_one(clixon_plugin_t *cp, clixon_handle h, transaction_data_t *td);
 int plugin_transaction_begin_all(clixon_handle h, transaction_data_t *td);

include/clixon_custom.h

@@ -213,8 +213,8 @@
  * This causes xml_cmp to show that the datastores are unequal and may cause a wrong diff, or
  * worse case an overwrite.
  */
-
 #undef SYSTEM_ONLY_CONFIG_CANDIDATE_CLEAR
+
 /*! In full XPath namespace resolve, match even if namespace not resolved
  *
  * In the case of xpath lookup functions (eg xpath_vec_ctx) where nsc is defined, then
@@ -222,6 +222,14 @@
  * However, some code is OK with the XPATH NSC being unresolved to NULL, even if the XML
  * namespace is defined.
  * This seems wrong and should be changed, but need further investigation
+ * @see https://github.com/clicon/clixon/issues/588
  */
 #define XPATH_NS_ACCEPT_UNRESOLVED
 
+/*! Default algorithm needs two passes to resolve "when"-protected non-presence containers
+ *
+ * A non-presence container may be protected by a YANG "when" statement which relies on
+ * default values that have not yet been resolved.
+ * A more intelligent algorithm is needed
+ */
+#define XML_DEFAULT_WHEN_TWICE

lib/clixon/clixon_datastore.h

@@ -76,6 +76,8 @@ int xmldb_get(clixon_handle h, const char *db, cvec *nsc, char *xpath, cxobj **x
 int xmldb_get0(clixon_handle h, const char *db, yang_bind yb,
                cvec *nsc, const char *xpath, int copy, withdefaults_type wdef,
                cxobj **xret, modstate_diff_t *msd, cxobj **xerr);
+int xmldb_get_cache(clixon_handle h, const char *db, yang_bind yb,
+                    cxobj **xtp, modstate_diff_t *msdiff, cxobj **xerr);
 /* in clixon_datastore_write.[ch]: */
 int xmldb_put(clixon_handle h, const char *db, enum operation_type op, cxobj *xt, char *username, cbuf *cbret);
 int xmldb_dump(clixon_handle h, FILE *f, cxobj *xt, enum format_enum format, int pretty, withdefaults_type wdef, int multi, const char *multidb);

lib/clixon/clixon_xml.h

@@ -209,6 +209,7 @@ enum format_enum{
 #define XML_FLAG_BODYKEY  0x100 /* Text parsing key to be translated from body to key */
 #define XML_FLAG_ANYDATA  0x200 /* Treat as anydata, eg mount-points before bound */
 #define XML_FLAG_CACHE_DIRTY 0x400 /* This part of XML tree is not synced to disk */
+#define XML_FLAG_SKIP      0x800 /* Node is skipped in xml_diff */
 
 /*
  * Prototypes

lib/src/clixon_datastore.c

@@ -717,6 +717,7 @@ xmldb_db_reset(clixon_handle h,
  * @param[in]  h    Clixon handle
  * @param[in]  db   Database name
  * @retval     xml  XML cached tree or NULL
+ * @see xmldb_get_cache  Read from store if miss
  */
 cxobj *
 xmldb_cache_get(clixon_handle h,
@@ -950,6 +951,7 @@ xmldb_rename(clixon_handle h,
  * @retval     1      OK
  * @retval     0      YANG assigment and default assignment not made
  * @retval    -1      General error, check specific clicon_errno, clicon_suberrno
+ * @see xmldb_get_cache  Consider using this instead
  */
 int
 xmldb_populate(clixon_handle h,

lib/src/clixon_datastore_read.c

@@ -764,7 +764,78 @@ xmldb_readfile(clixon_handle    h,
     goto done;
 }
 
-/*! Get content of database using xpath. return a set of matching sub-trees
+/*! Get cache directly, read from datastore file if cache-miss
+ *
+ * Return the actual cache tree. With-default is report-all.
+ * @param[in]  h      Clixon handle
+ * @param[in]  db     Name of database to search in (filename including dir path
+ * @param[in]  yb     How to bind yang to XML top-level when parsing
+ * @param[out] xtp    Top-level XML tree, direct cache pointer
+ * @param[out] msdiff If set, return modules-state differences
+ * @param[out] xerr   XML error if retval is 0
+ * @retval     1      OK
+ * @retval     0      Parse OK but yang assigment not made (or only partial) and xerr set
+ * @retval    -1      Error
+ * @see xmldb_get_copy
+ * @note Do not modify or free xtp
+ */
+int
+xmldb_get_cache(clixon_handle     h,
+                const char       *db,
+                yang_bind         yb,
+                cxobj           **xtp,
+                modstate_diff_t  *msdiff,
+                cxobj           **xerr)
+{
+    int        retval = -1;
+    cxobj     *xt = NULL; /* (cached) top of tree */
+    db_elmnt  *de = NULL;
+    db_elmnt   de0 = {0,};
+    yang_stmt *yspec0;
+    int        ret;
+
+    clixon_debug(CLIXON_DBG_DATASTORE, "db %s", db);
+    de = clicon_db_elmnt_get(h, db);
+    if ((yspec0 = clicon_dbspec_yang(h)) == NULL){
+        clixon_err(OE_YANG, ENOENT, "No yang spec");
+        goto done;
+    }
+    if (de == NULL || de->de_xml == NULL){ /* Cache miss, read XML from file */
+        /* If there is no xml x0 tree (in cache), then read it from file */
+        /* xml looks like: <top><config><x>... where "x" is a top-level symbol in a module */
+        if ((ret = xmldb_readfile(h, db, yb, yspec0, &xt, &de0, msdiff, xerr)) < 0)
+            goto done;
+        if (ret == 0)
+            goto fail;
+        /* Should we validate file if read from disk?
+         * No, argument against: we may want to have a semantically wrong file and wish to edit?
+         */
+        de0.de_xml = xt;
+        if (de)
+            de0.de_id = de->de_id;
+        clicon_db_elmnt_set(h, db, &de0); /* Content is copied */
+        /* Add default global values (to make xpath below include defaults) */
+        // Alt:  xmldb_populate(h, db)
+        if (yb != YB_NONE) {
+            if (xml_global_defaults(h, xt, NULL, "/", yspec0, 0) < 0)
+                goto done;
+            /* Add default recursive values */
+            if (xml_default_recurse(xt, 0, 0) < 0)
+                goto done;
+        }
+    } /* xt == NULL */
+    else
+        xt = de->de_xml;
+    *xtp = xt;
+    retval = 1;
+ done:
+    return retval;
+ fail:
+    retval = 0;
+    goto done;
+}
+
+/*! Get a copy of datastore cache using xpath. return a set of matching sub-trees
  *
  * The function returns a minimal tree that includes all sub-trees that match
  * xpath.
@@ -781,28 +852,27 @@ xmldb_readfile(clixon_handle    h,
  * @retval     1      OK
  * @retval     0      Parse OK but yang assigment not made (or only partial) and xerr set
  * @retval    -1      Error
+ * @see xmldb_get_cache
  */
 static int
-xmldb_get_cache(clixon_handle     h,
+xmldb_get_copy(clixon_handle     h,
-                const char       *db,
+               const char       *db,
-                yang_bind         yb,
+               yang_bind         yb,
-                cvec             *nsc,
+               cvec             *nsc,
-                const char       *xpath,
+               const char       *xpath,
-                withdefaults_type wdef,
+               withdefaults_type wdef,
-                cxobj           **xret,
+               cxobj           **xret,
-                modstate_diff_t  *msdiff,
+               modstate_diff_t  *msdiff,
-                cxobj           **xerr)
+               cxobj           **xerr)
 {
     int        retval = -1;
-    yang_stmt *yspec;
+    yang_stmt *yspec0;
     cxobj     *x0t = NULL; /* (cached) top of tree */
     cxobj     *x0;
     cxobj    **xvec = NULL;
     size_t     xlen;
     int        i;
-    db_elmnt  *de = NULL;
     cxobj     *x1t = NULL;
-    db_elmnt   de0 = {0,};
     int        ret;
 
     clixon_debug(CLIXON_DBG_DATASTORE, "db %s", db);
@@ -810,37 +880,14 @@ xmldb_get_cache(clixon_handle     h,
         clixon_err(OE_DB, EINVAL, "xret is NULL");
         return -1;
     }
-    if ((yspec = clicon_dbspec_yang(h)) == NULL){
+    if ((yspec0 = clicon_dbspec_yang(h)) == NULL){
         clixon_err(OE_YANG, ENOENT, "No yang spec");
         goto done;
     }
-    de = clicon_db_elmnt_get(h, db);
+    if ((ret = xmldb_get_cache(h, db, yb, &x0t, msdiff, xerr)) < 0)
-    if (de == NULL || de->de_xml == NULL){ /* Cache miss, read XML from file */
+        goto done;
-        /* If there is no xml x0 tree (in cache), then read it from file */
+    if (ret == 0)
-        /* xml looks like: <top><config><x>... where "x" is a top-level symbol in a module */
+        goto fail;
-        if ((ret = xmldb_readfile(h, db, yb, yspec, &x0t, &de0, msdiff, xerr)) < 0)
-            goto done;
-        if (ret == 0)
-            goto fail;
-        /* Should we validate file if read from disk?
-         * No, argument against: we may want to have a semantically wrong file and wish to edit?
-         */
-        de0.de_xml = x0t;
-        if (de)
-            de0.de_id = de->de_id;
-        clicon_db_elmnt_set(h, db, &de0); /* Content is copied */
-        /* Add default global values (to make xpath below include defaults) */
-        // Alt:  xmldb_populate(h, db)
-        if (yb != YB_NONE) {
-            if (xml_global_defaults(h, x0t, nsc, xpath, yspec, 0) < 0)
-                goto done;
-            /* Add default recursive values */
-            if (xml_default_recurse(x0t, 0, 0) < 0)
-                goto done;
-        }
-    } /* x0t == NULL */
-    else
-        x0t = de->de_xml;
     /* Here x0t looks like: <config>...</config> */
     /* Given the xpath, return a vector of matches in xvec
      * Can we do everything in one go?
@@ -897,7 +944,7 @@ xmldb_get_cache(clixon_handle     h,
     /* If empty NACM config, then disable NACM if loaded
      */
     if (clicon_option_bool(h, "CLICON_NACM_DISABLED_ON_EMPTY")){
-        if (disable_nacm_on_empty(x1t, yspec) < 0)
+        if (disable_nacm_on_empty(x1t, yspec0) < 0)
             goto done;
     }
     clixon_debug_xml(CLIXON_DBG_DATASTORE | CLIXON_DBG_DETAIL, x1t, "");
@@ -995,7 +1042,7 @@ xmldb_get0(clixon_handle    h,
            yang_bind        yb,
            cvec            *nsc,
            const char      *xpath,
-           int              copy,
+           int              copy, // XXX notused
            withdefaults_type wdef,
            cxobj          **xret,
            modstate_diff_t *msdiff,
@@ -1006,8 +1053,8 @@ xmldb_get0(clixon_handle    h,
     cxobj *x = NULL;
 
     if (wdef != WITHDEFAULTS_EXPLICIT)
-        return xmldb_get_cache(h, db, yb, nsc, xpath, 0, xret, msdiff, xerr);
+        return xmldb_get_copy(h, db, yb, nsc, xpath, 0, xret, msdiff, xerr);
-    if ((ret = xmldb_get_cache(h, db, yb, nsc, xpath, 0, &x, msdiff, xerr)) < 0)
+    if ((ret = xmldb_get_copy(h, db, yb, nsc, xpath, 0, &x, msdiff, xerr)) < 0)
         goto done;
     if (ret == 0)
         goto fail;

lib/src/clixon_datastore_write.c

@@ -1482,6 +1482,12 @@ xmldb_put(clixon_handle       h,
     /* Add default recursive values */
     if (xml_default_recurse(x0, 0, XML_FLAG_ADD|XML_FLAG_DEL) < 0)
         goto done;
+#ifdef XML_DEFAULT_WHEN_TWICE
+    /* Defaults a second time for when statements that depend on defaults that have not yet been evaluated
+     */
+    if (xml_default_recurse(x0, 0, XML_FLAG_ADD|XML_FLAG_DEL) < 0)
+        goto done;
+#endif
     /* Write back to datastore cache if first time */
     if (de != NULL)
         de0 = *de;

lib/src/clixon_xml_map.c

@@ -345,6 +345,9 @@ xml_diff_ordered_by_user(cxobj     *x0,
  * (*) "comparing" a&b here is made by xml_cmp() which judges equality from a structural
  *     perspective, ie both have the same yang spec, if they are lists, they have the
  *     the same keys. NOT that the values are equal!
+ * Also, a node is skipped if:
+ * 1) its xml flag has XML_FLAG_SKIP
+ * 2) its yang has extension clixon-lib:ignore-compare
  * @see xml_diff2cbuf, clixon_text_diff2cbuf  for +/- diff for XML and TEXT formats
  * @see text_diff2cbuf for curly
  * @see xml_tree_equal Equal or not
@@ -383,29 +386,35 @@ xml_diff1(cxobj     *x0,
         y0c = NULL;
         y1c = NULL;
         /* If cl:ignore-compare extension, return equal */
-        if (x0c && (y0c = xml_spec(x0c)) != NULL){
+        if (x0c) {
-            if (yang_extension_value(y0c, "ignore-compare", CLIXON_LIB_NS, &extflag, NULL) < 0)
+            if (xml_flag(x0c, XML_FLAG_SKIP) != 0x0){ /* skip */
-                goto done;
+                x0c = xml_child_each(x0, x0c, CX_ELMNT);
-            if (extflag){ /* skip */
+                continue;
-                if (x1c) {
-                    x0c = xml_child_each(x0, x0c, CX_ELMNT);
-                    continue;
-                }
-                else
-                    goto ok;
             }
+            else
+                if ((y0c = xml_spec(x0c)) != NULL){
+                    if (yang_extension_value(y0c, "ignore-compare", CLIXON_LIB_NS, &extflag, NULL) < 0)
+                        goto done;
+                    if (extflag){ /* skip */
+                        x0c = xml_child_each(x0, x0c, CX_ELMNT);
+                        continue;
+                    }
+                }
         }
-        if (x1c && (y1c = xml_spec(x1c)) != NULL){
+        if (x1c) {
-            if (yang_extension_value(y1c, "ignore-compare", CLIXON_LIB_NS, &extflag, NULL) < 0)
+            if (xml_flag(x1c, XML_FLAG_SKIP) != 0x0){ /* skip */
-                goto done;
+                x1c = xml_child_each(x1, x1c, CX_ELMNT);
-            if (extflag){ /* skip */
+                continue;
-                if (x1c) {
-                    x1c = xml_child_each(x1, x1c, CX_ELMNT);
-                    continue;
-                }
-                else
-                    goto ok;
             }
+            else
+                if ((y1c = xml_spec(x1c)) != NULL){
+                    if (yang_extension_value(y1c, "ignore-compare", CLIXON_LIB_NS, &extflag, NULL) < 0)
+                        goto done;
+                    if (extflag){ /* skip */
+                        x1c = xml_child_each(x1, x1c, CX_ELMNT);
+                        continue;
+                    }
+                }
         }
         if (x0c == NULL){
             if (cxvec_append(x1c, x1vec, x1veclen) < 0)