Merge pull request #13766 from LabNConsulting/chopps/mgmtd-cleanup

mgmtd cleanup/simplify some code

lib/northbound_cli.c

@@ -202,7 +202,7 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 			return CMD_SUCCESS;
 
 		implicit_commit = vty_needs_implicit_commit(vty);
-		ret = vty_mgmt_send_config_data(vty);
+		ret = vty_mgmt_send_config_data(vty, implicit_commit);
 		if (ret >= 0 && !implicit_commit)
 			vty->mgmt_num_pending_setcfg++;
 		return ret;
@@ -229,9 +229,16 @@ int nb_cli_apply_changes_clear_pending(struct vty *vty,
 
 	if (vty_mgmt_should_process_cli_apply_changes(vty)) {
 		VTY_CHECK_XPATH;
-
+		/*
+		 * The legacy user wanted to clear pending (i.e., perform a
+		 * commit immediately) due to some non-yang compatible
+		 * functionality. This new mgmtd code however, continues to send
+		 * changes putting off the commit until XFRR_end is received
+		 * (i.e., end-of-config-file). This should be fine b/c all
+		 * conversions to mgmtd require full proper implementations.
+		 */
 		implicit_commit = vty_needs_implicit_commit(vty);
-		ret = vty_mgmt_send_config_data(vty);
+		ret = vty_mgmt_send_config_data(vty, implicit_commit);
 		if (ret >= 0 && !implicit_commit)
 			vty->mgmt_num_pending_setcfg++;
 		return ret;

lib/vty.c

@@ -3652,7 +3652,7 @@ int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id,
 	return 0;
 }
 
-int vty_mgmt_send_config_data(struct vty *vty)
+int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 {
 	Mgmtd__YangDataValue value[VTY_MAXCFGCHANGES];
 	Mgmtd__YangData cfg_data[VTY_MAXCFGCHANGES];
@@ -3660,7 +3660,6 @@ int vty_mgmt_send_config_data(struct vty *vty)
 	Mgmtd__YangCfgDataReq *cfgreq[VTY_MAXCFGCHANGES] = {0};
 	size_t indx;
 	int cnt;
-	bool implicit_commit = false;
 
 	if (vty->type == VTY_FILE) {
 		/*
@@ -3734,7 +3733,6 @@ int vty_mgmt_send_config_data(struct vty *vty)
 		}
 
 		vty->mgmt_req_id++;
-		implicit_commit = vty_needs_implicit_commit(vty);
 		if (cnt && mgmt_fe_send_setcfg_req(
 				   mgmt_fe_client, vty->mgmt_session_id,
 				   vty->mgmt_req_id, MGMTD_DS_CANDIDATE, cfgreq,

lib/vty.h

@@ -147,7 +147,6 @@ struct vty {
 	/* Dynamic transaction information. */
 	bool pending_allowed;
 	bool pending_commit;
-	bool no_implicit_commit;
 	char *pending_cmds_buf;
 	size_t pending_cmds_buflen;
 	size_t pending_cmds_bufpos;
@@ -408,7 +407,7 @@ extern bool vty_mgmt_fe_enabled(void);
 extern bool vty_mgmt_should_process_cli_apply_changes(struct vty *vty);
 
 extern bool mgmt_vty_read_configs(void);
-extern int vty_mgmt_send_config_data(struct vty *vty);
+extern int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit);
 extern int vty_mgmt_send_commit_config(struct vty *vty, bool validate_only,
 				       bool abort);
 extern int vty_mgmt_send_get_config(struct vty *vty,
@@ -422,11 +421,7 @@ extern void vty_mgmt_resume_response(struct vty *vty, bool success);
 
 static inline bool vty_needs_implicit_commit(struct vty *vty)
 {
-	return (frr_get_cli_mode() == FRR_CLI_CLASSIC
+	return frr_get_cli_mode() == FRR_CLI_CLASSIC && !vty->pending_allowed;
-			? ((vty->pending_allowed || vty->no_implicit_commit)
-				   ? false
-				   : true)
-			: false);
 }
 
 #ifdef __cplusplus

mgmtd/mgmt_txn.c

@@ -494,6 +494,8 @@ static void mgmt_txn_req_free(struct mgmt_txn_req **txn_req)
 	struct mgmt_txn_reqs_head *pending_list = NULL;
 	enum mgmt_be_client_id id;
 	struct mgmt_be_client_adapter *adapter;
+	struct mgmt_commit_cfg_req *ccreq;
+	bool cleanup;
 
 	switch ((*txn_req)->req_event) {
 	case MGMTD_TXN_PROC_SETCFG:
@@ -526,32 +528,38 @@ static void mgmt_txn_req_free(struct mgmt_txn_req **txn_req)
 		MGMTD_TXN_DBG("Deleting COMMITCFG req-id: %" PRIu64
 			      " txn-id: %" PRIu64,
 			      (*txn_req)->req_id, (*txn_req)->txn->txn_id);
+
+		ccreq = &(*txn_req)->req.commit_cfg;
+		cleanup = (ccreq->curr_phase >= MGMTD_COMMIT_PHASE_TXN_CREATE &&
+			   ccreq->curr_phase < MGMTD_COMMIT_PHASE_TXN_DELETE);
+
 		FOREACH_MGMTD_BE_CLIENT_ID (id) {
 			/*
 			 * Send TXN_DELETE to cleanup state for this
 			 * transaction on backend
 			 */
-			if ((*txn_req)->req.commit_cfg.curr_phase >=
+
-				    MGMTD_COMMIT_PHASE_TXN_CREATE &&
+			/*
-			    (*txn_req)->req.commit_cfg.curr_phase <
+			 * Get rid of the batches first so we don't end up doing
-				    MGMTD_COMMIT_PHASE_TXN_DELETE &&
+			 * anything more with them
-			    (*txn_req)
+			 */
-				    ->req.commit_cfg.subscr_info
+			mgmt_txn_cleanup_be_cfg_batches((*txn_req)->txn, id);
-				    .xpath_subscr[id]) {
+			if (ccreq->batches) {
-				adapter = mgmt_be_get_adapter_by_id(id);
+				hash_clean(ccreq->batches,
-				if (adapter)
+					   mgmt_txn_cfgbatch_hash_free);
-					mgmt_txn_send_be_txn_delete(
+				hash_free(ccreq->batches);
-						(*txn_req)->txn, adapter);
+				ccreq->batches = NULL;
 			}
 
-			mgmt_txn_cleanup_be_cfg_batches((*txn_req)->txn,
+			/*
-							    id);
+			 * If we were in the middle of the state machine then
-			if ((*txn_req)->req.commit_cfg.batches) {
+			 * send a txn delete message
-				hash_clean((*txn_req)->req.commit_cfg.batches,
+			 */
-					   mgmt_txn_cfgbatch_hash_free);
+			adapter = mgmt_be_get_adapter_by_id(id);
-				hash_free((*txn_req)->req.commit_cfg.batches);
+			if (adapter && cleanup &&
-				(*txn_req)->req.commit_cfg.batches = NULL;
+			    ccreq->subscr_info.xpath_subscr[id])
-			}
+				mgmt_txn_send_be_txn_delete((*txn_req)->txn,
+							    adapter);
 		}
 		break;
 	case MGMTD_TXN_PROC_GETCFG:
@@ -1424,24 +1432,16 @@ static int
 mgmt_txn_send_be_txn_delete(struct mgmt_txn_ctx *txn,
 				 struct mgmt_be_client_adapter *adapter)
 {
-	struct mgmt_commit_cfg_req *cmtcfg_req;
+	struct mgmt_commit_cfg_req *cmtcfg_req =
-	struct mgmt_txn_be_cfg_batch *cfg_btch;
+		&txn->commit_cfg_req->req.commit_cfg;
 
-	assert(txn->type == MGMTD_TXN_TYPE_CONFIG && txn->commit_cfg_req);
+	assert(txn->type == MGMTD_TXN_TYPE_CONFIG);
+	assert(!mgmt_txn_batches_count(&cmtcfg_req->curr_batches[adapter->id]));
 
-	cmtcfg_req = &txn->commit_cfg_req->req.commit_cfg;
+	if (!cmtcfg_req->subscr_info.xpath_subscr[adapter->id])
-	if (cmtcfg_req->subscr_info.xpath_subscr[adapter->id]) {
+		return 0;
-		adapter = mgmt_be_get_adapter_by_id(adapter->id);
-		(void)mgmt_be_send_txn_req(adapter, txn->txn_id, false);
 
-		FOREACH_TXN_CFG_BATCH_IN_LIST (
+	return mgmt_be_send_txn_req(adapter, txn->txn_id, false);
-			&txn->commit_cfg_req->req.commit_cfg
-				 .curr_batches[adapter->id],
-			cfg_btch)
-			cfg_btch->comm_phase = MGMTD_COMMIT_PHASE_TXN_DELETE;
-	}
-
-	return 0;
 }
 
 static void mgmt_txn_cfg_commit_timedout(struct event *thread)

mgmtd/mgmt_vty.c

@@ -157,9 +157,7 @@ DEFPY(mgmt_set_config_data, mgmt_set_config_data_cmd,
 	vty->cfg_changes[0].operation = NB_OP_CREATE;
 	vty->num_cfg_changes = 1;
 
-	vty->no_implicit_commit = true;
+	vty_mgmt_send_config_data(vty, false);
-	vty_mgmt_send_config_data(vty);
-	vty->no_implicit_commit = false;
 	return CMD_SUCCESS;
 }
 
@@ -176,9 +174,7 @@ DEFPY(mgmt_delete_config_data, mgmt_delete_config_data_cmd,
 	vty->cfg_changes[0].operation = NB_OP_DESTROY;
 	vty->num_cfg_changes = 1;
 
-	vty->no_implicit_commit = true;
+	vty_mgmt_send_config_data(vty, false);
-	vty_mgmt_send_config_data(vty);
-	vty->no_implicit_commit = false;
 	return CMD_SUCCESS;
 }