[PATCH 0/7] nfs-utils: nfsconf cli tool and code tests

[PATCH 0/7] nfs-utils: nfsconf cli tool and code tests

[Justin Mitchell]

This series introduces a new cli tool to nfs-utils which provides
for the dumping of the current configuration, querying of individual
settings in a scripting friendly manner and by extension the 
scripted testing and verification of nfsconf functionality.

The previously unreferenced conf_report() is modified to produce
a sorted output, which allows you to --dump the current config in
a way thats easier to diff

As well as some improved error messages a script is included which
tests the current config parsing code by supplying various example
config files and comparing the results of the --dump output.

The cli tool also allows a simple test to check if a config item
has been set, and to retrieve the current value in a way that is
easy to call from a shell script.

As the next stage I hope to implement config setting via the cli 
tool in order to provide an api for automation and management tools

Signed-off-by: Justin Mitchell <jumitche@redhat.com>

[PATCH 1/7] nfs-utils: Fix minor memory leaks

[Justin Mitchell]

Fix some minor memory leaks, a null pointer reference and a typo.

Signed-off-by: Justin Mitchell <jumitche@redhat.com>
---
 support/nfs/conffile.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 29f132d..a4cc236 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -186,7 +186,7 @@ conf_set_now(const char *section, const char *arg, const char *tag,
 		conf_remove_now(section, tag);
 	else if (conf_get_section(section, arg, tag)) {
 		if (!is_default) {
-			xlog(LOG_INFO, "conf_set: duplicate tag [%s]:%s, ignoring...\n", 
+			xlog(LOG_INFO, "conf_set: duplicate tag [%s]:%s, ignoring...", 
 				section, tag);
 		}
 		return 1;
@@ -659,7 +659,7 @@ retry:
 	for (; cb; cb = LIST_NEXT (cb, link)) {
 		if (strcasecmp(section, cb->section) != 0)
 			continue;
-		if (arg && strcasecmp(arg, cb->arg) != 0)
+		if (arg && (cb->arg == NULL || strcasecmp(arg, cb->arg) != 0))
 			continue;
 		if (strcasecmp(tag, cb->tag) != 0)
 			continue;
@@ -917,6 +917,8 @@ conf_set(int transaction, const char *section, const char *arg,
 fail:
 	if (node->tag)
 		free(node->tag);
+	if (node->arg)
+		free(node->arg);
 	if (node->section)
 		free(node->section);
 	if (node)
@@ -1004,6 +1006,8 @@ conf_end(int transaction, int commit)
 			TAILQ_REMOVE (&conf_trans_queue, node, link);
 			if (node->section)
 				free(node->section);
+			if (node->arg)
+				free(node->arg);
 			if (node->tag)
 				free(node->tag);
 			if (node->value)
-- 
1.8.3.1

[PATCH 2/7] nfs-utils: Make config includes relative to current config

[Justin Mitchell]

When including additional config files with the include directive
make relative paths be relative to the current config file instead
of to the process's cwd.

Signed-off-by: Justin Mitchell <jumitche@redhat.com>
---
 support/nfs/conffile.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 9 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index a4cc236..42c0913 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -49,6 +49,7 @@
 #include <errno.h>
 #include <err.h>
 #include <syslog.h>
+#include <libgen.h>
 
 #include "conffile.h"
 #include "xlog.h"
@@ -60,7 +61,7 @@ static char * conf_readfile(const char *path);
 static int conf_set(int , const char *, const char *, const char *, 
 	const char *, int , int );
 static void conf_parse(int trans, char *buf, 
-	char **section, char **subsection);
+	char **section, char **subsection, const char *filename);
 
 struct conf_trans {
 	TAILQ_ENTRY (conf_trans) link;
@@ -206,12 +207,40 @@ conf_set_now(const char *section, const char *arg, const char *tag,
 	return 0;
 }
 
+/* Attempt to construct a relative path to the new file */
+static char *
+relative_path(const char *oldfile, const char *newfile)
+{
+	char * tmpcopy = NULL;
+	char * dir = NULL;
+	char * relpath = NULL;
+
+	if (!newfile) return strdup(oldfile);
+	if (newfile[0] == '/') return strdup(newfile);
+
+	tmpcopy = strdup(oldfile);
+	if (!tmpcopy) goto mem_err;
+
+	dir = dirname(tmpcopy);
+	size_t pathlen =  strlen(dir)+strlen(newfile)+2;
+	relpath = calloc(1, pathlen);
+	if (!relpath) goto mem_err;
+	snprintf(relpath, pathlen, "%s/%s", dir, newfile);
+
+	free(tmpcopy);
+	return relpath;
+mem_err:
+	if (tmpcopy) free(tmpcopy);
+	return NULL;
+}
+
+
 /*
  * Parse the line LINE of SZ bytes.  Skip Comments, recognize section
  * headers and feed tag-value pairs into our configuration database.
  */
 static void
-conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection)
+conf_parse_line(int trans, char *line, const char *filename, int lineno, char **section, char **subsection)
 {
 	char *val, *ptr;
 
@@ -366,10 +395,12 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
 
 	if (strcasecmp(line, "include")==0) {
 		/* load and parse subordinate config files */
-		char * subconf = conf_readfile(val);
+		char * relpath = relative_path(filename, val);
+		char * subconf = conf_readfile(relpath);
 		if (subconf == NULL) {
-			xlog_warn("config file error: line %d: "
-			"error loading included config", lineno);
+			xlog_warn("%s:%d: error loading included config %s", 
+				  filename, lineno, relpath);
+			if (relpath) free(relpath);
 			return;
 		}
 
@@ -383,10 +414,11 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
 				inc_subsection = strdup(*subsection);
 		}
 
-		conf_parse(trans, subconf, &inc_section, &inc_subsection);
+		conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath);
 
 		if (inc_section) free(inc_section);
 		if (inc_subsection) free(inc_subsection);
+		if (relpath) free(relpath);
 		free(subconf);
 	} else {
 		/* XXX Perhaps should we not ignore errors?  */
@@ -396,7 +428,7 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
 
 /* Parse the mapped configuration file.  */
 static void
-conf_parse(int trans, char *buf, char **section, char **subsection)
+conf_parse(int trans, char *buf, char **section, char **subsection, const char *filename)
 {
 	char *cp = buf;
 	char *bufend = NULL;
@@ -413,7 +445,7 @@ conf_parse(int trans, char *buf, char **section, char **subsection)
 			else {
 				*cp = '\0';
 				lineno++;
-				conf_parse_line(trans, line, lineno, section, subsection);
+				conf_parse_line(trans, line, filename, lineno, section, subsection);
 				line = cp + 1;
 			}
 		}
@@ -508,7 +540,7 @@ conf_load_file(const char *conf_file)
 	/* Parse config contents into the transaction queue */
 	char *section = NULL;
 	char *subsection = NULL;
-	conf_parse(trans, conf_data, &section, &subsection);
+	conf_parse(trans, conf_data, &section, &subsection, conf_file);
 	if (section) free(section);
 	if (subsection) free(subsection);
 	free(conf_data);
-- 
1.8.3.1

[PATCH 3/7] nfs-utils: Use config file name in error messages

[Justin Mitchell]

Improve readability of error configuration error messages
by including the filename currently being read.

Signed-off-by: Justin Mitchell <jumitche@redhat.com>
---
 support/nfs/conffile.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 42c0913..fe41de5 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -276,8 +276,8 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 		/* find the closing ] */
 		ptr = strchr(line, ']');
 		if (ptr == NULL) {
-			xlog_warn("config file error: line %d: "
- 				"non-matched ']', ignoring until next section", lineno);
+			xlog_warn("config error at %s:%d: "
+ 				"non-matched ']', ignoring until next section", filename, lineno);
 			return;
 		}
 
@@ -302,7 +302,7 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 		/* copy the section name */
 		*section = strdup(line);
 		if (!*section) {
-			xlog_warn("conf_parse_line: %d: malloc failed", lineno);
+			xlog_warn("config error at %s:%d: malloc failed", filename, lineno);
 			return;
 		}
 
@@ -312,14 +312,14 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 		/* check for the closing " */
 		ptr = strchr(val, '"');
 		if (ptr == NULL) {
-			xlog_warn("config file error: line %d: "
- 				"non-matched '\"', ignoring until next section", lineno);
+			xlog_warn("config error at %s:%d: "
+ 				"non-matched '\"', ignoring until next section", filename, lineno);
 			return;
 		}
 		*ptr = '\0';
 		*subsection = strdup(val);
 		if (!*subsection) 
-			xlog_warn("conf_parse_line: %d: malloc arg failed", lineno);
+			xlog_warn("config error at %s:%d: malloc failed", filename, lineno);
 		return;
 	}
 
@@ -330,15 +330,15 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 	if (ptr == NULL) {
 		/* Other non-empty lines are weird.  */
 		if (line[strspn(line, " \t")])
-			xlog_warn("config file error: line %d: "
-				"line not empty and not an assignment", lineno);
+			xlog_warn("config error at %s:%d: "
+				"line not empty and not an assignment", filename, lineno);
 		return;
 	}
 
 	/* If no section, we are ignoring the line.  */
 	if (!*section) {
-		xlog_warn("config file error: line %d: "
-			"ignoring line due to no section", lineno);
+		xlog_warn("config error at %s:%d: "
+			"ignoring line not in a section", filename, lineno);
 		return;
 	}
 
@@ -355,8 +355,8 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 		val++;
 		ptr = strchr(val, '"');
 		if (ptr == NULL) {
-			xlog_warn("config file error: line %d: "
-				"unmatched quotes", lineno);
+			xlog_warn("config error at %s:%d: "
+				"unmatched quotes", filename, lineno);
 			return;
 		}
 		*ptr = '\0';
@@ -365,8 +365,8 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 		val++;
 		ptr = strchr(val, '\'');
 		if (ptr == NULL) {
-			xlog_warn("config file error: line %d: "
-				"unmatched quotes", lineno);
+			xlog_warn("config error at %s:%d: "
+				"unmatched quotes", filename, lineno);
 			return;
 		}
 		*ptr = '\0';
@@ -383,13 +383,13 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 	}
 
 	if (*line == '\0') {
-		xlog_warn("config file error: line %d: "
-			"missing tag in assignment", lineno);
+		xlog_warn("config error at %s:%d: "
+			"missing tag in assignment", filename, lineno);
 		return;
 	}
 	if (*val == '\0') {
-		xlog_warn("config file error: line %d: "
-			"missing value in assignment", lineno);
+		xlog_warn("config error at %s:%d: "
+			"missing value in assignment", filename, lineno);
 		return;
 	}
 
@@ -398,7 +398,7 @@ conf_parse_line(int trans, char *line, const char *filename, int lineno, char **
 		char * relpath = relative_path(filename, val);
 		char * subconf = conf_readfile(relpath);
 		if (subconf == NULL) {
-			xlog_warn("%s:%d: error loading included config %s", 
+			xlog_warn("config error at %s:%d: error loading included config %s", 
 				  filename, lineno, relpath);
 			if (relpath) free(relpath);
 			return;
-- 
1.8.3.1

[PATCH 4/7] nfs-utils: Indicate if config file was missing

[Justin Mitchell]

Return an indication that the config file could not be loaded
for processes that want to differentiate this from empty config

Signed-off-by: Justin Mitchell <jumitche@redhat.com>
---
 support/include/conffile.h |  2 +-
 support/nfs/conffile.c     | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/support/include/conffile.h b/support/include/conffile.h
index ad20067..6baaf9a 100644
--- a/support/include/conffile.h
+++ b/support/include/conffile.h
@@ -60,7 +60,7 @@ extern _Bool    conf_get_bool(const char *, const char *, _Bool);
 extern char    *conf_get_str(const char *, const char *);
 extern char    *conf_get_str_with_def(const char *, const char *, char *);
 extern char    *conf_get_section(const char *, const char *, const char *);
-extern void     conf_init_file(const char *);
+extern int      conf_init_file(const char *);
 extern void     conf_cleanup(void);
 extern int      conf_match_num(const char *, const char *, int);
 extern int      conf_remove(int, const char *, const char *);
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index fe41de5..7ed5646 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -522,7 +522,7 @@ static void conf_free_bindings(void)
 }
 
 /* Open the config file and map it into our address space, then parse it.  */
-static void
+static int
 conf_load_file(const char *conf_file)
 {
 	int trans;
@@ -532,7 +532,7 @@ conf_load_file(const char *conf_file)
 	conf_data = conf_readfile(conf_file);
 
 	if (conf_data == NULL)
-		return;
+		return 1;
 
 	/* Load default configuration values.  */
 	conf_load_defaults();
@@ -550,10 +550,10 @@ conf_load_file(const char *conf_file)
 
 	/* Apply the new configuration values */
 	conf_end(trans, 1);
-	return;
+	return 0;
 }
 
-void
+int
 conf_init_file(const char *conf_file)
 {
 	unsigned int i;
@@ -564,7 +564,7 @@ conf_init_file(const char *conf_file)
 	TAILQ_INIT (&conf_trans_queue);
 
 	if (conf_file == NULL) conf_file=NFS_CONFFILE;
-	conf_load_file(conf_file);
+	return conf_load_file(conf_file);
 }
 
 /* 
-- 
1.8.3.1

[PATCH 5/7] nfs-utils: tidy up output of conf_report

[Justin Mitchell]

Previously unused conf_report() function is modified to output
an alphabetically sorted version of the current configuration
to the nominated FILE handle.

Signed-off-by: Justin Mitchell <jumitche@redhat.com>
---
 support/include/conffile.h |   3 +-
 support/nfs/conffile.c     | 237 ++++++++++++++++++++++++++++-----------------
 2 files changed, 151 insertions(+), 89 deletions(-)

diff --git a/support/include/conffile.h b/support/include/conffile.h
index 6baaf9a..bc2d61f 100644
--- a/support/include/conffile.h
+++ b/support/include/conffile.h
@@ -37,6 +37,7 @@
 #include <ctype.h>
 #include <stdint.h>
 #include <stdbool.h>
+#include <stdio.h>
 
 struct conf_list_node {
 	TAILQ_ENTRY(conf_list_node) link;
@@ -65,7 +66,7 @@ extern void     conf_cleanup(void);
 extern int      conf_match_num(const char *, const char *, int);
 extern int      conf_remove(int, const char *, const char *);
 extern int      conf_remove_section(int, const char *);
-extern void     conf_report(void);
+extern void     conf_report(FILE *);
 
 /*
  * Convert letter from upper case to lower case
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 7ed5646..9148557 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -1055,117 +1055,178 @@ conf_end(int transaction, int commit)
  * Configuration is "stored in reverse order", so reverse it again.
  */
 struct dumper {
-	char *s, *v;
+	char *section;
+	char *arg;
+	char *tag;
+	char *value;
 	struct dumper *next;
 };
 
+static int dumper_section_compare(const struct dumper *nodea, const struct dumper *nodeb)
+{
+	int ret;
+
+	// missing node, shouldnt happen
+	if (!nodea || !nodeb) return -1;
+	// if only one has a section name, the blank one goes first
+	if (!nodea->section &&  nodeb->section) return -1;
+	if ( nodea->section && !nodeb->section) return  1;
+	// no section names at all
+	if (!nodea->section && !nodeb->section) return  0;
+	// two section names, do they match
+	ret = strcmp(nodea->section, nodeb->section);
+	// section names differ, that was easy
+	if (ret != 0) return ret;
+	// sections matched, but how about sub-sections, again blank goes first
+	if (!nodea->arg &&  nodeb->arg) return -1;
+	if ( nodea->arg && !nodeb->arg) return  1;
+
+	// both have sub-section args and they differ
+	if (nodea->arg && nodeb->arg && (ret=strcmp(nodea->arg, nodeb->arg))!=0)
+		return ret;
+
+	return 0;
+}
+
+static bool 
+should_escape(const char * text)
+{
+	int len;
+	if (!text) return false;
+	if (isspace(text[0])) return true;
+	len = strlen(text);
+	if (isspace(text[len-1])) return true;
+	return false;
+}
+
 static void
-conf_report_dump(struct dumper *node)
+conf_report_dump_text(struct dumper *head, FILE *ff)
 {
-	/* Recursive, cleanup when we're done.  */
-	if (node->next)
-		conf_report_dump(node->next);
-
-	if (node->v)
-		xlog(LOG_INFO, "%s=\t%s", node->s, node->v);
-	else if (node->s) {
-		xlog(LOG_INFO, "%s", node->s);
-		if (strlen(node->s) > 0)
-			free(node->s);
+	const struct dumper *node = head;
+	const struct dumper *last = NULL;
+
+	for (node=head; node!=NULL; node=node->next) {
+		if (dumper_section_compare(last, node)!=0) {
+			if (node != head) fprintf(ff, "\n");
+			if (node->arg)
+				fprintf(ff, "[%s \"%s\"]\n", node->section, node->arg);
+			else
+				fprintf(ff, "[%s]\n", node->section);
+		}
+		fprintf(ff, " %s", node->tag);
+		if (node->value) {
+			if (should_escape(node->value))
+				fprintf(ff, " = \"%s\"", node->value);
+			else
+				fprintf(ff, " = %s", node->value);
+		}
+		fprintf(ff, "\n");
+
+		last = node;
 	}
+}
 
-	free (node);
+/* compare two dumper nodes for sorting */
+static int dumper_compare(const void * a, const void * b)
+{
+	const struct dumper *nodea = *(struct dumper **)a;
+	const struct dumper *nodeb = *(struct dumper **)b;
+	int ret;
+
+	// missing node, shouldnt happen
+	if (!nodea || !nodeb) return -1;
+
+	ret = dumper_section_compare(nodea, nodeb);
+	if (ret != 0) return ret;
+
+	// sub-sections match (both blank, or both same)
+	// so we compare the tag names
+	
+	// blank tags shouldnt happen, but paranoia
+	if (!nodea->tag &&  nodeb->tag) return -1;
+	if ( nodea->tag && !nodeb->tag) return  1;
+	if (!nodea->tag && !nodeb->tag) return  0;
+
+	// last test, compare the tags directly
+	ret = strcmp(nodea->tag, nodeb->tag);
+	return ret;
 }
 
-void
-conf_report (void)
+static struct dumper *conf_report_sort(struct dumper *start)
 {
-	struct conf_binding *cb, *last = 0;
-	unsigned int i, len, diff_arg = 0;
-	char *current_section = (char *)0;
-	char *current_arg = (char *)0;
-	struct dumper *dumper, *dnode;
+	struct dumper **list;
+	struct dumper *node;
+	unsigned int count = 0;
 
-	dumper = dnode = (struct dumper *)calloc(1, sizeof *dumper);
-	if (!dumper)
-		goto mem_fail;
+	// how long is this list
+	for (node=start; node!=NULL; node=node->next) count++;
+
+	// no need to sort a list of 1 or less
+	if (count < 2) return start;
+
+	// make an array of all the nodes
+	list = calloc(count, sizeof(struct dumper *));
+	if (!list) goto mem_err;
+
+	unsigned int i=0;
+	for (node=start; node!=NULL; node=node->next) {
+		list[i++] = node;
+	}
+
+	// sort the array
+	qsort(list, count, sizeof(struct dumper *), dumper_compare);
+
+	// turn sorted array back into a linked list
+	for (i=0; i<count-1; i++) {
+		list[i]->next = list[i+1];
+	}
+	list[count-1]->next = NULL;
+
+	// remember the new head of list and cleanup
+	node = list[0];
+	free(list);
+
+	// return the new head of list
+	return node;
+
+mem_err:
+	free(list);
+	return NULL;
+}
+
+void
+conf_report (FILE *outfile)
+{
+	struct conf_binding *cb = NULL;
+	unsigned int i;
+	struct dumper *dumper = NULL, *dnode = NULL;
 
 	xlog(LOG_INFO, "conf_report: dumping running configuration");
 
-	for (i = 0; i < sizeof conf_bindings / sizeof conf_bindings[0]; i++)
+	for (i = 0; i < sizeof conf_bindings / sizeof conf_bindings[0]; i++) {
 		for (cb = LIST_FIRST(&conf_bindings[i]); cb; cb = LIST_NEXT(cb, link)) {
-			if (!cb->is_default) {
-				/* Make sure the Section arugment is the same */
-				if (current_arg && current_section && cb->arg) {
-					if (strcmp(cb->section, current_section) == 0 &&
-						strcmp(cb->arg, current_arg) != 0)
-					diff_arg = 1;
-				}
-				/* Dump this entry.  */
-				if (!current_section || strcmp(cb->section, current_section) 
-							|| diff_arg) {
-					if (current_section || diff_arg) {
-						len = strlen (current_section) + 3;
-						if (current_arg)
-							len += strlen(current_arg) + 3;
-						dnode->s = malloc(len);
-						if (!dnode->s)
-							goto mem_fail;
-
-						if (current_arg)
-							snprintf(dnode->s, len, "[%s \"%s\"]", 
-								current_section, current_arg);
-						else
-							snprintf(dnode->s, len, "[%s]", current_section);
-
-						dnode->next = 
-							(struct dumper *)calloc(1, sizeof (struct dumper));
-						dnode = dnode->next;
-						if (!dnode)
-							goto mem_fail;
-
-						dnode->s = "";
-						dnode->next = 
-							(struct dumper *)calloc(1, sizeof (struct dumper));
-						dnode = dnode->next;
-						if (!dnode)
-						goto mem_fail;
-					}
-					current_section = cb->section;
-					current_arg = cb->arg;
-					diff_arg = 0;
-				}
-				dnode->s = cb->tag;
-				dnode->v = cb->value;
-				dnode->next = (struct dumper *)calloc (1, sizeof (struct dumper));
-				dnode = dnode->next;
-				if (!dnode)
-					goto mem_fail;
-				last = cb;
+			struct dumper *newnode = calloc(1, sizeof (struct dumper));
+			if (!newnode) goto mem_fail;
+
+			newnode->next = dumper;
+			dumper = newnode;
+
+			newnode->section = cb->section;
+			newnode->arg = cb->arg;
+			newnode->tag = cb->tag;
+			newnode->value = cb->value;
 		}
 	}
 
-	if (last) {
-		len = strlen(last->section) + 3;
-		if (last->arg)
-			len += strlen(last->arg) + 3;
-		dnode->s = malloc(len);
-		if (!dnode->s)
-			goto mem_fail;
-		if (last->arg)
-			snprintf(dnode->s, len, "[%s \"%s\"]", last->section, last->arg);
-		else
-			snprintf(dnode->s, len, "[%s]", last->section);
-	}
-	conf_report_dump(dumper);
-	return;
+	dumper = conf_report_sort(dumper);
+	conf_report_dump_text(dumper, outfile);
+	goto cleanup;
 
 mem_fail:
 	xlog_warn("conf_report: malloc/calloc failed");
+cleanup:
 	while ((dnode = dumper) != 0) {
 		dumper = dumper->next;
-		if (dnode->s)
-			free(dnode->s);
 		free(dnode);
 	}
 	return;
-- 
1.8.3.1

[PATCH 6/7] nfs-utils: Add nfsconftool cli

[Justin Mitchell]

This tool uses the conffile facilities to allow commandline
querying of configuration settings and to dump the current
config for diagnosis and testing

Signed-off-by: Justin Mitchell <jumitche@redhat.com>
---
 Makefile.am                |   2 +-
 configure.ac               |   1 +
 tools/Makefile.am          |   2 +
 tools/nfsconf/Makefile.am  |   9 +++
 tools/nfsconf/nfsconfcli.c | 144 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 157 insertions(+), 1 deletion(-)
 create mode 100644 tools/nfsconf/Makefile.am
 create mode 100644 tools/nfsconf/nfsconfcli.c

diff --git a/Makefile.am b/Makefile.am
index e1f39aa..0022084 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2,7 +2,7 @@
 
 AUTOMAKE_OPTIONS = foreign
 
-SUBDIRS = tools support utils linux-nfs tests systemd
+SUBDIRS = support tools utils linux-nfs tests systemd
 
 MAINTAINERCLEANFILES = Makefile.in
 
diff --git a/configure.ac b/configure.ac
index 5a11636..b925666 100644
--- a/configure.ac
+++ b/configure.ac
@@ -616,6 +616,7 @@ AC_CONFIG_FILES([
 	tools/rpcgen/Makefile
 	tools/mountstats/Makefile
 	tools/nfs-iostat/Makefile
+	tools/nfsconf/Makefile
 	utils/Makefile
 	utils/blkmapd/Makefile
 	utils/nfsdcltrack/Makefile
diff --git a/tools/Makefile.am b/tools/Makefile.am
index f2ce282..4266da4 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -6,6 +6,8 @@ if CONFIG_RPCGEN
 OPTDIRS += rpcgen
 endif
 
+OPTDIRS += nfsconf
+
 SUBDIRS = locktest rpcdebug nlmtest mountstats nfs-iostat $(OPTDIRS)
 
 MAINTAINERCLEANFILES = Makefile.in
diff --git a/tools/nfsconf/Makefile.am b/tools/nfsconf/Makefile.am
new file mode 100644
index 0000000..4baaa26
--- /dev/null
+++ b/tools/nfsconf/Makefile.am
@@ -0,0 +1,9 @@
+## Process this file with automake to produce Makefile.in
+
+bin_PROGRAMS = nfsconftool
+
+nfsconftool_SOURCES = nfsconfcli.c
+nfsconftool_LDADD = ../../support/nfs/libnfsconf.la
+
+MAINTAINERCLEANFILES = Makefile.in
+
diff --git a/tools/nfsconf/nfsconfcli.c b/tools/nfsconf/nfsconfcli.c
new file mode 100644
index 0000000..1443e97
--- /dev/null
+++ b/tools/nfsconf/nfsconfcli.c
@@ -0,0 +1,144 @@
+#include <stdlib.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <getopt.h>
+#include <string.h>
+#include <errno.h>
+#include "config.h"
+#include "conffile.h"
+#include "xlog.h"
+
+typedef enum {
+	MODE_NONE,
+	MODE_GET,
+	MODE_ISSET,
+	MODE_DUMP
+} confmode_t;
+
+static void usage(const char *name)
+{
+	fprintf(stderr, "Usage: %s [-v] [--file filename.conf] ...\n", name);
+	fprintf(stderr, "Options:\n");
+	fprintf(stderr, " -v			Increase Verbosity\n");
+	fprintf(stderr, " --file filename.conf	Load this config file\n");
+	fprintf(stderr, "     (Default config file: " NFS_CONFFILE "\n");
+	fprintf(stderr, "Modes:\n");
+	fprintf(stderr, "  --dump [outputfile]\n");
+	fprintf(stderr, "      Outputs the configuration to the named file\n");
+	fprintf(stderr, "  --get [--arg subsection] {section} {tag}\n");
+	fprintf(stderr, "      Output one specific config value\n");
+	fprintf(stderr, "  --isset [--arg subsection] {section} {tag}\n");
+	fprintf(stderr, "      Return code indicates if config value is present\n");
+}
+
+int main(int argc, char **argv)
+{
+	const char * val;
+	char * confpath = NFS_CONFFILE;
+	char * arg = NULL;
+	int verbose=0;
+	int ret = 0;
+	char * dumpfile = NULL;
+
+	confmode_t mode = MODE_NONE;
+
+	while (1) {
+		int c;
+		int index = 0;
+		struct option long_options[] = {
+			{"get",		no_argument, 0, 'g' },
+			{"arg",	  required_argument, 0, 'a' },
+			{"isset", 	no_argument, 0, 'i' },
+			{"dump",  optional_argument, 0, 'd' },
+			{"file",  required_argument, 0, 'f' },
+			{"verbose",	no_argument, 0, 'v' },
+			{NULL,			  0, 0, 0 }
+		};
+
+		c = getopt_long(argc, argv, "ga:id::f:v", long_options, &index);
+		if (c == -1) break;
+
+		switch (c) {
+			case 0:
+				break;
+			case 'f':
+				confpath = optarg;
+				break;
+			case 'a':
+				arg = optarg;
+				break;
+			case 'v':
+				verbose++;
+				break;
+			case 'g':
+				mode = MODE_GET;
+				break;
+			case 'i':
+				mode = MODE_ISSET;
+				break;
+			case 'd':
+				if (optarg==NULL && argv[optind]!=NULL && argv[optind][0]!='-')
+					optarg = argv[optind++];
+				mode = MODE_DUMP;
+				dumpfile = optarg;
+				break;
+			default:
+				usage(argv[0]);
+				return 1;
+		}
+	}
+
+	if (verbose) xlog_config(D_ALL, 1);
+	xlog_stderr(1);
+	xlog_syslog(0);
+	xlog_open("nfsconf");
+
+	if (mode == MODE_NONE) {
+		fprintf(stderr, "Error: No MODE selected.\n");
+		usage(argv[0]);
+		return 1;
+	}
+
+	if (conf_init_file(confpath)) {
+		if (verbose || mode != MODE_ISSET) fprintf(stderr, "Error loading config file %s\n", confpath);
+		if (mode != MODE_ISSET) return 1;
+	}
+
+	if (mode == MODE_DUMP) {
+		FILE *out = stdout;
+		if (dumpfile) {
+			if ((out=fopen(dumpfile, "w"))==NULL) {
+				fprintf(stderr, "Error opening dumpfile %s: %s\n", dumpfile, strerror(errno));
+				ret = 2;
+				goto cleanup;
+			}
+			if (verbose) printf("Dumping config to %s\n", dumpfile);
+		}
+		conf_report(out);
+		if (dumpfile) fclose(out);
+	} else
+	if (mode == MODE_GET || mode == MODE_ISSET) {
+		if (optind+1 >= argc) {
+			fprintf(stderr, "Error: insufficient arguments for mode\n");
+			usage(argv[0]);
+			ret = 2;
+			goto cleanup;
+		}
+		char * section = argv[optind++];
+		char * tag = argv[optind++];
+
+		if ((val=conf_get_section(section, arg, tag))!=NULL) {
+			if (mode == MODE_GET) printf("%s\n", val);
+		} else {
+			if (mode == MODE_GET && verbose) fprintf(stderr, "Tag '%s' not found\n", tag);
+			ret = 1;
+		}
+	} else {
+		fprintf(stderr, "Mode not yet implimented.\n");
+		ret = 2;
+	}
+
+cleanup:
+	conf_cleanup();
+	return ret;
+}
-- 
1.8.3.1

[PATCH 7/7] nfs-utils: use nfsconftool cli to test library function

[Justin Mitchell]

Uses the nfsconftool cli to test that the configuration
library functions are operating as expected

Signed-off-by: Justin Mitchell <jumitche@redhat.com>
---
 tests/Makefile.am            |  3 ++-
 tests/nfsconf/01-errors.conf | 20 ++++++++++++++++++++
 tests/nfsconf/01-errors.exp  | 14 ++++++++++++++
 tests/nfsconf/02-valid.conf  | 25 +++++++++++++++++++++++++
 tests/nfsconf/02-valid.exp   | 26 ++++++++++++++++++++++++++
 tests/nfsconf/02-valid.sub   |  7 +++++++
 tests/t0002-nfsconf.sh       | 38 ++++++++++++++++++++++++++++++++++++++
 7 files changed, 132 insertions(+), 1 deletion(-)
 create mode 100644 tests/nfsconf/01-errors.conf
 create mode 100644 tests/nfsconf/01-errors.exp
 create mode 100644 tests/nfsconf/02-valid.conf
 create mode 100644 tests/nfsconf/02-valid.exp
 create mode 100644 tests/nfsconf/02-valid.sub
 create mode 100755 tests/t0002-nfsconf.sh

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1f96264..60691cc 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -10,5 +10,6 @@ SUBDIRS = nsm_client
 
 MAINTAINERCLEANFILES = Makefile.in
 
-TESTS = t0001-statd-basic-mon-unmon.sh
+TESTS = t0001-statd-basic-mon-unmon.sh \
+	t0002-nfsconf.sh
 EXTRA_DIST = test-lib.sh $(TESTS)
diff --git a/tests/nfsconf/01-errors.conf b/tests/nfsconf/01-errors.conf
new file mode 100644
index 0000000..ca64d2c
--- /dev/null
+++ b/tests/nfsconf/01-errors.conf
@@ -0,0 +1,20 @@
+# file of deliberate errors
+[default]
+
+
+[ one
+
+[ two " foo ]
+aa = foo
+
+[ three 
+val = none
+
+[four]
+one
+ = two
+three =
+four = foo = bar
+five = " nothing
+six = normal
+
diff --git a/tests/nfsconf/01-errors.exp b/tests/nfsconf/01-errors.exp
new file mode 100644
index 0000000..2bf1b8c
--- /dev/null
+++ b/tests/nfsconf/01-errors.exp
@@ -0,0 +1,14 @@
+nfsconf: config error at 01-errors.conf:5: non-matched ']', ignoring until next section
+nfsconf: config error at 01-errors.conf:7: non-matched '"', ignoring until next section
+nfsconf: config error at 01-errors.conf:10: non-matched ']', ignoring until next section
+nfsconf: config error at 01-errors.conf:11: ignoring line not in a section
+nfsconf: config error at 01-errors.conf:14: line not empty and not an assignment
+nfsconf: config error at 01-errors.conf:15: missing tag in assignment
+nfsconf: config error at 01-errors.conf:16: missing value in assignment
+nfsconf: config error at 01-errors.conf:18: unmatched quotes
+[four]
+ four = foo = bar
+ six = normal
+
+[two]
+ aa = foo
diff --git a/tests/nfsconf/02-valid.conf b/tests/nfsconf/02-valid.conf
new file mode 100644
index 0000000..ca8ccab
--- /dev/null
+++ b/tests/nfsconf/02-valid.conf
@@ -0,0 +1,25 @@
+[environment]
+one = 1
+two = 2
+three = 3
+
+[section_one]
+one = 11
+two = 22
+three = $three
+ four   =   "six  "
+ five six = seven eight
+
+[section_two "two"]
+one = Un
+two = Dau
+include = "02-valid.sub"
+
+[section_two "one"]
+one = Un
+two = Deux
+
+[section_two "two"]
+four = Pedwar
+   five   =    " Pump "
+
diff --git a/tests/nfsconf/02-valid.exp b/tests/nfsconf/02-valid.exp
new file mode 100644
index 0000000..379d7a4
--- /dev/null
+++ b/tests/nfsconf/02-valid.exp
@@ -0,0 +1,26 @@
+[environment]
+ one = 1
+ three = 3
+ two = 2
+
+[extra_section]
+ bar = baz
+ foo = bar
+
+[section_one]
+ five six = seven eight
+ four = "six  "
+ one = 11
+ three = $three
+ two = 22
+
+[section_two "one"]
+ one = Un
+ two = Deux
+
+[section_two "two"]
+ five = " Pump "
+ four = Pedwar
+ one = Un
+ three = Tri
+ two = Dau
diff --git a/tests/nfsconf/02-valid.sub b/tests/nfsconf/02-valid.sub
new file mode 100644
index 0000000..ab7beda
--- /dev/null
+++ b/tests/nfsconf/02-valid.sub
@@ -0,0 +1,7 @@
+# Included configs don't need a section, it is inherited
+three=Tri
+
+# But if they do, that works also
+[extra_section]
+foo = bar
+bar = baz
diff --git a/tests/t0002-nfsconf.sh b/tests/t0002-nfsconf.sh
new file mode 100755
index 0000000..511d248
--- /dev/null
+++ b/tests/t0002-nfsconf.sh
@@ -0,0 +1,38 @@
+#!/bin/bash
+
+TESTFILES=nfsconf
+DUMPER=`realpath ../tools/nfsconf/nfsconftool`
+
+BASEDIR=`dirname "$0"`
+pushd $BASEDIR/$TESTFILES
+
+rm -f *.out
+
+TCOUNT=0
+TPASS=0
+
+for i in *.conf
+do
+TNAME=`basename "$i" .conf`
+
+echo "Running test $TNAME"
+TCOUNT=$((TCOUNT + 1))
+
+if ! $DUMPER --file "$i" --dump - > "$TNAME.out" 2>&1
+then
+echo "Error running test $TNAME"
+elif ! diff -u "$TNAME.exp" "$TNAME.out"
+then
+echo "FAIL differences detected in test $TNAME"
+else
+echo "PASS $TNAME"
+TPASS=$((TPASS + 1))
+fi
+
+done
+
+echo "nfsconf tests complete. $TPASS of $TCOUNT tests passed"
+
+if [ $TPASS -lt $TCOUNT ]; then
+exit 1
+fi
-- 
1.8.3.1

Re: [PATCH 2/7] nfs-utils: Make config includes relative to current config

[Steve Dickson]

Hi Justin,

Sorry for taking so long to get to this... 

Most of my comments is about the style you are using.

I would like to keep the coding style somewhat the same 
and in a few places that is not just not happening...

On 04/23/2018 11:25 AM, Justin Mitchell wrote:
> When including additional config files with the include directive
> make relative paths be relative to the current config file instead
> of to the process's cwd.
> 
> Signed-off-by: Justin Mitchell <jumitche@redhat.com>
> ---
>  support/nfs/conffile.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index a4cc236..42c0913 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -49,6 +49,7 @@
>  #include <errno.h>
>  #include <err.h>
>  #include <syslog.h>
> +#include <libgen.h>
>  
>  #include "conffile.h"
>  #include "xlog.h"
> @@ -60,7 +61,7 @@ static char * conf_readfile(const char *path);
>  static int conf_set(int , const char *, const char *, const char *, 
>  	const char *, int , int );
>  static void conf_parse(int trans, char *buf, 
> -	char **section, char **subsection);
> +	char **section, char **subsection, const char *filename);
>  
>  struct conf_trans {
>  	TAILQ_ENTRY (conf_trans) link;
> @@ -206,12 +207,40 @@ conf_set_now(const char *section, const char *arg, const char *tag,
>  	return 0;
>  }
>  
> +/* Attempt to construct a relative path to the new file */
> +static char *
> +relative_path(const char *oldfile, const char *newfile)
> +{
> +	char * tmpcopy = NULL;
> +	char * dir = NULL;
> +	char * relpath = NULL;
The space between the '*' and variable is not need.
 
> +
> +	if (!newfile) return strdup(oldfile);
> +	if (newfile[0] == '/') return strdup(newfile);
Go a head a put a newline and tab after the if statement.

> +
> +	tmpcopy = strdup(oldfile);
> +	if (!tmpcopy) goto mem_err;
> +
> +	dir = dirname(tmpcopy);
> +	size_t pathlen =  strlen(dir)+strlen(newfile)+2;
> +	relpath = calloc(1, pathlen);
> +	if (!relpath) goto mem_err;
> +	snprintf(relpath, pathlen, "%s/%s", dir, newfile);
Same here... That is just hard to read IMHO...

Also put all variable at the top of the routine. 

 
> +
> +	free(tmpcopy);
> +	return relpath;
> +mem_err:
> +	if (tmpcopy) free(tmpcopy);
> +	return NULL;
> +}
> +
> +
>  /*
>   * Parse the line LINE of SZ bytes.  Skip Comments, recognize section
>   * headers and feed tag-value pairs into our configuration database.
>   */
>  static void
> -conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection)
> +conf_parse_line(int trans, char *line, const char *filename, int lineno, char **section, char **subsection)
>  {
>  	char *val, *ptr;
>  
> @@ -366,10 +395,12 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
>  
>  	if (strcasecmp(line, "include")==0) {
>  		/* load and parse subordinate config files */
> -		char * subconf = conf_readfile(val);
> +		char * relpath = relative_path(filename, val);
> +		char * subconf = conf_readfile(relpath);
Same here no space also isn't subconf being over written?

>  		if (subconf == NULL) {
> -			xlog_warn("config file error: line %d: "
> -			"error loading included config", lineno);
> +			xlog_warn("%s:%d: error loading included config %s", 
> +				  filename, lineno, relpath);
> +			if (relpath) free(relpath);
>  			return;
>  		}
>  
> @@ -383,10 +414,11 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
>  				inc_subsection = strdup(*subsection);
>  		}
>  
> -		conf_parse(trans, subconf, &inc_section, &inc_subsection);
> +		conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath);
>  
>  		if (inc_section) free(inc_section);
>  		if (inc_subsection) free(inc_subsection);
> +		if (relpath) free(relpath);
Are these if even needed? Won't these either be NULL or allocated memory?

Again... I'm trying to is keep the coding similar which hopefully
will make the code easier to read...
 
>  		free(subconf);
>  	} else {
>  		/* XXX Perhaps should we not ignore errors?  */
> @@ -396,7 +428,7 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
>  
>  /* Parse the mapped configuration file.  */
>  static void
> -conf_parse(int trans, char *buf, char **section, char **subsection)
> +conf_parse(int trans, char *buf, char **section, char **subsection, const char *filename)
>  {
>  	char *cp = buf;
>  	char *bufend = NULL;
> @@ -413,7 +445,7 @@ conf_parse(int trans, char *buf, char **section, char **subsection)
>  			else {
>  				*cp = '\0';
>  				lineno++;
> -				conf_parse_line(trans, line, lineno, section, subsection);
> +				conf_parse_line(trans, line, filename, lineno, section, subsection);
>  				line = cp + 1;
>  			}
>  		}
> @@ -508,7 +540,7 @@ conf_load_file(const char *conf_file)
>  	/* Parse config contents into the transaction queue */
>  	char *section = NULL;
>  	char *subsection = NULL;
> -	conf_parse(trans, conf_data, &section, &subsection);
> +	conf_parse(trans, conf_data, &section, &subsection, conf_file);
>  	if (section) free(section);
>  	if (subsection) free(subsection);
Same here...

steved.
>  	free(conf_data);
> 

Re: [PATCH 5/7] nfs-utils: tidy up output of conf_report

[Steve Dickson]

More styling comments... 

On 04/23/2018 11:27 AM, Justin Mitchell wrote:
> Previously unused conf_report() function is modified to output
> an alphabetically sorted version of the current configuration
> to the nominated FILE handle.
> 
> Signed-off-by: Justin Mitchell <jumitche@redhat.com>
> ---
>  support/include/conffile.h |   3 +-
>  support/nfs/conffile.c     | 237 ++++++++++++++++++++++++++++-----------------
>  2 files changed, 151 insertions(+), 89 deletions(-)
> 
> diff --git a/support/include/conffile.h b/support/include/conffile.h
> index 6baaf9a..bc2d61f 100644
> --- a/support/include/conffile.h
> +++ b/support/include/conffile.h
> @@ -37,6 +37,7 @@
>  #include <ctype.h>
>  #include <stdint.h>
>  #include <stdbool.h>
> +#include <stdio.h>
>  
>  struct conf_list_node {
>  	TAILQ_ENTRY(conf_list_node) link;
> @@ -65,7 +66,7 @@ extern void     conf_cleanup(void);
>  extern int      conf_match_num(const char *, const char *, int);
>  extern int      conf_remove(int, const char *, const char *);
>  extern int      conf_remove_section(int, const char *);
> -extern void     conf_report(void);
> +extern void     conf_report(FILE *);
>  
>  /*
>   * Convert letter from upper case to lower case
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index 7ed5646..9148557 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -1055,117 +1055,178 @@ conf_end(int transaction, int commit)
>   * Configuration is "stored in reverse order", so reverse it again.
>   */
>  struct dumper {
> -	char *s, *v;
> +	char *section;
> +	char *arg;
> +	char *tag;
> +	char *value;
>  	struct dumper *next;
>  };
>  
> +static int dumper_section_compare(const struct dumper *nodea, const struct dumper *nodeb)
> +{
> +	int ret;
> +
> +	// missing node, shouldnt happen
> +	if (!nodea || !nodeb) return -1;
> +	// if only one has a section name, the blank one goes first
> +	if (!nodea->section &&  nodeb->section) return -1;
> +	if ( nodea->section && !nodeb->section) return  1;
> +	// no section names at all
> +	if (!nodea->section && !nodeb->section) return  0;
> +	// two section names, do they match
> +	ret = strcmp(nodea->section, nodeb->section);
> +	// section names differ, that was easy
> +	if (ret != 0) return ret;
> +	// sections matched, but how about sub-sections, again blank goes first
> +	if (!nodea->arg &&  nodeb->arg) return -1;
> +	if ( nodea->arg && !nodeb->arg) return  1;
> +
> +	// both have sub-section args and they differ
> +	if (nodea->arg && nodeb->arg && (ret=strcmp(nodea->arg, nodeb->arg))!=0)
> +		return ret;
Please use normal comment braces and this code is just
very hard to read do to one-lining your if statements and spacing.

> +
> +	return 0;
> +}
> +
> +static bool 
> +should_escape(const char * text)
> +{
> +	int len;
> +	if (!text) return false;
> +	if (isspace(text[0])) return true;
> +	len = strlen(text);
> +	if (isspace(text[len-1])) return true;
> +	return false;
Again... hard to read... New lines are a good thing! :-) 

> +}
> +
>  static void
> -conf_report_dump(struct dumper *node)
> +conf_report_dump_text(struct dumper *head, FILE *ff)
>  {
> -	/* Recursive, cleanup when we're done.  */
> -	if (node->next)
> -		conf_report_dump(node->next);
> -
> -	if (node->v)
> -		xlog(LOG_INFO, "%s=\t%s", node->s, node->v);
> -	else if (node->s) {
> -		xlog(LOG_INFO, "%s", node->s);
> -		if (strlen(node->s) > 0)
> -			free(node->s);
> +	const struct dumper *node = head;
> +	const struct dumper *last = NULL;
> +
> +	for (node=head; node!=NULL; node=node->next) {
> +		if (dumper_section_compare(last, node)!=0) {
> +			if (node != head) fprintf(ff, "\n");
> +			if (node->arg)
> +				fprintf(ff, "[%s \"%s\"]\n", node->section, node->arg);
> +			else
> +				fprintf(ff, "[%s]\n", node->section);
> +		}
> +		fprintf(ff, " %s", node->tag);
> +		if (node->value) {
> +			if (should_escape(node->value))
> +				fprintf(ff, " = \"%s\"", node->value);
> +			else
> +				fprintf(ff, " = %s", node->value);
> +		}
> +		fprintf(ff, "\n");
> +
> +		last = node;
>  	}
> +}
>  
> -	free (node);
> +/* compare two dumper nodes for sorting */
> +static int dumper_compare(const void * a, const void * b)
> +{
> +	const struct dumper *nodea = *(struct dumper **)a;
> +	const struct dumper *nodeb = *(struct dumper **)b;
> +	int ret;
> +
> +	// missing node, shouldnt happen
> +	if (!nodea || !nodeb) return -1;
> +
> +	ret = dumper_section_compare(nodea, nodeb);
> +	if (ret != 0) return ret;
> +
> +	// sub-sections match (both blank, or both same)
> +	// so we compare the tag names
> +	
> +	// blank tags shouldnt happen, but paranoia
> +	if (!nodea->tag &&  nodeb->tag) return -1;
> +	if ( nodea->tag && !nodeb->tag) return  1;
> +	if (!nodea->tag && !nodeb->tag) return  0;
> +
> +	// last test, compare the tags directly
> +	ret = strcmp(nodea->tag, nodeb->tag);
> +	return ret;
>  }
>  
> -void
> -conf_report (void)
> +static struct dumper *conf_report_sort(struct dumper *start)
>  {
> -	struct conf_binding *cb, *last = 0;
> -	unsigned int i, len, diff_arg = 0;
> -	char *current_section = (char *)0;
> -	char *current_arg = (char *)0;
> -	struct dumper *dumper, *dnode;
> +	struct dumper **list;
> +	struct dumper *node;
> +	unsigned int count = 0;
>  
> -	dumper = dnode = (struct dumper *)calloc(1, sizeof *dumper);
> -	if (!dumper)
> -		goto mem_fail;
> +	// how long is this list
> +	for (node=start; node!=NULL; node=node->next) count++;
> +
> +	// no need to sort a list of 1 or less
> +	if (count < 2) return start;
> +
> +	// make an array of all the node
> +	list = calloc(count, sizeof(struct dumper *));
> +	if (!list) goto mem_err;
> +
> +	unsigned int i=0;
> +	for (node=start; node!=NULL; node=node->next) {
> +		list[i++] = node;
> +	}
> +
> +	// sort the array
> +	qsort(list, count, sizeof(struct dumper *), dumper_compare);
> +
> +	// turn sorted array back into a linked list
Normal comments braces... please.. 

steved.
> +	for (i=0; i<count-1; i++) {
> +		list[i]->next = list[i+1];
> +	}
> +	list[count-1]->next = NULL;
> +
> +	// remember the new head of list and cleanup
> +	node = list[0];
> +	free(list);
> +
> +	// return the new head of list
> +	return node;
> +
> +mem_err:
> +	free(list);
> +	return NULL;
> +}
> +
> +void
> +conf_report (FILE *outfile)
> +{
> +	struct conf_binding *cb = NULL;
> +	unsigned int i;
> +	struct dumper *dumper = NULL, *dnode = NULL;
>  
>  	xlog(LOG_INFO, "conf_report: dumping running configuration");
>  
> -	for (i = 0; i < sizeof conf_bindings / sizeof conf_bindings[0]; i++)
> +	for (i = 0; i < sizeof conf_bindings / sizeof conf_bindings[0]; i++) {
>  		for (cb = LIST_FIRST(&conf_bindings[i]); cb; cb = LIST_NEXT(cb, link)) {
> -			if (!cb->is_default) {
> -				/* Make sure the Section arugment is the same */
> -				if (current_arg && current_section && cb->arg) {
> -					if (strcmp(cb->section, current_section) == 0 &&
> -						strcmp(cb->arg, current_arg) != 0)
> -					diff_arg = 1;
> -				}
> -				/* Dump this entry.  */
> -				if (!current_section || strcmp(cb->section, current_section) 
> -							|| diff_arg) {
> -					if (current_section || diff_arg) {
> -						len = strlen (current_section) + 3;
> -						if (current_arg)
> -							len += strlen(current_arg) + 3;
> -						dnode->s = malloc(len);
> -						if (!dnode->s)
> -							goto mem_fail;
> -
> -						if (current_arg)
> -							snprintf(dnode->s, len, "[%s \"%s\"]", 
> -								current_section, current_arg);
> -						else
> -							snprintf(dnode->s, len, "[%s]", current_section);
> -
> -						dnode->next = 
> -							(struct dumper *)calloc(1, sizeof (struct dumper));
> -						dnode = dnode->next;
> -						if (!dnode)
> -							goto mem_fail;
> -
> -						dnode->s = "";
> -						dnode->next = 
> -							(struct dumper *)calloc(1, sizeof (struct dumper));
> -						dnode = dnode->next;
> -						if (!dnode)
> -						goto mem_fail;
> -					}
> -					current_section = cb->section;
> -					current_arg = cb->arg;
> -					diff_arg = 0;
> -				}
> -				dnode->s = cb->tag;
> -				dnode->v = cb->value;
> -				dnode->next = (struct dumper *)calloc (1, sizeof (struct dumper));
> -				dnode = dnode->next;
> -				if (!dnode)
> -					goto mem_fail;
> -				last = cb;
> +			struct dumper *newnode = calloc(1, sizeof (struct dumper));
> +			if (!newnode) goto mem_fail;
> +
> +			newnode->next = dumper;
> +			dumper = newnode;
> +
> +			newnode->section = cb->section;
> +			newnode->arg = cb->arg;
> +			newnode->tag = cb->tag;
> +			newnode->value = cb->value;
>  		}
>  	}
>  
> -	if (last) {
> -		len = strlen(last->section) + 3;
> -		if (last->arg)
> -			len += strlen(last->arg) + 3;
> -		dnode->s = malloc(len);
> -		if (!dnode->s)
> -			goto mem_fail;
> -		if (last->arg)
> -			snprintf(dnode->s, len, "[%s \"%s\"]", last->section, last->arg);
> -		else
> -			snprintf(dnode->s, len, "[%s]", last->section);
> -	}
> -	conf_report_dump(dumper);
> -	return;
> +	dumper = conf_report_sort(dumper);
> +	conf_report_dump_text(dumper, outfile);
> +	goto cleanup;
>  
>  mem_fail:
>  	xlog_warn("conf_report: malloc/calloc failed");
> +cleanup:
>  	while ((dnode = dumper) != 0) {
>  		dumper = dumper->next;
> -		if (dnode->s)
> -			free(dnode->s);
>  		free(dnode);
>  	}
>  	return;
> 

Re: [PATCH 6/7] nfs-utils: Add nfsconftool cli

[Steve Dickson]

On 04/23/2018 11:34 AM, Justin Mitchell wrote:
> This tool uses the conffile facilities to allow commandline
> querying of configuration settings and to dump the current
> config for diagnosis and testing
> 
> Signed-off-by: Justin Mitchell <jumitche@redhat.com>
> ---
>  Makefile.am                |   2 +-
>  configure.ac               |   1 +
>  tools/Makefile.am          |   2 +
>  tools/nfsconf/Makefile.am  |   9 +++
>  tools/nfsconf/nfsconfcli.c | 144 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 157 insertions(+), 1 deletion(-)
>  create mode 100644 tools/nfsconf/Makefile.am
>  create mode 100644 tools/nfsconf/nfsconfcli.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index e1f39aa..0022084 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2,7 +2,7 @@
>  
>  AUTOMAKE_OPTIONS = foreign
>  
> -SUBDIRS = tools support utils linux-nfs tests systemd
> +SUBDIRS = support tools utils linux-nfs tests systemd
>  
>  MAINTAINERCLEANFILES = Makefile.in
>  
> diff --git a/configure.ac b/configure.ac
> index 5a11636..b925666 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -616,6 +616,7 @@ AC_CONFIG_FILES([
>  	tools/rpcgen/Makefile
>  	tools/mountstats/Makefile
>  	tools/nfs-iostat/Makefile
> +	tools/nfsconf/Makefile
>  	utils/Makefile
>  	utils/blkmapd/Makefile
>  	utils/nfsdcltrack/Makefile
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index f2ce282..4266da4 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -6,6 +6,8 @@ if CONFIG_RPCGEN
>  OPTDIRS += rpcgen
>  endif
>  
> +OPTDIRS += nfsconf
> +
>  SUBDIRS = locktest rpcdebug nlmtest mountstats nfs-iostat $(OPTDIRS)
>  
>  MAINTAINERCLEANFILES = Makefile.in
> diff --git a/tools/nfsconf/Makefile.am b/tools/nfsconf/Makefile.am
> new file mode 100644
> index 0000000..4baaa26
> --- /dev/null
> +++ b/tools/nfsconf/Makefile.am
> @@ -0,0 +1,9 @@
> +## Process this file with automake to produce Makefile.in
> +
> +bin_PROGRAMS = nfsconftool
> +
> +nfsconftool_SOURCES = nfsconfcli.c
> +nfsconftool_LDADD = ../../support/nfs/libnfsconf.la
> +
> +MAINTAINERCLEANFILES = Makefile.in
> +
> diff --git a/tools/nfsconf/nfsconfcli.c b/tools/nfsconf/nfsconfcli.c
> new file mode 100644
> index 0000000..1443e97
> --- /dev/null
> +++ b/tools/nfsconf/nfsconfcli.c
> @@ -0,0 +1,144 @@
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <getopt.h>
> +#include <string.h>
> +#include <errno.h>
> +#include "config.h"
> +#include "conffile.h"
> +#include "xlog.h"
> +
> +typedef enum {
> +	MODE_NONE,
> +	MODE_GET,
> +	MODE_ISSET,
> +	MODE_DUMP
> +} confmode_t;
> +
> +static void usage(const char *name)
> +{
> +	fprintf(stderr, "Usage: %s [-v] [--file filename.conf] ...\n", name);
> +	fprintf(stderr, "Options:\n");
> +	fprintf(stderr, " -v			Increase Verbosity\n");
> +	fprintf(stderr, " --file filename.conf	Load this config file\n");
> +	fprintf(stderr, "     (Default config file: " NFS_CONFFILE "\n");
> +	fprintf(stderr, "Modes:\n");
> +	fprintf(stderr, "  --dump [outputfile]\n");
> +	fprintf(stderr, "      Outputs the configuration to the named file\n");
> +	fprintf(stderr, "  --get [--arg subsection] {section} {tag}\n");
> +	fprintf(stderr, "      Output one specific config value\n");
> +	fprintf(stderr, "  --isset [--arg subsection] {section} {tag}\n");
> +	fprintf(stderr, "      Return code indicates if config value is present\n");
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	const char * val;
> +	char * confpath = NFS_CONFFILE;
> +	char * arg = NULL;
> +	int verbose=0;
> +	int ret = 0;
> +	char * dumpfile = NULL;
> +
> +	confmode_t mode = MODE_NONE;
> +
> +	while (1) {
> +		int c;
> +		int index = 0;
> +		struct option long_options[] = {
> +			{"get",		no_argument, 0, 'g' },
> +			{"arg",	  required_argument, 0, 'a' },
> +			{"isset", 	no_argument, 0, 'i' },
> +			{"dump",  optional_argument, 0, 'd' },
> +			{"file",  required_argument, 0, 'f' },
> +			{"verbose",	no_argument, 0, 'v' },
> +			{NULL,			  0, 0, 0 }
> +		};
> +
> +		c = getopt_long(argc, argv, "ga:id::f:v", long_options, &index);
> +		if (c == -1) break;
> +
> +		switch (c) {
> +			case 0:
> +				break;
> +			case 'f':
> +				confpath = optarg;
> +				break;
> +			case 'a':
> +				arg = optarg;
> +				break;
> +			case 'v':
> +				verbose++;
> +				break;
> +			case 'g':
> +				mode = MODE_GET;
> +				break;
> +			case 'i':
> +				mode = MODE_ISSET;
> +				break;
> +			case 'd':
> +				if (optarg==NULL && argv[optind]!=NULL && argv[optind][0]!='-')
> +					optarg = argv[optind++];
> +				mode = MODE_DUMP;
> +				dumpfile = optarg;
> +				break;
> +			default:
> +				usage(argv[0]);
> +				return 1;
> +		}
> +	}
> +
> +	if (verbose) xlog_config(D_ALL, 1);
New line please.. 
> +	xlog_stderr(1);
> +	xlog_syslog(0);
> +	xlog_open("nfsconf");
> +
> +	if (mode == MODE_NONE) {
> +		fprintf(stderr, "Error: No MODE selected.\n");
> +		usage(argv[0]);
> +		return 1;
> +	}
> +
> +	if (conf_init_file(confpath)) {
> +		if (verbose || mode != MODE_ISSET) fprintf(stderr, "Error loading config file %s\n", confpath);
> +		if (mode != MODE_ISSET) return 1;
> +	}
> +
> +	if (mode == MODE_DUMP) {
> +		FILE *out = stdout;
Declarations up to[ please... and I don't understand why this assignment is even needed
 
> +		if (dumpfile) {
> +			if ((out=fopen(dumpfile, "w"))==NULL) {
> +				fprintf(stderr, "Error opening dumpfile %s: %s\n", dumpfile, strerror(errno));
> +				ret = 2;
> +				goto cleanup;
> +			}
> +			if (verbose) printf("Dumping config to %s\n", dumpfile);
> +		}
> +		conf_report(out);
> +		if (dumpfile) fclose(out);
closing stdout??

> +	} else
> +	if (mode == MODE_GET || mode == MODE_ISSET) {
> +		if (optind+1 >= argc) {
> +			fprintf(stderr, "Error: insufficient arguments for mode\n");
> +			usage(argv[0]);
> +			ret = 2;
> +			goto cleanup;
> +		}
> +		char * section = argv[optind++];
> +		char * tag = argv[optind++];
Up top please... 
> +
> +		if ((val=conf_get_section(section, arg, tag))!=NULL) {
> +			if (mode == MODE_GET) printf("%s\n", val);
> +		} else {
> +			if (mode == MODE_GET && verbose) fprintf(stderr, "Tag '%s' not found\n", tag);
> +			ret = 1;
> +		}
There is a lot going on here... having a few newlines and spacing would help a lot.

steved.
> +	} else {
> +		fprintf(stderr, "Mode not yet implimented.\n");
> +		ret = 2;
> +	}
> +
> +cleanup:
> +	conf_cleanup();
> +	return ret;
> +}
> 

Re: [PATCH 2/7] nfs-utils: Make config includes relative to current config

[Justin Mitchell]

On Wed, 2018-05-02 at 14:21 -0400, Steve Dickson wrote:
> Hi Justin,
> 
> Sorry for taking so long to get to this... 
> 
> Most of my comments is about the style you are using.
> 
> I would like to keep the coding style somewhat the same 
> and in a few places that is not just not happening...

Yeah sorry I should have paid more attention to maintaining style,
will fix it all up and repost.

> >  /*
> >   * Parse the line LINE of SZ bytes.  Skip Comments, recognize section
> >   * headers and feed tag-value pairs into our configuration database.
> >   */
> >  static void
> > -conf_parse_line(int trans, char *line, int lineno, char **section, char **subsection)
> > +conf_parse_line(int trans, char *line, const char *filename, int lineno, char **section, char **subsection)
> >  {
> >  	char *val, *ptr;
> >  
> > @@ -366,10 +395,12 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
> >  
> >  	if (strcasecmp(line, "include")==0) {
> >  		/* load and parse subordinate config files */
> > -		char * subconf = conf_readfile(val);
> > +		char * relpath = relative_path(filename, val);
> > +		char * subconf = conf_readfile(relpath);
> Same here no space also isn't subconf being over written?
nope, conf_readfile() returns a malloced chunk that gets freed at the
end of the include handler


> > @@ -383,10 +414,11 @@ conf_parse_line(int trans, char *line, int lineno, char **section, char **subsec
> >  				inc_subsection = strdup(*subsection);
> >  		}
> >  
> > -		conf_parse(trans, subconf, &inc_section, &inc_subsection);
> > +		conf_parse(trans, subconf, &inc_section, &inc_subsection, relpath);
> >  
> >  		if (inc_section) free(inc_section);
> >  		if (inc_subsection) free(inc_subsection);
> > +		if (relpath) free(relpath);
> Are these if even needed? Won't these either be NULL or allocated memory?
yes, inc_section and inc_subsection are strdup()ed copies of the current
section titles because conf_parse() will alter them if any new section
headers are encountered, yet we needed to know what section we were in
when we entered this included conf file.

the way it is written will correctly handle recursive includes.

in general all my code changes are avoiding any use of globals and
statics in the hope of eventually making the code base thread-safe and
suitable for use as a public library, so that tends to mean more freeing
of short lived malloced chunks.