Fixed: [Problems with diff of YANG lists ordered-by user](clicon/clix…

…on#496)
clicon · Feb 23, 2024 · b69cb89 · b69cb89
1 parent 847cb1a
commit b69cb89
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 19 deletions.
diff --git a/src/controller_device_send.c b/src/controller_device_send.c
@@ -389,11 +389,12 @@ device_create_edit_config_diff(clixon_handle h,
     int    i;
     cxobj *xt = NULL;
     cxobj *xn;
-    cxobj *xroot;
     cxobj *xa;
     char  *reason = NULL;
     int    ret;
     cvec  *nsc = NULL;
+    cxobj **xvec = NULL;
+    size_t  xveclen;
 
     clixon_debug(1, "%s", __FUNCTION__);
     if (cbret == NULL){
@@ -410,7 +411,7 @@ device_create_edit_config_diff(clixon_handle h,
         if (xml_value_set(xa, xml_operation2str(OP_REMOVE)) < 0)
             goto done;
         xml_flag_set(xn, XML_FLAG_MARK);
-        /* Remove any subtree under xn (expect for list keys) */
+        /* Remove any subtree under xn (except for list keys) */
         if (remove_subtree(xn) < 0)
             goto done;
     }
@@ -439,15 +440,16 @@ device_create_edit_config_diff(clixon_handle h,
         goto done;
     if (xml_tree_prune_flagged_sub(x1, XML_FLAG_MARK, 1, NULL) < 0)
         goto done;
-    /* 3. Merge deleted nodes in x0 with added/changed nodes in x1 (diff-tree)*/
-    if ((ret = xml_merge(x0, x1, yspec, &reason)) < 0)
-        goto done;
     // XXX validate
     /* 4. Create an edit-config message and parse it */
     if ((cb = cbuf_new()) == NULL){
         clixon_err(OE_PLUGIN, errno, "cbuf_new");
         goto done;
     }
+    if (xml_name_set(x0, "config") < 0)
+        goto done;
+    if (xml_name_set(x1, "config") < 0)
+        goto done;
     cprintf(cb, "<rpc xmlns=\"%s\" xmlns:nc=\"%s\" message-id=\"%" PRIu64 "\">",
             NETCONF_BASE_NAMESPACE,
             NETCONF_BASE_NAMESPACE,
@@ -456,31 +458,39 @@ device_create_edit_config_diff(clixon_handle h,
     cprintf(cb, "<target><candidate/></target>");
     cprintf(cb, "<default-operation>none</default-operation>");
     cprintf(cb, "</edit-config>");
+    cprintf(cb, "<edit-config>");
+    cprintf(cb, "<target><candidate/></target>");
+    cprintf(cb, "<default-operation>none</default-operation>");
+    cprintf(cb, "</edit-config>");
     cprintf(cb, "</rpc>");
     if ((ret = clixon_xml_parse_string(cbuf_get(cb), YB_NONE, NULL, &xt, NULL)) < 0)
         goto done;
-    // XXX validate
-    xroot = xpath_first(xt, nsc, "rpc/edit-config");
-    if (xml_name_set(x0, "config") < 0)
+    if (xpath_vec(xt, nsc, "rpc/edit-config", &xvec, &xveclen) < 0)
+        goto done;
+    if (xveclen != 2){
+        clixon_err(OE_XML, 0, "Unexpected len %lu should be 2", xveclen);
+        goto done;
+    }
+    if (xml_addsub(xvec[0], x0) < 0)
         goto done;
-    /* 5. Add diff-tree to an outgoing netconf edit-config
-     * Local tree xt and external tree x0 temporarily grafted, must be pruned directly after use
-     */
-    if (xml_addsub(xroot, x0) < 0)
+    if (xml_addsub(xvec[1], x1) < 0)
         goto done;
     cbuf_reset(cb);
     if (clixon_xml2cbuf(cb, xt, 0, 0, NULL, -1, 1) < 0){
         xml_rm(x0);
         goto done;
     }
     xml_rm(x0);
+    xml_rm(x1);
     encap = device_handle_framing_type_get(dh);
     if (netconf_output_encap(encap, cb) < 0)
         goto done;
     *cbret = cb;
     cb = NULL;
     retval = 0;
  done:
+    if (xvec)
+        free(xvec);
     if (reason)
         free(reason);
     if (cb)

diff --git a/src/controller_rpc.c b/src/controller_rpc.c
@@ -300,10 +300,6 @@ push_device_one(clixon_handle           h,
         cprintf(*cberr, "Device not configured");
         goto failed;
     }
-#if 0 // debug
-    fprintf(stderr, "%s before push x1 db:%s:\n", __FUNCTION__, db);
-    xml_creator_print(stderr, x1);
-#endif
     yspec = NULL;
     if (controller_mount_yspec_get(h, name, &yspec) < 0)
         goto done;

diff --git a/test/test-cli-order.sh b/test/test-cli-order.sh
@@ -58,25 +58,60 @@ expectpart "$($clixon_cli -1 -m configure -f $CFG set devices device openconfig1
 new "add 2.2.2.2"
 expectpart "$($clixon_cli -1  -m configure -f $CFG set devices device openconfig1 config system dns servers server 2.2.2.2 config address 2.2.2.2)" 0 ""
 
-new "commit"
+new "commit 1"
 expectpart "$($clixon_cli -1 -m configure -f $CFG commit)" 0 ""
 
 new "delete middle 3.3.3.3"
 expectpart "$($clixon_cli -1 -m configure -f $CFG delete devices device openconfig1 config system dns servers server 3.3.3.3)" 0 ""
 
+new "show compare xml"
 expectpart "$($clixon_cli -1 -m configure -f $CFG show compare xml)" 0 "\-\ *<address>3.3.3.3</address>" --not-- "+" "1.1.1.1" "2.2.2.2"
 
+new "show compare curly"
 expectpart "$($clixon_cli -1 -m configure -f $CFG show compare text)" 0 "\-\ *server 3.3.3.3" --not-- "+" "1.1.1.1" "2.2.2.2"
 
-new "commit"
+new "commit 2"
 expectpart "$($clixon_cli -1 -m configure -f $CFG commit)" 0 ""
 
+new "show config"
 expectpart "$($clixon_cli -1 -f $CFG show config)" 0 "<address>1.1.1.1</address>" "<address>2.2.2.2</address>" --not-- "<address>3.3.3.3</address>"
 
 new "delete first 1.1.1.1"
 expectpart "$($clixon_cli -1 -m configure -f $CFG delete devices device openconfig1 config system dns servers server 1.1.1.1)" 0 ""
 
-new "commit"
+new "commit 3"
 expectpart "$($clixon_cli -1 -m configure -f $CFG commit)" 0 ""
 
+# now do https://github.com/clicon/clixon/issues/496
+# Add 1.1.1.1/2.2.2.2 in that order
+new "delete 2.2.2.2"
+expectpart "$($clixon_cli -1 -m configure -f $CFG delete devices device openconfig1 config system dns servers server 2.2.2.2)" 0 ""
+
+new "add 1.1.1.1"
+expectpart "$($clixon_cli -1  -m configure -f $CFG set devices device openconfig1 config system dns servers server 1.1.1.1 config address 1.1.1.1)" 0 ""
+
+new "add 2.2.2.2"
+expectpart "$($clixon_cli -1  -m configure -f $CFG set devices device openconfig1 config system dns servers server 2.2.2.2 config address 2.2.2.2)" 0 ""
+
+# Shows only 1.1.1.1 as added, but 2.2.2.2 is reordered
+new "commit diff a"
+expectpart "$($clixon_cli -1  -m configure -f $CFG commit diff)" 0 "\+\ *<address>1.1.1.1" --not-- "2.2.2.2" "\-\ *"
+
+new "commit push xxx"
+echo "$clixon_cli -1  -m configure -f $CFG commit push"
+expectpart "$($clixon_cli -1  -m configure -f $CFG commit push)" 0 "^$"
+
+# Here device and controller have different orders
+new "set anything"
+expectpart "$($clixon_cli -1  -m configure -f $CFG set devices device openconfig1 config system config login-banner kalle)" 0 "^$"
+
+new "commit fail"
+echo "$clixon_cli -1  -m configure -f $CFG commit push"
+expectpart "$($clixon_cli -1  -m configure -f $CFG commit push 2>&1)" 0 "^OK$"
+
+if $BE; then
+    new "Kill old backend"
+    stop_backend -f $CFG
+fi
+
 endtest