From 1be4decb04be48d73ea90f6feb0f33af25499c19 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 21:23:36 -0300 Subject: [PATCH 1/8] lib: northbound style fixes Signed-off-by: Renato Westphal --- lib/northbound.c | 11 +++++------ lib/northbound.h | 4 ++-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index 18bd4f5fd8..c49aff70be 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -735,7 +735,7 @@ int nb_running_lock(enum nb_client client, const void *user) { int ret = -1; - frr_with_mutex(&running_config_mgmt_lock.mtx) { + frr_with_mutex (&running_config_mgmt_lock.mtx) { if (!running_config_mgmt_lock.locked) { running_config_mgmt_lock.locked = true; running_config_mgmt_lock.owner_client = client; @@ -751,7 +751,7 @@ int nb_running_unlock(enum nb_client client, const void *user) { int ret = -1; - frr_with_mutex(&running_config_mgmt_lock.mtx) { + frr_with_mutex (&running_config_mgmt_lock.mtx) { if (running_config_mgmt_lock.locked && running_config_mgmt_lock.owner_client == client && running_config_mgmt_lock.owner_user == user) { @@ -769,7 +769,7 @@ int nb_running_lock_check(enum nb_client client, const void *user) { int ret = -1; - frr_with_mutex(&running_config_mgmt_lock.mtx) { + frr_with_mutex (&running_config_mgmt_lock.mtx) { if (!running_config_mgmt_lock.locked || (running_config_mgmt_lock.owner_client == client && running_config_mgmt_lock.owner_user == user)) @@ -962,7 +962,6 @@ static int nb_callback_configuration(const enum nb_event event, union nb_resource *resource; int ret = NB_ERR; - if (event == NB_EV_VALIDATE) resource = NULL; else @@ -1014,8 +1013,8 @@ static int nb_callback_configuration(const enum nb_event event, break; default: flog_err(EC_LIB_DEVELOPMENT, - "%s: unknown event (%u) [xpath %s]", - __func__, event, xpath); + "%s: unknown event (%u) [xpath %s]", __func__, + event, xpath); exit(1); } diff --git a/lib/northbound.h b/lib/northbound.h index 84382eeb60..44727ca0df 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -1044,8 +1044,8 @@ extern void *nb_running_unset_entry(const struct lyd_node *dnode); * Returns: * User pointer if found, NULL otherwise. */ -extern void *nb_running_get_entry(const struct lyd_node *dnode, const char *xpath, - bool abort_if_not_found); +extern void *nb_running_get_entry(const struct lyd_node *dnode, + const char *xpath, bool abort_if_not_found); /* * Return a human-readable string representing a northbound event. From 13d6b9c1343a1f925e3ffd7be0938bf1f395b461 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Mon, 27 Apr 2020 13:13:57 -0300 Subject: [PATCH 2/8] lib: introduce the northbound context structure The new northbound context structure contains information about the client performing a configuration transaction. This information will be made available to all configuration callbacks through the args->context parameter. The usefulness of this structure comes from the fact that it can be used as a communication channel (both input and output) between the northbound callbacks and the northbound clients. This can be done through its "client_data" field which contains client-specific data. This should cover some very specific scenarios where a northbound callback should perform an action only if the configuration change is coming from a given client. An example would be sending a PCEP response to a PCE when an SR-TE policy is created or modified through the PCEP northbound client (for that to happen, the northbound callbacks need to have access to the PCEP request ID, which needs to be available). Signed-off-by: Renato Westphal --- lib/libfrr.c | 8 ++-- lib/northbound.c | 93 ++++++++++++++++++++++++---------------- lib/northbound.h | 75 ++++++++++++++++++++++++-------- lib/northbound_cli.c | 32 ++++++++++---- lib/northbound_confd.c | 6 ++- lib/northbound_db.c | 2 +- lib/northbound_grpc.cpp | 12 ++++-- lib/northbound_sysrepo.c | 10 +++-- lib/vty.c | 9 ++-- 9 files changed, 168 insertions(+), 79 deletions(-) diff --git a/lib/libfrr.c b/lib/libfrr.c index ac165f254e..d52cef3fe9 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -902,11 +902,13 @@ static int frr_config_read_in(struct thread *t) * reading the configuration file. */ if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) { + struct nb_context context = {}; int ret; - ret = nb_candidate_commit(vty_shared_candidate_config, - NB_CLIENT_CLI, NULL, true, - "Read configuration file", NULL); + context.client = NB_CLIENT_CLI; + ret = nb_candidate_commit(&context, vty_shared_candidate_config, + true, "Read configuration file", + NULL); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) zlog_err("%s: failed to read configuration file.", __func__); diff --git a/lib/northbound.c b/lib/northbound.c index c49aff70be..e5349ad4eb 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -62,14 +62,15 @@ static struct { */ static bool transaction_in_progress; -static int nb_callback_pre_validate(const struct nb_node *nb_node, +static int nb_callback_pre_validate(struct nb_context *context, + const struct nb_node *nb_node, const struct lyd_node *dnode); -static int nb_callback_configuration(const enum nb_event event, +static int nb_callback_configuration(struct nb_context *context, + const enum nb_event event, struct nb_config_change *change); -static struct nb_transaction *nb_transaction_new(struct nb_config *config, +static struct nb_transaction *nb_transaction_new(struct nb_context *context, + struct nb_config *config, struct nb_config_cbs *changes, - enum nb_client client, - const void *user, const char *comment); static void nb_transaction_free(struct nb_transaction *transaction); static int nb_transaction_process(enum nb_event event, @@ -592,7 +593,8 @@ static int nb_candidate_validate_yang(struct nb_config *candidate) } /* Perform code-level validation using the northbound callbacks. */ -static int nb_candidate_validate_code(struct nb_config *candidate, +static int nb_candidate_validate_code(struct nb_context *context, + struct nb_config *candidate, struct nb_config_cbs *changes) { struct nb_config_cb *cb; @@ -608,7 +610,7 @@ static int nb_candidate_validate_code(struct nb_config *candidate, if (!nb_node->cbs.pre_validate) goto next; - ret = nb_callback_pre_validate(nb_node, child); + ret = nb_callback_pre_validate(context, nb_node, child); if (ret != NB_OK) return NB_ERR_VALIDATION; @@ -621,7 +623,8 @@ static int nb_candidate_validate_code(struct nb_config *candidate, RB_FOREACH (cb, nb_config_cbs, changes) { struct nb_config_change *change = (struct nb_config_change *)cb; - ret = nb_callback_configuration(NB_EV_VALIDATE, change); + ret = nb_callback_configuration(context, NB_EV_VALIDATE, + change); if (ret != NB_OK) return NB_ERR_VALIDATION; } @@ -629,7 +632,8 @@ static int nb_candidate_validate_code(struct nb_config *candidate, return NB_OK; } -int nb_candidate_validate(struct nb_config *candidate) +int nb_candidate_validate(struct nb_context *context, + struct nb_config *candidate) { struct nb_config_cbs changes; int ret; @@ -639,14 +643,14 @@ int nb_candidate_validate(struct nb_config *candidate) RB_INIT(nb_config_cbs, &changes); nb_config_diff(running_config, candidate, &changes); - ret = nb_candidate_validate_code(candidate, &changes); + ret = nb_candidate_validate_code(context, candidate, &changes); nb_config_diff_del_changes(&changes); return ret; } -int nb_candidate_commit_prepare(struct nb_config *candidate, - enum nb_client client, const void *user, +int nb_candidate_commit_prepare(struct nb_context *context, + struct nb_config *candidate, const char *comment, struct nb_transaction **transaction) { @@ -664,7 +668,7 @@ int nb_candidate_commit_prepare(struct nb_config *candidate, if (RB_EMPTY(nb_config_cbs, &changes)) return NB_ERR_NO_CHANGES; - if (nb_candidate_validate_code(candidate, &changes) != NB_OK) { + if (nb_candidate_validate_code(context, candidate, &changes) != NB_OK) { flog_warn(EC_LIB_NB_CANDIDATE_INVALID, "%s: failed to validate candidate configuration", __func__); @@ -673,7 +677,7 @@ int nb_candidate_commit_prepare(struct nb_config *candidate, } *transaction = - nb_transaction_new(candidate, &changes, client, user, comment); + nb_transaction_new(context, candidate, &changes, comment); if (*transaction == NULL) { flog_warn(EC_LIB_NB_TRANSACTION_CREATION_FAILED, "%s: failed to create transaction", __func__); @@ -709,14 +713,14 @@ void nb_candidate_commit_apply(struct nb_transaction *transaction, nb_transaction_free(transaction); } -int nb_candidate_commit(struct nb_config *candidate, enum nb_client client, - const void *user, bool save_transaction, - const char *comment, uint32_t *transaction_id) +int nb_candidate_commit(struct nb_context *context, struct nb_config *candidate, + bool save_transaction, const char *comment, + uint32_t *transaction_id) { struct nb_transaction *transaction = NULL; int ret; - ret = nb_candidate_commit_prepare(candidate, client, user, comment, + ret = nb_candidate_commit_prepare(context, candidate, comment, &transaction); /* * Apply the changes if the preparation phase succeeded. Otherwise abort @@ -801,7 +805,8 @@ static void nb_log_config_callback(const enum nb_event event, value); } -static int nb_callback_create(const struct nb_node *nb_node, +static int nb_callback_create(struct nb_context *context, + const struct nb_node *nb_node, enum nb_event event, const struct lyd_node *dnode, union nb_resource *resource) { @@ -809,13 +814,15 @@ static int nb_callback_create(const struct nb_node *nb_node, nb_log_config_callback(event, NB_OP_CREATE, dnode); + args.context = context; args.event = event; args.dnode = dnode; args.resource = resource; return nb_node->cbs.create(&args); } -static int nb_callback_modify(const struct nb_node *nb_node, +static int nb_callback_modify(struct nb_context *context, + const struct nb_node *nb_node, enum nb_event event, const struct lyd_node *dnode, union nb_resource *resource) { @@ -823,13 +830,15 @@ static int nb_callback_modify(const struct nb_node *nb_node, nb_log_config_callback(event, NB_OP_MODIFY, dnode); + args.context = context; args.event = event; args.dnode = dnode; args.resource = resource; return nb_node->cbs.modify(&args); } -static int nb_callback_destroy(const struct nb_node *nb_node, +static int nb_callback_destroy(struct nb_context *context, + const struct nb_node *nb_node, enum nb_event event, const struct lyd_node *dnode) { @@ -837,24 +846,28 @@ static int nb_callback_destroy(const struct nb_node *nb_node, nb_log_config_callback(event, NB_OP_DESTROY, dnode); + args.context = context; args.event = event; args.dnode = dnode; return nb_node->cbs.destroy(&args); } -static int nb_callback_move(const struct nb_node *nb_node, enum nb_event event, +static int nb_callback_move(struct nb_context *context, + const struct nb_node *nb_node, enum nb_event event, const struct lyd_node *dnode) { struct nb_cb_move_args args = {}; nb_log_config_callback(event, NB_OP_MOVE, dnode); + args.context = context; args.event = event; args.dnode = dnode; return nb_node->cbs.move(&args); } -static int nb_callback_pre_validate(const struct nb_node *nb_node, +static int nb_callback_pre_validate(struct nb_context *context, + const struct nb_node *nb_node, const struct lyd_node *dnode) { struct nb_cb_pre_validate_args args = {}; @@ -865,13 +878,15 @@ static int nb_callback_pre_validate(const struct nb_node *nb_node, return nb_node->cbs.pre_validate(&args); } -static void nb_callback_apply_finish(const struct nb_node *nb_node, +static void nb_callback_apply_finish(struct nb_context *context, + const struct nb_node *nb_node, const struct lyd_node *dnode) { struct nb_cb_apply_finish_args args = {}; nb_log_config_callback(NB_EV_APPLY, NB_OP_APPLY_FINISH, dnode); + args.context = context; args.dnode = dnode; nb_node->cbs.apply_finish(&args); } @@ -952,7 +967,8 @@ int nb_callback_rpc(const struct nb_node *nb_node, const char *xpath, * Call the northbound configuration callback associated to a given * configuration change. */ -static int nb_callback_configuration(const enum nb_event event, +static int nb_callback_configuration(struct nb_context *context, + const enum nb_event event, struct nb_config_change *change) { enum nb_operation operation = change->cb.operation; @@ -969,16 +985,18 @@ static int nb_callback_configuration(const enum nb_event event, switch (operation) { case NB_OP_CREATE: - ret = nb_callback_create(nb_node, event, dnode, resource); + ret = nb_callback_create(context, nb_node, event, dnode, + resource); break; case NB_OP_MODIFY: - ret = nb_callback_modify(nb_node, event, dnode, resource); + ret = nb_callback_modify(context, nb_node, event, dnode, + resource); break; case NB_OP_DESTROY: - ret = nb_callback_destroy(nb_node, event, dnode); + ret = nb_callback_destroy(context, nb_node, event, dnode); break; case NB_OP_MOVE: - ret = nb_callback_move(nb_node, event, dnode); + ret = nb_callback_move(context, nb_node, event, dnode); break; default: yang_dnode_get_path(dnode, xpath, sizeof(xpath)); @@ -1027,13 +1045,14 @@ static int nb_callback_configuration(const enum nb_event event, return ret; } -static struct nb_transaction * -nb_transaction_new(struct nb_config *config, struct nb_config_cbs *changes, - enum nb_client client, const void *user, const char *comment) +static struct nb_transaction *nb_transaction_new(struct nb_context *context, + struct nb_config *config, + struct nb_config_cbs *changes, + const char *comment) { struct nb_transaction *transaction; - if (nb_running_lock_check(client, user)) { + if (nb_running_lock_check(context->client, context->user)) { flog_warn( EC_LIB_NB_TRANSACTION_CREATION_FAILED, "%s: running configuration is locked by another client", @@ -1051,7 +1070,7 @@ nb_transaction_new(struct nb_config *config, struct nb_config_cbs *changes, transaction_in_progress = true; transaction = XCALLOC(MTYPE_TMP, sizeof(*transaction)); - transaction->client = client; + transaction->context = context; if (comment) strlcpy(transaction->comment, comment, sizeof(transaction->comment)); @@ -1086,7 +1105,8 @@ static int nb_transaction_process(enum nb_event event, break; /* Call the appropriate callback. */ - ret = nb_callback_configuration(event, change); + ret = nb_callback_configuration(transaction->context, event, + change); switch (event) { case NB_EV_PREPARE: if (ret != NB_OK) @@ -1199,7 +1219,8 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) /* Call the 'apply_finish' callbacks, sorted by their priorities. */ RB_FOREACH (cb, nb_config_cbs, &cbs) - nb_callback_apply_finish(cb->nb_node, cb->dnode); + nb_callback_apply_finish(transaction->context, cb->nb_node, + cb->dnode); /* Release memory. */ while (!RB_EMPTY(nb_config_cbs, &cbs)) { diff --git a/lib/northbound.h b/lib/northbound.h index 44727ca0df..caa7a9f82d 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -91,6 +91,9 @@ union nb_resource { */ struct nb_cb_create_args { + /* Context of the configuration transaction. */ + struct nb_context *context; + /* * The transaction phase. Refer to the documentation comments of * nb_event for more details. @@ -110,6 +113,9 @@ struct nb_cb_create_args { }; struct nb_cb_modify_args { + /* Context of the configuration transaction. */ + struct nb_context *context; + /* * The transaction phase. Refer to the documentation comments of * nb_event for more details. @@ -129,6 +135,9 @@ struct nb_cb_modify_args { }; struct nb_cb_destroy_args { + /* Context of the configuration transaction. */ + struct nb_context *context; + /* * The transaction phase. Refer to the documentation comments of * nb_event for more details. @@ -140,6 +149,9 @@ struct nb_cb_destroy_args { }; struct nb_cb_move_args { + /* Context of the configuration transaction. */ + struct nb_context *context; + /* * The transaction phase. Refer to the documentation comments of * nb_event for more details. @@ -151,11 +163,17 @@ struct nb_cb_move_args { }; struct nb_cb_pre_validate_args { + /* Context of the configuration transaction. */ + struct nb_context *context; + /* libyang data node associated with the 'pre_validate' callback. */ const struct lyd_node *dnode; }; struct nb_cb_apply_finish_args { + /* Context of the configuration transaction. */ + struct nb_context *context; + /* libyang data node associated with the 'apply_finish' callback. */ const struct lyd_node *dnode; }; @@ -538,6 +556,29 @@ enum nb_client { NB_CLIENT_GRPC, }; +/* Northbound context. */ +struct nb_context { + /* Northbound client. */ + enum nb_client client; + + /* Northbound user (can be NULL). */ + const void *user; + + /* Client-specific data. */ +#if 0 + union { + struct { + } cli; + struct { + } confd; + struct { + } sysrepo; + struct { + } grpc; + } client_data; +#endif +}; + /* Northbound configuration. */ struct nb_config { struct lyd_node *dnode; @@ -564,7 +605,7 @@ struct nb_config_change { /* Northbound configuration transaction. */ struct nb_transaction { - enum nb_client client; + struct nb_context *context; char comment[80]; struct nb_config *config; struct nb_config_cbs changes; @@ -762,28 +803,29 @@ extern int nb_candidate_update(struct nb_config *candidate); * WARNING: the candidate can be modified as part of the validation process * (e.g. add default nodes). * + * context + * Context of the northbound transaction. + * * candidate * Candidate configuration to validate. * * Returns: * NB_OK on success, NB_ERR_VALIDATION otherwise. */ -extern int nb_candidate_validate(struct nb_config *candidate); +extern int nb_candidate_validate(struct nb_context *context, + struct nb_config *candidate); /* * Create a new configuration transaction but do not commit it yet. Only * validate the candidate and prepare all resources required to apply the * configuration changes. * + * context + * Context of the northbound transaction. + * * candidate * Candidate configuration to commit. * - * client - * Northbound client performing the commit. - * - * user - * Northbound user performing the commit (can be NULL). - * * comment * Optional comment describing the commit. * @@ -803,8 +845,8 @@ extern int nb_candidate_validate(struct nb_config *candidate); * the candidate configuration. * - NB_ERR for other errors. */ -extern int nb_candidate_commit_prepare(struct nb_config *candidate, - enum nb_client client, const void *user, +extern int nb_candidate_commit_prepare(struct nb_context *context, + struct nb_config *candidate, const char *comment, struct nb_transaction **transaction); @@ -842,16 +884,13 @@ extern void nb_candidate_commit_apply(struct nb_transaction *transaction, * take into account the results of the preparation phase of multiple managed * entities. * + * context + * Context of the northbound transaction. + * * candidate * Candidate configuration to commit. It's preserved regardless if the commit * operation fails or not. * - * client - * Northbound client performing the commit. - * - * user - * Northbound user performing the commit (can be NULL). - * * save_transaction * Specify whether the transaction should be recorded in the transactions log * or not. @@ -872,8 +911,8 @@ extern void nb_candidate_commit_apply(struct nb_transaction *transaction, * the candidate configuration. * - NB_ERR for other errors. */ -extern int nb_candidate_commit(struct nb_config *candidate, - enum nb_client client, const void *user, +extern int nb_candidate_commit(struct nb_context *context, + struct nb_config *candidate, bool save_transaction, const char *comment, uint32_t *transaction_id); diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index d4467facaf..544b2434eb 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -169,8 +169,12 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) /* Do an implicit "commit" when using the classic CLI mode. */ if (frr_get_cli_mode() == FRR_CLI_CLASSIC) { - ret = nb_candidate_commit(vty->candidate_config, NB_CLIENT_CLI, - vty, false, NULL, NULL); + struct nb_context context = {}; + + context.client = NB_CLIENT_CLI; + context.user = vty; + ret = nb_candidate_commit(&context, vty->candidate_config, + false, NULL, NULL); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) { vty_out(vty, "%% Configuration failed: %s.\n\n", nb_err_name(ret)); @@ -217,12 +221,16 @@ void nb_cli_confirmed_commit_clean(struct vty *vty) int nb_cli_confirmed_commit_rollback(struct vty *vty) { + struct nb_context context = {}; uint32_t transaction_id; int ret; + /* Perform the rollback. */ + context.client = NB_CLIENT_CLI; + context.user = vty; ret = nb_candidate_commit( - vty->confirmed_commit_rollback, NB_CLIENT_CLI, vty, true, + &context, vty->confirmed_commit_rollback, true, "Rollback to previous configuration - confirmed commit has timed out", &transaction_id); if (ret == NB_OK) @@ -252,6 +260,7 @@ static int nb_cli_confirmed_commit_timeout(struct thread *thread) static int nb_cli_commit(struct vty *vty, bool force, unsigned int confirmed_timeout, char *comment) { + struct nb_context context = {}; uint32_t transaction_id = 0; int ret; @@ -295,8 +304,10 @@ static int nb_cli_commit(struct vty *vty, bool force, &vty->t_confirmed_commit_timeout); } - ret = nb_candidate_commit(vty->candidate_config, NB_CLIENT_CLI, vty, - true, comment, &transaction_id); + context.client = NB_CLIENT_CLI; + context.user = vty; + ret = nb_candidate_commit(&context, vty->candidate_config, true, + comment, &transaction_id); /* Map northbound return code to CLI return code. */ switch (ret) { @@ -690,9 +701,12 @@ DEFPY (config_commit_check, "Commit changes into the running configuration\n" "Check if the configuration changes are valid\n") { + struct nb_context context = {}; int ret; - ret = nb_candidate_validate(vty->candidate_config); + context.client = NB_CLIENT_CLI; + context.user = vty; + ret = nb_candidate_validate(&context, vty->candidate_config); if (ret != NB_OK) { vty_out(vty, "%% Failed to validate candidate configuration.\n\n"); @@ -1530,6 +1544,7 @@ DEFPY (show_yang_module_translator, static int nb_cli_rollback_configuration(struct vty *vty, uint32_t transaction_id) { + struct nb_context context = {}; struct nb_config *candidate; char comment[80]; int ret; @@ -1544,8 +1559,9 @@ static int nb_cli_rollback_configuration(struct vty *vty, snprintf(comment, sizeof(comment), "Rollback to transaction %u", transaction_id); - ret = nb_candidate_commit(candidate, NB_CLIENT_CLI, vty, true, comment, - NULL); + context.client = NB_CLIENT_CLI; + context.user = vty; + ret = nb_candidate_commit(&context, candidate, true, comment, NULL); nb_config_free(candidate); switch (ret) { case NB_OK: diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c index 2fc3c81cf2..50daa62e5a 100644 --- a/lib/northbound_confd.c +++ b/lib/northbound_confd.c @@ -285,6 +285,7 @@ frr_confd_cdb_diff_iter(confd_hkeypath_t *kp, enum cdb_iter_op cdb_op, static int frr_confd_cdb_read_cb_prepare(int fd, int *subp, int reslen) { + struct nb_context context = {}; struct nb_config *candidate; struct cdb_iter_args iter_args; int ret; @@ -321,8 +322,9 @@ static int frr_confd_cdb_read_cb_prepare(int fd, int *subp, int reslen) * required to apply them. */ transaction = NULL; - ret = nb_candidate_commit_prepare(candidate, NB_CLIENT_CONFD, NULL, - NULL, &transaction); + context.client = NB_CLIENT_CONFD; + ret = nb_candidate_commit_prepare(&context, candidate, NULL, + &transaction); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) { enum confd_errcode errcode; const char *errmsg; diff --git a/lib/northbound_db.c b/lib/northbound_db.c index 598805b961..244e760b2b 100644 --- a/lib/northbound_db.c +++ b/lib/northbound_db.c @@ -86,7 +86,7 @@ int nb_db_transaction_save(const struct nb_transaction *transaction, if (!ss) goto exit; - client_name = nb_client_name(transaction->client); + client_name = nb_client_name(transaction->context->client); /* Always record configurations in the XML format. */ if (lyd_print_mem(&config_str, transaction->config->dnode, LYD_XML, LYP_FORMAT | LYP_WITHSIBLINGS) diff --git a/lib/northbound_grpc.cpp b/lib/northbound_grpc.cpp index 2962a977eb..b038d10118 100644 --- a/lib/northbound_grpc.cpp +++ b/lib/northbound_grpc.cpp @@ -672,13 +672,17 @@ class NorthboundImpl // Execute the user request. + struct nb_context context = {}; + context.client = NB_CLIENT_GRPC; + switch (phase) { case frr::CommitRequest::VALIDATE: - ret = nb_candidate_validate(candidate->config); + ret = nb_candidate_validate(&context, + candidate->config); break; case frr::CommitRequest::PREPARE: ret = nb_candidate_commit_prepare( - candidate->config, NB_CLIENT_GRPC, NULL, + &context, candidate->config, comment.c_str(), &candidate->transaction); break; @@ -693,8 +697,8 @@ class NorthboundImpl break; case frr::CommitRequest::ALL: ret = nb_candidate_commit( - candidate->config, NB_CLIENT_GRPC, NULL, - true, comment.c_str(), &transaction_id); + &context, candidate->config, true, + comment.c_str(), &transaction_id); break; } diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index b94c939763..4d15c80a2b 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -245,6 +245,7 @@ static int frr_sr_config_change_cb_verify(sr_session_ctx_t *session, sr_change_oper_t sr_op; sr_val_t *sr_old_val, *sr_new_val; char xpath[XPATH_MAXLEN]; + struct nb_context context = {}; struct nb_config *candidate; snprintf(xpath, sizeof(xpath), "/%s:*", module_name); @@ -276,21 +277,22 @@ static int frr_sr_config_change_cb_verify(sr_session_ctx_t *session, } transaction = NULL; + context.client = NB_CLIENT_SYSREPO; if (startup_config) { /* * sysrepod sends the entire startup configuration using a * single event (SR_EV_ENABLED). This means we need to perform * the full two-phase commit protocol in one go here. */ - ret = nb_candidate_commit(candidate, NB_CLIENT_SYSREPO, NULL, - true, NULL, NULL); + ret = nb_candidate_commit(&context, candidate, true, NULL, + NULL); } else { /* * Validate the configuration changes and allocate all resources * required to apply them. */ - ret = nb_candidate_commit_prepare(candidate, NB_CLIENT_SYSREPO, - NULL, NULL, &transaction); + ret = nb_candidate_commit_prepare(&context, candidate, NULL, + &transaction); } /* Map northbound return code to sysrepo return code. */ diff --git a/lib/vty.c b/lib/vty.c index 784f9cf2ac..146666ec6d 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -2349,9 +2349,12 @@ static void vty_read_file(struct nb_config *config, FILE *confp) * reading the configuration file. */ if (config == NULL) { - ret = nb_candidate_commit(vty->candidate_config, NB_CLIENT_CLI, - vty, true, "Read configuration file", - NULL); + struct nb_context context = {}; + + context.client = NB_CLIENT_CLI; + context.user = vty; + ret = nb_candidate_commit(&context, vty->candidate_config, true, + "Read configuration file", NULL); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) zlog_err("%s: failed to read configuration file.", __func__); From df5eda3d8783a3436f43821a2840911d610fd89d Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 12:30:34 -0300 Subject: [PATCH 3/8] lib: return human-readable error messages to the northbound clients Instead of returning only error codes (e.g. NB_ERR_VALIDATION) to the northbound clients, do better than that and also return a human-readable error message. This should make FRR more automation-friendly since operators won't need to dig into system logs to find out what went wrong in the case of an error. Signed-off-by: Renato Westphal --- lib/libfrr.c | 10 ++- lib/northbound.c | 168 +++++++++++++++++++++++++-------------- lib/northbound.h | 63 ++++++++++++++- lib/northbound_cli.c | 79 +++++++++--------- lib/northbound_confd.c | 9 +-- lib/northbound_grpc.cpp | 89 +++++++++------------ lib/northbound_sysrepo.c | 18 ++++- lib/vty.c | 9 ++- lib/yang.c | 28 +++++++ lib/yang.h | 18 +++++ 10 files changed, 323 insertions(+), 168 deletions(-) diff --git a/lib/libfrr.c b/lib/libfrr.c index d52cef3fe9..cac9929577 100644 --- a/lib/libfrr.c +++ b/lib/libfrr.c @@ -903,15 +903,17 @@ static int frr_config_read_in(struct thread *t) */ if (frr_get_cli_mode() == FRR_CLI_TRANSACTIONAL) { struct nb_context context = {}; + char errmsg[BUFSIZ] = {0}; int ret; context.client = NB_CLIENT_CLI; ret = nb_candidate_commit(&context, vty_shared_candidate_config, - true, "Read configuration file", - NULL); + true, "Read configuration file", NULL, + errmsg, sizeof(errmsg)); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) - zlog_err("%s: failed to read configuration file.", - __func__); + zlog_err( + "%s: failed to read configuration file: %s (%s)", + __func__, nb_err_name(ret), errmsg); } return 0; diff --git a/lib/northbound.c b/lib/northbound.c index e5349ad4eb..14b31ef157 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -64,18 +64,22 @@ static bool transaction_in_progress; static int nb_callback_pre_validate(struct nb_context *context, const struct nb_node *nb_node, - const struct lyd_node *dnode); + const struct lyd_node *dnode, char *errmsg, + size_t errmsg_len); static int nb_callback_configuration(struct nb_context *context, const enum nb_event event, - struct nb_config_change *change); -static struct nb_transaction *nb_transaction_new(struct nb_context *context, - struct nb_config *config, - struct nb_config_cbs *changes, - const char *comment); + struct nb_config_change *change, + char *errmsg, size_t errmsg_len); +static struct nb_transaction * +nb_transaction_new(struct nb_context *context, struct nb_config *config, + struct nb_config_cbs *changes, const char *comment, + char *errmsg, size_t errmsg_len); static void nb_transaction_free(struct nb_transaction *transaction); static int nb_transaction_process(enum nb_event event, - struct nb_transaction *transaction); -static void nb_transaction_apply_finish(struct nb_transaction *transaction); + struct nb_transaction *transaction, + char *errmsg, size_t errmsg_len); +static void nb_transaction_apply_finish(struct nb_transaction *transaction, + char *errmsg, size_t errmsg_len); static int nb_oper_data_iter_node(const struct lys_node *snode, const char *xpath, const void *list_entry, const struct yang_list_keys *list_keys, @@ -581,13 +585,16 @@ int nb_candidate_update(struct nb_config *candidate) * WARNING: lyd_validate() can change the configuration as part of the * validation process. */ -static int nb_candidate_validate_yang(struct nb_config *candidate) +static int nb_candidate_validate_yang(struct nb_config *candidate, char *errmsg, + size_t errmsg_len) { if (lyd_validate(&candidate->dnode, LYD_OPT_STRICT | LYD_OPT_CONFIG | LYD_OPT_WHENAUTODEL, ly_native_ctx) - != 0) + != 0) { + yang_print_errors(ly_native_ctx, errmsg, errmsg_len); return NB_ERR_VALIDATION; + } return NB_OK; } @@ -595,7 +602,8 @@ static int nb_candidate_validate_yang(struct nb_config *candidate) /* Perform code-level validation using the northbound callbacks. */ static int nb_candidate_validate_code(struct nb_context *context, struct nb_config *candidate, - struct nb_config_cbs *changes) + struct nb_config_cbs *changes, + char *errmsg, size_t errmsg_len) { struct nb_config_cb *cb; struct lyd_node *root, *next, *child; @@ -610,7 +618,8 @@ static int nb_candidate_validate_code(struct nb_context *context, if (!nb_node->cbs.pre_validate) goto next; - ret = nb_callback_pre_validate(context, nb_node, child); + ret = nb_callback_pre_validate(context, nb_node, child, + errmsg, errmsg_len); if (ret != NB_OK) return NB_ERR_VALIDATION; @@ -623,8 +632,8 @@ static int nb_candidate_validate_code(struct nb_context *context, RB_FOREACH (cb, nb_config_cbs, changes) { struct nb_config_change *change = (struct nb_config_change *)cb; - ret = nb_callback_configuration(context, NB_EV_VALIDATE, - change); + ret = nb_callback_configuration(context, NB_EV_VALIDATE, change, + errmsg, errmsg_len); if (ret != NB_OK) return NB_ERR_VALIDATION; } @@ -633,17 +642,20 @@ static int nb_candidate_validate_code(struct nb_context *context, } int nb_candidate_validate(struct nb_context *context, - struct nb_config *candidate) + struct nb_config *candidate, char *errmsg, + size_t errmsg_len) { struct nb_config_cbs changes; int ret; - if (nb_candidate_validate_yang(candidate) != NB_OK) + if (nb_candidate_validate_yang(candidate, errmsg, sizeof(errmsg_len)) + != NB_OK) return NB_ERR_VALIDATION; RB_INIT(nb_config_cbs, &changes); nb_config_diff(running_config, candidate, &changes); - ret = nb_candidate_validate_code(context, candidate, &changes); + ret = nb_candidate_validate_code(context, candidate, &changes, errmsg, + errmsg_len); nb_config_diff_del_changes(&changes); return ret; @@ -652,11 +664,13 @@ int nb_candidate_validate(struct nb_context *context, int nb_candidate_commit_prepare(struct nb_context *context, struct nb_config *candidate, const char *comment, - struct nb_transaction **transaction) + struct nb_transaction **transaction, + char *errmsg, size_t errmsg_len) { struct nb_config_cbs changes; - if (nb_candidate_validate_yang(candidate) != NB_OK) { + if (nb_candidate_validate_yang(candidate, errmsg, errmsg_len) + != NB_OK) { flog_warn(EC_LIB_NB_CANDIDATE_INVALID, "%s: failed to validate candidate configuration", __func__); @@ -668,7 +682,9 @@ int nb_candidate_commit_prepare(struct nb_context *context, if (RB_EMPTY(nb_config_cbs, &changes)) return NB_ERR_NO_CHANGES; - if (nb_candidate_validate_code(context, candidate, &changes) != NB_OK) { + if (nb_candidate_validate_code(context, candidate, &changes, errmsg, + errmsg_len) + != NB_OK) { flog_warn(EC_LIB_NB_CANDIDATE_INVALID, "%s: failed to validate candidate configuration", __func__); @@ -676,29 +692,37 @@ int nb_candidate_commit_prepare(struct nb_context *context, return NB_ERR_VALIDATION; } - *transaction = - nb_transaction_new(context, candidate, &changes, comment); + *transaction = nb_transaction_new(context, candidate, &changes, comment, + errmsg, errmsg_len); if (*transaction == NULL) { flog_warn(EC_LIB_NB_TRANSACTION_CREATION_FAILED, - "%s: failed to create transaction", __func__); + "%s: failed to create transaction: %s", __func__, + errmsg); nb_config_diff_del_changes(&changes); return NB_ERR_LOCKED; } - return nb_transaction_process(NB_EV_PREPARE, *transaction); + return nb_transaction_process(NB_EV_PREPARE, *transaction, errmsg, + errmsg_len); } void nb_candidate_commit_abort(struct nb_transaction *transaction) { - (void)nb_transaction_process(NB_EV_ABORT, transaction); + char errmsg[BUFSIZ] = {0}; + + (void)nb_transaction_process(NB_EV_ABORT, transaction, errmsg, + sizeof(errmsg)); nb_transaction_free(transaction); } void nb_candidate_commit_apply(struct nb_transaction *transaction, bool save_transaction, uint32_t *transaction_id) { - (void)nb_transaction_process(NB_EV_APPLY, transaction); - nb_transaction_apply_finish(transaction); + char errmsg[BUFSIZ] = {0}; + + (void)nb_transaction_process(NB_EV_APPLY, transaction, errmsg, + sizeof(errmsg)); + nb_transaction_apply_finish(transaction, errmsg, sizeof(errmsg)); /* Replace running by candidate. */ transaction->config->version++; @@ -715,13 +739,14 @@ void nb_candidate_commit_apply(struct nb_transaction *transaction, int nb_candidate_commit(struct nb_context *context, struct nb_config *candidate, bool save_transaction, const char *comment, - uint32_t *transaction_id) + uint32_t *transaction_id, char *errmsg, + size_t errmsg_len) { struct nb_transaction *transaction = NULL; int ret; ret = nb_candidate_commit_prepare(context, candidate, comment, - &transaction); + &transaction, errmsg, errmsg_len); /* * Apply the changes if the preparation phase succeeded. Otherwise abort * the transaction. @@ -808,7 +833,8 @@ static void nb_log_config_callback(const enum nb_event event, static int nb_callback_create(struct nb_context *context, const struct nb_node *nb_node, enum nb_event event, const struct lyd_node *dnode, - union nb_resource *resource) + union nb_resource *resource, char *errmsg, + size_t errmsg_len) { struct nb_cb_create_args args = {}; @@ -818,13 +844,16 @@ static int nb_callback_create(struct nb_context *context, args.event = event; args.dnode = dnode; args.resource = resource; + args.errmsg = errmsg; + args.errmsg_len = errmsg_len; return nb_node->cbs.create(&args); } static int nb_callback_modify(struct nb_context *context, const struct nb_node *nb_node, enum nb_event event, const struct lyd_node *dnode, - union nb_resource *resource) + union nb_resource *resource, char *errmsg, + size_t errmsg_len) { struct nb_cb_modify_args args = {}; @@ -834,13 +863,16 @@ static int nb_callback_modify(struct nb_context *context, args.event = event; args.dnode = dnode; args.resource = resource; + args.errmsg = errmsg; + args.errmsg_len = errmsg_len; return nb_node->cbs.modify(&args); } static int nb_callback_destroy(struct nb_context *context, const struct nb_node *nb_node, enum nb_event event, - const struct lyd_node *dnode) + const struct lyd_node *dnode, char *errmsg, + size_t errmsg_len) { struct nb_cb_destroy_args args = {}; @@ -849,12 +881,15 @@ static int nb_callback_destroy(struct nb_context *context, args.context = context; args.event = event; args.dnode = dnode; + args.errmsg = errmsg; + args.errmsg_len = errmsg_len; return nb_node->cbs.destroy(&args); } static int nb_callback_move(struct nb_context *context, const struct nb_node *nb_node, enum nb_event event, - const struct lyd_node *dnode) + const struct lyd_node *dnode, char *errmsg, + size_t errmsg_len) { struct nb_cb_move_args args = {}; @@ -863,24 +898,30 @@ static int nb_callback_move(struct nb_context *context, args.context = context; args.event = event; args.dnode = dnode; + args.errmsg = errmsg; + args.errmsg_len = errmsg_len; return nb_node->cbs.move(&args); } static int nb_callback_pre_validate(struct nb_context *context, const struct nb_node *nb_node, - const struct lyd_node *dnode) + const struct lyd_node *dnode, char *errmsg, + size_t errmsg_len) { struct nb_cb_pre_validate_args args = {}; nb_log_config_callback(NB_EV_VALIDATE, NB_OP_PRE_VALIDATE, dnode); args.dnode = dnode; + args.errmsg = errmsg; + args.errmsg_len = errmsg_len; return nb_node->cbs.pre_validate(&args); } static void nb_callback_apply_finish(struct nb_context *context, const struct nb_node *nb_node, - const struct lyd_node *dnode) + const struct lyd_node *dnode, char *errmsg, + size_t errmsg_len) { struct nb_cb_apply_finish_args args = {}; @@ -888,6 +929,8 @@ static void nb_callback_apply_finish(struct nb_context *context, args.context = context; args.dnode = dnode; + args.errmsg = errmsg; + args.errmsg_len = errmsg_len; nb_node->cbs.apply_finish(&args); } @@ -969,7 +1012,8 @@ int nb_callback_rpc(const struct nb_node *nb_node, const char *xpath, */ static int nb_callback_configuration(struct nb_context *context, const enum nb_event event, - struct nb_config_change *change) + struct nb_config_change *change, + char *errmsg, size_t errmsg_len) { enum nb_operation operation = change->cb.operation; char xpath[XPATH_MAXLEN]; @@ -986,17 +1030,19 @@ static int nb_callback_configuration(struct nb_context *context, switch (operation) { case NB_OP_CREATE: ret = nb_callback_create(context, nb_node, event, dnode, - resource); + resource, errmsg, errmsg_len); break; case NB_OP_MODIFY: ret = nb_callback_modify(context, nb_node, event, dnode, - resource); + resource, errmsg, errmsg_len); break; case NB_OP_DESTROY: - ret = nb_callback_destroy(context, nb_node, event, dnode); + ret = nb_callback_destroy(context, nb_node, event, dnode, + errmsg, errmsg_len); break; case NB_OP_MOVE: - ret = nb_callback_move(context, nb_node, event, dnode); + ret = nb_callback_move(context, nb_node, event, dnode, errmsg, + errmsg_len); break; default: yang_dnode_get_path(dnode, xpath, sizeof(xpath)); @@ -1037,34 +1083,36 @@ static int nb_callback_configuration(struct nb_context *context, } flog(priority, ref, - "%s: error processing configuration change: error [%s] event [%s] operation [%s] xpath [%s]", - __func__, nb_err_name(ret), nb_event_name(event), + "error processing configuration change: error [%s] event [%s] operation [%s] xpath [%s]", + nb_err_name(ret), nb_event_name(event), nb_operation_name(operation), xpath); + if (strlen(errmsg) > 0) + flog(priority, ref, + "error processing configuration change: %s", + errmsg); } return ret; } -static struct nb_transaction *nb_transaction_new(struct nb_context *context, - struct nb_config *config, - struct nb_config_cbs *changes, - const char *comment) +static struct nb_transaction * +nb_transaction_new(struct nb_context *context, struct nb_config *config, + struct nb_config_cbs *changes, const char *comment, + char *errmsg, size_t errmsg_len) { struct nb_transaction *transaction; if (nb_running_lock_check(context->client, context->user)) { - flog_warn( - EC_LIB_NB_TRANSACTION_CREATION_FAILED, - "%s: running configuration is locked by another client", - __func__); + strlcpy(errmsg, + "running configuration is locked by another client", + errmsg_len); return NULL; } if (transaction_in_progress) { - flog_warn( - EC_LIB_NB_TRANSACTION_CREATION_FAILED, - "%s: error - there's already another transaction in progress", - __func__); + strlcpy(errmsg, + "there's already another transaction in progress", + errmsg_len); return NULL; } transaction_in_progress = true; @@ -1089,7 +1137,8 @@ static void nb_transaction_free(struct nb_transaction *transaction) /* Process all configuration changes associated to a transaction. */ static int nb_transaction_process(enum nb_event event, - struct nb_transaction *transaction) + struct nb_transaction *transaction, + char *errmsg, size_t errmsg_len) { struct nb_config_cb *cb; @@ -1106,7 +1155,7 @@ static int nb_transaction_process(enum nb_event event, /* Call the appropriate callback. */ ret = nb_callback_configuration(transaction->context, event, - change); + change, errmsg, errmsg_len); switch (event) { case NB_EV_PREPARE: if (ret != NB_OK) @@ -1160,7 +1209,8 @@ nb_apply_finish_cb_find(struct nb_config_cbs *cbs, } /* Call the 'apply_finish' callbacks. */ -static void nb_transaction_apply_finish(struct nb_transaction *transaction) +static void nb_transaction_apply_finish(struct nb_transaction *transaction, + char *errmsg, size_t errmsg_len) { struct nb_config_cbs cbs; struct nb_config_cb *cb; @@ -1220,7 +1270,7 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction) /* Call the 'apply_finish' callbacks, sorted by their priorities. */ RB_FOREACH (cb, nb_config_cbs, &cbs) nb_callback_apply_finish(transaction->context, cb->nb_node, - cb->dnode); + cb->dnode, errmsg, errmsg_len); /* Release memory. */ while (!RB_EMPTY(nb_config_cbs, &cbs)) { @@ -1935,7 +1985,7 @@ const char *nb_err_name(enum nb_error error) case NB_ERR_LOCKED: return "resource is locked"; case NB_ERR_VALIDATION: - return "validation error"; + return "validation"; case NB_ERR_RESOURCE: return "failed to allocate resource"; case NB_ERR_INCONSISTENCY: diff --git a/lib/northbound.h b/lib/northbound.h index caa7a9f82d..40a97dad6b 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -110,6 +110,12 @@ struct nb_cb_create_args { * resource(s). It's set to NULL when the event is NB_EV_VALIDATE. */ union nb_resource *resource; + + /* Buffer to store human-readable error message in case of error. */ + char *errmsg; + + /* Size of errmsg. */ + size_t errmsg_len; }; struct nb_cb_modify_args { @@ -132,6 +138,12 @@ struct nb_cb_modify_args { * resource(s). It's set to NULL when the event is NB_EV_VALIDATE. */ union nb_resource *resource; + + /* Buffer to store human-readable error message in case of error. */ + char *errmsg; + + /* Size of errmsg. */ + size_t errmsg_len; }; struct nb_cb_destroy_args { @@ -146,6 +158,12 @@ struct nb_cb_destroy_args { /* libyang data node that is being deleted. */ const struct lyd_node *dnode; + + /* Buffer to store human-readable error message in case of error. */ + char *errmsg; + + /* Size of errmsg. */ + size_t errmsg_len; }; struct nb_cb_move_args { @@ -160,6 +178,12 @@ struct nb_cb_move_args { /* libyang data node that is being moved. */ const struct lyd_node *dnode; + + /* Buffer to store human-readable error message in case of error. */ + char *errmsg; + + /* Size of errmsg. */ + size_t errmsg_len; }; struct nb_cb_pre_validate_args { @@ -168,6 +192,12 @@ struct nb_cb_pre_validate_args { /* libyang data node associated with the 'pre_validate' callback. */ const struct lyd_node *dnode; + + /* Buffer to store human-readable error message in case of error. */ + char *errmsg; + + /* Size of errmsg. */ + size_t errmsg_len; }; struct nb_cb_apply_finish_args { @@ -176,6 +206,12 @@ struct nb_cb_apply_finish_args { /* libyang data node associated with the 'apply_finish' callback. */ const struct lyd_node *dnode; + + /* Buffer to store human-readable error message in case of error. */ + char *errmsg; + + /* Size of errmsg. */ + size_t errmsg_len; }; struct nb_cb_get_elem_args { @@ -809,11 +845,18 @@ extern int nb_candidate_update(struct nb_config *candidate); * candidate * Candidate configuration to validate. * + * errmsg + * Buffer to store human-readable error message in case of error. + * + * errmsg_len + * Size of errmsg. + * * Returns: * NB_OK on success, NB_ERR_VALIDATION otherwise. */ extern int nb_candidate_validate(struct nb_context *context, - struct nb_config *candidate); + struct nb_config *candidate, char *errmsg, + size_t errmsg_len); /* * Create a new configuration transaction but do not commit it yet. Only @@ -835,6 +878,12 @@ extern int nb_candidate_validate(struct nb_context *context, * nb_candidate_commit_abort() or committed using * nb_candidate_commit_apply(). * + * errmsg + * Buffer to store human-readable error message in case of error. + * + * errmsg_len + * Size of errmsg. + * * Returns: * - NB_OK on success. * - NB_ERR_NO_CHANGES when the candidate is identical to the running @@ -848,7 +897,8 @@ extern int nb_candidate_validate(struct nb_context *context, extern int nb_candidate_commit_prepare(struct nb_context *context, struct nb_config *candidate, const char *comment, - struct nb_transaction **transaction); + struct nb_transaction **transaction, + char *errmsg, size_t errmsg_len); /* * Abort a previously created configuration transaction, releasing all resources @@ -901,6 +951,12 @@ extern void nb_candidate_commit_apply(struct nb_transaction *transaction, * transaction_id * Optional output parameter providing the ID of the committed transaction. * + * errmsg + * Buffer to store human-readable error message in case of error. + * + * errmsg_len + * Size of errmsg. + * * Returns: * - NB_OK on success. * - NB_ERR_NO_CHANGES when the candidate is identical to the running @@ -914,7 +970,8 @@ extern void nb_candidate_commit_apply(struct nb_transaction *transaction, extern int nb_candidate_commit(struct nb_context *context, struct nb_config *candidate, bool save_transaction, const char *comment, - uint32_t *transaction_id); + uint32_t *transaction_id, char *errmsg, + size_t errmsg_len); /* * Lock the running configuration. diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c index 544b2434eb..105fc83cef 100644 --- a/lib/northbound_cli.c +++ b/lib/northbound_cli.c @@ -46,23 +46,11 @@ struct debug nb_dbg_libyang = {0, "libyang debugging"}; struct nb_config *vty_shared_candidate_config; static struct thread_master *master; -static void vty_show_libyang_errors(struct vty *vty, struct ly_ctx *ly_ctx) +static void vty_show_nb_errors(struct vty *vty, int error, const char *errmsg) { - struct ly_err_item *ei; - const char *path; - - ei = ly_err_first(ly_ctx); - if (!ei) - return; - - for (; ei; ei = ei->next) - vty_out(vty, "%s\n", ei->msg); - - path = ly_errpath(ly_ctx); - if (path) - vty_out(vty, "YANG path: %s\n", path); - - ly_err_clean(ly_ctx, NULL); + vty_out(vty, "Error type: %s\n", nb_err_name(error)); + if (strlen(errmsg) > 0) + vty_out(vty, "Error description: %s\n", errmsg); } void nb_cli_enqueue_change(struct vty *vty, const char *xpath, @@ -158,28 +146,31 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...) } if (error) { + char buf[BUFSIZ]; + /* * Failure to edit the candidate configuration should never * happen in practice, unless there's a bug in the code. When * that happens, log the error but otherwise ignore it. */ vty_out(vty, "%% Failed to edit configuration.\n\n"); - vty_show_libyang_errors(vty, ly_native_ctx); + vty_out(vty, "%s", + yang_print_errors(ly_native_ctx, buf, sizeof(buf))); } /* Do an implicit "commit" when using the classic CLI mode. */ if (frr_get_cli_mode() == FRR_CLI_CLASSIC) { struct nb_context context = {}; + char errmsg[BUFSIZ] = {0}; context.client = NB_CLIENT_CLI; context.user = vty; ret = nb_candidate_commit(&context, vty->candidate_config, - false, NULL, NULL); + false, NULL, NULL, errmsg, + sizeof(errmsg)); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) { - vty_out(vty, "%% Configuration failed: %s.\n\n", - nb_err_name(ret)); - vty_out(vty, - "Please check the logs for more details.\n"); + vty_out(vty, "%% Configuration failed.\n\n"); + vty_show_nb_errors(vty, ret, errmsg); /* Regenerate candidate for consistency. */ nb_config_replace(vty->candidate_config, running_config, @@ -223,22 +214,25 @@ int nb_cli_confirmed_commit_rollback(struct vty *vty) { struct nb_context context = {}; uint32_t transaction_id; + char errmsg[BUFSIZ] = {0}; int ret; - /* Perform the rollback. */ context.client = NB_CLIENT_CLI; context.user = vty; ret = nb_candidate_commit( &context, vty->confirmed_commit_rollback, true, "Rollback to previous configuration - confirmed commit has timed out", - &transaction_id); + &transaction_id, errmsg, sizeof(errmsg)); if (ret == NB_OK) vty_out(vty, "Rollback performed successfully (Transaction ID #%u).\n", transaction_id); - else - vty_out(vty, "Failed to rollback to previous configuration.\n"); + else { + vty_out(vty, + "Failed to rollback to previous configuration.\n\n"); + vty_show_nb_errors(vty, ret, errmsg); + } return ret; } @@ -262,6 +256,7 @@ static int nb_cli_commit(struct vty *vty, bool force, { struct nb_context context = {}; uint32_t transaction_id = 0; + char errmsg[BUFSIZ] = {0}; int ret; /* Check if there's a pending confirmed commit. */ @@ -307,7 +302,8 @@ static int nb_cli_commit(struct vty *vty, bool force, context.client = NB_CLIENT_CLI; context.user = vty; ret = nb_candidate_commit(&context, vty->candidate_config, true, - comment, &transaction_id); + comment, &transaction_id, errmsg, + sizeof(errmsg)); /* Map northbound return code to CLI return code. */ switch (ret) { @@ -323,9 +319,8 @@ static int nb_cli_commit(struct vty *vty, bool force, return CMD_SUCCESS; default: vty_out(vty, - "%% Failed to commit candidate configuration: %s.\n\n", - nb_err_name(ret)); - vty_out(vty, "Please check the logs for more details.\n"); + "%% Failed to commit candidate configuration.\n\n"); + vty_show_nb_errors(vty, ret, errmsg); return CMD_WARNING; } } @@ -339,6 +334,7 @@ static int nb_cli_candidate_load_file(struct vty *vty, struct lyd_node *dnode; struct ly_ctx *ly_ctx; int ly_format; + char buf[BUFSIZ]; switch (format) { case NB_CFG_FMT_CMDS: @@ -361,7 +357,9 @@ static int nb_cli_candidate_load_file(struct vty *vty, flog_warn(EC_LIB_LIBYANG, "%s: lyd_parse_path() failed", __func__); vty_out(vty, "%% Failed to load configuration:\n\n"); - vty_show_libyang_errors(vty, ly_ctx); + vty_out(vty, "%s", + yang_print_errors(ly_native_ctx, buf, + sizeof(buf))); return CMD_WARNING; } if (translator @@ -382,7 +380,8 @@ static int nb_cli_candidate_load_file(struct vty *vty, != NB_OK) { vty_out(vty, "%% Failed to merge the loaded configuration:\n\n"); - vty_show_libyang_errors(vty, ly_native_ctx); + vty_out(vty, "%s", + yang_print_errors(ly_native_ctx, buf, sizeof(buf))); return CMD_WARNING; } @@ -394,6 +393,7 @@ static int nb_cli_candidate_load_transaction(struct vty *vty, bool replace) { struct nb_config *loaded_config; + char buf[BUFSIZ]; loaded_config = nb_db_transaction_load(transaction_id); if (!loaded_config) { @@ -408,7 +408,8 @@ static int nb_cli_candidate_load_transaction(struct vty *vty, != NB_OK) { vty_out(vty, "%% Failed to merge the loaded configuration:\n\n"); - vty_show_libyang_errors(vty, ly_native_ctx); + vty_out(vty, "%s", + yang_print_errors(ly_native_ctx, buf, sizeof(buf))); return CMD_WARNING; } @@ -702,15 +703,17 @@ DEFPY (config_commit_check, "Check if the configuration changes are valid\n") { struct nb_context context = {}; + char errmsg[BUFSIZ] = {0}; int ret; context.client = NB_CLIENT_CLI; context.user = vty; - ret = nb_candidate_validate(&context, vty->candidate_config); + ret = nb_candidate_validate(&context, vty->candidate_config, errmsg, + sizeof(errmsg)); if (ret != NB_OK) { vty_out(vty, "%% Failed to validate candidate configuration.\n\n"); - vty_show_libyang_errors(vty, ly_native_ctx); + vty_show_nb_errors(vty, ret, errmsg); return CMD_WARNING; } @@ -1547,6 +1550,7 @@ static int nb_cli_rollback_configuration(struct vty *vty, struct nb_context context = {}; struct nb_config *candidate; char comment[80]; + char errmsg[BUFSIZ] = {0}; int ret; candidate = nb_db_transaction_load(transaction_id); @@ -1561,7 +1565,8 @@ static int nb_cli_rollback_configuration(struct vty *vty, context.client = NB_CLIENT_CLI; context.user = vty; - ret = nb_candidate_commit(&context, candidate, true, comment, NULL); + ret = nb_candidate_commit(&context, candidate, true, comment, NULL, + errmsg, sizeof(errmsg)); nb_config_free(candidate); switch (ret) { case NB_OK: @@ -1574,7 +1579,7 @@ static int nb_cli_rollback_configuration(struct vty *vty, return CMD_WARNING; default: vty_out(vty, "%% Rollback failed.\n\n"); - vty_out(vty, "Please check the logs for more details.\n"); + vty_show_nb_errors(vty, ret, errmsg); return CMD_WARNING; } } diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c index 50daa62e5a..8d8944aeab 100644 --- a/lib/northbound_confd.c +++ b/lib/northbound_confd.c @@ -288,6 +288,7 @@ static int frr_confd_cdb_read_cb_prepare(int fd, int *subp, int reslen) struct nb_context context = {}; struct nb_config *candidate; struct cdb_iter_args iter_args; + char errmsg[BUFSIZ] = {0}; int ret; candidate = nb_config_dup(running_config); @@ -324,23 +325,19 @@ static int frr_confd_cdb_read_cb_prepare(int fd, int *subp, int reslen) transaction = NULL; context.client = NB_CLIENT_CONFD; ret = nb_candidate_commit_prepare(&context, candidate, NULL, - &transaction); + &transaction, errmsg, sizeof(errmsg)); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) { enum confd_errcode errcode; - const char *errmsg; switch (ret) { case NB_ERR_LOCKED: errcode = CONFD_ERRCODE_IN_USE; - errmsg = "Configuration is locked by another process"; break; case NB_ERR_RESOURCE: errcode = CONFD_ERRCODE_RESOURCE_DENIED; - errmsg = "Failed do allocate resources"; break; default: - errcode = CONFD_ERRCODE_INTERNAL; - errmsg = "Internal error"; + errcode = CONFD_ERRCODE_APPLICATION; break; } diff --git a/lib/northbound_grpc.cpp b/lib/northbound_grpc.cpp index b038d10118..5dbfe877f0 100644 --- a/lib/northbound_grpc.cpp +++ b/lib/northbound_grpc.cpp @@ -674,17 +674,20 @@ class NorthboundImpl // Execute the user request. struct nb_context context = {}; context.client = NB_CLIENT_GRPC; + char errmsg[BUFSIZ] = {0}; switch (phase) { case frr::CommitRequest::VALIDATE: - ret = nb_candidate_validate(&context, - candidate->config); + ret = nb_candidate_validate( + &context, candidate->config, errmsg, + sizeof(errmsg)); break; case frr::CommitRequest::PREPARE: ret = nb_candidate_commit_prepare( &context, candidate->config, comment.c_str(), - &candidate->transaction); + &candidate->transaction, errmsg, + sizeof(errmsg)); break; case frr::CommitRequest::ABORT: nb_candidate_commit_abort( @@ -698,70 +701,50 @@ class NorthboundImpl case frr::CommitRequest::ALL: ret = nb_candidate_commit( &context, candidate->config, true, - comment.c_str(), &transaction_id); + comment.c_str(), &transaction_id, + errmsg, sizeof(errmsg)); break; } - // Map northbound error codes to gRPC error codes. + // Map northbound error codes to gRPC status codes. + grpc::Status status; switch (ret) { + case NB_OK: + status = grpc::Status::OK; + break; case NB_ERR_NO_CHANGES: - tag->responder.Finish( - tag->response, - grpc::Status( - grpc::StatusCode::ABORTED, - "No configuration changes detected"), - tag); - tag->state = FINISH; - return; + status = grpc::Status(grpc::StatusCode::ABORTED, + errmsg); + break; case NB_ERR_LOCKED: - tag->responder.Finish( - tag->response, - grpc::Status( - grpc::StatusCode::UNAVAILABLE, - "There's already a transaction in progress"), - tag); - tag->state = FINISH; - return; + status = grpc::Status( + grpc::StatusCode::UNAVAILABLE, errmsg); + break; case NB_ERR_VALIDATION: - tag->responder.Finish( - tag->response, - grpc::Status(grpc::StatusCode:: - INVALID_ARGUMENT, - "Validation error"), - tag); - tag->state = FINISH; - return; + status = grpc::Status( + grpc::StatusCode::INVALID_ARGUMENT, + errmsg); + break; case NB_ERR_RESOURCE: - tag->responder.Finish( - tag->response, - grpc::Status( - grpc::StatusCode:: - RESOURCE_EXHAUSTED, - "Failed do allocate resources"), - tag); - tag->state = FINISH; - return; + status = grpc::Status( + grpc::StatusCode::RESOURCE_EXHAUSTED, + errmsg); + break; case NB_ERR: - tag->responder.Finish( - tag->response, - grpc::Status(grpc::StatusCode::INTERNAL, - "Internal error"), - tag); - tag->state = FINISH; - return; default: + status = grpc::Status( + grpc::StatusCode::INTERNAL, errmsg); break; } + if (ret == NB_OK) { + // Response: uint32 transaction_id = 1; + if (transaction_id) + tag->response.set_transaction_id( + transaction_id); + } - // Response: uint32 transaction_id = 1; - if (transaction_id) - tag->response.set_transaction_id( - transaction_id); - - tag->responder.Finish(tag->response, grpc::Status::OK, - tag); + tag->responder.Finish(tag->response, status, tag); tag->state = FINISH; - break; } case FINISH: diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c index 4d15c80a2b..500203173c 100644 --- a/lib/northbound_sysrepo.c +++ b/lib/northbound_sysrepo.c @@ -247,6 +247,7 @@ static int frr_sr_config_change_cb_verify(sr_session_ctx_t *session, char xpath[XPATH_MAXLEN]; struct nb_context context = {}; struct nb_config *candidate; + char errmsg[BUFSIZ] = {0}; snprintf(xpath, sizeof(xpath), "/%s:*", module_name); ret = sr_get_changes_iter(session, xpath, &it); @@ -284,15 +285,26 @@ static int frr_sr_config_change_cb_verify(sr_session_ctx_t *session, * single event (SR_EV_ENABLED). This means we need to perform * the full two-phase commit protocol in one go here. */ - ret = nb_candidate_commit(&context, candidate, true, NULL, - NULL); + ret = nb_candidate_commit(&context, candidate, true, NULL, NULL, + errmsg, sizeof(errmsg)); + if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) + flog_warn( + EC_LIB_LIBSYSREPO, + "%s: failed to apply startup configuration: %s (%s)", + __func__, nb_err_name(ret), errmsg); } else { /* * Validate the configuration changes and allocate all resources * required to apply them. */ ret = nb_candidate_commit_prepare(&context, candidate, NULL, - &transaction); + &transaction, errmsg, + sizeof(errmsg)); + if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) + flog_warn( + EC_LIB_LIBSYSREPO, + "%s: failed to prepare configuration transaction: %s (%s)", + __func__, nb_err_name(ret), errmsg); } /* Map northbound return code to sysrepo return code. */ diff --git a/lib/vty.c b/lib/vty.c index 146666ec6d..ffef05e4dc 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -2350,14 +2350,17 @@ static void vty_read_file(struct nb_config *config, FILE *confp) */ if (config == NULL) { struct nb_context context = {}; + char errmsg[BUFSIZ] = {0}; context.client = NB_CLIENT_CLI; context.user = vty; ret = nb_candidate_commit(&context, vty->candidate_config, true, - "Read configuration file", NULL); + "Read configuration file", NULL, + errmsg, sizeof(errmsg)); if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) - zlog_err("%s: failed to read configuration file.", - __func__); + zlog_err( + "%s: failed to read configuration file: %s (%s)", + __func__, nb_err_name(ret), errmsg); } vty_close(vty); diff --git a/lib/yang.c b/lib/yang.c index c80bf20306..5fca686c5a 100644 --- a/lib/yang.c +++ b/lib/yang.c @@ -621,6 +621,34 @@ static void ly_log_cb(LY_LOG_LEVEL level, const char *msg, const char *path) zlog(priority, "libyang: %s", msg); } +const char *yang_print_errors(struct ly_ctx *ly_ctx, char *buf, size_t buf_len) +{ + struct ly_err_item *ei; + const char *path; + + ei = ly_err_first(ly_ctx); + if (!ei) + return ""; + + strlcpy(buf, "YANG error(s):\n", buf_len); + for (; ei; ei = ei->next) { + strlcat(buf, " ", buf_len); + strlcat(buf, ei->msg, buf_len); + strlcat(buf, "\n", buf_len); + } + + path = ly_errpath(ly_ctx); + if (path) { + strlcat(buf, " YANG path: ", buf_len); + strlcat(buf, path, buf_len); + strlcat(buf, "\n", buf_len); + } + + ly_err_clean(ly_ctx, NULL); + + return buf; +} + void yang_debugging_set(bool enable) { if (enable) { diff --git a/lib/yang.h b/lib/yang.h index 126521707b..2f3017e8ad 100644 --- a/lib/yang.h +++ b/lib/yang.h @@ -496,6 +496,24 @@ extern struct ly_ctx *yang_ctx_new_setup(bool embedded_modules); */ extern void yang_debugging_set(bool enable); +/* + * Print libyang error messages into the provided buffer. + * + * ly_ctx + * libyang context to operate on. + * + * buf + * Buffer to store the libyang error messages. + * + * buf_len + * Size of buf. + * + * Returns: + * The provided buffer. + */ +extern const char *yang_print_errors(struct ly_ctx *ly_ctx, char *buf, + size_t buf_len); + /* * Initialize the YANG subsystem. Should be called only once during the * daemon initialization process. From 10bdc68f0c13e064dc487b4fdafc7d9124d9d697 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 21:34:12 -0300 Subject: [PATCH 4/8] *: convert northbound callbacks to new error handling model The northbound configuration callbacks should now print error messages to the provided buffer (args->errmsg) instead of logging them directly. This will allow the northbound layer to forward the error messages to the northbound clients in addition to logging them. NOTE: many callbacks are returning errors without providing any error message. This needs to be fixed long term. Signed-off-by: Renato Westphal --- isisd/isis_nb_config.c | 105 ++++++++++++++++++++++++---------------- lib/if.c | 4 +- lib/vrf.c | 4 +- zebra/zebra_nb_config.c | 50 +++++++++++-------- 4 files changed, 97 insertions(+), 66 deletions(-) diff --git a/isisd/isis_nb_config.c b/isisd/isis_nb_config.c index a649e896fa..9633e46415 100644 --- a/isisd/isis_nb_config.c +++ b/isisd/isis_nb_config.c @@ -116,8 +116,8 @@ int isis_instance_area_address_create(struct nb_cb_create_args *args) addr.addr_len = dotformat2buff(buff, net_title); memcpy(addr.area_addr, buff, addr.addr_len); if (addr.area_addr[addr.addr_len - 1] != 0) { - flog_warn( - EC_LIB_NB_CB_CONFIG_VALIDATE, + snprintf( + args->errmsg, args->errmsg_len, "nsel byte (last byte) in area address must be 0"); return NB_ERR_VALIDATION; } @@ -125,8 +125,8 @@ int isis_instance_area_address_create(struct nb_cb_create_args *args) /* Check that the SystemID portions match */ if (memcmp(isis->sysid, GETSYSID((&addr)), ISIS_SYS_ID_LEN)) { - flog_warn( - EC_LIB_NB_CB_CONFIG_VALIDATE, + snprintf( + args->errmsg, args->errmsg_len, "System ID must not change when defining additional area addresses"); return NB_ERR_VALIDATION; } @@ -332,8 +332,8 @@ int isis_instance_lsp_mtu_modify(struct nb_cb_modify_args *args) && circuit->state != C_STATE_UP) continue; if (lsp_mtu > isis_circuit_pdu_size(circuit)) { - flog_warn( - EC_LIB_NB_CB_CONFIG_VALIDATE, + snprintf( + args->errmsg, args->errmsg_len, "ISIS area contains circuit %s, which has a maximum PDU size of %zu", circuit->interface->name, isis_circuit_pdu_size(circuit)); @@ -1047,6 +1047,7 @@ int isis_instance_redistribute_ipv6_metric_modify( */ static int isis_multi_topology_common(enum nb_event event, const struct lyd_node *dnode, + char *errmsg, size_t errmsg_len, const char *topology, bool create) { struct isis_area *area; @@ -1056,8 +1057,8 @@ static int isis_multi_topology_common(enum nb_event event, switch (event) { case NB_EV_VALIDATE: if (mtid == (uint16_t)-1) { - flog_warn(EC_LIB_NB_CB_CONFIG_VALIDATE, - "Unknown topology %s", topology); + snprintf(errmsg, errmsg_len, "Unknown topology %s", + topology); return NB_ERR_VALIDATION; } break; @@ -1100,6 +1101,7 @@ int isis_instance_multi_topology_ipv4_multicast_create( struct nb_cb_create_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv4-multicast", true); } @@ -1107,6 +1109,7 @@ int isis_instance_multi_topology_ipv4_multicast_destroy( struct nb_cb_destroy_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv4-multicast", false); } @@ -1126,15 +1129,17 @@ int isis_instance_multi_topology_ipv4_multicast_overload_modify( int isis_instance_multi_topology_ipv4_management_create( struct nb_cb_create_args *args) { - return isis_multi_topology_common(args->event, args->dnode, "ipv4-mgmt", - true); + return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, + "ipv4-mgmt", true); } int isis_instance_multi_topology_ipv4_management_destroy( struct nb_cb_destroy_args *args) { - return isis_multi_topology_common(args->event, args->dnode, "ipv4-mgmt", - false); + return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, + "ipv4-mgmt", false); } /* @@ -1154,6 +1159,7 @@ int isis_instance_multi_topology_ipv6_unicast_create( struct nb_cb_create_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv6-unicast", true); } @@ -1161,6 +1167,7 @@ int isis_instance_multi_topology_ipv6_unicast_destroy( struct nb_cb_destroy_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv6-unicast", false); } @@ -1181,6 +1188,7 @@ int isis_instance_multi_topology_ipv6_multicast_create( struct nb_cb_create_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv6-multicast", true); } @@ -1188,6 +1196,7 @@ int isis_instance_multi_topology_ipv6_multicast_destroy( struct nb_cb_destroy_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv6-multicast", false); } @@ -1207,15 +1216,17 @@ int isis_instance_multi_topology_ipv6_multicast_overload_modify( int isis_instance_multi_topology_ipv6_management_create( struct nb_cb_create_args *args) { - return isis_multi_topology_common(args->event, args->dnode, "ipv6-mgmt", - true); + return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, + "ipv6-mgmt", true); } int isis_instance_multi_topology_ipv6_management_destroy( struct nb_cb_destroy_args *args) { - return isis_multi_topology_common(args->event, args->dnode, "ipv6-mgmt", - false); + return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, + "ipv6-mgmt", false); } /* @@ -1235,6 +1246,7 @@ int isis_instance_multi_topology_ipv6_dstsrc_create( struct nb_cb_create_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv6-dstsrc", true); } @@ -1242,6 +1254,7 @@ int isis_instance_multi_topology_ipv6_dstsrc_destroy( struct nb_cb_destroy_args *args) { return isis_multi_topology_common(args->event, args->dnode, + args->errmsg, args->errmsg_len, "ipv6-dstsrc", false); } @@ -1721,10 +1734,10 @@ int lib_interface_isis_create(struct nb_cb_create_args *args) min_mtu = DEFAULT_LSP_MTU; #endif /* ifndef FABRICD */ if (actual_mtu < min_mtu) { - flog_warn(EC_LIB_NB_CB_CONFIG_VALIDATE, - "Interface %s has MTU %" PRIu32 - ", minimum MTU for the area is %" PRIu32 "", - ifp->name, actual_mtu, min_mtu); + snprintf(args->errmsg, args->errmsg_len, + "Interface %s has MTU %" PRIu32 + ", minimum MTU for the area is %" PRIu32 "", + ifp->name, actual_mtu, min_mtu); return NB_ERR_VALIDATION; } break; @@ -1801,9 +1814,9 @@ int lib_interface_isis_area_tag_modify(struct nb_cb_modify_args *args) area_tag = yang_dnode_get_string(args->dnode, NULL); if (circuit && circuit->area && circuit->area->area_tag && strcmp(circuit->area->area_tag, area_tag)) { - flog_warn(EC_LIB_NB_CB_CONFIG_VALIDATE, - "ISIS circuit is already defined on %s", - circuit->area->area_tag); + snprintf(args->errmsg, args->errmsg_len, + "ISIS circuit is already defined on %s", + circuit->area->area_tag); return NB_ERR_VALIDATION; } } @@ -1839,9 +1852,9 @@ int lib_interface_isis_circuit_type_modify(struct nb_cb_modify_args *args) if (circuit && circuit->state == C_STATE_UP && circuit->area->is_type != IS_LEVEL_1_AND_2 && circuit->area->is_type != circ_type) { - flog_warn(EC_LIB_NB_CB_CONFIG_VALIDATE, - "Invalid circuit level for area %s", - circuit->area->area_tag); + snprintf(args->errmsg, args->errmsg_len, + "Invalid circuit level for area %s", + circuit->area->area_tag); return NB_ERR_VALIDATION; } break; @@ -2163,16 +2176,16 @@ int lib_interface_isis_network_type_modify(struct nb_cb_modify_args *args) if (!circuit) break; if (circuit->circ_type == CIRCUIT_T_LOOPBACK) { - flog_warn( - EC_LIB_NB_CB_CONFIG_VALIDATE, + snprintf( + args->errmsg, args->errmsg_len, "Cannot change network type on loopback interface"); return NB_ERR_VALIDATION; } if (net_type == CIRCUIT_T_BROADCAST && circuit->state == C_STATE_UP && !if_is_broadcast(circuit->interface)) { - flog_warn( - EC_LIB_NB_CB_CONFIG_VALIDATE, + snprintf( + args->errmsg, args->errmsg_len, "Cannot configure non-broadcast interface for broadcast operation"); return NB_ERR_VALIDATION; } @@ -2208,8 +2221,8 @@ int lib_interface_isis_passive_modify(struct nb_cb_modify_args *args) if (!ifp) return NB_OK; if (if_is_loopback(ifp)) { - flog_warn(EC_LIB_NB_CB_CONFIG_VALIDATE, - "Loopback is always passive"); + snprintf(args->errmsg, args->errmsg_len, + "Loopback is always passive"); return NB_ERR_VALIDATION; } } @@ -2312,7 +2325,8 @@ int lib_interface_isis_disable_three_way_handshake_modify( * /frr-interface:lib/interface/frr-isisd:isis/multi-topology/ipv4-unicast */ static int lib_interface_isis_multi_topology_common( - enum nb_event event, const struct lyd_node *dnode, uint16_t mtid) + enum nb_event event, const struct lyd_node *dnode, char *errmsg, + size_t errmsg_len, uint16_t mtid) { struct isis_circuit *circuit; bool value; @@ -2321,8 +2335,8 @@ static int lib_interface_isis_multi_topology_common( case NB_EV_VALIDATE: circuit = nb_running_get_entry(dnode, NULL, false); if (circuit && circuit->area && circuit->area->oldmetric) { - flog_warn( - EC_LIB_NB_CB_CONFIG_VALIDATE, + snprintf( + errmsg, errmsg_len, "Multi topology IS-IS can only be used with wide metrics"); return NB_ERR_VALIDATION; } @@ -2344,7 +2358,8 @@ int lib_interface_isis_multi_topology_ipv4_unicast_modify( struct nb_cb_modify_args *args) { return lib_interface_isis_multi_topology_common( - args->event, args->dnode, ISIS_MT_IPV4_UNICAST); + args->event, args->dnode, args->errmsg, args->errmsg_len, + ISIS_MT_IPV4_UNICAST); } /* @@ -2355,7 +2370,8 @@ int lib_interface_isis_multi_topology_ipv4_multicast_modify( struct nb_cb_modify_args *args) { return lib_interface_isis_multi_topology_common( - args->event, args->dnode, ISIS_MT_IPV4_MULTICAST); + args->event, args->dnode, args->errmsg, args->errmsg_len, + ISIS_MT_IPV4_MULTICAST); } /* @@ -2366,7 +2382,8 @@ int lib_interface_isis_multi_topology_ipv4_management_modify( struct nb_cb_modify_args *args) { return lib_interface_isis_multi_topology_common( - args->event, args->dnode, ISIS_MT_IPV4_MGMT); + args->event, args->dnode, args->errmsg, args->errmsg_len, + ISIS_MT_IPV4_MGMT); } /* @@ -2377,7 +2394,8 @@ int lib_interface_isis_multi_topology_ipv6_unicast_modify( struct nb_cb_modify_args *args) { return lib_interface_isis_multi_topology_common( - args->event, args->dnode, ISIS_MT_IPV6_UNICAST); + args->event, args->dnode, args->errmsg, args->errmsg_len, + ISIS_MT_IPV6_UNICAST); } /* @@ -2388,7 +2406,8 @@ int lib_interface_isis_multi_topology_ipv6_multicast_modify( struct nb_cb_modify_args *args) { return lib_interface_isis_multi_topology_common( - args->event, args->dnode, ISIS_MT_IPV6_MULTICAST); + args->event, args->dnode, args->errmsg, args->errmsg_len, + ISIS_MT_IPV6_MULTICAST); } /* @@ -2399,7 +2418,8 @@ int lib_interface_isis_multi_topology_ipv6_management_modify( struct nb_cb_modify_args *args) { return lib_interface_isis_multi_topology_common( - args->event, args->dnode, ISIS_MT_IPV6_MGMT); + args->event, args->dnode, args->errmsg, args->errmsg_len, + ISIS_MT_IPV6_MGMT); } /* @@ -2409,5 +2429,6 @@ int lib_interface_isis_multi_topology_ipv6_dstsrc_modify( struct nb_cb_modify_args *args) { return lib_interface_isis_multi_topology_common( - args->event, args->dnode, ISIS_MT_IPV6_DSTSRC); + args->event, args->dnode, args->errmsg, args->errmsg_len, + ISIS_MT_IPV6_DSTSRC); } diff --git a/lib/if.c b/lib/if.c index 9efd298a4f..fbe6f836a3 100644 --- a/lib/if.c +++ b/lib/if.c @@ -1561,8 +1561,8 @@ static int lib_interface_destroy(struct nb_cb_destroy_args *args) case NB_EV_VALIDATE: ifp = nb_running_get_entry(args->dnode, NULL, true); if (CHECK_FLAG(ifp->status, ZEBRA_INTERFACE_ACTIVE)) { - zlog_warn("%s: only inactive interfaces can be deleted", - __func__); + snprintf(args->errmsg, args->errmsg_len, + "only inactive interfaces can be deleted"); return NB_ERR_VALIDATION; } break; diff --git a/lib/vrf.c b/lib/vrf.c index 9df5d19516..fb64589287 100644 --- a/lib/vrf.c +++ b/lib/vrf.c @@ -1082,8 +1082,8 @@ static int lib_vrf_destroy(struct nb_cb_destroy_args *args) case NB_EV_VALIDATE: vrfp = nb_running_get_entry(args->dnode, NULL, true); if (CHECK_FLAG(vrfp->status, VRF_ACTIVE)) { - zlog_debug("%s Only inactive VRFs can be deleted", - __func__); + snprintf(args->errmsg, args->errmsg_len, + "Only inactive VRFs can be deleted"); return NB_ERR_VALIDATION; } break; diff --git a/zebra/zebra_nb_config.c b/zebra/zebra_nb_config.c index 5b87a18a06..948ef51320 100644 --- a/zebra/zebra_nb_config.c +++ b/zebra/zebra_nb_config.c @@ -941,13 +941,15 @@ int lib_interface_zebra_ip_addrs_create(struct nb_cb_create_args *args) case NB_EV_VALIDATE: if (prefix.family == AF_INET && ipv4_martian(&prefix.u.prefix4)) { - zlog_debug("invalid address %s", - prefix2str(&prefix, buf, sizeof(buf))); + snprintf(args->errmsg, args->errmsg_len, + "invalid address %s", + prefix2str(&prefix, buf, sizeof(buf))); return NB_ERR_VALIDATION; } else if (prefix.family == AF_INET6 && ipv6_martian(&prefix.u.prefix6)) { - zlog_debug("invalid address %s", - prefix2str(&prefix, buf, sizeof(buf))); + snprintf(args->errmsg, args->errmsg_len, + "invalid address %s", + prefix2str(&prefix, buf, sizeof(buf))); return NB_ERR_VALIDATION; } break; @@ -982,16 +984,18 @@ int lib_interface_zebra_ip_addrs_destroy(struct nb_cb_destroy_args *args) /* Check current interface address. */ ifc = connected_check_ptp(ifp, &prefix, NULL); if (!ifc) { - zlog_debug("interface %s Can't find address\n", - ifp->name); + snprintf(args->errmsg, args->errmsg_len, + "interface %s Can't find address\n", + ifp->name); return NB_ERR_VALIDATION; } } else if (prefix.family == AF_INET6) { /* Check current interface address. */ ifc = connected_check(ifp, &prefix); if (!ifc) { - zlog_debug("interface can't find address %s", - ifp->name); + snprintf(args->errmsg, args->errmsg_len, + "interface can't find address %s", + ifp->name); return NB_ERR_VALIDATION; } } else @@ -999,7 +1003,8 @@ int lib_interface_zebra_ip_addrs_destroy(struct nb_cb_destroy_args *args) /* This is not configured address. */ if (!CHECK_FLAG(ifc->conf, ZEBRA_IFC_CONFIGURED)) { - zlog_debug("interface %s not configured", ifp->name); + snprintf(args->errmsg, args->errmsg_len, + "interface %s not configured", ifp->name); return NB_ERR_VALIDATION; } @@ -1244,8 +1249,8 @@ int lib_vrf_zebra_ribs_rib_create(struct nb_cb_create_args *args) switch (args->event) { case NB_EV_VALIDATE: if (!zrt) { - zlog_debug("%s: vrf %s table is not found.", __func__, - vrf->name); + snprintf(args->errmsg, args->errmsg_len, + "vrf %s table is not found.", vrf->name); return NB_ERR_VALIDATION; } break; @@ -1376,7 +1381,8 @@ int lib_route_map_entry_match_condition_source_protocol_modify( case NB_EV_VALIDATE: type = yang_dnode_get_string(args->dnode, NULL); if (proto_name2num(type) == -1) { - zlog_warn("%s: invalid protocol: %s", __func__, type); + snprintf(args->errmsg, args->errmsg_len, + "invalid protocol: %s", type); return NB_ERR_VALIDATION; } return NB_OK; @@ -1470,8 +1476,9 @@ int lib_route_map_entry_set_action_source_v4_modify( memset(&p, 0, sizeof(p)); yang_dnode_get_ipv4p(&p, args->dnode, NULL); if (zebra_check_addr(&p) == 0) { - zlog_warn("%s: invalid IPv4 address: %s", __func__, - yang_dnode_get_string(args->dnode, NULL)); + snprintf(args->errmsg, args->errmsg_len, + "invalid IPv4 address: %s", + yang_dnode_get_string(args->dnode, NULL)); return NB_ERR_VALIDATION; } @@ -1482,8 +1489,9 @@ int lib_route_map_entry_set_action_source_v4_modify( break; } if (pif == NULL) { - zlog_warn("%s: is not a local adddress: %s", __func__, - yang_dnode_get_string(args->dnode, NULL)); + snprintf(args->errmsg, args->errmsg_len, + "is not a local adddress: %s", + yang_dnode_get_string(args->dnode, NULL)); return NB_ERR_VALIDATION; } return NB_OK; @@ -1536,8 +1544,9 @@ int lib_route_map_entry_set_action_source_v6_modify( memset(&p, 0, sizeof(p)); yang_dnode_get_ipv6p(&p, args->dnode, NULL); if (zebra_check_addr(&p) == 0) { - zlog_warn("%s: invalid IPv6 address: %s", __func__, - yang_dnode_get_string(args->dnode, NULL)); + snprintf(args->errmsg, args->errmsg_len, + "invalid IPv6 address: %s", + yang_dnode_get_string(args->dnode, NULL)); return NB_ERR_VALIDATION; } @@ -1548,8 +1557,9 @@ int lib_route_map_entry_set_action_source_v6_modify( break; } if (pif == NULL) { - zlog_warn("%s: is not a local adddress: %s", __func__, - yang_dnode_get_string(args->dnode, NULL)); + snprintf(args->errmsg, args->errmsg_len, + "is not a local adddress: %s", + yang_dnode_get_string(args->dnode, NULL)); return NB_ERR_VALIDATION; } return NB_OK; From 789193362bbf3d39bb2750693fa089de22778d88 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 16:46:19 -0300 Subject: [PATCH 5/8] lib: silence -Wformat-truncation warnings in the confd plugin Signed-off-by: Renato Westphal --- lib/northbound_confd.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c index 8d8944aeab..a3aaf02f08 100644 --- a/lib/northbound_confd.c +++ b/lib/northbound_confd.c @@ -611,7 +611,7 @@ static int frr_confd_data_get_elem(struct confd_trans_ctx *tctx, confd_hkeypath_t *kp) { struct nb_node *nb_node; - char xpath[BUFSIZ]; + char xpath[XPATH_MAXLEN]; struct yang_data *data; confd_value_t v; const void *list_entry = NULL; @@ -649,7 +649,7 @@ static int frr_confd_data_get_next(struct confd_trans_ctx *tctx, confd_hkeypath_t *kp, long next) { struct nb_node *nb_node; - char xpath[BUFSIZ]; + char xpath[XPATH_MAXLEN]; struct yang_data *data; const void *parent_list_entry, *nb_next; confd_value_t v[LIST_MAXKEYS]; @@ -757,8 +757,8 @@ static int frr_confd_data_get_object(struct confd_trans_ctx *tctx, { struct nb_node *nb_node; const struct lys_node *child; - char xpath[BUFSIZ]; - char xpath_child[XPATH_MAXLEN]; + char xpath[XPATH_MAXLEN]; + char xpath_child[XPATH_MAXLEN * 2]; struct list *elements; struct yang_data *data; const void *list_entry; @@ -831,7 +831,7 @@ static int frr_confd_data_get_object(struct confd_trans_ctx *tctx, static int frr_confd_data_get_next_object(struct confd_trans_ctx *tctx, confd_hkeypath_t *kp, long next) { - char xpath[BUFSIZ]; + char xpath[XPATH_MAXLEN]; struct nb_node *nb_node; struct list *elements; const void *parent_list_entry; @@ -915,7 +915,7 @@ static int frr_confd_data_get_next_object(struct confd_trans_ctx *tctx, /* Loop through list child nodes. */ LY_TREE_FOR (nb_node->snode->child, child) { struct nb_node *nb_node_child = child->priv; - char xpath_child[XPATH_MAXLEN]; + char xpath_child[XPATH_MAXLEN * 2]; confd_value_t *v; if (nvalues > CONFD_MAX_CHILD_NODES) @@ -1058,7 +1058,7 @@ static int frr_confd_action_execute(struct confd_user_info *uinfo, struct xml_tag *name, confd_hkeypath_t *kp, confd_tag_value_t *params, int nparams) { - char xpath[BUFSIZ]; + char xpath[XPATH_MAXLEN]; struct nb_node *nb_node; struct list *input; struct list *output; @@ -1090,7 +1090,7 @@ static int frr_confd_action_execute(struct confd_user_info *uinfo, /* Process input nodes. */ for (int i = 0; i < nparams; i++) { - char xpath_input[BUFSIZ]; + char xpath_input[XPATH_MAXLEN * 2]; char value_str[YANG_VALUE_MAXLEN]; snprintf(xpath_input, sizeof(xpath_input), "%s/%s", xpath, From 0b3eed388c88576b947b64863d89bf102547cf07 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 16:49:33 -0300 Subject: [PATCH 6/8] lib: raise VTY_MAXCFGCHANGES to accommodate more complex commands Signed-off-by: Renato Westphal --- lib/vty.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vty.h b/lib/vty.h index 694aac3944..5d5676199b 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -43,7 +43,7 @@ extern "C" { #define VTY_MAXHIST 20 #define VTY_MAXDEPTH 8 -#define VTY_MAXCFGCHANGES 8 +#define VTY_MAXCFGCHANGES 16 struct vty_error { char error_buf[VTY_BUFSIZ]; From 0e10aeeb1821729d24871e12d867d6037ebf6012 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 19:30:29 -0300 Subject: [PATCH 7/8] lib: fix issue were a few gRPC RPCs were being logged twice Signed-off-by: Renato Westphal --- lib/northbound_grpc.cpp | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/lib/northbound_grpc.cpp b/lib/northbound_grpc.cpp index 5dbfe877f0..83d7e0ce95 100644 --- a/lib/northbound_grpc.cpp +++ b/lib/northbound_grpc.cpp @@ -175,14 +175,13 @@ class NorthboundImpl void HandleGetCapabilities(RpcState *tag) { - if (nb_dbg_client_grpc) - zlog_debug("received RPC GetCapabilities()"); - switch (tag->state) { case CREATE: REQUEST_RPC(GetCapabilities); tag->state = PROCESS; case PROCESS: { + if (nb_dbg_client_grpc) + zlog_debug("received RPC GetCapabilities()"); // Response: string frr_version = 1; tag->response.set_frr_version(FRR_VERSION); @@ -298,14 +297,14 @@ class NorthboundImpl void HandleCreateCandidate(RpcState *tag) { - if (nb_dbg_client_grpc) - zlog_debug("received RPC CreateCandidate()"); - switch (tag->state) { case CREATE: REQUEST_RPC(CreateCandidate); tag->state = PROCESS; case PROCESS: { + if (nb_dbg_client_grpc) + zlog_debug("received RPC CreateCandidate()"); + struct candidate *candidate = create_candidate(); if (!candidate) { tag->responder.Finish( @@ -756,9 +755,6 @@ class NorthboundImpl HandleListTransactions(RpcState *tag) { - if (nb_dbg_client_grpc) - zlog_debug("received RPC ListTransactions()"); - switch (tag->state) { case CREATE: REQUEST_RPC_STREAMING(ListTransactions); @@ -768,6 +764,9 @@ class NorthboundImpl tag->context); tag->state = PROCESS; case PROCESS: { + if (nb_dbg_client_grpc) + zlog_debug("received RPC ListTransactions()"); + auto list = static_cast> *>( tag->context); @@ -876,14 +875,14 @@ class NorthboundImpl void HandleLockConfig( RpcState *tag) { - if (nb_dbg_client_grpc) - zlog_debug("received RPC LockConfig()"); - switch (tag->state) { case CREATE: REQUEST_RPC(LockConfig); tag->state = PROCESS; case PROCESS: { + if (nb_dbg_client_grpc) + zlog_debug("received RPC LockConfig()"); + if (nb_running_lock(NB_CLIENT_GRPC, NULL)) { tag->responder.Finish( tag->response, @@ -909,14 +908,14 @@ class NorthboundImpl void HandleUnlockConfig(RpcState *tag) { - if (nb_dbg_client_grpc) - zlog_debug("received RPC UnlockConfig()"); - switch (tag->state) { case CREATE: REQUEST_RPC(UnlockConfig); tag->state = PROCESS; case PROCESS: { + if (nb_dbg_client_grpc) + zlog_debug("received RPC UnlockConfig()"); + if (nb_running_unlock(NB_CLIENT_GRPC, NULL)) { tag->responder.Finish( tag->response, From 1abe6c535e89f8b445c7d5808bc7972a6b60236d Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 21:18:26 -0300 Subject: [PATCH 8/8] lib: detect and log unexpected return values from northbound callbacks Each northbound callback has a set of valid return values, some of which might depend on the transaction phase. The valid return values for each callback are documented in the northbound main header. Add some code to detect when a callback returns an unexpected value and log the occurrence. This should help us to identify and fix such problems. Signed-off-by: Renato Westphal --- lib/northbound.c | 140 +++++++++++++++++++++++++++++++++++++++++++++-- lib/northbound.h | 4 ++ 2 files changed, 139 insertions(+), 5 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index 14b31ef157..e31c63606f 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -837,6 +837,8 @@ static int nb_callback_create(struct nb_context *context, size_t errmsg_len) { struct nb_cb_create_args args = {}; + bool unexpected_error = false; + int ret; nb_log_config_callback(event, NB_OP_CREATE, dnode); @@ -846,7 +848,35 @@ static int nb_callback_create(struct nb_context *context, args.resource = resource; args.errmsg = errmsg; args.errmsg_len = errmsg_len; - return nb_node->cbs.create(&args); + ret = nb_node->cbs.create(&args); + + /* Detect and log unexpected errors. */ + switch (ret) { + case NB_OK: + case NB_ERR: + break; + case NB_ERR_VALIDATION: + if (event != NB_EV_VALIDATE) + unexpected_error = true; + break; + case NB_ERR_RESOURCE: + if (event != NB_EV_PREPARE) + unexpected_error = true; + break; + case NB_ERR_INCONSISTENCY: + if (event == NB_EV_VALIDATE) + unexpected_error = true; + break; + default: + unexpected_error = true; + break; + } + if (unexpected_error) + DEBUGD(&nb_dbg_cbs_config, + "northbound callback: unexpected return value: %s", + nb_err_name(ret)); + + return ret; } static int nb_callback_modify(struct nb_context *context, @@ -856,6 +886,8 @@ static int nb_callback_modify(struct nb_context *context, size_t errmsg_len) { struct nb_cb_modify_args args = {}; + bool unexpected_error = false; + int ret; nb_log_config_callback(event, NB_OP_MODIFY, dnode); @@ -865,7 +897,35 @@ static int nb_callback_modify(struct nb_context *context, args.resource = resource; args.errmsg = errmsg; args.errmsg_len = errmsg_len; - return nb_node->cbs.modify(&args); + ret = nb_node->cbs.modify(&args); + + /* Detect and log unexpected errors. */ + switch (ret) { + case NB_OK: + case NB_ERR: + break; + case NB_ERR_VALIDATION: + if (event != NB_EV_VALIDATE) + unexpected_error = true; + break; + case NB_ERR_RESOURCE: + if (event != NB_EV_PREPARE) + unexpected_error = true; + break; + case NB_ERR_INCONSISTENCY: + if (event == NB_EV_VALIDATE) + unexpected_error = true; + break; + default: + unexpected_error = true; + break; + } + if (unexpected_error) + DEBUGD(&nb_dbg_cbs_config, + "northbound callback: unexpected return value: %s", + nb_err_name(ret)); + + return ret; } static int nb_callback_destroy(struct nb_context *context, @@ -875,6 +935,8 @@ static int nb_callback_destroy(struct nb_context *context, size_t errmsg_len) { struct nb_cb_destroy_args args = {}; + bool unexpected_error = false; + int ret; nb_log_config_callback(event, NB_OP_DESTROY, dnode); @@ -883,7 +945,31 @@ static int nb_callback_destroy(struct nb_context *context, args.dnode = dnode; args.errmsg = errmsg; args.errmsg_len = errmsg_len; - return nb_node->cbs.destroy(&args); + ret = nb_node->cbs.destroy(&args); + + /* Detect and log unexpected errors. */ + switch (ret) { + case NB_OK: + case NB_ERR: + break; + case NB_ERR_VALIDATION: + if (event != NB_EV_VALIDATE) + unexpected_error = true; + break; + case NB_ERR_INCONSISTENCY: + if (event == NB_EV_VALIDATE) + unexpected_error = true; + break; + default: + unexpected_error = true; + break; + } + if (unexpected_error) + DEBUGD(&nb_dbg_cbs_config, + "northbound callback: unexpected return value: %s", + nb_err_name(ret)); + + return ret; } static int nb_callback_move(struct nb_context *context, @@ -892,6 +978,8 @@ static int nb_callback_move(struct nb_context *context, size_t errmsg_len) { struct nb_cb_move_args args = {}; + bool unexpected_error = false; + int ret; nb_log_config_callback(event, NB_OP_MOVE, dnode); @@ -900,7 +988,31 @@ static int nb_callback_move(struct nb_context *context, args.dnode = dnode; args.errmsg = errmsg; args.errmsg_len = errmsg_len; - return nb_node->cbs.move(&args); + ret = nb_node->cbs.move(&args); + + /* Detect and log unexpected errors. */ + switch (ret) { + case NB_OK: + case NB_ERR: + break; + case NB_ERR_VALIDATION: + if (event != NB_EV_VALIDATE) + unexpected_error = true; + break; + case NB_ERR_INCONSISTENCY: + if (event == NB_EV_VALIDATE) + unexpected_error = true; + break; + default: + unexpected_error = true; + break; + } + if (unexpected_error) + DEBUGD(&nb_dbg_cbs_config, + "northbound callback: unexpected return value: %s", + nb_err_name(ret)); + + return ret; } static int nb_callback_pre_validate(struct nb_context *context, @@ -909,13 +1021,31 @@ static int nb_callback_pre_validate(struct nb_context *context, size_t errmsg_len) { struct nb_cb_pre_validate_args args = {}; + bool unexpected_error = false; + int ret; nb_log_config_callback(NB_EV_VALIDATE, NB_OP_PRE_VALIDATE, dnode); args.dnode = dnode; args.errmsg = errmsg; args.errmsg_len = errmsg_len; - return nb_node->cbs.pre_validate(&args); + ret = nb_node->cbs.pre_validate(&args); + + /* Detect and log unexpected errors. */ + switch (ret) { + case NB_OK: + case NB_ERR_VALIDATION: + break; + default: + unexpected_error = true; + break; + } + if (unexpected_error) + DEBUGD(&nb_dbg_cbs_config, + "northbound callback: unexpected return value: %s", + nb_err_name(ret)); + + return ret; } static void nb_callback_apply_finish(struct nb_context *context, diff --git a/lib/northbound.h b/lib/northbound.h index 40a97dad6b..c5f20d15be 100644 --- a/lib/northbound.h +++ b/lib/northbound.h @@ -359,6 +359,10 @@ struct nb_callbacks { * args * Refer to the documentation comments of nb_cb_pre_validate_args for * details. + * + * Returns: + * - NB_OK on success. + * - NB_ERR_VALIDATION when a validation error occurred. */ int (*pre_validate)(struct nb_cb_pre_validate_args *args);