mgmt, lib: differentiate DELETE and REMOVE operations

Currently, there's a single operation type which doesn't return error
if the object doesn't exists. To be compatible with NETCONF/RESTCONF,
we should support differentiate between DELETE (fails when object
doesn't exist) and REMOVE (doesn't fail if the object doesn't exist).

Signed-off-by: Igor Ryzhov <[email protected]>

doc/developer/northbound/retrofitting-configuration-commands.rst

@@ -1023,7 +1023,7 @@ configuration. Here’s the declaration of this structure (taken from the
            char xpath[XPATH_MAXLEN];
 
            /*
-            * Operation to apply (either NB_OP_MODIFY or NB_OP_DELETE).
+            * Operation to apply (either NB_OP_MODIFY or NB_OP_DESTROY).
             */
            enum nb_operation operation;
 
@@ -1223,7 +1223,7 @@ This example shows how to create a list entry:
                    },
                    {
                            .xpath = "./access-list",
-                           .operation = acl ? NB_OP_MODIFY : NB_OP_DELETE,
+                           .operation = acl ? NB_OP_MODIFY : NB_OP_DESTROY,
                            .value = acl,
                    },
            };
@@ -1260,7 +1260,7 @@ When deleting a list entry, all non-key leaves can be ignored:
            struct cli_config_change changes[] = {
                    {
                            .xpath = ".",
-                           .operation = NB_OP_DELETE,
+                           .operation = NB_OP_DESTROY,
                    },
            };
 

lib/mgmt.proto

