Merge pull request #18601 from LabNConsulting/chopps/mgmtd-candidate-overwrite

mgmtd: remove bogus "hedge" code which corrupted active candidate DS

4 files changed, 23 insertions, 40 deletions

diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c
index a50ba015a3..3a85525d93 100644
--- a/mgmtd/mgmt_ds.c
+++ b/mgmtd/mgmt_ds.c
@@ -74,8 +74,7 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx,
 	return 0;
 }
 
-static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
-					   struct mgmt_ds_ctx *dst)
+static int ds_copy(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
 {
 	if (!src || !dst)
 		return -1;
@@ -95,8 +94,7 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
 	return 0;
 }
 
-static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src,
-					 struct mgmt_ds_ctx *dst)
+static int ds_merge(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
 {
 	int ret;
 
@@ -250,14 +248,13 @@ void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx)
 	ds_ctx->locked = 0;
 }
 
-int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx,
-		     struct mgmt_ds_ctx *dst_ds_ctx, bool updt_cmt_rec)
+int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool updt_cmt_rec)
 {
-	if (mgmt_ds_replace_dst_with_src_ds(src_ds_ctx, dst_ds_ctx) != 0)
+	if (ds_copy(dst, src) != 0)
 		return -1;
 
-	if (updt_cmt_rec && dst_ds_ctx->ds_id == MGMTD_DS_RUNNING)
-		mgmt_history_new_record(dst_ds_ctx);
+	if (updt_cmt_rec && dst->ds_id == MGMTD_DS_RUNNING)
+		mgmt_history_new_record(dst);
 
 	return 0;
 }
@@ -415,9 +412,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst,
 	parsed.ds_id = dst->ds_id;
 
 	if (merge)
-		mgmt_ds_merge_src_with_dst_ds(&parsed, dst);
+		ds_merge(dst, &parsed);
 	else
-		mgmt_ds_replace_dst_with_src_ds(&parsed, dst);
+		ds_copy(dst, &parsed);
 
 	nb_config_free(parsed.root.cfg_root);
 
diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h
index b8e77e330a..f7e1d7c5ee 100644
--- a/mgmtd/mgmt_ds.h
+++ b/mgmtd/mgmt_ds.h
@@ -196,21 +196,19 @@ extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx);
 /*
  * Copy from source to destination datastore.
  *
- * src_ds
- *    Source datastore handle (ds to be copied from).
- *
- * dst_ds
+ * dst
  *    Destination datastore handle (ds to be copied to).
  *
+ * src
+ *    Source datastore handle (ds to be copied from).
+ *
  * update_cmd_rec
  *    TRUE if need to update commit record, FALSE otherwise.
  *
  * Returns:
  *    0 on success, -1 on failure.
  */
-extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx,
-			    struct mgmt_ds_ctx *dst_ds_ctx,
-			    bool update_cmt_rec);
+extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool update_cmt_rec);
 
 /*
  * Fetch northbound configuration for a given datastore context.
diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c
index ff3165f369..39686091e1 100644
--- a/mgmtd/mgmt_fe_adapter.c
+++ b/mgmtd/mgmt_fe_adapter.c
@@ -217,12 +217,6 @@ static void
 mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session)
 {
 	/*
-	 * Ensure any uncommitted changes in Candidate DS
-	 * is discarded.
-	 */
-	mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false);
-
-	/*
 	 * Destroy the actual transaction created earlier.
 	 */
 	if (session->cfg_txn_id != MGMTD_TXN_ID_NONE)
diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c
index 9e06ce9f5f..96ca62c847 100644
--- a/mgmtd/mgmt_txn.c
+++ b/mgmtd/mgmt_txn.c
@@ -764,17 +764,15 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 			 !txn->commit_cfg_req->req.commit_cfg.rollback);
 
 		/*
-		 * Successful commit: Merge Src DS into Dst DS if and only if
+		 * Successful commit: Copy Src DS to Dst DS if and only if
 		 * this was not a validate-only or abort request.
 		 */
 		if ((txn->session_id &&
 		     !txn->commit_cfg_req->req.commit_cfg.validate_only &&
 		     !txn->commit_cfg_req->req.commit_cfg.abort) ||
 		    txn->commit_cfg_req->req.commit_cfg.rollback) {
-			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
-						 .src_ds_ctx,
-					 txn->commit_cfg_req->req.commit_cfg
-						 .dst_ds_ctx,
+			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx,
+					 txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
 					 create_cmt_info_rec);
 		}
 
@@ -783,22 +781,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
 		 * request.
 		 */
 		if (txn->session_id && txn->commit_cfg_req->req.commit_cfg.abort)
-			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
-						 .dst_ds_ctx,
-					 txn->commit_cfg_req->req.commit_cfg
-						 .src_ds_ctx,
-					 false);
+			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
+					 txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
 	} else {
 		/*
 		 * The commit has failied. For implicit commit requests restore
-		 * back the contents of the candidate DS.
+		 * back the contents of the candidate DS. For non-implicit
+		 * commit we want to allow the user to re-commit on the changes
+		 * (whether further modified or not).
 		 */
 		if (txn->commit_cfg_req->req.commit_cfg.implicit)
-			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
-						 .dst_ds_ctx,
-					 txn->commit_cfg_req->req.commit_cfg
-						 .src_ds_ctx,
-					 false);
+			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
+					 txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
 	}
 
 	if (txn->commit_cfg_req->req.commit_cfg.rollback) {