From babbdd43d5319dd370672f794a44f33bdcdc36e5 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Fri, 16 Jun 2023 07:19:53 -0400 Subject: [PATCH 1/5] lib: mgmtd: re-purpose is_short_circuit and fix depth variable inc/dec `is_short_circuit` now is set to true when a message is being short-circuit handled. `short_circuit_depth` was being inc/dec inside conditional macro, move that out of the macro. Signed-off-by: Christian Hopps --- lib/mgmt_msg.c | 12 ++++++++---- lib/mgmt_msg.h | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/mgmt_msg.c b/lib/mgmt_msg.c index ba69c20aba..ee5c1008bd 100644 --- a/lib/mgmt_msg.c +++ b/lib/mgmt_msg.c @@ -548,20 +548,26 @@ int msg_conn_send_msg(struct msg_conn *conn, uint8_t version, void *msg, if (conn->remote_conn && short_circuit_ok) { uint8_t *buf = msg; size_t n = mlen; + bool old; if (packf) { buf = XMALLOC(MTYPE_TMP, mlen); n = packf(msg, buf); } + ++conn->short_circuit_depth; MGMT_MSG_DBG(dbgtag, "SC send: depth %u msg: %p", - ++conn->short_circuit_depth, msg); + conn->short_circuit_depth, msg); + old = conn->remote_conn->is_short_circuit; + conn->remote_conn->is_short_circuit = true; conn->remote_conn->handle_msg(version, buf, n, conn->remote_conn); + conn->remote_conn->is_short_circuit = old; + --conn->short_circuit_depth; MGMT_MSG_DBG(dbgtag, "SC return from depth: %u msg: %p", - conn->short_circuit_depth--, msg); + conn->short_circuit_depth, msg); if (packf) XFREE(MTYPE_TMP, buf); @@ -661,12 +667,10 @@ static bool msg_client_connect_short_circuit(struct msg_client *client) set_nonblocking(sockets[0]); setsockopt_so_sendbuf(sockets[0], client->conn.mstate.max_write_buf); setsockopt_so_recvbuf(sockets[0], client->conn.mstate.max_read_buf); - client->conn.is_short_circuit = true; /* server side */ memset(&su, 0, sizeof(union sockunion)); server_conn = server->create(sockets[1], &su); - server_conn->is_short_circuit = true; client->conn.remote_conn = server_conn; server_conn->remote_conn = &client->conn; diff --git a/lib/mgmt_msg.h b/lib/mgmt_msg.h index 9fdcb9ecd3..dd7ae59f91 100644 --- a/lib/mgmt_msg.h +++ b/lib/mgmt_msg.h @@ -98,8 +98,8 @@ struct msg_conn { struct msg_conn *conn); void *user; uint short_circuit_depth; + bool is_short_circuit; /* true when the message being handled is SC */ bool is_client; - bool is_short_circuit; bool debug; }; From f8500d484997f34d0484488216c98017745f1a37 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Mon, 12 Jun 2023 04:59:19 -0400 Subject: [PATCH 2/5] lib: mgmtd: use short-circuit for locking Signed-off-by: Christian Hopps --- lib/mgmt_fe_client.c | 4 +- lib/mgmt_fe_client.h | 3 +- lib/vty.c | 170 +++++++++++++++++++++++----------------- lib/vty.h | 3 +- mgmtd/mgmt_fe_adapter.c | 7 +- 5 files changed, 109 insertions(+), 78 deletions(-) diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c index be7263f21b..22d085cbce 100644 --- a/lib/mgmt_fe_client.c +++ b/lib/mgmt_fe_client.c @@ -164,7 +164,7 @@ static int mgmt_fe_send_session_req(struct mgmt_fe_client *client, int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client, uint64_t session_id, uint64_t req_id, Mgmtd__DatastoreId ds_id, - bool lock) + bool lock, bool scok) { (void)req_id; Mgmtd__FeMessage fe_msg; @@ -185,7 +185,7 @@ int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client, uint64_t session_id, lock ? "" : "UN", dsid2name(ds_id), session_id); - return mgmt_fe_client_send_msg(client, &fe_msg, false); + return mgmt_fe_client_send_msg(client, &fe_msg, scok); } int mgmt_fe_send_setcfg_req(struct mgmt_fe_client *client, uint64_t session_id, diff --git a/lib/mgmt_fe_client.h b/lib/mgmt_fe_client.h index b0ac44bb3e..e3c016f9b6 100644 --- a/lib/mgmt_fe_client.h +++ b/lib/mgmt_fe_client.h @@ -219,7 +219,8 @@ mgmt_fe_destroy_client_session(struct mgmt_fe_client *client, */ extern int mgmt_fe_send_lockds_req(struct mgmt_fe_client *client, uint64_t session_id, uint64_t req_id, - Mgmtd__DatastoreId ds_id, bool lock_ds); + Mgmtd__DatastoreId ds_id, bool lock_ds, + bool scok); /* * Send SET_CONFIG_REQ to MGMTD for one or more config data(s). diff --git a/lib/vty.c b/lib/vty.c index fd00e11c5f..c1f4d7f47d 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -70,7 +70,6 @@ struct nb_config *vty_mgmt_candidate_config; static struct mgmt_fe_client *mgmt_fe_client; static bool mgmt_fe_connected; -static bool mgmt_candidate_ds_wr_locked; static uint64_t mgmt_client_id_next; static uint64_t mgmt_last_req_id = UINT64_MAX; @@ -129,6 +128,35 @@ char const *const mgmt_daemons[] = { }; uint mgmt_daemons_count = array_size(mgmt_daemons); + +static int vty_mgmt_lock_candidate_inline(struct vty *vty) +{ + assert(!vty->mgmt_locked_candidate_ds); + (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, true, true); + return vty->mgmt_locked_candidate_ds ? 0 : -1; +} + +static int vty_mgmt_unlock_candidate_inline(struct vty *vty) +{ + assert(vty->mgmt_locked_candidate_ds); + (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false, true); + return vty->mgmt_locked_candidate_ds ? -1 : 0; +} + +static int vty_mgmt_lock_running_inline(struct vty *vty) +{ + assert(!vty->mgmt_locked_running_ds); + (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_RUNNING, true, true); + return vty->mgmt_locked_running_ds ? 0 : -1; +} + +static int vty_mgmt_unlock_running_inline(struct vty *vty) +{ + assert(vty->mgmt_locked_running_ds); + (void)vty_mgmt_send_lockds_req(vty, MGMTD_DS_RUNNING, false, true); + return vty->mgmt_locked_running_ds ? -1 : 0; +} + void vty_mgmt_resume_response(struct vty *vty, bool success) { uint8_t header[4] = {0, 0, 0, 0}; @@ -2202,10 +2230,8 @@ bool mgmt_vty_read_configs(void) vty->node = CONFIG_NODE; vty->config = true; vty->pending_allowed = true; - vty->candidate_config = vty_shared_candidate_config; - vty->mgmt_locked_candidate_ds = true; - mgmt_candidate_ds_wr_locked = true; + vty->candidate_config = vty_shared_candidate_config; for (index = 0; index < array_size(mgmt_daemons); index++) { snprintf(path, sizeof(path), "%s/%s.conf", frr_sysconfdir, @@ -2252,9 +2278,6 @@ bool mgmt_vty_read_configs(void) vty->pending_allowed = false; - vty->mgmt_locked_candidate_ds = false; - mgmt_candidate_ds_wr_locked = false; - if (!count) vty_close(vty); else @@ -2367,7 +2390,7 @@ static void vtysh_read(struct event *thread) */ if (vty->mgmt_req_pending_cmd) { MGMTD_FE_CLIENT_DBG( - "postpone CLI cmd response pending mgmtd %s on vty session-id %" PRIu64, + "postpone CLI response pending mgmtd %s on vty session-id %" PRIu64, vty->mgmt_req_pending_cmd, vty->mgmt_session_id); return; @@ -2453,6 +2476,9 @@ void vty_close(struct vty *vty) MGMTD_FE_CLIENT_ERR( "vty closed, uncommitted config will be lost."); + /* Drop out of configure / transaction if needed. */ + vty_config_exit(vty); + if (mgmt_fe_client && vty->mgmt_session_id) { MGMTD_FE_CLIENT_DBG("closing vty session"); mgmt_fe_destroy_client_session(mgmt_fe_client, @@ -2460,9 +2486,6 @@ void vty_close(struct vty *vty) vty->mgmt_session_id = 0; } - /* Drop out of configure / transaction if needed. */ - vty_config_exit(vty); - /* Cancel threads.*/ EVENT_OFF(vty->t_read); EVENT_OFF(vty->t_write); @@ -2828,21 +2851,26 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive) return CMD_WARNING; } - if (vty_mgmt_fe_enabled()) { - if (!mgmt_candidate_ds_wr_locked) { - if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, - true) != 0) { - vty_out(vty, "Not able to lock candidate DS\n"); - return CMD_WARNING; - } - } else { - vty_out(vty, - "Candidate DS already locked by different session\n"); - return CMD_WARNING; - } - + /* + * We only need to do a lock when reading a config file as we will be + * sending a batch of setcfg changes followed by a single commit + * message. For user interactive mode we are doing implicit commits + * those will obtain the lock (or not) when they try and commit. + */ + if (vty_mgmt_fe_enabled() && vty->pending_allowed && !private_config) { + /* + * lock using short-circuit, we set the locked boolean to true + * here so that it can be flipped to false by our locked_notify + * handler during the synchronous call. + */ vty->mgmt_locked_candidate_ds = true; - mgmt_candidate_ds_wr_locked = true; + if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, true, + true) || + !vty->mgmt_locked_candidate_ds) { + vty_out(vty, + "%% Can't enter config; candidate datastore locked by another session\n"); + return CMD_WARNING_CONFIG_FAILED; + } } vty->node = CONFIG_NODE; @@ -2855,23 +2883,24 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive) vty->candidate_config_base = nb_config_dup(running_config); vty_out(vty, "Warning: uncommitted changes will be discarded on exit.\n\n"); - } else { - /* - * NOTE: On the MGMTD daemon we point the VTY candidate DS to - * the global MGMTD candidate DS. Else we point to the VTY - * Shared Candidate Config. - */ - vty->candidate_config = vty_mgmt_candidate_config - ? vty_mgmt_candidate_config - : vty_shared_candidate_config; - if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) - vty->candidate_config_base = - nb_config_dup(running_config); + return CMD_SUCCESS; } + /* + * NOTE: On the MGMTD daemon we point the VTY candidate DS to + * the global MGMTD candidate DS. Else we point to the VTY + * Shared Candidate Config. + */ + vty->candidate_config = vty_mgmt_candidate_config + ? vty_mgmt_candidate_config + : vty_shared_candidate_config; + if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) + vty->candidate_config_base = nb_config_dup(running_config); + return CMD_SUCCESS; } + void vty_config_exit(struct vty *vty) { enum node_type node = vty->node; @@ -2894,22 +2923,17 @@ void vty_config_exit(struct vty *vty) int vty_config_node_exit(struct vty *vty) { + int ret; + vty->xpath_index = 0; - /* - * If we are not reading config file and we are mgmtd FE and we are - * locked then unlock. - */ - if (vty->type != VTY_FILE && vty_mgmt_fe_enabled() && - mgmt_candidate_ds_wr_locked && vty->mgmt_locked_candidate_ds) { - if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false) != - 0) { - vty_out(vty, "Not able to unlock candidate DS\n"); - return CMD_WARNING; - } - + if (vty->mgmt_locked_candidate_ds) { + assert(vty->type != VTY_FILE); + /* use short-circuit call to immediately unlock */ + ret = vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false, + true); + assert(!ret); vty->mgmt_locked_candidate_ds = false; - mgmt_candidate_ds_wr_locked = false; } /* Perform any pending commits. */ @@ -3493,17 +3517,21 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client, vty = (struct vty *)session_ctx; - if (!success) { - zlog_err("%socking for DS %u failed, Err: '%s'", - lock_ds ? "L" : "Unl", ds_id, errmsg_if_any); - vty_out(vty, "ERROR: %socking for DS %u failed, Err: '%s'\n", - lock_ds ? "L" : "Unl", ds_id, errmsg_if_any); - } else { + assert(ds_id == MGMTD_DS_CANDIDATE || ds_id == MGMTD_DS_RUNNING); + if (!success) + zlog_err("%socking for DS %u failed, Err: '%s' vty %p", + lock_ds ? "L" : "Unl", ds_id, errmsg_if_any, vty); + else { MGMTD_FE_CLIENT_DBG("%socked DS %u successfully", lock_ds ? "L" : "Unl", ds_id); + if (ds_id == MGMTD_DS_CANDIDATE) + vty->mgmt_locked_candidate_ds = lock_ds; + else + vty->mgmt_locked_running_ds = lock_ds; } - vty_mgmt_resume_response(vty, success); + if (vty->mgmt_req_pending_cmd) + vty_mgmt_resume_response(vty, success); } static void vty_mgmt_set_config_result_notified( @@ -3632,23 +3660,24 @@ bool vty_mgmt_should_process_cli_apply_changes(struct vty *vty) } int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id, - bool lock) + bool lock, bool scok) { - if (mgmt_fe_client && vty->mgmt_session_id) { - vty->mgmt_req_id++; - if (mgmt_fe_send_lockds_req(mgmt_fe_client, - vty->mgmt_session_id, - vty->mgmt_req_id, ds_id, lock)) { - zlog_err("Failed sending %sLOCK-DS-REQ req-id %" PRIu64, - lock ? "" : "UN", vty->mgmt_req_id); - vty_out(vty, "Failed to send %sLOCK-DS-REQ to MGMTD!\n", - lock ? "" : "UN"); - return -1; - } + assert(mgmt_fe_client); + assert(vty->mgmt_session_id); - vty->mgmt_req_pending_cmd = "MESSAGE_LOCKDS_REQ"; + vty->mgmt_req_id++; + if (mgmt_fe_send_lockds_req(mgmt_fe_client, vty->mgmt_session_id, + vty->mgmt_req_id, ds_id, lock, scok)) { + zlog_err("Failed sending %sLOCK-DS-REQ req-id %" PRIu64, + lock ? "" : "UN", vty->mgmt_req_id); + vty_out(vty, "Failed to send %sLOCK-DS-REQ to MGMTD!\n", + lock ? "" : "UN"); + return -1; } + if (!scok) + vty->mgmt_req_pending_cmd = "MESSAGE_LOCKDS_REQ"; + return 0; } @@ -3667,7 +3696,6 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit) * changes until we are done reading the file and have modified * the local candidate DS. */ - assert(vty->mgmt_locked_candidate_ds); /* no-one else should be sending data right now */ assert(!vty->mgmt_num_pending_setcfg); return 0; diff --git a/lib/vty.h b/lib/vty.h index 3b651d20a2..7e27b52fe1 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -230,6 +230,7 @@ struct vty { * vty user. */ const char *mgmt_req_pending_cmd; bool mgmt_locked_candidate_ds; + bool mgmt_locked_running_ds; }; static inline void vty_push_context(struct vty *vty, int node, uint64_t id) @@ -416,7 +417,7 @@ extern int vty_mgmt_send_get_config(struct vty *vty, extern int vty_mgmt_send_get_data(struct vty *vty, Mgmtd__DatastoreId datastore, const char **xpath_list, int num_req); extern int vty_mgmt_send_lockds_req(struct vty *vty, Mgmtd__DatastoreId ds_id, - bool lock); + bool lock, bool scok); extern void vty_mgmt_resume_response(struct vty *vty, bool success); static inline bool vty_needs_implicit_commit(struct vty *vty) diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index e9cbd444e8..4b104abb98 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -389,6 +389,7 @@ static int mgmt_fe_send_lockds_reply(struct mgmt_fe_session_ctx *session, { Mgmtd__FeMessage fe_msg; Mgmtd__FeLockDsReply lockds_reply; + bool scok = session->adapter->conn->is_short_circuit; assert(session->adapter); @@ -406,10 +407,10 @@ static int mgmt_fe_send_lockds_reply(struct mgmt_fe_session_ctx *session, fe_msg.lockds_reply = &lockds_reply; MGMTD_FE_ADAPTER_DBG( - "Sending LOCK_DS_REPLY message to MGMTD Frontend client '%s'", - session->adapter->name); + "Sending LOCK_DS_REPLY message to MGMTD Frontend client '%s' scok: %d", + session->adapter->name, scok); - return mgmt_fe_adapter_send_msg(session->adapter, &fe_msg, false); + return mgmt_fe_adapter_send_msg(session->adapter, &fe_msg, scok); } static int mgmt_fe_send_setcfg_reply(struct mgmt_fe_session_ctx *session, From 04b4ede097c94f04cc1d14ce90ee82e35a63d670 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Wed, 14 Jun 2023 09:32:16 -0400 Subject: [PATCH 3/5] mgmtd: simplify locking, removing read locks Signed-off-by: Christian Hopps --- mgmtd/mgmt_be_adapter.c | 11 +- mgmtd/mgmt_be_adapter.h | 6 +- mgmtd/mgmt_ds.c | 84 ++++++------- mgmtd/mgmt_ds.h | 21 ++-- mgmtd/mgmt_fe_adapter.c | 261 ++++++++++++---------------------------- mgmtd/mgmt_history.c | 4 +- mgmtd/mgmt_txn.c | 152 ++++++++--------------- mgmtd/mgmt_txn.h | 21 ++-- 8 files changed, 198 insertions(+), 362 deletions(-) diff --git a/mgmtd/mgmt_be_adapter.c b/mgmtd/mgmt_be_adapter.c index 49a307e9c2..512cc49feb 100644 --- a/mgmtd/mgmt_be_adapter.c +++ b/mgmtd/mgmt_be_adapter.c @@ -648,8 +648,7 @@ static void mgmt_be_adapter_process_msg(uint8_t version, uint8_t *data, mgmtd__be_message__free_unpacked(be_msg, NULL); } -static void mgmt_be_iter_and_get_cfg(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, struct lyd_node *node, +static void mgmt_be_iter_and_get_cfg(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx) { struct mgmt_be_get_adapter_config_params *parms = ctx; @@ -806,10 +805,10 @@ mgmt_be_get_adapter_by_name(const char *name) } int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, - struct mgmt_ds_ctx *ds_ctx, - struct nb_config_cbs **cfg_chgs) + struct nb_config_cbs **cfg_chgs) { struct mgmt_be_get_adapter_config_params parms; + struct nb_config *cfg_root = mgmt_ds_get_nb_config(mm->running_ds); assert(cfg_chgs); @@ -825,8 +824,8 @@ int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, parms.cfg_chgs = &adapter->cfg_chgs; parms.seq = 0; - mgmt_ds_iter_data(ds_ctx, "", mgmt_be_iter_and_get_cfg, - (void *)&parms); + mgmt_ds_iter_data(MGMTD_DS_RUNNING, cfg_root, "", + mgmt_be_iter_and_get_cfg, (void *)&parms); } *cfg_chgs = &adapter->cfg_chgs; diff --git a/mgmtd/mgmt_be_adapter.h b/mgmtd/mgmt_be_adapter.h index e1676e63af..ca8f55c457 100644 --- a/mgmtd/mgmt_be_adapter.h +++ b/mgmtd/mgmt_be_adapter.h @@ -110,10 +110,8 @@ extern struct mgmt_be_client_adapter * mgmt_be_get_adapter_by_id(enum mgmt_be_client_id id); /* Fetch backend adapter config. */ -extern int -mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, - struct mgmt_ds_ctx *ds_ctx, - struct nb_config_cbs **cfg_chgs); +extern int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, + struct nb_config_cbs **cfg_chgs); /* Create/destroy a transaction. */ extern int mgmt_be_send_txn_req(struct mgmt_be_client_adapter *adapter, diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index 5a4b00d309..027e306141 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -22,7 +22,9 @@ struct mgmt_ds_ctx { Mgmtd__DatastoreId ds_id; - int lock; /* 0 unlocked, >0 read locked < write locked */ + + bool locked; + uint64_t vty_session_id; /* Owner of the lock or 0 */ bool config_ds; @@ -244,40 +246,33 @@ bool mgmt_ds_is_config(struct mgmt_ds_ctx *ds_ctx) return ds_ctx->config_ds; } -int mgmt_ds_read_lock(struct mgmt_ds_ctx *ds_ctx) +bool mgmt_ds_is_locked(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id) { - if (!ds_ctx) - return EINVAL; - if (ds_ctx->lock < 0) + assert(ds_ctx); + return (ds_ctx->locked && ds_ctx->vty_session_id == session_id); +} + +int mgmt_ds_lock(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id) +{ + assert(ds_ctx); + + if (ds_ctx->locked) return EBUSY; - ++ds_ctx->lock; + + ds_ctx->locked = true; + ds_ctx->vty_session_id = session_id; return 0; } -int mgmt_ds_write_lock(struct mgmt_ds_ctx *ds_ctx) +void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx) { - if (!ds_ctx) - return EINVAL; - if (ds_ctx->lock != 0) - return EBUSY; - ds_ctx->lock = -1; - return 0; -} - -int mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx) -{ - if (!ds_ctx) - return EINVAL; - if (ds_ctx->lock > 0) - --ds_ctx->lock; - else if (ds_ctx->lock < 0) { - assert(ds_ctx->lock == -1); - ds_ctx->lock = 0; - } else { - assert(ds_ctx->lock != 0); - return EINVAL; - } - return 0; + assert(ds_ctx); + if (!ds_ctx->locked) + zlog_warn( + "%s: WARNING: unlock on unlocked in DS:%s last session-id %" PRIu64, + __func__, mgmt_ds_id2name(ds_ctx->ds_id), + ds_ctx->vty_session_id); + ds_ctx->locked = 0; } int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, @@ -314,10 +309,9 @@ struct nb_config *mgmt_ds_get_nb_config(struct mgmt_ds_ctx *ds_ctx) } static int mgmt_walk_ds_nodes( - struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, + struct nb_config *root, const char *base_xpath, struct lyd_node *base_dnode, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, struct lyd_node *node, + void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), void *ctx) { @@ -336,10 +330,7 @@ static int mgmt_walk_ds_nodes( * This function only returns the first node of a possible set * of matches issuing a warning if more than 1 matches */ - base_dnode = yang_dnode_get( - ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode - : ds_ctx->root.dnode_root, - base_xpath); + base_dnode = yang_dnode_get(root->dnode, base_xpath); if (!base_dnode) return -1; @@ -348,7 +339,7 @@ static int mgmt_walk_ds_nodes( sizeof(xpath))); nbnode = (struct nb_node *)base_dnode->schema->priv; - (*mgmt_ds_node_iter_fn)(ds_ctx, base_xpath, base_dnode, nbnode, ctx); + (*mgmt_ds_node_iter_fn)(base_xpath, base_dnode, nbnode, ctx); /* * If the base_xpath points to a leaf node we can skip the tree walk. @@ -370,7 +361,7 @@ static int mgmt_walk_ds_nodes( MGMTD_DS_DBG(" -- Child xpath: %s", xpath); - ret = mgmt_walk_ds_nodes(ds_ctx, xpath, dnode, + ret = mgmt_walk_ds_nodes(root, xpath, dnode, mgmt_ds_node_iter_fn, ctx); if (ret != 0) break; @@ -459,9 +450,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst, return 0; } -int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, +int mgmt_ds_iter_data(Mgmtd__DatastoreId ds_id, struct nb_config *root, + const char *base_xpath, + void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), @@ -472,7 +463,7 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, struct lyd_node *base_dnode = NULL; struct lyd_node *node; - if (!ds_ctx) + if (!root) return -1; strlcpy(xpath, base_xpath, sizeof(xpath)); @@ -484,12 +475,11 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, * Oper-state should be kept in mind though for the prefix walk */ - MGMTD_DS_DBG(" -- START DS walk for DSid: %d", ds_ctx->ds_id); + MGMTD_DS_DBG(" -- START DS walk for DSid: %d", ds_id); /* If the base_xpath is empty then crawl the sibblings */ if (xpath[0] == 0) { - base_dnode = ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode - : ds_ctx->root.dnode_root; + base_dnode = root->dnode; /* get first top-level sibling */ while (base_dnode->parent) @@ -499,11 +489,11 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, base_dnode = base_dnode->prev; LY_LIST_FOR (base_dnode, node) { - ret = mgmt_walk_ds_nodes(ds_ctx, xpath, node, + ret = mgmt_walk_ds_nodes(root, xpath, node, mgmt_ds_node_iter_fn, ctx); } } else - ret = mgmt_walk_ds_nodes(ds_ctx, xpath, base_dnode, + ret = mgmt_walk_ds_nodes(root, xpath, base_dnode, mgmt_ds_node_iter_fn, ctx); return ret; diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h index e5c88742dd..1cf4816027 100644 --- a/mgmtd/mgmt_ds.h +++ b/mgmtd/mgmt_ds.h @@ -179,19 +179,19 @@ extern struct mgmt_ds_ctx *mgmt_ds_get_ctx_by_id(struct mgmt_master *mm, extern bool mgmt_ds_is_config(struct mgmt_ds_ctx *ds_ctx); /* - * Acquire read lock to a ds given a ds_handle + * Check if a given datastore is locked by a given session */ -extern int mgmt_ds_read_lock(struct mgmt_ds_ctx *ds_ctx); +extern bool mgmt_ds_is_locked(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id); /* * Acquire write lock to a ds given a ds_handle */ -extern int mgmt_ds_write_lock(struct mgmt_ds_ctx *ds_ctx); +extern int mgmt_ds_lock(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id); /* * Remove a lock from ds given a ds_handle */ -extern int mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx); +extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx); /* * Copy from source to destination datastore. @@ -233,8 +233,11 @@ extern int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx, /* * Iterate over datastore data. * - * ds_ctx - * Datastore context. + * ds_id + * Datastore ID.. + * + * root + * The root of the tree to iterate over. * * base_xpath * Base YANG xpath from where needs to be iterated. @@ -252,9 +255,9 @@ extern int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx, * 0 on success, -1 on failure. */ extern int mgmt_ds_iter_data( - struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, struct lyd_node *node, + Mgmtd__DatastoreId ds_id, struct nb_config *root, + const char *base_xpath, + void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), void *ctx); diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 4b104abb98..1b6f45a05d 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -40,8 +40,7 @@ struct mgmt_fe_session_ctx { uint64_t client_id; uint64_t txn_id; uint64_t cfg_txn_id; - uint8_t ds_write_locked[MGMTD_DS_MAX_ID]; - uint8_t ds_read_locked[MGMTD_DS_MAX_ID]; + uint8_t ds_locked[MGMTD_DS_MAX_ID]; uint8_t ds_locked_implict[MGMTD_DS_MAX_ID]; struct event *proc_cfg_txn_clnp; struct event *proc_show_txn_clnp; @@ -72,8 +71,12 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id, struct mgmt_ds_ctx *ds_ctx, struct mgmt_fe_session_ctx *session) { - if (!session->ds_write_locked[ds_id]) { - if (mgmt_ds_write_lock(ds_ctx) != 0) { + if (session->ds_locked[ds_id]) + zlog_warn("multiple lock taken by session-id: %" PRIu64 + " on DS:%s", + session->session_id, mgmt_ds_id2name(ds_id)); + else { + if (mgmt_ds_lock(ds_ctx, session->session_id)) { MGMTD_FE_ADAPTER_DBG( "Failed to lock the DS:%s for session-id: %" PRIu64 " from %s!", @@ -82,7 +85,7 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id, return -1; } - session->ds_write_locked[ds_id] = true; + session->ds_locked[ds_id] = true; MGMTD_FE_ADAPTER_DBG( "Write-Locked the DS:%s for session-id: %" PRIu64 " from %s", @@ -93,74 +96,22 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id, return 0; } -static int -mgmt_fe_session_read_lock_ds(Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - struct mgmt_fe_session_ctx *session) +static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, + struct mgmt_ds_ctx *ds_ctx, + struct mgmt_fe_session_ctx *session) { - if (!session->ds_read_locked[ds_id]) { - if (mgmt_ds_read_lock(ds_ctx) != 0) { - MGMTD_FE_ADAPTER_DBG( - "Failed to lock the DS:%s for session-is: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - return -1; - } + if (!session->ds_locked[ds_id]) + zlog_warn("unlock unlocked by session-id: %" PRIu64 " on DS:%s", + session->session_id, mgmt_ds_id2name(ds_id)); - session->ds_read_locked[ds_id] = true; - MGMTD_FE_ADAPTER_DBG( - "Read-Locked the DS:%s for session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - } - - return 0; -} - -static int mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - struct mgmt_fe_session_ctx *session, - bool unlock_write, bool unlock_read) -{ - if (unlock_write && session->ds_write_locked[ds_id]) { - session->ds_write_locked[ds_id] = false; - session->ds_locked_implict[ds_id] = false; - if (mgmt_ds_unlock(ds_ctx) != 0) { - MGMTD_FE_ADAPTER_DBG( - "Failed to unlock the DS:%s taken earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - return -1; - } - - MGMTD_FE_ADAPTER_DBG( - "Unlocked DS:%s write-locked earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - } else if (unlock_read && session->ds_read_locked[ds_id]) { - session->ds_read_locked[ds_id] = false; - session->ds_locked_implict[ds_id] = false; - if (mgmt_ds_unlock(ds_ctx) != 0) { - MGMTD_FE_ADAPTER_DBG( - "Failed to unlock the DS:%s taken earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - return -1; - } - - MGMTD_FE_ADAPTER_DBG( - "Unlocked DS:%s read-locked earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - } - - return 0; + session->ds_locked[ds_id] = false; + session->ds_locked_implict[ds_id] = false; + mgmt_ds_unlock(ds_ctx); + MGMTD_FE_ADAPTER_DBG( + "Unlocked DS:%s write-locked earlier by session-id: %" PRIu64 + " from %s", + mgmt_ds_id2name(ds_id), session->session_id, + session->adapter->name); } static void @@ -177,11 +128,8 @@ mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session) for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); - if (ds_ctx) { - if (session->ds_locked_implict[ds_id]) - mgmt_fe_session_unlock_ds( - ds_id, ds_ctx, session, true, false); - } + if (ds_ctx && session->ds_locked_implict[ds_id]) + mgmt_fe_session_unlock_ds(ds_id, ds_ctx, session); } /* @@ -194,17 +142,6 @@ mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session) static void mgmt_fe_session_show_txn_cleanup(struct mgmt_fe_session_ctx *session) { - Mgmtd__DatastoreId ds_id; - struct mgmt_ds_ctx *ds_ctx; - - for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { - ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); - if (ds_ctx) { - mgmt_fe_session_unlock_ds(ds_id, ds_ctx, session, - false, true); - } - } - /* * Destroy the transaction created recently. */ @@ -245,25 +182,29 @@ mgmt_fe_session_compute_commit_timers(struct mgmt_commit_stats *cmt_stats) } } -static void mgmt_fe_cleanup_session(struct mgmt_fe_session_ctx **session) +static void mgmt_fe_cleanup_session(struct mgmt_fe_session_ctx **sessionp) { - if ((*session)->adapter) { - mgmt_fe_session_cfg_txn_cleanup((*session)); - mgmt_fe_session_show_txn_cleanup((*session)); - mgmt_fe_session_unlock_ds(MGMTD_DS_CANDIDATE, mm->candidate_ds, - *session, true, true); - mgmt_fe_session_unlock_ds(MGMTD_DS_RUNNING, mm->running_ds, - *session, true, true); + Mgmtd__DatastoreId ds_id; + struct mgmt_ds_ctx *ds_ctx; + struct mgmt_fe_session_ctx *session = *sessionp; - mgmt_fe_sessions_del(&(*session)->adapter->fe_sessions, - *session); - assert((*session)->adapter->refcount > 1); - mgmt_fe_adapter_unlock(&(*session)->adapter); + if (session->adapter) { + mgmt_fe_session_cfg_txn_cleanup(session); + mgmt_fe_session_show_txn_cleanup(session); + for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { + ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); + if (ds_ctx && session->ds_locked[ds_id]) + mgmt_fe_session_unlock_ds(ds_id, ds_ctx, + session); + } + mgmt_fe_sessions_del(&session->adapter->fe_sessions, session); + assert(session->adapter->refcount > 1); + mgmt_fe_adapter_unlock(&session->adapter); } - hash_release(mgmt_fe_sessions, *session); - XFREE(MTYPE_MGMTD_FE_SESSION, *session); - *session = NULL; + hash_release(mgmt_fe_sessions, session); + XFREE(MTYPE_MGMTD_FE_SESSION, session); + *sessionp = NULL; } static struct mgmt_fe_session_ctx * @@ -735,7 +676,7 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, session->ds_locked_implict[lockds_req->ds_id] = false; } else { - if (!session->ds_write_locked[lockds_req->ds_id]) { + if (!session->ds_locked[lockds_req->ds_id]) { mgmt_fe_send_lockds_reply( session, lockds_req->ds_id, lockds_req->req_id, lockds_req->lock, false, @@ -743,8 +684,7 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, return 0; } - (void)mgmt_fe_session_unlock_ds(lockds_req->ds_id, ds_ctx, - session, true, false); + mgmt_fe_session_unlock_ds(lockds_req->ds_id, ds_ctx, session); } if (mgmt_fe_send_lockds_reply(session, lockds_req->ds_id, @@ -821,7 +761,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, /* * Try taking write-lock on the requested DS (if not already). */ - if (!session->ds_write_locked[setcfg_req->ds_id]) { + if (!session->ds_locked[setcfg_req->ds_id]) { MGMTD_FE_ADAPTER_ERR( "SETCFG_REQ on session-id: %" PRIu64 " without obtaining lock", @@ -832,7 +772,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, mgmt_fe_send_setcfg_reply( session, setcfg_req->ds_id, setcfg_req->req_id, false, - "Failed to lock the DS!", + "Failed to lock the target DS", setcfg_req->implicit_commit); goto mgmt_fe_sess_handle_setcfg_req_failed; } @@ -915,9 +855,8 @@ mgmt_fe_sess_handle_setcfg_req_failed: */ if (session->cfg_txn_id != MGMTD_TXN_ID_NONE) mgmt_destroy_txn(&session->cfg_txn_id); - if (ds_ctx && session->ds_write_locked[setcfg_req->ds_id]) - mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session, - true, false); + if (ds_ctx && session->ds_locked[setcfg_req->ds_id]) + mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session); return 0; } @@ -927,6 +866,7 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, Mgmtd__FeGetConfigReq *getcfg_req) { struct mgmt_ds_ctx *ds_ctx; + struct nb_config *cfg_root = NULL; /* * Get the DS handle. @@ -939,11 +879,7 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, return 0; } - /* - * Next check first if the GETCFG_REQ is for Candidate DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ + /* GETCFG must be on candidate or running DS */ if (getcfg_req->ds_id != MGMTD_DS_CANDIDATE && getcfg_req->ds_id != MGMTD_DS_RUNNING) { mgmt_fe_send_getcfg_reply( @@ -954,27 +890,6 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, } if (session->txn_id == MGMTD_TXN_ID_NONE) { - /* - * Try taking read-lock on the requested DS (if not already - * locked). If the DS has already been write-locked by a ongoing - * CONFIG transaction we may allow reading the contents of the - * same DS. - */ - if (!session->ds_read_locked[getcfg_req->ds_id] - && !session->ds_write_locked[getcfg_req->ds_id]) { - if (mgmt_fe_session_read_lock_ds(getcfg_req->ds_id, - ds_ctx, session) - != 0) { - mgmt_fe_send_getcfg_reply( - session, getcfg_req->ds_id, - getcfg_req->req_id, false, NULL, - "Failed to lock the DS! Another session might have locked it!"); - goto mgmt_fe_sess_handle_getcfg_req_failed; - } - - session->ds_locked_implict[getcfg_req->ds_id] = true; - } - /* * Start a SHOW Transaction (if not started already) */ @@ -992,19 +907,24 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, " for session-id: %" PRIu64, session->txn_id, session->session_id); } else { + /* XXX chopps: Why would we already have a TXN here? */ MGMTD_FE_ADAPTER_DBG("Show txn-id: %" PRIu64 " for session-id: %" PRIu64 " already created", session->txn_id, session->session_id); } + /* + * Get a copy of the datastore config root, avoids locking. + */ + cfg_root = nb_config_dup(mgmt_ds_get_nb_config(ds_ctx)); + /* * Create a GETConfig request under the transaction. */ - if (mgmt_txn_send_get_config_req(session->txn_id, getcfg_req->req_id, - getcfg_req->ds_id, ds_ctx, - getcfg_req->data, getcfg_req->n_data) - != 0) { + if (mgmt_txn_send_get_config_req( + session->txn_id, getcfg_req->req_id, getcfg_req->ds_id, + cfg_root, getcfg_req->data, getcfg_req->n_data) != 0) { mgmt_fe_send_getcfg_reply( session, getcfg_req->ds_id, getcfg_req->req_id, false, NULL, "Request processing for GET-CONFIG failed!"); @@ -1015,14 +935,13 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, mgmt_fe_sess_handle_getcfg_req_failed: + if (cfg_root) + nb_config_free(cfg_root); /* * Destroy the transaction created recently. */ if (session->txn_id != MGMTD_TXN_ID_NONE) mgmt_destroy_txn(&session->txn_id); - if (ds_ctx && session->ds_read_locked[getcfg_req->ds_id]) - mgmt_fe_session_unlock_ds(getcfg_req->ds_id, ds_ctx, session, - false, true); return -1; } @@ -1044,28 +963,16 @@ mgmt_fe_session_handle_getdata_req_msg(struct mgmt_fe_session_ctx *session, return 0; } + /* GETDATA must be on operational DS */ + if (getdata_req->ds_id != MGMTD_DS_OPERATIONAL) { + mgmt_fe_send_getdata_reply( + session, getdata_req->ds_id, getdata_req->req_id, false, + NULL, + "Get-Data on datastore other than Operational DS not permitted!"); + return 0; + } + if (session->txn_id == MGMTD_TXN_ID_NONE) { - /* - * Try taking read-lock on the requested DS (if not already - * locked). If the DS has already been write-locked by a ongoing - * CONFIG transaction we may allow reading the contents of the - * same DS. - */ - if (!session->ds_read_locked[getdata_req->ds_id] - && !session->ds_write_locked[getdata_req->ds_id]) { - if (mgmt_fe_session_read_lock_ds(getdata_req->ds_id, - ds_ctx, session) - != 0) { - mgmt_fe_send_getdata_reply( - session, getdata_req->ds_id, - getdata_req->req_id, false, NULL, - "Failed to lock the DS! Another session might have locked it!"); - goto mgmt_fe_sess_handle_getdata_req_failed; - } - - session->ds_locked_implict[getdata_req->ds_id] = true; - } - /* * Start a SHOW Transaction (if not started already) */ @@ -1092,9 +999,8 @@ mgmt_fe_session_handle_getdata_req_msg(struct mgmt_fe_session_ctx *session, * Create a GETData request under the transaction. */ if (mgmt_txn_send_get_data_req(session->txn_id, getdata_req->req_id, - getdata_req->ds_id, ds_ctx, - getdata_req->data, getdata_req->n_data) - != 0) { + getdata_req->ds_id, getdata_req->data, + getdata_req->n_data) != 0) { mgmt_fe_send_getdata_reply( session, getdata_req->ds_id, getdata_req->req_id, false, NULL, "Request processing for GET-CONFIG failed!"); @@ -1111,10 +1017,6 @@ mgmt_fe_sess_handle_getdata_req_failed: if (session->txn_id != MGMTD_TXN_ID_NONE) mgmt_destroy_txn(&session->txn_id); - if (ds_ctx && session->ds_read_locked[getdata_req->ds_id]) - mgmt_fe_session_unlock_ds(getdata_req->ds_id, ds_ctx, - session, false, true); - return -1; } @@ -1192,7 +1094,7 @@ static int mgmt_fe_session_handle_commit_config_req_msg( /* * Try taking write-lock on the destination DS (if not already). */ - if (!session->ds_write_locked[commcfg_req->dst_ds_id]) { + if (!session->ds_locked[commcfg_req->dst_ds_id]) { if (mgmt_fe_session_write_lock_ds(commcfg_req->dst_ds_id, dst_ds_ctx, session) != 0) { @@ -1215,8 +1117,7 @@ static int mgmt_fe_session_handle_commit_config_req_msg( session->cfg_txn_id, commcfg_req->req_id, commcfg_req->src_ds_id, src_ds_ctx, commcfg_req->dst_ds_id, dst_ds_ctx, commcfg_req->validate_only, commcfg_req->abort, - false) - != 0) { + false) != 0) { mgmt_fe_send_commitcfg_reply( session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, commcfg_req->req_id, MGMTD_INTERNAL_ERROR, @@ -1744,16 +1645,12 @@ void mgmt_fe_adapter_status_write(struct vty *vty, bool detail) session->session_id); vty_out(vty, " DS-Locks:\n"); FOREACH_MGMTD_DS_ID (ds_id) { - if (session->ds_write_locked[ds_id] - || session->ds_read_locked[ds_id]) { + if (session->ds_locked[ds_id]) { locked = true; - vty_out(vty, - " %s\t\t\t%s, %s\n", + vty_out(vty, " %s\t\t\t%s\n", mgmt_ds_id2name(ds_id), - session->ds_write_locked[ds_id] - ? "Write" - : "Read", - session->ds_locked_implict[ds_id] + session->ds_locked_implict + [ds_id] ? "Implicit" : "Explicit"); } diff --git a/mgmtd/mgmt_history.c b/mgmtd/mgmt_history.c index 54eb45fdf4..89a7a60166 100644 --- a/mgmtd/mgmt_history.c +++ b/mgmtd/mgmt_history.c @@ -211,7 +211,7 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, return -1; } - ret = mgmt_ds_write_lock(dst_ds_ctx); + ret = mgmt_ds_lock(dst_ds_ctx, vty->mgmt_session_id); if (ret != 0) { vty_out(vty, "Failed to lock the DS %u for rollback Reason: %s!\n", @@ -243,6 +243,8 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, mgmt_history_dump_cmt_record_index(); + /* XXX chopps when does this get unlocked? */ + /* * Block the rollback command from returning till the rollback * is completed. On rollback completion mgmt_history_rollback_complete() diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index e64cbe1425..c1f674556a 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -164,7 +164,7 @@ struct mgmt_get_data_reply { struct mgmt_get_data_req { Mgmtd__DatastoreId ds_id; - struct mgmt_ds_ctx *ds_ctx; + struct nb_config *cfg_root; char *xpaths[MGMTD_MAX_NUM_DATA_REQ_IN_BATCH]; int num_xpaths; @@ -576,6 +576,10 @@ static void mgmt_txn_req_free(struct mgmt_txn_req **txn_req) if ((*txn_req)->req.get_data->reply) XFREE(MTYPE_MGMTD_TXN_GETDATA_REPLY, (*txn_req)->req.get_data->reply); + + if ((*txn_req)->req.get_data->cfg_root) + nb_config_free((*txn_req)->req.get_data->cfg_root); + XFREE(MTYPE_MGMTD_TXN_GETDATA_REQ, (*txn_req)->req.get_data); break; case MGMTD_TXN_PROC_GETDATA: @@ -683,8 +687,8 @@ static void mgmt_txn_process_set_cfg(struct event *thread) assert(mgmt_txn_reqs_count(&txn->set_cfg_reqs) == 1); assert(txn_req->req.set_cfg->dst_ds_ctx); - ret = mgmt_ds_write_lock( - txn_req->req.set_cfg->dst_ds_ctx); + ret = mgmt_ds_lock(txn_req->req.set_cfg->dst_ds_ctx, + txn->session_id); if (ret != 0) { MGMTD_TXN_ERR( "Failed to lock DS %u txn-id: %" PRIu64 @@ -703,8 +707,8 @@ static void mgmt_txn_process_set_cfg(struct event *thread) txn_req->req.set_cfg->ds_id, txn_req->req.set_cfg->ds_ctx, txn_req->req.set_cfg->dst_ds_id, - txn_req->req.set_cfg->dst_ds_ctx, false, - false, true); + txn_req->req.set_cfg->dst_ds_ctx, false, false, + true); if (mm->perf_stats_en) gettimeofday(&cmt_stats->last_start, NULL); @@ -746,7 +750,6 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, enum mgmt_result result, const char *error_if_any) { - int ret = 0; bool success, create_cmt_info_rec; if (!txn->commit_cfg_req) @@ -830,12 +833,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.rollback) { - ret = mgmt_ds_unlock( - txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); - if (ret != 0) - MGMTD_TXN_ERR( - "Failed to unlock the dst DS during rollback : %s", - strerror(ret)); + mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); /* * Resume processing the rollback command. @@ -844,12 +842,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.implicit) - if (mgmt_ds_unlock( - txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx) - != 0) - MGMTD_TXN_ERR( - "Failed to unlock the dst DS during implicit : %s", - strerror(ret)); + mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); txn->commit_cfg_req->req.commit_cfg.cmt_stats = NULL; mgmt_txn_req_free(&txn->commit_cfg_req); @@ -1724,8 +1717,7 @@ static void mgmt_txn_send_getcfg_reply_data(struct mgmt_txn_req *txn_req, mgmt_reset_get_data_reply_buf(get_req); } -static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, +static void mgmt_txn_iter_and_send_get_cfg_reply(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx) @@ -1770,7 +1762,7 @@ static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, struct mgmt_txn_req *txn_req, - struct mgmt_ds_ctx *ds_ctx) + struct nb_config *root) { int indx; struct mgmt_get_data_req *get_data; @@ -1805,7 +1797,8 @@ static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, * want to also use an xpath regexp we need to add this * functionality. */ - if (mgmt_ds_iter_data(get_data->ds_ctx, get_data->xpaths[indx], + if (mgmt_ds_iter_data(get_data->ds_id, root, + get_data->xpaths[indx], mgmt_txn_iter_and_send_get_cfg_reply, (void *)txn_req) == -1) { MGMTD_TXN_DBG("Invalid Xpath '%s", @@ -1837,7 +1830,7 @@ static void mgmt_txn_process_get_cfg(struct event *thread) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; - struct mgmt_ds_ctx *ds_ctx; + struct nb_config *cfg_root; int num_processed = 0; bool error; @@ -1852,18 +1845,10 @@ static void mgmt_txn_process_get_cfg(struct event *thread) FOREACH_TXN_REQ_IN_LIST (&txn->get_cfg_reqs, txn_req) { error = false; assert(txn_req->req_event == MGMTD_TXN_PROC_GETCFG); - ds_ctx = txn_req->req.get_data->ds_ctx; - if (!ds_ctx) { - mgmt_fe_send_get_cfg_reply( - txn->session_id, txn->txn_id, - txn_req->req.get_data->ds_id, txn_req->req_id, - MGMTD_INTERNAL_ERROR, NULL, - "No such datastore!"); - error = true; - goto mgmt_txn_process_get_cfg_done; - } + cfg_root = txn_req->req.get_data->cfg_root; + assert(cfg_root); - if (mgmt_txn_get_config(txn, txn_req, ds_ctx) != 0) { + if (mgmt_txn_get_config(txn, txn_req, cfg_root) != 0) { MGMTD_TXN_ERR( "Unable to retrieve config from DS %d txn-id: %" PRIu64 " session-id: %" PRIu64 " req-id: %" PRIu64, @@ -1872,8 +1857,6 @@ static void mgmt_txn_process_get_cfg(struct event *thread) error = true; } - mgmt_txn_process_get_cfg_done: - if (error) { /* * Delete the txn request. @@ -1904,9 +1887,7 @@ static void mgmt_txn_process_get_data(struct event *thread) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; - struct mgmt_ds_ctx *ds_ctx; int num_processed = 0; - bool error; txn = (struct mgmt_txn_ctx *)EVENT_ARG(thread); assert(txn); @@ -1917,54 +1898,23 @@ static void mgmt_txn_process_get_data(struct event *thread) txn->session_id); FOREACH_TXN_REQ_IN_LIST (&txn->get_data_reqs, txn_req) { - error = false; assert(txn_req->req_event == MGMTD_TXN_PROC_GETDATA); - ds_ctx = txn_req->req.get_data->ds_ctx; - if (!ds_ctx) { - mgmt_fe_send_get_data_reply( - txn->session_id, txn->txn_id, - txn_req->req.get_data->ds_id, txn_req->req_id, - MGMTD_INTERNAL_ERROR, NULL, - "No such datastore!"); - error = true; - goto mgmt_txn_process_get_data_done; - } - if (mgmt_ds_is_config(ds_ctx)) { - if (mgmt_txn_get_config(txn, txn_req, ds_ctx) - != 0) { - MGMTD_TXN_ERR( - "Unable to retrieve config from DS %d txn-id %" PRIu64 - " session-id: %" PRIu64 - " req-id: %" PRIu64, - txn_req->req.get_data->ds_id, - txn->txn_id, txn->session_id, - txn_req->req_id); - error = true; - } - } else { - /* - * TODO: Trigger GET procedures for Backend - * For now return back error. - */ - mgmt_fe_send_get_data_reply( - txn->session_id, txn->txn_id, - txn_req->req.get_data->ds_id, txn_req->req_id, - MGMTD_INTERNAL_ERROR, NULL, - "GET-DATA on Oper DS is not supported yet!"); - error = true; - } - - mgmt_txn_process_get_data_done: - - if (error) { - /* - * Delete the txn request. - * Note: The following will remove it from the list - * as well. - */ - mgmt_txn_req_free(&txn_req); - } + /* + * TODO: Trigger GET procedures for Backend + * For now return back error. + */ + mgmt_fe_send_get_data_reply( + txn->session_id, txn->txn_id, + txn_req->req.get_data->ds_id, txn_req->req_id, + MGMTD_INTERNAL_ERROR, NULL, + "GET-DATA on Oper DS is not supported yet!"); + /* + * Delete the txn request. + * Note: The following will remove it from the list + * as well. + */ + mgmt_txn_req_free(&txn_req); /* * Else the transaction would have been already deleted or @@ -2344,12 +2294,12 @@ int mgmt_txn_send_set_config_req(uint64_t txn_id, uint64_t req_id, } int mgmt_txn_send_commit_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId src_ds_id, - struct mgmt_ds_ctx *src_ds_ctx, - Mgmtd__DatastoreId dst_ds_id, - struct mgmt_ds_ctx *dst_ds_ctx, - bool validate_only, bool abort, - bool implicit) + Mgmtd__DatastoreId src_ds_id, + struct mgmt_ds_ctx *src_ds_ctx, + Mgmtd__DatastoreId dst_ds_id, + struct mgmt_ds_ctx *dst_ds_ctx, + bool validate_only, bool abort, + bool implicit) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; @@ -2395,9 +2345,8 @@ int mgmt_txn_notify_be_adapter_conn(struct mgmt_be_client_adapter *adapter, memset(&dummy_stats, 0, sizeof(dummy_stats)); if (connect) { /* Get config for this single backend client */ - mgmt_be_get_adapter_config(adapter, mm->running_ds, - &adapter_cfgs); + mgmt_be_get_adapter_config(adapter, &adapter_cfgs); if (!adapter_cfgs || RB_EMPTY(nb_config_cbs, adapter_cfgs)) { SET_FLAG(adapter->flags, MGMTD_BE_ADAPTER_FLAGS_CFG_SYNCED); @@ -2619,10 +2568,10 @@ int mgmt_txn_notify_be_cfg_apply_reply(uint64_t txn_id, bool success, } int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - Mgmtd__YangGetDataReq **data_req, - size_t num_reqs) + Mgmtd__DatastoreId ds_id, + struct nb_config *cfg_root, + Mgmtd__YangGetDataReq **data_req, + size_t num_reqs) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; @@ -2634,7 +2583,7 @@ int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, txn_req = mgmt_txn_req_alloc(txn, req_id, MGMTD_TXN_PROC_GETCFG); txn_req->req.get_data->ds_id = ds_id; - txn_req->req.get_data->ds_ctx = ds_ctx; + txn_req->req.get_data->cfg_root = cfg_root; for (indx = 0; indx < num_reqs && indx < MGMTD_MAX_NUM_DATA_REPLY_IN_BATCH; indx++) { @@ -2650,10 +2599,9 @@ int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, } int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - Mgmtd__YangGetDataReq **data_req, - size_t num_reqs) + Mgmtd__DatastoreId ds_id, + Mgmtd__YangGetDataReq **data_req, + size_t num_reqs) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; @@ -2665,7 +2613,7 @@ int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id, txn_req = mgmt_txn_req_alloc(txn, req_id, MGMTD_TXN_PROC_GETDATA); txn_req->req.get_data->ds_id = ds_id; - txn_req->req.get_data->ds_ctx = ds_ctx; + txn_req->req.get_data->cfg_root = NULL; for (indx = 0; indx < num_reqs && indx < MGMTD_MAX_NUM_DATA_REPLY_IN_BATCH; indx++) { diff --git a/mgmtd/mgmt_txn.h b/mgmtd/mgmt_txn.h index 1a9f6d8502..69d75fed07 100644 --- a/mgmtd/mgmt_txn.h +++ b/mgmtd/mgmt_txn.h @@ -169,12 +169,12 @@ extern int mgmt_txn_send_set_config_req(uint64_t txn_id, uint64_t req_id, * 0 on success, -1 on failures. */ extern int mgmt_txn_send_commit_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId src_ds_id, - struct mgmt_ds_ctx *dst_ds_ctx, - Mgmtd__DatastoreId dst_ds_id, - struct mgmt_ds_ctx *src_ds_ctx, - bool validate_only, bool abort, - bool implicit); + Mgmtd__DatastoreId src_ds_id, + struct mgmt_ds_ctx *dst_ds_ctx, + Mgmtd__DatastoreId dst_ds_id, + struct mgmt_ds_ctx *src_ds_ctx, + bool validate_only, bool abort, + bool implicit); /* * Send get-config request to be processed later in transaction. @@ -182,10 +182,10 @@ extern int mgmt_txn_send_commit_config_req(uint64_t txn_id, uint64_t req_id, * Similar to set-config request. */ extern int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - Mgmtd__YangGetDataReq **data_req, - size_t num_reqs); + Mgmtd__DatastoreId ds_id, + struct nb_config *cfg_root, + Mgmtd__YangGetDataReq **data_req, + size_t num_reqs); /* * Send get-data request to be processed later in transaction. @@ -194,7 +194,6 @@ extern int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, */ extern int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id, Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, Mgmtd__YangGetDataReq **data_req, size_t num_reqs); From df0173ceeb93572329b04f1bfc5a8925e60513e3 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Sun, 18 Jun 2023 16:19:54 -0400 Subject: [PATCH 4/5] mgmtd: KISS the locking code Move away from things like "lock if not locked" type code, require the user has locked prior to geting to that point. For now we warn if we are taking a lock we already had; however, this should really be a failure point. New requirements: SETCFG - not implicit commit - requires user has locked candidate DS and they must unlock after implicit commit - requires user has locked candidate and running DS both locks will be unlocked on reply to the SETCFG COMMITCFG - requires user has locked candidate and running DS and they must unlock after rollback - this code now get both locks and then does an unlock and early return thing on the adapter side. It needs to be un-special cased in follow up work that would also include tests for this functionality. Signed-off-by: Christian Hopps --- lib/command.c | 5 +- lib/mgmt.proto | 3 +- lib/mgmt_fe_client.c | 1 + lib/mgmt_fe_client.h | 2 +- lib/northbound_cli.c | 4 +- lib/vty.c | 217 +++++++++++++++++++---------------- lib/vty.h | 2 +- mgmtd/mgmt_fe_adapter.c | 245 +++++++++++----------------------------- mgmtd/mgmt_history.c | 36 +++--- mgmtd/mgmt_txn.c | 29 +++-- tests/lib/test_grpc.cpp | 2 +- vtysh/vtysh.c | 5 +- vtysh/vtysh_config.c | 2 +- 13 files changed, 242 insertions(+), 311 deletions(-) diff --git a/lib/command.c b/lib/command.c index 0995637219..8025ab534f 100644 --- a/lib/command.c +++ b/lib/command.c @@ -1333,11 +1333,12 @@ int config_from_file(struct vty *vty, FILE *fp, unsigned int *line_num) /* Configuration from terminal */ DEFUN (config_terminal, config_terminal_cmd, - "configure [terminal]", + "configure [terminal [file-lock]]", "Configuration from vty interface\n" + "Configuration with locked datastores\n" "Configuration terminal\n") { - return vty_config_enter(vty, false, false); + return vty_config_enter(vty, false, false, argc == 3); } /* Enable command */ diff --git a/lib/mgmt.proto b/lib/mgmt.proto index 8a11ff0fa5..ac44eefd9e 100644 --- a/lib/mgmt.proto +++ b/lib/mgmt.proto @@ -243,7 +243,8 @@ message FeSetConfigReply { required DatastoreId ds_id = 2; required uint64 req_id = 3; required bool success = 4; - optional string error_if_any = 5; + required bool implicit_commit = 5; + optional string error_if_any = 6; } message FeCommitConfigReq { diff --git a/lib/mgmt_fe_client.c b/lib/mgmt_fe_client.c index 22d085cbce..45d57175d6 100644 --- a/lib/mgmt_fe_client.c +++ b/lib/mgmt_fe_client.c @@ -411,6 +411,7 @@ static int mgmt_fe_client_handle_msg(struct mgmt_fe_client *client, session->user_ctx, fe_msg->setcfg_reply->req_id, fe_msg->setcfg_reply->success, fe_msg->setcfg_reply->ds_id, + fe_msg->setcfg_reply->implicit_commit, fe_msg->setcfg_reply->error_if_any); break; case MGMTD__FE_MESSAGE__MESSAGE_COMMCFG_REPLY: diff --git a/lib/mgmt_fe_client.h b/lib/mgmt_fe_client.h index e3c016f9b6..532fee4397 100644 --- a/lib/mgmt_fe_client.h +++ b/lib/mgmt_fe_client.h @@ -91,7 +91,7 @@ struct mgmt_fe_client_cbs { uintptr_t session_id, uintptr_t user_session_client, uint64_t req_id, bool success, - Mgmtd__DatastoreId ds_id, + Mgmtd__DatastoreId ds_id, bool implcit_commit, char *errmsg_if_any); void (*commit_config_notify)(struct mgmt_fe_client *client, diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index 9d6ec66689..8003679ed5 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -763,7 +763,7 @@ DEFUN (config_exclusive, "Configuration from vty interface\n" "Configure exclusively from this terminal\n") { - return vty_config_enter(vty, true, true); + return vty_config_enter(vty, true, true, false); } /* Configure using a private candidate configuration. */ @@ -773,7 +773,7 @@ DEFUN (config_private, "Configuration from vty interface\n" "Configure using a private candidate configuration\n") { - return vty_config_enter(vty, true, false); + return vty_config_enter(vty, true, false, false); } DEFPY (config_commit, diff --git a/lib/vty.c b/lib/vty.c index c1f4d7f47d..fc6bed6a0a 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -1676,12 +1676,12 @@ struct vty *vty_new(void) if (!mgmt_client_id_next) mgmt_client_id_next++; new->mgmt_client_id = mgmt_client_id_next++; - if (mgmt_fe_create_client_session( - mgmt_fe_client, new->mgmt_client_id, - (uintptr_t) new) != MGMTD_SUCCESS) - zlog_err( - "Failed to open a MGMTD Frontend session for VTY session %p!!", - new); + new->mgmt_session_id = 0; + mgmt_fe_create_client_session( + mgmt_fe_client, new->mgmt_client_id, (uintptr_t) new); + /* we short-circuit create the session so it must be set now */ + assertf(new->mgmt_session_id != 0, + "Failed to create client session for VTY"); } return new; @@ -2233,6 +2233,9 @@ bool mgmt_vty_read_configs(void) vty->candidate_config = vty_shared_candidate_config; + vty_mgmt_lock_candidate_inline(vty); + vty_mgmt_lock_running_inline(vty); + for (index = 0; index < array_size(mgmt_daemons); index++) { snprintf(path, sizeof(path), "%s/%s.conf", frr_sysconfdir, mgmt_daemons[index]); @@ -2276,6 +2279,14 @@ bool mgmt_vty_read_configs(void) fclose(confp); } + /* Conditionally unlock as the config file may have "exit"d early which + * would then have unlocked things. + */ + if (vty->mgmt_locked_running_ds) + vty_mgmt_unlock_running_inline(vty); + if (vty->mgmt_locked_candidate_ds) + vty_mgmt_unlock_candidate_inline(vty); + vty->pending_allowed = false; if (!count) @@ -2844,9 +2855,11 @@ bool vty_read_config(struct nb_config *config, const char *config_file, return true; } -int vty_config_enter(struct vty *vty, bool private_config, bool exclusive) +int vty_config_enter(struct vty *vty, bool private_config, bool exclusive, + bool file_lock) { - if (exclusive && nb_running_lock(NB_CLIENT_CLI, vty)) { + if (exclusive && !vty_mgmt_fe_enabled() && + nb_running_lock(NB_CLIENT_CLI, vty)) { vty_out(vty, "%% Configuration is locked by other client\n"); return CMD_WARNING; } @@ -2857,20 +2870,20 @@ int vty_config_enter(struct vty *vty, bool private_config, bool exclusive) * message. For user interactive mode we are doing implicit commits * those will obtain the lock (or not) when they try and commit. */ - if (vty_mgmt_fe_enabled() && vty->pending_allowed && !private_config) { - /* - * lock using short-circuit, we set the locked boolean to true - * here so that it can be flipped to false by our locked_notify - * handler during the synchronous call. - */ - vty->mgmt_locked_candidate_ds = true; - if (vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, true, - true) || - !vty->mgmt_locked_candidate_ds) { + if (file_lock && vty_mgmt_fe_enabled() && !private_config) { + if (vty_mgmt_lock_candidate_inline(vty)) { vty_out(vty, "%% Can't enter config; candidate datastore locked by another session\n"); return CMD_WARNING_CONFIG_FAILED; } + if (vty_mgmt_lock_running_inline(vty)) { + vty_out(vty, + "%% Can't enter config; running datastore locked by another session\n"); + vty_mgmt_unlock_candidate_inline(vty); + return CMD_WARNING_CONFIG_FAILED; + } + assert(vty->mgmt_locked_candidate_ds); + assert(vty->mgmt_locked_running_ds); } vty->node = CONFIG_NODE; @@ -2923,18 +2936,15 @@ void vty_config_exit(struct vty *vty) int vty_config_node_exit(struct vty *vty) { - int ret; - vty->xpath_index = 0; - if (vty->mgmt_locked_candidate_ds) { - assert(vty->type != VTY_FILE); - /* use short-circuit call to immediately unlock */ - ret = vty_mgmt_send_lockds_req(vty, MGMTD_DS_CANDIDATE, false, - true); - assert(!ret); - vty->mgmt_locked_candidate_ds = false; - } + /* TODO: could we check for un-commited changes here? */ + + if (vty->mgmt_locked_running_ds) + vty_mgmt_unlock_running_inline(vty); + + if (vty->mgmt_locked_candidate_ds) + vty_mgmt_unlock_candidate_inline(vty); /* Perform any pending commits. */ (void)nb_cli_pending_commit_check(vty); @@ -3537,7 +3547,8 @@ static void vty_mgmt_ds_lock_notified(struct mgmt_fe_client *client, static void vty_mgmt_set_config_result_notified( struct mgmt_fe_client *client, uintptr_t usr_data, uint64_t client_id, uintptr_t session_id, uintptr_t session_ctx, uint64_t req_id, - bool success, Mgmtd__DatastoreId ds_id, char *errmsg_if_any) + bool success, Mgmtd__DatastoreId ds_id, bool implicit_commit, + char *errmsg_if_any) { struct vty *vty; @@ -3555,6 +3566,12 @@ static void vty_mgmt_set_config_result_notified( client_id, req_id); } + if (implicit_commit) { + /* In this case the changes have been applied, we are done */ + vty_mgmt_unlock_candidate_inline(vty); + vty_mgmt_unlock_running_inline(vty); + } + vty_mgmt_resume_response(vty, success); } @@ -3688,7 +3705,6 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit) Mgmtd__YangCfgDataReq cfg_req[VTY_MAXCFGCHANGES]; Mgmtd__YangCfgDataReq *cfgreq[VTY_MAXCFGCHANGES] = {0}; size_t indx; - int cnt; if (vty->type == VTY_FILE) { /* @@ -3701,80 +3717,87 @@ int vty_mgmt_send_config_data(struct vty *vty, bool implicit_commit) return 0; } + /* If we are FE client and we have a vty then we have a session */ + assert(mgmt_fe_client && vty->mgmt_client_id && vty->mgmt_session_id); - if (mgmt_fe_client && vty->mgmt_client_id && !vty->mgmt_session_id) { - /* - * We are connected to mgmtd but we do not yet have an - * established session. this means we need to send any changes - * made during this "down-time" to all backend clients when this - * FE client finishes coming up. - */ - MGMTD_FE_CLIENT_DBG("skipping as no session exists"); + if (!vty->num_cfg_changes) return 0; - } - if (mgmt_fe_client && vty->mgmt_session_id) { - cnt = 0; - for (indx = 0; indx < vty->num_cfg_changes; indx++) { - mgmt_yang_data_init(&cfg_data[cnt]); - - if (vty->cfg_changes[indx].value) { - mgmt_yang_data_value_init(&value[cnt]); - value[cnt].encoded_str_val = - (char *)vty->cfg_changes[indx].value; - value[cnt].value_case = - MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL; - cfg_data[cnt].value = &value[cnt]; - } - - cfg_data[cnt].xpath = vty->cfg_changes[indx].xpath; - - mgmt_yang_cfg_data_req_init(&cfg_req[cnt]); - cfg_req[cnt].data = &cfg_data[cnt]; - switch (vty->cfg_changes[indx].operation) { - case NB_OP_DESTROY: - cfg_req[cnt].req_type = - MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA; - break; - - case NB_OP_CREATE: - case NB_OP_MODIFY: - case NB_OP_MOVE: - case NB_OP_PRE_VALIDATE: - case NB_OP_APPLY_FINISH: - cfg_req[cnt].req_type = - MGMTD__CFG_DATA_REQ_TYPE__SET_DATA; - break; - case NB_OP_GET_ELEM: - case NB_OP_GET_NEXT: - case NB_OP_GET_KEYS: - case NB_OP_LOOKUP_ENTRY: - case NB_OP_RPC: - assert(!"Invalid type of operation"); - break; - default: - assert(!"non-enum value, invalid"); - } - - cfgreq[cnt] = &cfg_req[cnt]; - cnt++; - } - - vty->mgmt_req_id++; - if (cnt && mgmt_fe_send_setcfg_req( - mgmt_fe_client, vty->mgmt_session_id, - vty->mgmt_req_id, MGMTD_DS_CANDIDATE, cfgreq, - cnt, implicit_commit, - MGMTD_DS_RUNNING) != MGMTD_SUCCESS) { - zlog_err("Failed to send %d Config Xpaths to MGMTD!!", - (int)indx); - vty_out(vty, "Failed to send SETCFG-REQ to MGMTD!\n"); + /* grab the candidate and running lock prior to sending implicit commit + * command + */ + if (implicit_commit) { + if (vty_mgmt_lock_candidate_inline(vty)) { + vty_out(vty, + "%% command failed, could not lock candidate DS\n"); + return -1; + } else if (vty_mgmt_lock_running_inline(vty)) { + vty_out(vty, + "%% command failed, could not lock running DS\n"); return -1; } - - vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ"; } + for (indx = 0; indx < vty->num_cfg_changes; indx++) { + mgmt_yang_data_init(&cfg_data[indx]); + + if (vty->cfg_changes[indx].value) { + mgmt_yang_data_value_init(&value[indx]); + value[indx].encoded_str_val = + (char *)vty->cfg_changes[indx].value; + value[indx].value_case = + MGMTD__YANG_DATA_VALUE__VALUE_ENCODED_STR_VAL; + cfg_data[indx].value = &value[indx]; + } + + cfg_data[indx].xpath = vty->cfg_changes[indx].xpath; + + 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: + cfg_req[indx].req_type = + MGMTD__CFG_DATA_REQ_TYPE__DELETE_DATA; + break; + + case NB_OP_CREATE: + case NB_OP_MODIFY: + case NB_OP_MOVE: + case NB_OP_PRE_VALIDATE: + case NB_OP_APPLY_FINISH: + cfg_req[indx].req_type = + MGMTD__CFG_DATA_REQ_TYPE__SET_DATA; + break; + case NB_OP_GET_ELEM: + case NB_OP_GET_NEXT: + case NB_OP_GET_KEYS: + case NB_OP_LOOKUP_ENTRY: + case NB_OP_RPC: + default: + assertf(false, + "Invalid operation type for send config: %d", + vty->cfg_changes[indx].operation); + /*NOTREACHED*/ + abort(); + } + + cfgreq[indx] = &cfg_req[indx]; + } + if (!indx) + return 0; + + vty->mgmt_req_id++; + if (mgmt_fe_send_setcfg_req(mgmt_fe_client, vty->mgmt_session_id, + vty->mgmt_req_id, MGMTD_DS_CANDIDATE, + cfgreq, indx, implicit_commit, + MGMTD_DS_RUNNING)) { + zlog_err("Failed to send %zu config xpaths to mgmtd", indx); + vty_out(vty, "%% Failed to send commands to mgmtd\n"); + return -1; + } + + vty->mgmt_req_pending_cmd = "MESSAGE_SETCFG_REQ"; + return 0; } diff --git a/lib/vty.h b/lib/vty.h index 7e27b52fe1..8fb1483e5b 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -391,7 +391,7 @@ extern void vty_close(struct vty *); extern char *vty_get_cwd(void); extern void vty_update_xpath(const char *oldpath, const char *newpath); extern int vty_config_enter(struct vty *vty, bool private_config, - bool exclusive); + bool exclusive, bool file_lock); extern void vty_config_exit(struct vty *); extern int vty_config_node_exit(struct vty *); extern int vty_shell(struct vty *); diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 1b6f45a05d..70c08d5cb4 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -41,7 +41,6 @@ struct mgmt_fe_session_ctx { uint64_t txn_id; uint64_t cfg_txn_id; uint8_t ds_locked[MGMTD_DS_MAX_ID]; - uint8_t ds_locked_implict[MGMTD_DS_MAX_ID]; struct event *proc_cfg_txn_clnp; struct event *proc_show_txn_clnp; @@ -105,7 +104,6 @@ static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, session->session_id, mgmt_ds_id2name(ds_id)); session->ds_locked[ds_id] = false; - session->ds_locked_implict[ds_id] = false; mgmt_ds_unlock(ds_ctx); MGMTD_FE_ADAPTER_DBG( "Unlocked DS:%s write-locked earlier by session-id: %" PRIu64 @@ -117,21 +115,12 @@ static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, static void mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session) { - Mgmtd__DatastoreId ds_id; - struct mgmt_ds_ctx *ds_ctx; - /* * Ensure any uncommitted changes in Candidate DS * is discarded. */ mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false); - for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { - ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); - if (ds_ctx && session->ds_locked_implict[ds_id]) - mgmt_fe_session_unlock_ds(ds_id, ds_ctx, session); - } - /* * Destroy the actual transaction created earlier. */ @@ -374,6 +363,7 @@ static int mgmt_fe_send_setcfg_reply(struct mgmt_fe_session_ctx *session, setcfg_reply.ds_id = ds_id; setcfg_reply.req_id = req_id; setcfg_reply.success = success; + setcfg_reply.implicit_commit = implicit_commit; if (error_if_any) setcfg_reply.error_if_any = (char *)error_if_any; @@ -612,8 +602,7 @@ static int mgmt_fe_adapter_notify_disconnect(struct msg_conn *conn) } /* - * XXX chopps: get rid of this, we should have deleted sessions when there was a - * disconnect + * Purge any old connections that share the same client name with `adapter` */ static void mgmt_fe_adapter_cleanup_old_conn(struct mgmt_fe_client_adapter *adapter) @@ -621,17 +610,16 @@ mgmt_fe_adapter_cleanup_old_conn(struct mgmt_fe_client_adapter *adapter) struct mgmt_fe_client_adapter *old; FOREACH_ADAPTER_IN_LIST (old) { - if (old != adapter && - !strncmp(adapter->name, old->name, sizeof(adapter->name))) { - /* - * We have a Zombie lingering around - */ - MGMTD_FE_ADAPTER_DBG( - "Client '%s' (FD:%d) seems to have reconnected. Removing old connection (FD:%d)!", - adapter->name, adapter->conn->fd, - old->conn->fd); - msg_conn_disconnect(old->conn, false); - } + if (old == adapter) + continue; + if (strncmp(adapter->name, old->name, sizeof(adapter->name))) + continue; + + MGMTD_FE_ADAPTER_DBG( + "Client '%s' (FD:%d) seems to have reconnected. Removing old connection (FD:%d)", + adapter->name, adapter->conn->fd, + old->conn->fd); + msg_conn_disconnect(old->conn, false); } } @@ -641,16 +629,12 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, { struct mgmt_ds_ctx *ds_ctx; - /* - * Next check first if the SETCFG_REQ is for Candidate DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ - if (lockds_req->ds_id != MGMTD_DS_CANDIDATE) { + if (lockds_req->ds_id != MGMTD_DS_CANDIDATE && + lockds_req->ds_id != MGMTD_DS_RUNNING) { mgmt_fe_send_lockds_reply( session, lockds_req->ds_id, lockds_req->req_id, lockds_req->lock, false, - "Lock/Unlock on datastores other than Candidate DS not permitted!"); + "Lock/Unlock on DS other than candidate or running DS not supported"); return -1; } @@ -673,8 +657,6 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, "Lock already taken on DS by another session!"); return -1; } - - session->ds_locked_implict[lockds_req->ds_id] = false; } else { if (!session->ds_locked[lockds_req->ds_id]) { mgmt_fe_send_lockds_reply( @@ -710,79 +692,49 @@ static int mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, Mgmtd__FeSetConfigReq *setcfg_req) { - uint64_t cfg_session_id; - struct mgmt_ds_ctx *ds_ctx, *dst_ds_ctx; + struct mgmt_ds_ctx *ds_ctx, *dst_ds_ctx = NULL; + bool txn_created = false; if (mm->perf_stats_en) gettimeofday(&session->adapter->setcfg_stats.last_start, NULL); - /* - * Next check first if the SETCFG_REQ is for Candidate DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ + /* MGMTD currently only supports editing the candidate DS. */ if (setcfg_req->ds_id != MGMTD_DS_CANDIDATE) { mgmt_fe_send_setcfg_reply( session, setcfg_req->ds_id, setcfg_req->req_id, false, - "Set-Config on datastores other than Candidate DS not permitted!", + "Set-Config on datastores other than Candidate DS not supported", setcfg_req->implicit_commit); return 0; } - - /* - * Get the DS handle. - */ ds_ctx = mgmt_ds_get_ctx_by_id(mm, setcfg_req->ds_id); - if (!ds_ctx) { + assert(ds_ctx); + + /* MGMTD currently only supports targetting the running DS. */ + if (setcfg_req->implicit_commit && + setcfg_req->commit_ds_id != MGMTD_DS_RUNNING) { mgmt_fe_send_setcfg_reply( session, setcfg_req->ds_id, setcfg_req->req_id, false, - "No such DS exists!", setcfg_req->implicit_commit); + "Implicit commit on datastores other than running DS not supported", + setcfg_req->implicit_commit); + return 0; + } + dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, setcfg_req->commit_ds_id); + assert(dst_ds_ctx); + + /* User should have write lock to change the DS */ + if (!session->ds_locked[setcfg_req->ds_id]) { + mgmt_fe_send_setcfg_reply(session, setcfg_req->ds_id, + setcfg_req->req_id, false, + "Candidate DS is not locked", + setcfg_req->implicit_commit); return 0; } if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) { - /* - * Check first if the current session can run a CONFIG - * transaction or not. Report failure if a CONFIG transaction - * from another session is already in progress. - */ - cfg_session_id = mgmt_config_txn_in_progress(); - if (cfg_session_id != MGMTD_SESSION_ID_NONE) { - assert(cfg_session_id != session->session_id); - mgmt_fe_send_setcfg_reply( - session, setcfg_req->ds_id, setcfg_req->req_id, - false, - "Configuration already in-progress through a different user session!", - setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; - } + /* as we have the lock no-one else should have a config txn */ + assert(mgmt_config_txn_in_progress() == MGMTD_SESSION_ID_NONE); - - /* - * Try taking write-lock on the requested DS (if not already). - */ - if (!session->ds_locked[setcfg_req->ds_id]) { - MGMTD_FE_ADAPTER_ERR( - "SETCFG_REQ on session-id: %" PRIu64 - " without obtaining lock", - session->session_id); - if (mgmt_fe_session_write_lock_ds(setcfg_req->ds_id, - ds_ctx, session) - != 0) { - mgmt_fe_send_setcfg_reply( - session, setcfg_req->ds_id, - setcfg_req->req_id, false, - "Failed to lock the target DS", - setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; - } - - session->ds_locked_implict[setcfg_req->ds_id] = true; - } - - /* - * Start a CONFIG Transaction (if not started already) - */ + /* Start a CONFIG Transaction (if not started already) */ session->cfg_txn_id = mgmt_create_txn(session->session_id, MGMTD_TXN_TYPE_CONFIG); if (session->cfg_txn_id == MGMTD_SESSION_ID_NONE) { @@ -791,14 +743,15 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, false, "Failed to create a Configuration session!", setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; + return 0; } + txn_created = true; MGMTD_FE_ADAPTER_DBG("Created new Config txn-id: %" PRIu64 " for session-id %" PRIu64, session->cfg_txn_id, session->session_id); } else { - MGMTD_FE_ADAPTER_ERR("Config txn-id: %" PRIu64 + MGMTD_FE_ADAPTER_DBG("Config txn-id: %" PRIu64 " for session-id: %" PRIu64 " already created", session->cfg_txn_id, session->session_id); @@ -817,22 +770,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, } } - dst_ds_ctx = 0; - if (setcfg_req->implicit_commit) { - dst_ds_ctx = - mgmt_ds_get_ctx_by_id(mm, setcfg_req->commit_ds_id); - if (!dst_ds_ctx) { - mgmt_fe_send_setcfg_reply( - session, setcfg_req->ds_id, setcfg_req->req_id, - false, "No such commit DS exists!", - setcfg_req->implicit_commit); - return 0; - } - } - - /* - * Create the SETConfig request under the transaction. - */ + /* Create the SETConfig request under the transaction. */ if (mgmt_txn_send_set_config_req( session->cfg_txn_id, setcfg_req->req_id, setcfg_req->ds_id, ds_ctx, setcfg_req->data, setcfg_req->n_data, @@ -843,22 +781,13 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, session, setcfg_req->ds_id, setcfg_req->req_id, false, "Request processing for SET-CONFIG failed!", setcfg_req->implicit_commit); - goto mgmt_fe_sess_handle_setcfg_req_failed; + + /* delete transaction if we just created it */ + if (txn_created) + mgmt_destroy_txn(&session->cfg_txn_id); } return 0; - -mgmt_fe_sess_handle_setcfg_req_failed: - - /* - * Delete transaction created recently. - */ - if (session->cfg_txn_id != MGMTD_TXN_ID_NONE) - mgmt_destroy_txn(&session->cfg_txn_id); - if (ds_ctx && session->ds_locked[setcfg_req->ds_id]) - mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session); - - return 0; } static int @@ -907,7 +836,6 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, " for session-id: %" PRIu64, session->txn_id, session->session_id); } else { - /* XXX chopps: Why would we already have a TXN here? */ MGMTD_FE_ADAPTER_DBG("Show txn-id: %" PRIu64 " for session-id: %" PRIu64 " already created", @@ -1029,43 +957,30 @@ static int mgmt_fe_session_handle_commit_config_req_msg( if (mm->perf_stats_en) gettimeofday(&session->adapter->cmt_stats.last_start, NULL); session->adapter->cmt_stats.commit_cnt++; - /* - * Get the source DS handle. - */ + + /* Validate source and dest DS */ + if (commcfg_req->src_ds_id != MGMTD_DS_CANDIDATE || + commcfg_req->dst_ds_id != MGMTD_DS_RUNNING) { + mgmt_fe_send_commitcfg_reply( + session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, + commcfg_req->req_id, MGMTD_INTERNAL_ERROR, + commcfg_req->validate_only, + "Source/Dest for commit must be candidate/running DS"); + return 0; + } src_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->src_ds_id); - if (!src_ds_ctx) { - mgmt_fe_send_commitcfg_reply( - session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, - commcfg_req->req_id, MGMTD_INTERNAL_ERROR, - commcfg_req->validate_only, - "No such source DS exists!"); - return 0; - } - - /* - * Get the destination DS handle. - */ + assert(src_ds_ctx); dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, commcfg_req->dst_ds_id); - if (!dst_ds_ctx) { - mgmt_fe_send_commitcfg_reply( - session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, - commcfg_req->req_id, MGMTD_INTERNAL_ERROR, - commcfg_req->validate_only, - "No such destination DS exists!"); - return 0; - } + assert(dst_ds_ctx); - /* - * Next check first if the COMMCFG_REQ is for running DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ - if (commcfg_req->dst_ds_id != MGMTD_DS_RUNNING) { + /* User should have lock on both source and dest DS */ + if (!session->ds_locked[commcfg_req->dst_ds_id] || + !session->ds_locked[commcfg_req->src_ds_id]) { mgmt_fe_send_commitcfg_reply( session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, - commcfg_req->req_id, MGMTD_INTERNAL_ERROR, + commcfg_req->req_id, MGMTD_DS_LOCK_FAILED, commcfg_req->validate_only, - "Set-Config on datastores other than Running DS not permitted!"); + "Commit requires lock on candidate and/or running DS"); return 0; } @@ -1090,26 +1005,6 @@ static int mgmt_fe_session_handle_commit_config_req_msg( session->cfg_txn_id, session->session_id); } - - /* - * Try taking write-lock on the destination DS (if not already). - */ - if (!session->ds_locked[commcfg_req->dst_ds_id]) { - if (mgmt_fe_session_write_lock_ds(commcfg_req->dst_ds_id, - dst_ds_ctx, session) - != 0) { - mgmt_fe_send_commitcfg_reply( - session, commcfg_req->src_ds_id, - commcfg_req->dst_ds_id, commcfg_req->req_id, - MGMTD_DS_LOCK_FAILED, - commcfg_req->validate_only, - "Failed to lock the destination DS!"); - return 0; - } - - session->ds_locked_implict[commcfg_req->dst_ds_id] = true; - } - /* * Create COMMITConfig request under the transaction */ @@ -1647,12 +1542,8 @@ void mgmt_fe_adapter_status_write(struct vty *vty, bool detail) FOREACH_MGMTD_DS_ID (ds_id) { if (session->ds_locked[ds_id]) { locked = true; - vty_out(vty, " %s\t\t\t%s\n", - mgmt_ds_id2name(ds_id), - session->ds_locked_implict - [ds_id] - ? "Implicit" - : "Explicit"); + vty_out(vty, " %s\n", + mgmt_ds_id2name(ds_id)); } } if (!locked) diff --git a/mgmtd/mgmt_history.c b/mgmtd/mgmt_history.c index 89a7a60166..d4069325ca 100644 --- a/mgmtd/mgmt_history.c +++ b/mgmtd/mgmt_history.c @@ -196,23 +196,21 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, } src_ds_ctx = mgmt_ds_get_ctx_by_id(mm, MGMTD_DS_CANDIDATE); - if (!src_ds_ctx) { - vty_out(vty, "ERROR: Couldnot access Candidate datastore!\n"); - return -1; - } - - /* - * Note: Write lock on src_ds is not required. This is already - * taken in 'conf te'. - */ dst_ds_ctx = mgmt_ds_get_ctx_by_id(mm, MGMTD_DS_RUNNING); - if (!dst_ds_ctx) { - vty_out(vty, "ERROR: Couldnot access Running datastore!\n"); + assert(src_ds_ctx); + assert(dst_ds_ctx); + + ret = mgmt_ds_lock(src_ds_ctx, vty->mgmt_session_id); + if (ret != 0) { + vty_out(vty, + "Failed to lock the DS %u for rollback Reason: %s!\n", + MGMTD_DS_RUNNING, strerror(ret)); return -1; } ret = mgmt_ds_lock(dst_ds_ctx, vty->mgmt_session_id); if (ret != 0) { + mgmt_ds_unlock(src_ds_ctx); vty_out(vty, "Failed to lock the DS %u for rollback Reason: %s!\n", MGMTD_DS_RUNNING, strerror(ret)); @@ -223,27 +221,28 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, ret = mgmt_ds_load_config_from_file( src_ds_ctx, cmt_info->cmt_json_file, false); if (ret != 0) { - mgmt_ds_unlock(dst_ds_ctx); vty_out(vty, "Error with parsing the file with error code %d\n", ret); - return ret; + goto failed_unlock; } } /* Internally trigger a commit-request. */ ret = mgmt_txn_rollback_trigger_cfg_apply(src_ds_ctx, dst_ds_ctx); if (ret != 0) { - mgmt_ds_unlock(dst_ds_ctx); vty_out(vty, "Error with creating commit apply txn with error code %d\n", ret); - return ret; + goto failed_unlock; } mgmt_history_dump_cmt_record_index(); - /* XXX chopps when does this get unlocked? */ + /* + * TODO: Cleanup: the generic TXN code currently checks for rollback + * and does the unlock when it completes. + */ /* * Block the rollback command from returning till the rollback @@ -253,6 +252,11 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, vty->mgmt_req_pending_cmd = "ROLLBACK"; rollback_vty = vty; return 0; + +failed_unlock: + mgmt_ds_unlock(src_ds_ctx); + mgmt_ds_unlock(dst_ds_ctx); + return ret; } void mgmt_history_rollback_complete(bool success) diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index c1f674556a..de1ffa1a1f 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -687,18 +687,18 @@ static void mgmt_txn_process_set_cfg(struct event *thread) assert(mgmt_txn_reqs_count(&txn->set_cfg_reqs) == 1); assert(txn_req->req.set_cfg->dst_ds_ctx); - ret = mgmt_ds_lock(txn_req->req.set_cfg->dst_ds_ctx, - txn->session_id); - if (ret != 0) { + /* We expect the user to have locked the DST DS */ + if (!mgmt_ds_is_locked(txn_req->req.set_cfg->dst_ds_ctx, + txn->session_id)) { MGMTD_TXN_ERR( - "Failed to lock DS %u txn-id: %" PRIu64 + "DS %u not locked for implicit commit txn-id: %" PRIu64 " session-id: %" PRIu64 " err: %s", txn_req->req.set_cfg->dst_ds_id, txn->txn_id, txn->session_id, strerror(ret)); mgmt_txn_send_commit_cfg_reply( txn, MGMTD_DS_LOCK_FAILED, - "Lock running DS before implicit commit failed!"); + "running DS not locked for implicit commit"); goto mgmt_txn_process_set_cfg_done; } @@ -757,7 +757,12 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, success = (result == MGMTD_SUCCESS || result == MGMTD_NO_CFG_CHANGES); + /* TODO: these replies should not be send if it's a rollback + * b/c right now that is special cased.. that special casing should be + * removed; however... + */ if (!txn->commit_cfg_req->req.commit_cfg.implicit && txn->session_id + && !txn->commit_cfg_req->req.commit_cfg.rollback && mgmt_fe_send_commit_cfg_reply( txn->session_id, txn->txn_id, txn->commit_cfg_req->req.commit_cfg.src_ds_id, @@ -773,6 +778,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.implicit && txn->session_id + && !txn->commit_cfg_req->req.commit_cfg.rollback && mgmt_fe_send_set_cfg_reply( txn->session_id, txn->txn_id, txn->commit_cfg_req->req.commit_cfg.src_ds_id, @@ -787,6 +793,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, if (success) { /* Stop the commit-timeout timer */ + /* XXX why only on success? */ EVENT_OFF(txn->comm_cfg_timeout); create_cmt_info_rec = @@ -833,17 +840,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.rollback) { + mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx); mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); - /* * Resume processing the rollback command. + * + * TODO: there's no good reason to special case rollback, the + * rollback boolean should be passed back to the FE client and it + * can do the right thing. */ mgmt_history_rollback_complete(success); } - if (txn->commit_cfg_req->req.commit_cfg.implicit) - mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); - txn->commit_cfg_req->req.commit_cfg.cmt_stats = NULL; mgmt_txn_req_free(&txn->commit_cfg_req); @@ -2651,10 +2659,11 @@ int mgmt_txn_rollback_trigger_cfg_apply(struct mgmt_ds_ctx *src_ds_ctx, struct mgmt_ds_ctx *dst_ds_ctx) { static struct nb_config_cbs changes; + static struct mgmt_commit_stats dummy_stats; + struct nb_config_cbs *cfg_chgs = NULL; struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; - static struct mgmt_commit_stats dummy_stats; memset(&changes, 0, sizeof(changes)); memset(&dummy_stats, 0, sizeof(dummy_stats)); diff --git a/tests/lib/test_grpc.cpp b/tests/lib/test_grpc.cpp index fd30f04346..87530d41d8 100644 --- a/tests/lib/test_grpc.cpp +++ b/tests/lib/test_grpc.cpp @@ -114,7 +114,7 @@ static void static_startup(void) // Add a route vty = vty_new(); vty->type = vty::VTY_TERM; - vty_config_enter(vty, true, false); + vty_config_enter(vty, true, false, false); auto ret = cmd_execute(vty, "ip route 11.0.0.0/8 Null0", NULL, 0); assert(!ret); diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index c94b47fef5..ee52a98adb 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -2333,8 +2333,9 @@ DEFUNSH(VTYSH_REALLYALL, vtysh_disable, vtysh_disable_cmd, "disable", } DEFUNSH(VTYSH_REALLYALL, vtysh_config_terminal, vtysh_config_terminal_cmd, - "configure [terminal]", + "configure [terminal [file-lock]]", "Configuration from vty interface\n" + "Configuration with locked datastores\n" "Configuration terminal\n") { vty->node = CONFIG_NODE; @@ -2355,7 +2356,7 @@ static int vtysh_exit(struct vty *vty) if (vty->node == CONFIG_NODE) { /* resync in case one of the daemons is somewhere else */ vtysh_execute("end"); - vtysh_execute("configure"); + vtysh_execute("configure terminal file-lock"); } return CMD_SUCCESS; } diff --git a/vtysh/vtysh_config.c b/vtysh/vtysh_config.c index 2949faa427..a5f790bbc6 100644 --- a/vtysh/vtysh_config.c +++ b/vtysh/vtysh_config.c @@ -607,7 +607,7 @@ static int vtysh_read_file(FILE *confp, bool dry_run) vty->node = CONFIG_NODE; vtysh_execute_no_pager("enable"); - vtysh_execute_no_pager("configure terminal"); + vtysh_execute_no_pager("configure terminal file-lock"); if (!dry_run) vtysh_execute_no_pager("XFRR_start_configuration"); From 459848ded75d7327725f3baca05800c0bcf30c3c Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Sun, 18 Jun 2023 13:39:27 -0400 Subject: [PATCH 5/5] mgmtd: fix memleak Use northbound functions for replace and merge when possible, rather than duplicating the code. Signed-off-by: Christian Hopps --- mgmtd/mgmt_ds.c | 52 +++++++++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index 027e306141..a0e610c7c7 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -78,29 +78,20 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx, static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, struct mgmt_ds_ctx *dst) { - struct lyd_node *dst_dnode, *src_dnode; - if (!src || !dst) return -1; MGMTD_DS_DBG("Replacing %s with %s", mgmt_ds_id2name(dst->ds_id), mgmt_ds_id2name(src->ds_id)); - src_dnode = src->config_ds ? src->root.cfg_root->dnode - : dst->root.dnode_root; - dst_dnode = dst->config_ds ? dst->root.cfg_root->dnode - : dst->root.dnode_root; - - if (dst_dnode) - yang_dnode_free(dst_dnode); - - /* Not using nb_config_replace as the oper ds does not contain nb_config - */ - dst_dnode = yang_dnode_dup(src_dnode); - if (dst->config_ds) - dst->root.cfg_root->dnode = dst_dnode; - else - dst->root.dnode_root = dst_dnode; + if (src->config_ds && dst->config_ds) + nb_config_replace(dst->root.cfg_root, src->root.cfg_root, true); + else { + assert(!src->config_ds && !dst->config_ds); + if (dst->root.dnode_root) + yang_dnode_free(dst->root.dnode_root); + dst->root.dnode_root = yang_dnode_dup(src->root.dnode_root); + } if (src->ds_id == MGMTD_DS_CANDIDATE) { /* @@ -110,8 +101,6 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, nb_config_diff_del_changes(&src->root.cfg_root->cfg_chgs); } - /* TODO: Update the versions if nb_config present */ - return 0; } @@ -119,20 +108,21 @@ static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src, struct mgmt_ds_ctx *dst) { int ret; - struct lyd_node **dst_dnode, *src_dnode; if (!src || !dst) return -1; MGMTD_DS_DBG("Merging DS %d with %d", dst->ds_id, src->ds_id); - - src_dnode = src->config_ds ? src->root.cfg_root->dnode - : dst->root.dnode_root; - dst_dnode = dst->config_ds ? &dst->root.cfg_root->dnode - : &dst->root.dnode_root; - ret = lyd_merge_siblings(dst_dnode, src_dnode, 0); + if (src->config_ds && dst->config_ds) + ret = nb_config_merge(dst->root.cfg_root, src->root.cfg_root, + true); + else { + assert(!src->config_ds && !dst->config_ds); + ret = lyd_merge_siblings(&dst->root.dnode_root, + src->root.dnode_root, 0); + } if (ret != 0) { - MGMTD_DS_ERR("lyd_merge() failed with err %d", ret); + MGMTD_DS_ERR("merge failed with err: %d", ret); return ret; } @@ -214,9 +204,11 @@ int mgmt_ds_init(struct mgmt_master *mm) void mgmt_ds_destroy(void) { - /* - * TODO: Free the datastores. - */ + nb_config_free(candidate.root.cfg_root); + candidate.root.cfg_root = NULL; + + yang_dnode_free(oper.root.dnode_root); + oper.root.dnode_root = NULL; } struct mgmt_ds_ctx *mgmt_ds_get_ctx_by_id(struct mgmt_master *mm,