From 0913d9fc0ea4975eb0dd37f5ca84af2264833687 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Sat, 27 Jul 2024 01:07:57 -0400 Subject: [PATCH 1/4] lib: constify yang_resolve_snode_xpath results Signed-off-by: Christian Hopps ang --- lib/northbound.c | 2 +- lib/yang.c | 2 +- lib/yang.h | 3 ++- mgmtd/mgmt_fe_adapter.c | 4 +--- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index 0bc79d0277..63062ee336 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -178,7 +178,7 @@ struct nb_node *nb_node_find(const char *path) struct nb_node **nb_nodes_find(const char *xpath) { - struct lysc_node **snodes = NULL; + const struct lysc_node **snodes = NULL; struct nb_node **nb_nodes = NULL; bool simple; LY_ERR err; diff --git a/lib/yang.c b/lib/yang.c index 6c1aed00cc..14d5b118c6 100644 --- a/lib/yang.c +++ b/lib/yang.c @@ -286,7 +286,7 @@ void yang_snode_get_path(const struct lysc_node *snode, } LY_ERR yang_resolve_snode_xpath(struct ly_ctx *ly_ctx, const char *xpath, - struct lysc_node ***snodes, bool *simple) + const struct lysc_node ***snodes, bool *simple) { struct lysc_node *snode; struct ly_set *set; diff --git a/lib/yang.h b/lib/yang.h index 57131f478b..c4fc78b8ae 100644 --- a/lib/yang.h +++ b/lib/yang.h @@ -827,7 +827,8 @@ extern int yang_xpath_pop_node(char *xpath); * Return: a libyang error or LY_SUCCESS. */ extern LY_ERR yang_resolve_snode_xpath(struct ly_ctx *ly_ctx, const char *xpath, - struct lysc_node ***snodes, bool *simple); + const struct lysc_node ***snodes, + bool *simple); /* * Libyang future functions diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 09d1910cee..fc35e74610 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -1282,8 +1282,7 @@ static void fe_adapter_handle_get_data(struct mgmt_fe_session_ctx *session, void *__msg, size_t msg_len) { struct mgmt_msg_get_data *msg = __msg; - struct lysc_node **snodes = NULL; - char *xpath_resolved = NULL; + const struct lysc_node **snodes = NULL; uint64_t req_id = msg->req_id; Mgmtd__DatastoreId ds_id; uint64_t clients; @@ -1395,7 +1394,6 @@ static void fe_adapter_handle_get_data(struct mgmt_fe_session_ctx *session, } done: darr_free(snodes); - darr_free(xpath_resolved); } static void fe_adapter_handle_edit(struct mgmt_fe_session_ctx *session, From d57a6f761e56a44b4ac8b6b47fa54592127c1bb9 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Sat, 14 Sep 2024 06:52:43 -0400 Subject: [PATCH 2/4] mgmtd: allow dest DS "running" if implicit lock+commit Signed-off-by: Christian Hopps --- mgmtd/mgmt_fe_adapter.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index fc35e74610..8ab66de687 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -1406,7 +1406,12 @@ static void fe_adapter_handle_edit(struct mgmt_fe_session_ctx *session, bool lock, commit; int ret; - if (msg->datastore != MGMT_MSG_DATASTORE_CANDIDATE) { + lock = CHECK_FLAG(msg->flags, EDIT_FLAG_IMPLICIT_LOCK); + commit = CHECK_FLAG(msg->flags, EDIT_FLAG_IMPLICIT_COMMIT); + + if (lock && commit && msg->datastore == MGMT_MSG_DATASTORE_RUNNING) + ; + else if (msg->datastore != MGMT_MSG_DATASTORE_CANDIDATE) { fe_adapter_send_error(session, msg->req_id, false, -EINVAL, "Unsupported datastore"); return; @@ -1427,9 +1432,6 @@ static void fe_adapter_handle_edit(struct mgmt_fe_session_ctx *session, rds_ctx = mgmt_ds_get_ctx_by_id(mm, rds_id); assert(rds_ctx); - lock = CHECK_FLAG(msg->flags, EDIT_FLAG_IMPLICIT_LOCK); - commit = CHECK_FLAG(msg->flags, EDIT_FLAG_IMPLICIT_COMMIT); - if (lock) { if (mgmt_fe_session_write_lock_ds(ds_id, ds_ctx, session)) { fe_adapter_send_error(session, msg->req_id, false, From 96db155acde43be7cd02d727285ed96e792e0454 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Tue, 17 Sep 2024 02:27:03 -0400 Subject: [PATCH 3/4] lib: mgmtd: cleanup error value for native messaging - Now if positive it's libyang LY_ERR, otherwise it's `-errno` value. Signed-off-by: Christian Hopps --- lib/northbound.c | 6 ++++-- lib/northbound.h | 1 + mgmtd/mgmt_fe_adapter.h | 4 ++-- mgmtd/mgmt_txn.c | 10 ++++++---- mgmtd/mgmt_txn.h | 28 ++++++++++++++++++++++++++++ 5 files changed, 41 insertions(+), 8 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index 63062ee336..35d0596ed4 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -908,7 +908,7 @@ static int nb_candidate_edit_tree_add(struct nb_config *candidate, if (operation == NB_OP_CREATE_EXCL) { snprintf(errmsg, errmsg_len, "Data already exists"); - ret = NB_ERR; + ret = NB_ERR_EXISTS; goto done; } @@ -995,7 +995,7 @@ static int nb_candidate_edit_tree_del(struct nb_config *candidate, if (!dnode || (dnode->flags & LYD_DEFAULT)) { if (operation == NB_OP_DELETE) { snprintf(errmsg, errmsg_len, "Data missing"); - return NB_ERR; + return NB_ERR_NOT_FOUND; } else return NB_OK; } @@ -2605,6 +2605,8 @@ const char *nb_err_name(enum nb_error error) return "no changes"; case NB_ERR_NOT_FOUND: return "element not found"; + case NB_ERR_EXISTS: + return "element already exists"; case NB_ERR_LOCKED: return "resource is locked"; case NB_ERR_VALIDATION: diff --git a/lib/northbound.h b/lib/northbound.h index da5f5be1ee..b311affa31 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -678,6 +678,7 @@ enum nb_error { NB_ERR, NB_ERR_NO_CHANGES, NB_ERR_NOT_FOUND, + NB_ERR_EXISTS, NB_ERR_LOCKED, NB_ERR_VALIDATION, NB_ERR_RESOURCE, diff --git a/mgmtd/mgmt_fe_adapter.h b/mgmtd/mgmt_fe_adapter.h index 61d6cfae13..5a7dec3e6f 100644 --- a/mgmtd/mgmt_fe_adapter.h +++ b/mgmtd/mgmt_fe_adapter.h @@ -194,7 +194,7 @@ extern int mgmt_fe_adapter_send_rpc_reply(uint64_t session_id, uint64_t txn_id, * unlock: implicit-lock flag was set in the request * commit: implicit-commit flag was set in the request * xpath: the xpath of the data node that was created - * error: the error code, zero for successful request + * error: >0 LY_ERR, < 0 -errno * errstr: the error string, if error is non-zero */ extern int mgmt_fe_adapter_send_edit_reply(uint64_t session_id, uint64_t txn_id, @@ -210,7 +210,7 @@ extern int mgmt_fe_adapter_send_edit_reply(uint64_t session_id, uint64_t txn_id, * Args: * txn_id: the txn_id this error pertains to. * short_circuit_ok: True if OK to short-circuit the call. - * error: An integer error value. + * error: >0 LY_ERR, < 0 -errno * errfmt: An error format string (i.e., printfrr) * ...: args for use by the `errfmt` format string. * diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index ed133243a1..53d9f5c3fa 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -1335,7 +1335,8 @@ static int txn_get_tree_data_done(struct mgmt_txn_ctx *txn, " req_id %" PRIu64 " to requested type %u", txn->txn_id, req_id, get_tree->result_type); - (void)mgmt_fe_adapter_txn_error(txn->txn_id, req_id, false, ret, + (void)mgmt_fe_adapter_txn_error(txn->txn_id, req_id, false, + errno_from_nb_error(ret), "Error converting results of GETTREE"); } @@ -1351,7 +1352,7 @@ static int txn_rpc_done(struct mgmt_txn_ctx *txn, struct mgmt_txn_req *txn_req) EVENT_OFF(txn->rpc_timeout); if (rpc->errstr) - mgmt_fe_adapter_txn_error(txn->txn_id, req_id, false, -1, + mgmt_fe_adapter_txn_error(txn->txn_id, req_id, false, -EINVAL, rpc->errstr); else if (mgmt_fe_adapter_send_rpc_reply(txn->session_id, txn->txn_id, req_id, rpc->result_type, @@ -1360,7 +1361,8 @@ static int txn_rpc_done(struct mgmt_txn_ctx *txn, struct mgmt_txn_req *txn_req) " req_id %" PRIu64 " to requested type %u", txn->txn_id, req_id, rpc->result_type); - (void)mgmt_fe_adapter_txn_error(txn->txn_id, req_id, false, -1, + (void)mgmt_fe_adapter_txn_error(txn->txn_id, req_id, false, + -EINVAL, "Error converting results of RPC"); } @@ -2580,7 +2582,7 @@ int mgmt_txn_send_edit(uint64_t txn_id, uint64_t req_id, reply: mgmt_fe_adapter_send_edit_reply(txn->session_id, txn->txn_id, req_id, unlock, commit, edit->xpath_created, - ret ? -1 : 0, errstr); + errno_from_nb_error(ret), errstr); XFREE(MTYPE_MGMTD_TXN_REQ, edit); diff --git a/mgmtd/mgmt_txn.h b/mgmtd/mgmt_txn.h index b6ca288675..37dadc0171 100644 --- a/mgmtd/mgmt_txn.h +++ b/mgmtd/mgmt_txn.h @@ -69,6 +69,34 @@ static inline const char *mgmt_txn_type2str(enum mgmt_txn_type type) return "Unknown"; } + +static inline int16_t errno_from_nb_error(enum nb_error ret) +{ + switch (ret) { + case NB_OK: + return 0; + case NB_ERR_NO_CHANGES: + return -EALREADY; + case NB_ERR_NOT_FOUND: + return -ENOENT; + case NB_ERR_EXISTS: + return -EEXIST; + case NB_ERR_LOCKED: + return -EWOULDBLOCK; + case NB_ERR_VALIDATION: + return -EINVAL; + case NB_ERR_RESOURCE: + return -ENOMEM; + case NB_ERR: + case NB_ERR_INCONSISTENCY: + return -EINVAL; + case NB_YIELD: + default: + return -EINVAL; + } +} + + /* Initialise transaction module. */ extern int mgmt_txn_init(struct mgmt_master *cm, struct event_loop *tm); From b097a966cbe73d7f77fefc4b3c4baf16ce2be214 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Tue, 17 Sep 2024 02:27:31 -0400 Subject: [PATCH 4/4] lib: mgmtd: add `changed` and `created` to edit-reply msg - This is used for various return values in RESTCONF Signed-off-by: Christian Hopps --- lib/mgmt_msg_native.h | 11 +++++++++-- lib/northbound.c | 28 ++++++++++++++++++++-------- lib/northbound.h | 9 ++++++--- mgmtd/mgmt_fe_adapter.c | 24 ++++++++++++++++-------- mgmtd/mgmt_fe_adapter.h | 8 +++++--- mgmtd/mgmt_txn.c | 10 +++++++--- 6 files changed, 63 insertions(+), 27 deletions(-) diff --git a/lib/mgmt_msg_native.h b/lib/mgmt_msg_native.h index 76a52658cd..ef03b66edc 100644 --- a/lib/mgmt_msg_native.h +++ b/lib/mgmt_msg_native.h @@ -383,11 +383,18 @@ _Static_assert(sizeof(struct mgmt_msg_edit) == /** * struct mgmt_msg_edit_reply - frontend edit reply. * - * @data: the xpath of the data node that was created. + * @changed: If true then changes in datastore resulted. + * @created: If true then object was newly created (non-existing before) + * @data: @vsplit values, second value may be zero len. + * @data: [0] the xpath of the data node that was created. + * @data: [1] Possible structured data to pass back to client (e.g., non-"error" + * yang modeled error data). */ struct mgmt_msg_edit_reply { struct mgmt_msg_header; - uint8_t resv2[8]; + uint8_t changed; + uint8_t created; + uint8_t resv2[6]; alignas(8) char data[]; }; diff --git a/lib/northbound.c b/lib/northbound.c index 35d0596ed4..2dae21341e 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -816,8 +816,9 @@ int nb_candidate_edit(struct nb_config *candidate, const struct nb_node *nb_node static int nb_candidate_edit_tree_add(struct nb_config *candidate, enum nb_operation operation, LYD_FORMAT format, const char *xpath, - const char *data, char *xpath_created, - char *errmsg, size_t errmsg_len) + const char *data, bool *created, + char *xpath_created, char *errmsg, + size_t errmsg_len) { struct lyd_node *tree = NULL; struct lyd_node *parent = NULL; @@ -897,10 +898,18 @@ static int nb_candidate_edit_tree_add(struct nb_config *candidate, } /* check if the node already exists in candidate */ - if (operation == NB_OP_CREATE_EXCL || operation == NB_OP_REPLACE) { + if (operation == NB_OP_CREATE || operation == NB_OP_MODIFY) + existing = yang_dnode_get(candidate->dnode, xpath_created); + else if (operation == NB_OP_CREATE_EXCL || operation == NB_OP_REPLACE) { existing = yang_dnode_get(candidate->dnode, xpath_created); /* if the existing node is implicit default, ignore */ + /* Q: Is this correct for CREATE_EXCL which is supposed to error + * if the resouurce already exists? This is used by RESTCONF + * when processing the POST command, for example. RFC8040 + * doesn't say POST fails if resource exists "unless it was a + * default". + */ if (existing && (existing->flags & LYD_DEFAULT)) existing = NULL; @@ -930,7 +939,7 @@ static int nb_candidate_edit_tree_add(struct nb_config *candidate, LYD_MERGE_DESTRUCT | LYD_MERGE_WITH_FLAGS); if (err) { /* if replace failed, restore the original node */ - if (existing) { + if (existing && operation == NB_OP_REPLACE) { if (root) { /* Restoring the whole config. */ candidate->dnode = existing; @@ -954,6 +963,8 @@ static int nb_candidate_edit_tree_add(struct nb_config *candidate, ret = NB_ERR; goto done; } else { + if (!existing) + *created = true; /* * Free existing node after replace. * We're using `lyd_free_siblings` here to free the whole @@ -961,7 +972,7 @@ static int nb_candidate_edit_tree_add(struct nb_config *candidate, * siblings if it wasn't root, because the existing node * was unlinked from the tree. */ - if (existing) + if (existing && operation == NB_OP_REPLACE) lyd_free_siblings(existing); tree = NULL; /* LYD_MERGE_DESTRUCT deleted the tree */ @@ -1011,7 +1022,7 @@ static int nb_candidate_edit_tree_del(struct nb_config *candidate, int nb_candidate_edit_tree(struct nb_config *candidate, enum nb_operation operation, LYD_FORMAT format, - const char *xpath, const char *data, + const char *xpath, const char *data, bool *created, char *xpath_created, char *errmsg, size_t errmsg_len) { int ret = NB_ERR; @@ -1022,8 +1033,9 @@ int nb_candidate_edit_tree(struct nb_config *candidate, case NB_OP_MODIFY: case NB_OP_REPLACE: ret = nb_candidate_edit_tree_add(candidate, operation, format, - xpath, data, xpath_created, - errmsg, errmsg_len); + xpath, data, created, + xpath_created, errmsg, + errmsg_len); break; case NB_OP_DESTROY: case NB_OP_DELETE: diff --git a/lib/northbound.h b/lib/northbound.h index b311affa31..b2cccb6716 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -1016,6 +1016,9 @@ extern int nb_candidate_edit(struct nb_config *candidate, * data * New data tree for the node. * + * created + * OUT param set accordingly if a node was created or just updated + * * xpath_created * XPath of the created node if operation is "create". * @@ -1030,9 +1033,9 @@ extern int nb_candidate_edit(struct nb_config *candidate, * - NB_ERR for other errors. */ extern int nb_candidate_edit_tree(struct nb_config *candidate, - enum nb_operation operation, - LYD_FORMAT format, const char *xpath, - const char *data, char *xpath_created, + enum nb_operation operation, LYD_FORMAT format, + const char *xpath, const char *data, + bool *created, char *xpath_created, char *errmsg, size_t errmsg_len); /* diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 8ab66de687..8d305ed52f 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -1164,7 +1164,9 @@ done: } static int fe_adapter_send_edit_reply(struct mgmt_fe_session_ctx *session, - uint64_t req_id, const char *xpath) + uint64_t req_id, bool changed, + bool created, const char *xpath, + const char *data) { struct mgmt_msg_edit_reply *msg; int ret; @@ -1173,14 +1175,19 @@ static int fe_adapter_send_edit_reply(struct mgmt_fe_session_ctx *session, MTYPE_MSG_NATIVE_EDIT_REPLY); msg->refer_id = session->session_id; msg->req_id = req_id; + msg->changed = changed; + msg->created = created; msg->code = MGMT_MSG_CODE_EDIT_REPLY; mgmt_msg_native_xpath_encode(msg, xpath); + if (data) + mgmt_msg_native_append(msg, data, strlen(data) + 1); + __dbg("Sending edit-reply from adapter %s to session-id %" PRIu64 - " req-id %" PRIu64 " len %u", - session->adapter->name, session->session_id, req_id, - mgmt_msg_native_get_msg_len(msg)); + " req-id %" PRIu64 " changed %u created %u len %u", + session->adapter->name, session->session_id, req_id, changed, + created, mgmt_msg_native_get_msg_len(msg)); ret = fe_adapter_send_native_msg(session->adapter, msg, mgmt_msg_native_get_msg_len(msg), @@ -1977,8 +1984,8 @@ int mgmt_fe_adapter_send_rpc_reply(uint64_t session_id, uint64_t txn_id, int mgmt_fe_adapter_send_edit_reply(uint64_t session_id, uint64_t txn_id, uint64_t req_id, bool unlock, bool commit, - const char *xpath, int16_t error, - const char *errstr) + bool created, const char *xpath, + int16_t error, const char *errstr) { struct mgmt_fe_session_ctx *session; Mgmtd__DatastoreId ds_id, rds_id; @@ -2009,11 +2016,12 @@ int mgmt_fe_adapter_send_edit_reply(uint64_t session_id, uint64_t txn_id, } } - if (error) + if (error != 0 && error != -EALREADY) ret = fe_adapter_send_error(session, req_id, false, error, "%s", errstr); else - ret = fe_adapter_send_edit_reply(session, req_id, xpath); + ret = fe_adapter_send_edit_reply(session, req_id, created, + !error, xpath, errstr); if (session->cfg_txn_id != MGMTD_TXN_ID_NONE && !commit) mgmt_destroy_txn(&session->cfg_txn_id); diff --git a/mgmtd/mgmt_fe_adapter.h b/mgmtd/mgmt_fe_adapter.h index 5a7dec3e6f..4d94e7604f 100644 --- a/mgmtd/mgmt_fe_adapter.h +++ b/mgmtd/mgmt_fe_adapter.h @@ -193,14 +193,16 @@ extern int mgmt_fe_adapter_send_rpc_reply(uint64_t session_id, uint64_t txn_id, * req_id: the req id for the edit message * unlock: implicit-lock flag was set in the request * commit: implicit-commit flag was set in the request - * xpath: the xpath of the data node that was created + * created: true if the node was just created + * xpath: the xpath of the data node that was created/updated * error: >0 LY_ERR, < 0 -errno * errstr: the error string, if error is non-zero */ extern int mgmt_fe_adapter_send_edit_reply(uint64_t session_id, uint64_t txn_id, uint64_t req_id, bool unlock, - bool commit, const char *xpath, - int16_t error, const char *errstr); + bool commit, bool created, + const char *xpath, int16_t error, + const char *errstr); /** * Send an error back to the FE client using native messaging. diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 53d9f5c3fa..ccfdd7539f 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -94,6 +94,7 @@ DECLARE_LIST(mgmt_txn_batches, struct mgmt_txn_be_cfg_batch, list_linkage); struct mgmt_edit_req { char xpath_created[XPATH_MAXLEN]; + bool created; bool unlock; }; @@ -741,6 +742,8 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, txn->commit_cfg_req->req.commit_cfg .edit->unlock, true, + txn->commit_cfg_req->req.commit_cfg + .edit->created, txn->commit_cfg_req->req.commit_cfg .edit->xpath_created, success ? 0 : -1, @@ -2566,8 +2569,8 @@ int mgmt_txn_send_edit(uint64_t txn_id, uint64_t req_id, assert(nb_config); ret = nb_candidate_edit_tree(nb_config, operation, request_type, xpath, - data, edit->xpath_created, errstr, - sizeof(errstr)); + data, &edit->created, edit->xpath_created, + errstr, sizeof(errstr)); if (ret) goto reply; @@ -2581,7 +2584,8 @@ int mgmt_txn_send_edit(uint64_t txn_id, uint64_t req_id, } reply: mgmt_fe_adapter_send_edit_reply(txn->session_id, txn->txn_id, req_id, - unlock, commit, edit->xpath_created, + unlock, commit, edit->created, + edit->xpath_created, errno_from_nb_error(ret), errstr); XFREE(MTYPE_MGMTD_TXN_REQ, edit);