@@ -55,8 +55,9 @@ message YangData {
 enum CfgDataReqType {
   REQ_TYPE_NONE = 0;
   SET_DATA = 1;
-  DELETE_DATA = 2;
+  REMOVE_DATA = 2;
   CREATE_DATA = 3;
+  DELETE_DATA = 4;
 }
 
 message YangCfgDataReq {

lib/mgmt_be_client.c

@@ -556,11 +556,25 @@ static int mgmt_be_update_setcfg_in_batch(struct mgmt_be_client *client_ctx,
 	for (index = 0; index < num_req; index++) {
 		cfg_chg = &txn_req->req.set_cfg.cfg_changes[index];
 
-		if (cfg_req[index]->req_type
-		    == MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA)
+		/*
+		 * Treat all operations as destroy or modify, because we don't
+		 * need additional existence checks on the backend. Everything
+		 * is already checked by mgmtd.
+		 */
+		switch (cfg_req[index]->req_type) {
+		case MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA:
+		case MGMTD__CFG_DATA_REQ_TYPE__REMOVE_DATA:
 			cfg_chg->operation = NB_OP_DESTROY;
-		else
+			break;
+		case MGMTD__CFG_DATA_REQ_TYPE__SET_DATA:
+		case MGMTD__CFG_DATA_REQ_TYPE__CREATE_DATA:
 			cfg_chg->operation = NB_OP_MODIFY;
+			break;
+		case MGMTD__CFG_DATA_REQ_TYPE__REQ_TYPE_NONE:
+		case _MGMTD__CFG_DATA_REQ_TYPE_IS_INT_SIZE:
+		default:
+			continue;
+		}
 
 		strlcpy(cfg_chg->xpath, cfg_req[index]->data->xpath,
 			sizeof(cfg_chg->xpath));

lib/northbound.c

@@ -719,13 +719,14 @@ int nb_candidate_edit(struct nb_config *candidate,
 		}
 		break;
 	case NB_OP_DESTROY:
+	case NB_OP_DELETE:
 		dnode = yang_dnode_get(candidate->dnode, xpath_edit);
-		if (!dnode)
-			/*
-			 * Return a special error code so the caller can choose
-			 * whether to ignore it or not.
-			 */
-			return NB_ERR_NOT_FOUND;
+		if (!dnode) {
+			if (operation == NB_OP_DELETE)
+				return NB_ERR;
+			else
+				return NB_OK;
+		}
 		/* destroy dependant */
 		if (nb_node->dep_cbs.get_dependant_xpath) {
 			nb_node->dep_cbs.get_dependant_xpath(dnode, dep_xpath);
@@ -761,6 +762,7 @@ static void nb_update_candidate_changes(struct nb_config *candidate,
 		root = yang_dnode_get(candidate->dnode, xpath);
 		break;
 	case NB_OP_DESTROY:
+	case NB_OP_DELETE:
 		root = yang_dnode_get(running_config->dnode, xpath);
 		/* code */
 		break;
@@ -805,6 +807,8 @@ const char *nb_operation_name(enum nb_operation operation)
 		return "modify";
 	case NB_OP_DESTROY:
 		return "destroy";
+	case NB_OP_DELETE:
+		return "delete";
 	case NB_OP_MOVE:
 		return "move";
 	}
@@ -815,7 +819,7 @@ const char *nb_operation_name(enum nb_operation operation)
 bool nb_is_operation_allowed(struct nb_node *nb_node, enum nb_operation oper)
 {
 	if (lysc_is_key(nb_node->snode)) {
-		if (oper == NB_OP_MODIFY || oper == NB_OP_DESTROY)
+		if (oper == NB_OP_DESTROY || oper == NB_OP_DELETE)
 			return false;
 	}
 	return true;
@@ -887,7 +891,7 @@ void nb_candidate_edit_config_changes(
 		ret = nb_candidate_edit(candidate_config, nb_node,
 					change->operation, xpath, NULL, data);
 		yang_data_free(data);
-		if (ret != NB_OK && ret != NB_ERR_NOT_FOUND) {
+		if (ret != NB_OK) {
 			flog_warn(
 				EC_LIB_NB_CANDIDATE_EDIT_ERROR,
 				"%s: failed to edit candidate configuration: operation [%s] xpath [%s]",

lib/northbound.h

@@ -705,6 +705,7 @@ enum nb_operation {
 	NB_OP_CREATE,
 	NB_OP_MODIFY,
 	NB_OP_DESTROY,
+	NB_OP_DELETE,
 	NB_OP_MOVE,
 };
 
@@ -900,7 +901,6 @@ extern bool nb_is_operation_allowed(struct nb_node *nb_node,
  *
  * Returns:
  *    - NB_OK on success.
- *    - NB_ERR_NOT_FOUND when the element to be deleted was not found.
  *    - NB_ERR for other errors.
  */
 extern int nb_candidate_edit(struct nb_config *candidate,

lib/northbound_cli.h

@@ -32,7 +32,7 @@ extern struct nb_config *vty_shared_candidate_config;
  *    XPath (absolute or relative) of the configuration option being edited.
  *
  * operation
- *    Operation to apply (either NB_OP_MODIFY or NB_OP_DELETE).
+ *    Operation to apply (either NB_OP_MODIFY or NB_OP_DESTROY).
  *
  * value
  *    New value of the configuration option. Should be NULL for typeless YANG

lib/northbound_sysrepo.c

@@ -213,7 +213,7 @@ static int frr_sr_process_change(struct nb_config *candidate,
 
 	ret = nb_candidate_edit(candidate, nb_node, nb_op, xpath, NULL, data);
 	yang_data_free(data);
-	if (ret != NB_OK && ret != NB_ERR_NOT_FOUND) {
+	if (ret != NB_OK) {
 		flog_warn(
 			EC_LIB_NB_CANDIDATE_EDIT_ERROR,
 			"%s: failed to edit candidate configuration: operation [%s] xpath [%s]",

lib/vty.c

@@ -3773,11 +3773,16 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit)
 		mgmt_yang_cfg_data_req_init(&cfg_req[indx]);
 		cfg_req[indx].data = &cfg_data[indx];
 		switch (vty->cfg_changes[indx].operation) {
-		case NB_OP_DESTROY:
+		case NB_OP_DELETE:
 			cfg_req[indx].req_type =
 				MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA;
 			break;
 
+		case NB_OP_DESTROY:
+			cfg_req[indx].req_type =
+				MGMTD__CFG_DATA_REQ_TYPE__REMOVE_DATA;
+			break;
+
 		case NB_OP_CREATE:
 			cfg_req[indx].req_type =
 				MGMTD__CFG_DATA_REQ_TYPE__CREATE_DATA;

mgmtd/mgmt_txn.c

@@ -1074,7 +1074,7 @@ static int mgmt_txn_create_config_batches(struct mgmt_txn_req *txn_req,
 			 */
 			if (chg->cb.operation == NB_CB_DESTROY)
 				batch->cfg_data[batch->num_cfg_data].req_type =
-					MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA;
+					MGMTD__CFG_DATA_REQ_TYPE__REMOVE_DATA;
 			else
 				batch->cfg_data[batch->num_cfg_data].req_type =
 					MGMTD__CFG_DATA_REQ_TYPE__SET_DATA;
@@ -2221,6 +2221,9 @@ int mgmt_txn_send_set_config_req(uint64_t txn_id, uint64_t req_id,
 
 		switch (cfg_req[indx]->req_type) {
 		case MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA:
+			cfg_chg->operation = NB_OP_DELETE;
+			break;
+		case MGMTD__CFG_DATA_REQ_TYPE__REMOVE_DATA:
 			cfg_chg->operation = NB_OP_DESTROY;
 			break;
 		case MGMTD__CFG_DATA_REQ_TYPE__SET_DATA:

mgmtd/mgmt_vty.c

@@ -185,6 +185,23 @@ DEFPY(mgmt_delete_config_data, mgmt_delete_config_data_cmd,
       "XPath expression specifying the YANG data path\n")
 {
 
+	strlcpy(vty->cfg_changes[0].xpath, path,
+		sizeof(vty->cfg_changes[0].xpath));
+	vty->cfg_changes[0].value = NULL;
+	vty->cfg_changes[0].operation = NB_OP_DELETE;
+	vty->num_cfg_changes = 1;
+
+	vty_mgmt_send_config_data(vty, false);
+	return CMD_SUCCESS;
+}
+
+DEFPY(mgmt_remove_config_data, mgmt_remove_config_data_cmd,
+      "mgmt remove-config WORD$path",
+      MGMTD_STR
+      "Remove configuration data\n"
+      "XPath expression specifying the YANG data path\n")
+{
+
 	strlcpy(vty->cfg_changes[0].xpath, path,
 		sizeof(vty->cfg_changes[0].xpath));
 	vty->cfg_changes[0].value = NULL;
@@ -505,6 +522,7 @@ void mgmt_vty_init(void)
 	install_element(CONFIG_NODE, &mgmt_create_config_data_cmd);
 	install_element(CONFIG_NODE, &mgmt_set_config_data_cmd);
 	install_element(CONFIG_NODE, &mgmt_delete_config_data_cmd);
+	install_element(CONFIG_NODE, &mgmt_remove_config_data_cmd);
 	install_element(CONFIG_NODE, &mgmt_load_config_cmd);
 	install_element(CONFIG_NODE, &mgmt_save_config_cmd);
 	install_element(CONFIG_NODE, &mgmt_rollback_cmd);