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 <troglobit@gmail.com>

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) {

src/klish-plugin-infix/src/infix.c

@@ -2,6 +2,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <stdio.h>
+#include <stdarg.h>
 #include <string.h>
 #include <sys/types.h>
 
@@ -14,9 +15,42 @@
 
 #include <libyang/libyang.h>
 
+#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;
 	}