From df5eda3d8783a3436f43821a2840911d610fd89d Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Thu, 14 May 2020 12:30:34 -0300 Subject: [PATCH] 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. -- 2.39.5