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 be7263f21b..45d57175d6 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, @@ -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 b0ac44bb3e..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, @@ -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/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; }; 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 fd00e11c5f..fc6bed6a0a 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}; @@ -1648,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; @@ -2202,10 +2230,11 @@ 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; + + 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, @@ -2250,10 +2279,15 @@ bool mgmt_vty_read_configs(void) fclose(confp); } - vty->pending_allowed = false; + /* 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->mgmt_locked_candidate_ds = false; - mgmt_candidate_ds_wr_locked = false; + vty->pending_allowed = false; if (!count) vty_close(vty); @@ -2367,7 +2401,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 +2487,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 +2497,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); @@ -2821,28 +2855,35 @@ 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; } - 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 { + /* + * 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 (file_lock && vty_mgmt_fe_enabled() && !private_config) { + if (vty_mgmt_lock_candidate_inline(vty)) { vty_out(vty, - "Candidate DS already locked by different session\n"); - return CMD_WARNING; + "%% Can't enter config; candidate datastore locked by another session\n"); + return CMD_WARNING_CONFIG_FAILED; } - - vty->mgmt_locked_candidate_ds = true; - mgmt_candidate_ds_wr_locked = true; + 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; @@ -2855,23 +2896,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; @@ -2896,21 +2938,13 @@ int vty_config_node_exit(struct vty *vty) { 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; - } + /* TODO: could we check for un-commited changes here? */ - vty->mgmt_locked_candidate_ds = false; - mgmt_candidate_ds_wr_locked = false; - } + 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); @@ -3493,23 +3527,28 @@ 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( 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; @@ -3527,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); } @@ -3632,23 +3677,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; } @@ -3659,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) { /* @@ -3667,86 +3712,92 @@ 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; } + /* 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 3b651d20a2..8fb1483e5b 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) @@ -390,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 *); @@ -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_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..a0e610c7c7 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; @@ -76,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) { /* @@ -108,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; } @@ -117,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; } @@ -212,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, @@ -244,40 +238,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 +301,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 +322,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 +331,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 +353,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 +442,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 +455,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 +467,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 +481,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 e9cbd444e8..70c08d5cb4 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -40,9 +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_implict[MGMTD_DS_MAX_ID]; + uint8_t ds_locked[MGMTD_DS_MAX_ID]; struct event *proc_cfg_txn_clnp; struct event *proc_show_txn_clnp; @@ -72,8 +70,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 +84,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,97 +95,32 @@ 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; + 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 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) { - if (session->ds_locked_implict[ds_id]) - mgmt_fe_session_unlock_ds( - ds_id, ds_ctx, session, true, false); - } - } - /* * Destroy the actual transaction created earlier. */ @@ -194,17 +131,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 +171,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 * @@ -389,6 +319,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 +337,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, @@ -432,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; @@ -670,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) @@ -679,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); } } @@ -699,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; } @@ -731,10 +657,8 @@ 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_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, @@ -742,8 +666,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, @@ -769,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_write_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 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) { @@ -850,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); @@ -876,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, @@ -902,23 +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_write_locked[setcfg_req->ds_id]) - mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session, - true, false); - - return 0; } static int @@ -926,6 +795,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. @@ -938,11 +808,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( @@ -953,27 +819,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) */ @@ -997,13 +842,17 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, 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!"); @@ -1014,14 +863,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; } @@ -1043,28 +891,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) */ @@ -1091,9 +927,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!"); @@ -1110,10 +945,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; } @@ -1126,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; } @@ -1187,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_write_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 */ @@ -1214,8 +1012,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, @@ -1743,18 +1540,10 @@ 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", - mgmt_ds_id2name(ds_id), - session->ds_write_locked[ds_id] - ? "Write" - : "Read", - 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 54eb45fdf4..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_write_lock(dst_ds_ctx); + 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,26 +221,29 @@ 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(); + /* + * 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 * is completed. On rollback completion mgmt_history_rollback_complete() @@ -251,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 e64cbe1425..de1ffa1a1f 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,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_write_lock( - txn_req->req.set_cfg->dst_ds_ctx); - 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; } @@ -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) @@ -754,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, @@ -770,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, @@ -784,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 = @@ -830,27 +840,18 @@ 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.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) - 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)); - txn->commit_cfg_req->req.commit_cfg.cmt_stats = NULL; mgmt_txn_req_free(&txn->commit_cfg_req); @@ -1724,8 +1725,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 +1770,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 +1805,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 +1838,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 +1853,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 +1865,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 +1895,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 +1906,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 +2302,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 +2353,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 +2576,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 +2591,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 +2607,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 +2621,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++) { @@ -2703,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/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); 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");