From 3c2598a26ff01fc712d61233a6a627fdfa1a7b77 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Fri, 6 Oct 2023 15:01:16 +0300 Subject: [PATCH] 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 --- .../retrofitting-configuration-commands.rst | 6 +++--- lib/mgmt.proto | 3 ++- lib/mgmt_be_client.c | 20 ++++++++++++++++--- lib/northbound.c | 20 +++++++++++-------- lib/northbound.h | 2 +- lib/northbound_cli.h | 2 +- lib/northbound_sysrepo.c | 2 +- lib/vty.c | 7 ++++++- mgmtd/mgmt_txn.c | 5 ++++- mgmtd/mgmt_vty.c | 18 +++++++++++++++++ 10 files changed, 65 insertions(+), 20 deletions(-) diff --git a/doc/developer/northbound/retrofitting-configuration-commands.rst b/doc/developer/northbound/retrofitting-configuration-commands.rst index b407246049..41f9902b6a 100644 --- a/doc/developer/northbound/retrofitting-configuration-commands.rst +++ b/doc/developer/northbound/retrofitting-configuration-commands.rst @@ -1005,7 +1005,7 @@ configuration. Here’s the declaration of this structure (taken from the /* * Operation to apply (either NB_OP_CREATE, NB_OP_MODIFY or - * NB_OP_DELETE). + * NB_OP_DESTROY). */ enum nb_operation operation; @@ -1205,7 +1205,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, }, }; @@ -1242,7 +1242,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, }, }; diff --git a/lib/mgmt.proto b/lib/mgmt.proto index 9a39789c82..e0b6019d2b 100644 --- a/lib/mgmt.proto +++ b/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 { diff --git a/lib/mgmt_be_client.c b/lib/mgmt_be_client.c index dd2613f132..53df191b2c 100644 --- a/lib/mgmt_be_client.c +++ b/lib/mgmt_be_client.c @@ -565,11 +565,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)); diff --git a/lib/northbound.c b/lib/northbound.c index f7e0d698b3..01bb091859 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -731,13 +731,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); @@ -767,6 +768,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"; } @@ -777,7 +780,8 @@ 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_MODIFY || oper == NB_OP_DESTROY + || oper == NB_OP_DELETE) return false; } return true; @@ -842,7 +846,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]", diff --git a/lib/northbound.h b/lib/northbound.h index e11958c77d..ef774fb476 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -722,6 +722,7 @@ enum nb_operation { NB_OP_CREATE, NB_OP_MODIFY, NB_OP_DESTROY, + NB_OP_DELETE, NB_OP_MOVE, }; @@ -953,7 +954,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, diff --git a/lib/northbound_cli.h b/lib/northbound_cli.h index c8f8a8481a..1a45794d45 100644 --- a/lib/northbound_cli.h +++ b/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_CREATE, NB_OP_MODIFY or NB_OP_DELETE). + * Operation to apply (either NB_OP_CREATE, NB_OP_MODIFY or NB_OP_DESTROY). * * value * New value of the configuration option. Should be NULL for typeless YANG diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index 1a194935ac..198d96e381 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -221,7 +221,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]", diff --git a/lib/vty.c b/lib/vty.c index 69df0a0245..7e856e3741 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -3996,11 +3996,16 @@ int vty_mgmt_send_config_data(struct vty *vty, const char *xpath_base, 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_EXCL: cfg_req[indx].req_type = MGMTD__CFG_DATA_REQ_TYPE__CREATE_DATA; diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index abae2a6a53..dee75e6586 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -918,7 +918,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; @@ -2052,6 +2052,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: diff --git a/mgmtd/mgmt_vty.c b/mgmtd/mgmt_vty.c index aa46d5c783..b12f54ccbe 100644 --- a/mgmtd/mgmt_vty.c +++ b/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, NULL, 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; @@ -547,6 +564,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); -- 2.39.5