From 500ae3f8ddd19d80a4fa4970a799065b08823ae8 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Tue, 7 Nov 2023 19:36:55 +0100 Subject: [PATCH] Fix #125: improve feedback on invalid configure input This is an attempt at improving the error reporting from klish-plugin-infix, or more specifically commit@infix. Previously none of the sysrepo errors were shown, now all the latest errors, as well as a few new ones specific to interfaces have been added. Example (eth0 does not exist): admin@infix-00-00-00:/config/> edit interfaces interface eth0 admin@infix-00-00-00:/config/interfaces/interface/eth0/> leave Error: Mandatory node "type" instance does not exist. (Data location "/ietf-interfaces:interfaces/interface[name='eth0']".) Failed committing candidate to running: Validation failed admin@infix-00-00-00:/config/interfaces/interface/eth0/> set type ethernet admin@infix-00-00-00:/config/interfaces/interface/eth0/> leave Error: Cannot create fixed Ethernet interface eth0, wrong type or name. Please check your changes, try 'diff' and 'do show interfaces'. Signed-off-by: Joachim Wiberg --- src/confd/src/ietf-interfaces.c | 9 +++- src/klish-plugin-infix/src/infix.c | 83 +++++++++++++++++++----------- 2 files changed, 61 insertions(+), 31 deletions(-) diff --git a/src/confd/src/ietf-interfaces.c b/src/confd/src/ietf-interfaces.c index c044d4a6..5f4fb96b 100644 --- a/src/confd/src/ietf-interfaces.c +++ b/src/confd/src/ietf-interfaces.c @@ -933,6 +933,7 @@ static int netdag_gen_vlan(struct dagger *net, struct lyd_node *dif, static int netdag_gen_afspec_add(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif, FILE *ip) { + const char *ifname = lydx_get_cattr(cif, "name"); const char *iftype = lydx_get_cattr(cif, "type"); int err = 0; @@ -944,8 +945,12 @@ static int netdag_gen_afspec_add(struct dagger *net, struct lyd_node *dif, err = netdag_gen_veth(net, NULL, cif, ip); } else if (!strcmp(iftype, "infix-if-type:vlan")) { err = netdag_gen_vlan(net, NULL, cif, ip); + } else if (!strcmp(iftype, "infix-if-type:ethernet")) { + sr_session_set_error_message(net->session, "Cannot create fixed Ethernet interface %s," + " wrong type or name.", ifname); + return -ENOENT; } else { - ERROR("unsupported interface type \"%s\"", iftype); + sr_session_set_error_message(net->session, "Unsupported interface type \"%s\"", iftype); return -ENOSYS; } @@ -1049,7 +1054,7 @@ static sr_error_t netdag_gen_iface(struct dagger *net, fixed = iface_is_phys(ifname) || !strcmp(ifname, "lo"); - DEBUG("%s(%s) %s", ifname, fixed ? "fixed" : "dynamic", + ERROR("%s(%s) %s", ifname, fixed ? "fixed" : "dynamic", (op == LYDX_OP_NONE) ? "mod" : ((op == LYDX_OP_CREATE) ? "add" : "del")); if (op == LYDX_OP_DELETE) { diff --git a/src/klish-plugin-infix/src/infix.c b/src/klish-plugin-infix/src/infix.c index 5454dc4c..d3ea2807 100644 --- a/src/klish-plugin-infix/src/infix.c +++ b/src/klish-plugin-infix/src/infix.c @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -14,9 +15,42 @@ #include +#define ERRMSG "Error: " + const uint8_t kplugin_infix_major = 1; const uint8_t kplugin_infix_minor = 0; + +/* + * Print sysrepo session errors followed by an optional string. + */ +static void emsg(sr_session_ctx_t *sess, const char *fmt, ...) +{ + const sr_error_info_t *err = NULL; + va_list ap; + size_t i; + int rc; + + if (!sess) + goto end; + + rc = sr_session_get_error(sess, &err); + if ((rc != SR_ERR_OK) || !err) + goto end; + + // Show the first error only. Because probably next errors are + // originated from internal sysrepo code but is not from subscribers. +// for (i = 0; i < err->err_count; i++) + for (i = 0; i < (err->err_count < 1 ? err->err_count : 1); i++) + fprintf(stderr, ERRMSG "%s\n", err->err[i].message); +end: + if (fmt) { + va_start(ap, fmt); + vfprintf(stderr, fmt, ap); + va_end(ap); + } +} + int infix_files(kcontext_t *ctx) { const char *path; @@ -25,13 +59,13 @@ int infix_files(kcontext_t *ctx) path = kcontext_script(ctx); if (!path) { - fprintf(stderr, "Error: missing path argument to file search.\n"); + fprintf(stderr, ERRMSG "missing path argument to file search.\n"); return -1; } dir = opendir(path); if (!dir) { - fprintf(stderr, "Error: %s", strerror(errno)); + fprintf(stderr, ERRMSG "%s", strerror(errno)); return -1; } @@ -88,15 +122,11 @@ int infix_copy(kcontext_t *ctx) goto err; if (infix_ds_from_str(kparg_value(srcarg), &srcds)) { - fprintf(stderr, - "Error: \"%s\" is not the name of any known datastore\n", - kparg_value(srcarg)); + fprintf(stderr, ERRMSG "\"%s\" is not the name of any known datastore\n", kparg_value(srcarg)); goto err; } if (infix_ds_from_str(kparg_value(dstarg), &dstds)) { - fprintf(stderr, - "Error: \"%s\" is not the name of any known datastore\n", - kparg_value(dstarg)); + fprintf(stderr, ERRMSG "\"%s\" is not the name of any known datastore\n", kparg_value(dstarg)); goto err; } @@ -107,8 +137,7 @@ int infix_copy(kcontext_t *ctx) break; default: fprintf(stderr, - "Error: \"%s\" is not a valid source datastore\n", - kparg_value(srcarg)); + ERRMSG "\"%s\" is not a valid source datastore\n", kparg_value(srcarg)); goto err; } @@ -117,26 +146,22 @@ int infix_copy(kcontext_t *ctx) case SR_DS_RUNNING: break; default: - fprintf(stderr, - "Error: \"%s\" is not a valid destination datastore\n", - kparg_value(dstarg)); + fprintf(stderr, ERRMSG "\"%s\" is not a valid destination datastore\n", kparg_value(dstarg)); goto err; } if (sr_connect(SR_CONN_DEFAULT, &conn)) { - fprintf(stderr, "Error: Connection to datastore failed\n"); + fprintf(stderr, ERRMSG "connection to datastore failed\n"); goto err; } if (sr_session_start(conn, dstds, &sess)) { - fprintf(stderr, "Error: Unable to open transaction to %s\n", - kparg_value(dstarg)); + fprintf(stderr, ERRMSG "unable to open transaction to %s\n", kparg_value(dstarg)); goto err_disconnect; } err = sr_copy_config(sess, NULL, srcds, 0); if (err) { - fprintf(stderr, "Error: Unable to copy configuration (%d)\n", - err); + emsg(sess, ERRMSG "unable to copy configuration (%d)\n", err); goto err_disconnect; } @@ -158,22 +183,22 @@ int infix_commit(kcontext_t *ctx) (void)ctx; if (sr_connect(SR_CONN_DEFAULT, &conn)) { - fprintf(stderr, "Error: Connection to datastore failed\n"); + fprintf(stderr, ERRMSG "connection to datastore failed\n"); goto err; } if (sr_session_start(conn, SR_DS_RUNNING, &sess)) { - fprintf(stderr, - "Error: Unable to open transaction to running-config\n"); + fprintf(stderr, ERRMSG "unable to open transaction to running-config\n"); goto err_disconnect; } err = sr_copy_config(sess, NULL, SR_DS_CANDIDATE, 0); if (err) { - fprintf(stderr, - "Error: Unable to commit candidate to running (%d)\n", - err); - goto err; + if (err == SR_ERR_CALLBACK_FAILED) + emsg(sess, "Please check your changes, try 'diff' and 'do show interfaces'.\n"); + else + emsg(sess, "Failed committing candidate to running: %s\n", sr_strerror(err)); + goto err_disconnect; } sr_disconnect(conn); @@ -199,7 +224,7 @@ int infix_rpc(kcontext_t *ctx) xpath = kcontext_script(ctx); if (!xpath) { - fprintf(stderr, "Error: cannot find rpc xpath\n"); + fprintf(stderr, ERRMSG "cannot find rpc xpath\n"); goto err; } @@ -220,18 +245,18 @@ int infix_rpc(kcontext_t *ctx) } if (sr_connect(SR_CONN_DEFAULT, &conn)) { - fprintf(stderr, "Error: Connection to datastore failed\n"); + fprintf(stderr, ERRMSG "connection to datastore failed\n"); goto err; } if (sr_session_start(conn, SR_DS_OPERATIONAL, &sess)) { - fprintf(stderr, "Error: Unable to open transaction to running-config\n"); + emsg(sess, ERRMSG "unable to open transaction to running-config\n"); goto err_disconnect; } // fprintf(stderr, "%s(): sending RPC %s, icnt %zu\n", __func__, xpath, icnt); if ((err = sr_rpc_send(sess, xpath, input, icnt, 0, &output, &ocnt))) { - fprintf(stderr, "Failed sending RPC %s: %s\n", xpath, sr_strerror(err)); + emsg(sess, ERRMSG "failed sending RPC %s: %s\n", xpath, sr_strerror(err)); goto err_disconnect; }