From a9d1ab006c465092874b6f8c81a9b47072ebb24f Mon Sep 17 00:00:00 2001
From: Olof hagsand <olof@hagsand.se>
Date: Wed, 26 Oct 2022 20:18:57 +0200
Subject: [PATCH] Confirmed commit: lock check on running

---
 CHANGELOG.md                  |  2 --
 apps/backend/backend_client.c | 22 +++++++++++++++++++++-
 apps/backend/backend_commit.c | 30 ++++++++++++++----------------
 test/test_confirmed_commit.sh |  9 +++------
 test/test_netconf.sh          | 25 +++++++++++++++++++++++++
 5 files changed, 63 insertions(+), 25 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 748923d8..a899c7e2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -52,8 +52,6 @@ Expected: End of 2022
   * Added support for relevant arguments to CLI commit
     * See [example_cli.cli](https://github.com/clicon/clixon/blob/master/example/main/example_cli.cli)
   * See [Netconf Confirmed Commit Capability](https://github.com/clicon/clixon/issues/255)
-  * Known issues
-    * Lock check, see RFC 6241 7.5
 
 ### API changes on existing protocol/config features
 
diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c
index 4edc7915..1e4aba09 100644
--- a/apps/backend/backend_client.c
+++ b/apps/backend/backend_client.c
@@ -716,9 +716,15 @@ from_client_lock(clicon_handle h,
     struct client_entry *ce = (struct client_entry *)arg;
     uint32_t             id = ce->ce_id;
     uint32_t             iddb;
+    uint32_t             otherid;
     char                *db;
     cbuf                *cbx = NULL; /* Assist cbuf */
-    
+    yang_stmt           *yspec;
+
+    if ((yspec =  clicon_dbspec_yang(h)) == NULL){
+	clicon_err(OE_YANG, ENOENT, "No yang spec9");
+	goto done;
+    }
     if ((db = netconf_db_find(xe, "target")) == NULL){
 	if (netconf_missing_element(cbret, "protocol", "target", NULL) < 0)
 	    goto done;
@@ -754,6 +760,20 @@ from_client_lock(clicon_handle h,
 	    goto done;
 	goto ok;
     }
+    /* 3) The target configuration is <running>, and another NETCONF
+     *    session has an ongoing confirmed commi
+     */
+    if (strcmp(db, "running") == 0 &&
+	if_feature(yspec, "ietf-netconf", "confirmed-commit") &&
+	confirmed_commit_state_get(h) != INACTIVE){
+	if ((otherid = confirmed_commit_session_id_get(h)) != 0){
+	    cprintf(cbx, "<session-id>%u</session-id>", otherid);
+	    if (netconf_lock_denied(cbret, cbuf_get(cbx),
+				    "Operation failed, another session has an ongoing confirmed commit") < 0)
+		goto done;
+	    goto ok;
+	}
+    }
     if (xmldb_lock(h, db, id) < 0)
 	goto done;
      /* user callback */
diff --git a/apps/backend/backend_commit.c b/apps/backend/backend_commit.c
index f8337968..02edc316 100644
--- a/apps/backend/backend_commit.c
+++ b/apps/backend/backend_commit.c
@@ -761,6 +761,19 @@ candidate_commit(clicon_handle h,
  *  configuration datastore that are actually different and only check
  *  "create", "update", and "delete" access permissions for this set of
  *  nodes, which could be empty.
+ *
+ * Handling of the first phase of confirmed-commit:
+ * First, it must be determined if the given <commit> RPC constitutes a "confirming-commit", roughly meaning:
+ *   1) it was issued in the same session as a prior confirmed-commit
+ *   2) it bears a <persist-id> element matching the <persist> element that accompanied the prior confirmed-commit
+ *
+ *   If it is a valid "confirming-commit" and this RPC does not bear another <confirmed/> element, then the
+ *   confirmed-commit is complete, the rollback event can be cancelled and the rollback database deleted.
+ *
+ *   No further action is necessary as the candidate configuration was already copied to the running configuration.
+ *
+ *   If the RPC does bear another <confirmed/> element, that will be handled in phase two, from within the
+ *   candidate_commit() method.
  */
 int
 from_client_commit(clicon_handle h,
@@ -777,20 +790,6 @@ from_client_commit(clicon_handle h,
     int                  ret;
     yang_stmt           *yspec;
 
-    /* Handle the first phase of confirmed-commit
-     *
-     * First, it must be determined if the given <commit> RPC constitutes a "confirming-commit", roughly meaning:
-     *   1) it was issued in the same session as a prior confirmed-commit
-     *   2) it bears a <persist-id> element matching the <persist> element that accompanied the prior confirmed-commit
-     *
-     *   If it is a valid "confirming-commit" and this RPC does not bear another <confirmed/> element, then the
-     *   confirmed-commit is complete, the rollback event can be cancelled and the rollback database deleted.
-     *
-     *   No further action is necessary as the candidate configuration was already copied to the running configuration.
-     *
-     *   If the RPC does bear another <confirmed/> element, that will be handled in phase two, from within the
-     *   candidate_commit() method.
-     */
     if ((yspec = clicon_dbspec_yang(h)) == NULL) {
         clicon_err(OE_YANG, ENOENT, "No yang spec");
         goto done;
@@ -810,8 +809,7 @@ from_client_commit(clicon_handle h,
 	    clicon_err(OE_XML, errno, "cbuf_new");
 	    goto done;
 	}	
-	cprintf(cbx, "<session-id>%u</session-id>", iddb);
-	if (netconf_in_use(cbret, cbuf_get(cbx), "Operation failed, lock is already held") < 0)
+	if (netconf_in_use(cbret, "protocol", "Operation failed, lock is already held") < 0)
 	    goto done;
 	goto ok;
     }
diff --git a/test/test_confirmed_commit.sh b/test/test_confirmed_commit.sh
index 4abcb8be..1ddfe75f 100755
--- a/test/test_confirmed_commit.sh
+++ b/test/test_confirmed_commit.sh
@@ -312,7 +312,8 @@ PIDS=($(jobs -l % | cut -c 6- | awk '{print $1}'))
 assert_config_equals "running" "$CONFIGB"                       # assert config twice to prove it surives disconnect
 assert_config_equals "running" "$CONFIGB"                       # of ephemeral sessions
 
-kill -9 ${PIDS[0]}                                              # kill the while loop above to close STDIN on 1st
+new "soft kill ${PIDS[0]}"
+kill ${PIDS[0]}                   # kill the while loop above to close STDIN on 1st
 
 ################################################################################
 
@@ -433,15 +434,11 @@ expectpart "$(curl $CURLOPTS -X POST -H "Content-Type: application/yang-data+xml
 
 assert_config_equals "running" "$CONFIGBPLUSC"
 
-new "soft kill"
+new "soft kill ${PIDS[0]}"
 kill ${PIDS[0]}                   # kill the while loop above to close STDIN on 1st
 
 assert_config_equals "running" "$CONFIGBPLUSC"
 
-kill -9 ${PIDS[0]} 2> /dev/null   # kill the while loop above to close STDIN on 1st
-
-assert_config_equals "running" "$CONFIGBPLUSC"
-
 ################################################################################
 
 new "restconf persistid expect fail"
diff --git a/test/test_netconf.sh b/test/test_netconf.sh
index 7c2c2f10..8e7ed5f1 100755
--- a/test/test_netconf.sh
+++ b/test/test_netconf.sh
@@ -19,6 +19,7 @@ cat <<EOF > $cfg
 <clixon-config xmlns="http://clicon.org/config">
   <CLICON_CONFIGFILE>$cfg</CLICON_CONFIGFILE>
   <CLICON_FEATURE>ietf-netconf:startup</CLICON_FEATURE>
+  <CLICON_FEATURE>ietf-netconf:confirmed-commit</CLICON_FEATURE>
   <CLICON_MODULE_SET_ID>42</CLICON_MODULE_SET_ID>
   <CLICON_YANG_DIR>$dir</CLICON_YANG_DIR>
   <CLICON_YANG_DIR>${YANG_INSTALLDIR}</CLICON_YANG_DIR>
@@ -356,6 +357,30 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS>
 new "kill-session using prefix xx"
 expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<xx:rpc xmlns:xx=\"urn:ietf:params:xml:ns:netconf:base:1.0\" xx:message-id=\"42\"><xx:kill-session><xx:session-id>44</xx:session-id></xx:kill-session></xx:rpc>" "" "<rpc-reply xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\" xmlns:xx=\"urn:ietf:params:xml:ns:netconf:base:1.0\" xx:message-id=\"42\"><ok/></rpc-reply>"
 
+new "asynchronous lock running"
+sleep 60 |  cat <(echo "$HELLONO11<rpc $DEFAULTNS><lock><target><running/></target></lock></rpc>]]>]]>") -| $clixon_netconf -qf $cfg  >> /dev/null &
+
+PIDS=($(jobs -l % | cut -c 6- | awk '{print $1}'))
+
+new "try commit should fail"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><commit/></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>protocol</error-type><error-tag>in-use</error-tag><error-severity>error</error-severity><error-message>Operation failed, lock is already held</error-message></rpc-error></rpc-reply>"
+
+new "soft kill ${PIDS[0]}"
+kill ${PIDS[0]}                   # kill the while loop above to close STDIN on 1st
+
+new "asynchronous confirmed commit"
+sleep 60 |  cat <(echo "$HELLONO11<rpc $DEFAULTNS><commit><confirmed/><confirm-timeout>60</confirm-timeout></commit></rpc>]]>]]>") -| $clixon_netconf -qf $cfg  >> /dev/null &
+PIDS=($(jobs -l % | cut -c 6- | awk '{print $1}'))
+
+new "try lock should fail"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><lock><target><running/></target></lock></rpc>" "<rpc-reply $DEFAULTNS><rpc-error><error-type>protocol</error-type><error-tag>lock-denied</error-tag><error-info><session-id>[0-9]*</session-id></error-info><error-severity>error</error-severity><error-message>Operation failed, another session has an ongoing confirmed commit</error-message></rpc-error></rpc-reply>"
+
+new "soft kill ${PIDS[0]}"
+kill ${PIDS[0]}                   # kill the while loop above to close STDIN on 1st
+
+new "netconf discard-changes"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><discard-changes/></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
 # modify candidate, then lock, should fail.
 new "netconf edit config"
 expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><config><table xmlns=\"urn:example:clixon\"><parameter><name>a</name></parameter></table></config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"