conf: instantiate %i templates for the block format

A template in the block format registered garbage.  conf_parse_file()
routed every file with an '@' in its name straight to the legacy
parser, which read the block line by line: the section header became a
service whose command was the section title, and each key = value line
below it became an environment variable.

    service serv:%i { ... }   ->  service 'serv:eth0' with argument '{'

Substitute %i over the whole file before parsing instead, so format
detection and both parsers see finished text.  A bare name@.conf is
still skipped, it is the template rather than an instance of one.

The legacy parser no longer opens the file or substitutes per line, it
is handed the instantiated buffer, so the template convention now has
one implementation instead of two.  conf_is_template() applies
basenm(), a directory with an '@' in its name is not a template.

libconfuse cannot name a buffer it parses before 3.4, so a typo in a
template would be reported against "[buf]".  Parse through fmemopen()
with the file name preset until the floor moves.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

configure.ac

@@ -50,9 +50,11 @@ PKG_PROG_PKG_CONFIG
 # Check for required libraries
 PKG_CHECK_MODULES([uev],  [libuev >= 2.4.1])
 PKG_CHECK_MODULES([lite], [libite >= 2.6.1])
-# 3.3 is the floor: CFGF_KEYSTRVAL, which set {} and the free-form
-# cgroup keys are built on, does not exist before it.  3.3 parses both
-# correctly, see the XXX in src/conf.c before raising this to 3.4.
+# 3.3 is the floor: CFGF_KEYSTRVAL, which environment {} and the
+# free-form cgroup keys are built on, does not exist before it.  3.3
+# parses both correctly.  Two workarounds hang off this number, the
+# spurious KEYSTRVAL warning and cfg_parse_buf() losing the file name
+# of a template; grep the XXX notes in src/conf.c before raising it.
 PKG_CHECK_MODULES([confuse], [libconfuse >= 3.3])
 
 # Check for configured Finit features

src/conf.c

@@ -1525,6 +1525,105 @@ static int conf_parse_cfg(cfg_t *cfg, char *file, int is_rcsd)
 	return 0;
 }
 
+/*
+ * Substitute every %i in LINE with NAME, for template instantiation.
+ *
+ * Very simple and crude implementation, only supports '%i'
+ */
+static char *conf_instantiate(char *line, char *name)
+{
+	char *ptr, *end = strchr(line, 0);
+	char *pos = line;
+	size_t num = 0;
+
+	if (!name[0] || !end)
+		return line;
+
+	while ((ptr = strchr(pos, '%'))) {
+		num++;
+		pos = ptr + 1;
+	}
+
+	ptr = realloc(line, strlen(line) + num * strlen(name) + 1);
+	if (!ptr)
+		return line;
+
+	pos = line = ptr;
+	while ((ptr = strchr(pos, '%'))) {
+		if (!strncmp(ptr, "%i", 2)) {
+			char *rest = &ptr[2];
+			char *next = ptr + strlen(name);
+
+			memmove(next, rest, strlen(rest) + 1);
+			memcpy(ptr, name, strlen(name));
+
+			pos += strlen(name);
+		} else
+			pos++;
+	}
+
+	return line;
+}
+
+static int conf_is_template(const char *file, char *name, size_t len)
+{
+	char *ptr, *nm;
+	size_t i = 0;
+
+	/* the @ convention names the file, a directory may contain one */
+	ptr = strchr(basenm((char *)file), '@');
+	if (!ptr)
+		return 0;	/* not a template */
+
+	nm = ptr + 1;
+	ptr = strstr(nm, ".conf");
+	if (!strcmp(nm, ".conf") || !ptr)
+		return 1;	/* template itself or invalid */
+
+	if (!name)
+		return 1;
+
+	while (nm < ptr && i < len - 1)
+		name[i++] = *nm++;
+	name[i] = 0;
+
+	return 1;		/* instantiated template */
+}
+
+/*
+ * Parse from FILE, or from BUF when it is set, i.e. for a template
+ * whose %i has already been substituted.
+ *
+ * XXX: Workaround for libConfuse <3.4, whose cfg_parse_buf() replaces
+ *      cfg->filename with "[buf]", so every diagnostic from a template
+ *      would lose the file name.  cfg_parse_fp() keeps a name the
+ *      caller has already set, so go through fmemopen() instead.  With
+ *      a 3.4 floor this is just cfg_parse_buf(cfg, buf).
+ */
+static int conf_parse_any(cfg_t *cfg, char *file, char *buf)
+{
+	FILE *fp;
+	int rc;
+
+	if (!buf)
+		return cfg_parse(cfg, file);
+
+	/* fmemopen() rejects a zero length on older GLIBC */
+	if (!buf[0])
+		return CFG_SUCCESS;
+
+	fp = fmemopen(buf, strlen(buf), "r");
+	if (!fp)
+		return CFG_FILE_ERROR;
+
+	/* cfg_parse_fp() only names the stream when we have not */
+	cfg->filename = strdup(file);
+	rc = cfg_parse_fp(cfg, fp);
+	fclose(fp);
+
+	return rc;
+}
+
 /*
  * Is this a new-format file, or a legacy one?
  *
@@ -1539,7 +1638,7 @@ static int conf_parse_cfg(cfg_t *cfg, char *file, int is_rcsd)
  * creating them, so a lenient tree is missing every set{} variable
  * and every free-form cgroup key.
  */
-static int is_new_format(char *file)
+static int is_new_format(char *file, char *buf)
 {
 	cfg_t *cfg;
 	int rc;
@@ -1549,12 +1648,44 @@ static int is_new_format(char *file)
 		return 0;
 
 	cfg_set_error_function(cfg, cfg_error_quiet);
-	rc = cfg_parse(cfg, file);
+	rc = conf_parse_any(cfg, file, buf);
 	cfg_free(cfg);
 
 	return rc == CFG_SUCCESS;
 }
 
+/*
+ * Read FILE and substitute %i with NAME, for template instantiation.
+ * Returns a malloc()'ed buffer the caller frees.
+ */
+static char *conf_read_template(char *file, char *name)
+{
+	struct stat st;
+	char *buf = NULL;
+	size_t len;
+	FILE *fp;
+
+	fp = fopen(file, "r");
+	if (!fp)
+		return NULL;
+
+	/* fstat() the open fd, no window for the file to change under us */
+	if (fstat(fileno(fp), &st) || st.st_size < 0)
+		goto out;
+
+	buf = malloc((size_t)st.st_size + 1);
+	if (!buf)
+		goto out;
+
+	len = fread(buf, 1, (size_t)st.st_size, fp);
+	buf[len] = 0;
+	buf = conf_instantiate(buf, name);
+out:
+	fclose(fp);
+
+	return buf;
+}
+
 /*
  * Parse one Finit .conf file, in either format.  The legacy .conf
  * include directive routes back through this so included files are
@@ -1562,43 +1693,63 @@ static int is_new_format(char *file)
  */
 int conf_parse_file(char *file, int is_rcsd)
 {
+	char name[MAX_ID_LEN] = { 0 };
+	char *buf = NULL;
 	cfg_t *cfg;
 	int rc;
 
 	/*
-	 * Template files (name@.conf, name@id.conf): %i instantiation
-	 * for the new format is not yet supported, so these bypass
-	 * detection entirely and go to the legacy parser, which handles
-	 * templates per line.  See issue #148.
+	 * Template files (name@.conf, name@id.conf).  A bare name@.conf
+	 * is the template itself, there is nothing to instantiate from
+	 * it.  Otherwise %i is substituted over the whole file up front,
+	 * so both the format detection below and, on fallback, the
+	 * legacy parser see the finished text.
 	 */
-	if (strchr(basenm(file), '@'))
-		return legacy_parse_conf(file, is_rcsd);
+	if (conf_is_template(file, name, sizeof(name))) {
+		if (!name[0]) {
+			dbg("*** Skipping template file %s", file);
+			return 0;
+		}
+
+		dbg("*** instantiating %s from %s ...", name, file);
+		buf = conf_read_template(file, name);
+		if (!buf)
+			return 1;
+	}
 
 	cfg = cfg_init(conf_opts, CFGF_NONE);
-	if (!cfg)
-		return legacy_parse_conf(file, is_rcsd);
+	if (!cfg) {
+		rc = legacy_parse_conf(file, buf, is_rcsd);
+		goto done;
+	}
 
 	cfg_set_error_function(cfg, cfg_error_cb);
-	rc = cfg_parse(cfg, file);
+	rc = conf_parse_any(cfg, file, buf);
 	if (rc == CFG_SUCCESS) {
 		dbg("*** Parsing %s (new format)", file);
 		rc = conf_parse_cfg(cfg, file, is_rcsd);
 		cfg_free(cfg);
-		return rc;
+		goto done;
 	}
 	cfg_free(cfg);
 
-	if (rc == CFG_FILE_ERROR)
-		return 1;	/* like legacy fopen() failure */
+	if (rc == CFG_FILE_ERROR) {
+		rc = 1;		/* like legacy fopen() failure */
+		goto done;
+	}
 
-	if (is_new_format(file)) {
+	if (is_new_format(file, buf)) {
 		logit(LOG_ERR, "parse error: %s", cfg_errmsg);
-		return 1;
+		rc = 1;
+		goto done;
 	}
 
 	dbg("not in new format (%s), falling back to legacy parser", cfg_errmsg);
+	rc = legacy_parse_conf(file, buf, is_rcsd);
+done:
+	free(buf);
 
-	return legacy_parse_conf(file, is_rcsd);
+	return rc;
 }
 
 static void glob_append(glob_t *gl, int append, const char *fmt, ...)
@@ -1849,7 +2000,7 @@ static int conf_change_act(char *dir, char *name, uint32_t mask)
 		strlcpy(fn, dir, sizeof(fn));
 	dbg("path: %s mask: %08x", fn, mask);
 
-	if (strchr(name, '@')) {
+	if (conf_is_template(name, NULL, 0)) {
 		/* Skip realpath for templates */
 		rp = strdup(fn);
 	} else {
@@ -1903,7 +2054,7 @@ int conf_changed(char *file)
 	if (!file)
 		return 0;
 
-	if (strchr(file, '@'))
+	if (conf_is_template(file, NULL, 0))
 		rp = strdup(file);
 	else
 		rp = realpath(file, NULL);

src/conf.h

@@ -83,6 +83,7 @@ int   conf_parse_runlevels (const char *runlevels);
 void  conf_parse_cond      (svc_t *svc, char *cond);
 int   conf_parse_file      (char *file, int is_rcsd);
 
+
 #endif	/* FINIT_CONF_H_ */
 
 /**

src/legacy.c

@@ -319,79 +319,19 @@ static int parse_dynamic(char *line, struct rlimit rlimit[], char *file)
 }
 
 /*
- * Very simple and crude implementation, only supports '%i'
+ * Parse FILE, or BUF when the frontend has already read and
+ * instantiated it, i.e. for a template.  Either way the text arriving
+ * here is final, no %i is left to substitute.
  */
-static char *instantiate(char *line, char *name)
-{
-	char *ptr, *end = strchr(line, 0);
-	char *pos = line;
-	size_t num = 0;
-
-	if (!name[0] || !end)
-		return line;
-
-	while ((ptr = strchr(pos, '%'))) {
-		num++;
-		pos++;
-	}
-
-	ptr = realloc(line, strlen(line) + num * strlen(name) + 1);
-	if (!ptr)
-		return line;
-
-	pos = line = ptr;
-	while ((ptr = strchr(pos, '%'))) {
-		if (!strncmp(ptr, "%i", 2)) {
-			char *rest = &ptr[2];
-			char *next = ptr + strlen(name);
-
-			memmove(next, rest, strlen(rest) + 1);
-			memcpy(ptr, name, strlen(name));
-
-			pos += strlen(name);
-		} else
-			pos++;
-	}
-
-	return line;
-}
-
-static int is_template(const char *file, char *name, size_t len)
-{
-	char *ptr, *nm;
-	size_t i = 0;
-
-	ptr = strchr(file, '@');
-	if (!ptr)
-		return 0;	/* not a template */
-
-	nm = ptr + 1;
-	ptr = strstr(nm, ".conf");
-	if (!strcmp(nm, ".conf") || !ptr)
-		return 1;	/* template itself or invalid */
-
-	while (nm < ptr && i < len - 1)
-		name[i++] = *nm++;
-	name[i] = 0;
-
-	return 1;		/* instantiated template */
-}
-
-int legacy_parse_conf(char *file, int is_rcsd)
+int legacy_parse_conf(char *file, char *buf, int is_rcsd)
 {
 	struct rlimit rlimit[RLIMIT_NLIMITS];
-	char name[65] = { 0 };
 	FILE *fp;
 
-	if (is_template(file, name, sizeof(name))) {
-		if (!name[0]) {
-			dbg("*** Skipping template file %s", file);
-			return 0;
-		}
-		dbg("*** instantiating %s from %s ...", name, file);
-	}
-
-	fp = fopen(file, "r");
+	if (buf)
+		fp = fmemopen(buf, strlen(buf), "r");
+	else
+		fp = fopen(file, "r");
 	if (!fp)
 		return 1;
 
@@ -410,9 +350,6 @@ int legacy_parse_conf(char *file, int is_rcsd)
 			continue;
 
 		tabstospaces(line);
-//		dbg("raw: %s", line);
-		line = instantiate(line, name);
-//		dbg("ins: %s", line);
 
 		if (!parse_static(line, is_rcsd))
 			;

src/legacy.h

@@ -24,7 +24,7 @@
 #ifndef FINIT_LEGACY_H_
 #define FINIT_LEGACY_H_
 
-int   legacy_parse_conf (char *file, int is_rcsd);
+int   legacy_parse_conf (char *file, char *buf, int is_rcsd);
 void  legacy_parse_env  (char *line);
 
 #endif	/* FINIT_LEGACY_H_ */

test/Makefile.am

@@ -32,6 +32,7 @@ EXTRA_DIST		+= add-remove-dynamic-service-sub-config.sh
 EXTRA_DIST		+= bootstrap-crash.sh
 EXTRA_DIST		+= cond-start-task.sh
 EXTRA_DIST		+= conf-format.sh
+EXTRA_DIST		+= conf-template.sh
 EXTRA_DIST		+= crashing.sh
 EXTRA_DIST		+= dep-chain-reload.sh
 EXTRA_DIST		+= depserv.sh
@@ -77,6 +78,7 @@ TESTS			+= add-remove-dynamic-service-sub-config.sh
 TESTS			+= bootstrap-crash.sh
 TESTS			+= cond-start-task.sh
 TESTS			+= conf-format.sh
+TESTS			+= conf-template.sh
 TESTS			+= crashing.sh
 TESTS			+= dep-chain-reload.sh
 TESTS			+= depserv.sh

test/conf-template.sh

@@ -0,0 +1,82 @@
+#!/bin/sh
+# Verify %i template instantiation for both .conf formats: the block
+# format substitutes over the whole file before parsing, the legacy
+# one-liner format per line.  A bare name@.conf registers nothing.
+set -eu
+
+TEST_DIR=$(dirname "$0")
+
+test_teardown()
+{
+    say "Running test teardown."
+    run "rm -f $FINIT_RCSD/available/serv@.conf"
+    run "rm -f $FINIT_RCSD/enabled/serv@eth0.conf"
+    run "rm -f $FINIT_RCSD/enabled/serv@eth1.conf $FINIT_RCSD/enabled/serv@.conf"
+    run "rm -f /run/serv-eth0.pid /run/serv-eth1.pid"
+}
+
+# shellcheck source=/dev/null
+. "$TEST_DIR/lib/setup.sh"
+
+# %i has to reach three different places: the section title, which
+# becomes the instance identity, an ordinary value, and the command
+# line.  The per-instance PID file covers the last one, it can only
+# appear if the command was instantiated.
+say 'Install a block-format template'
+run "echo 'service serv:%i {'                          >  $FINIT_RCSD/available/serv@.conf"
+run "echo '    description = \"Template for %i\"'      >> $FINIT_RCSD/available/serv@.conf"
+run "echo '    pid         = \"/run/serv-%i.pid\"'     >> $FINIT_RCSD/available/serv@.conf"
+run "echo '    command     = \"serv -n -p -P /run/serv-%i.pid\"' >> $FINIT_RCSD/available/serv@.conf"
+run "echo '}'                                          >> $FINIT_RCSD/available/serv@.conf"
+
+say 'Enable two instances'
+run "initctl enable serv@eth0.conf"
+run "initctl enable serv@eth1.conf"
+run "initctl reload"
+
+retry 'assert_num_services 2 serv'
+assert_desc "Template for eth0" serv:eth0
+assert_desc "Template for eth1" serv:eth1
+
+say 'Both instances run, each with its own instantiated PID file'
+retry 'assert_num_children 2 serv'
+retry 'assert_file_exists /run/serv-eth0.pid'
+retry 'assert_file_exists /run/serv-eth1.pid'
+
+# available/ is never globbed, so enabling the bare template is the
+# only way the "skip the template itself" branch is reached at all.
+say 'A bare template in enabled/ registers nothing'
+run "ln -sf ../available/serv@.conf $FINIT_RCSD/enabled/serv@.conf"
+run "initctl reload"
+
+retry 'assert_num_services 2 serv'
+run "rm -f $FINIT_RCSD/enabled/serv@.conf"
+
+# assert_num_services cannot express "exactly one": initctl status
+# prints a detail block for a single match and a table only for
+# several, so the line count is 13, not 1.  Check the survivor and the
+# absence of the other instead.
+say 'Disable one instance'
+run "initctl disable serv@eth1.conf"
+run "initctl reload"
+
+retry 'assert_num_children 1 serv'
+assert_desc "Template for eth0" serv:eth0
+assert_num_services 0 serv:eth1
+
+say 'Legacy one-liner templates still instantiate'
+run "echo 'service :%i pid:/run/serv-%i.pid serv -n -p -P /run/serv-%i.pid -- Legacy template for %i' > $FINIT_RCSD/available/serv@.conf"
+run "initctl reload"
+
+retry 'assert_num_children 1 serv'
+assert_desc "Legacy template for eth0" serv:eth0
+
+say 'A typo in a block template is rejected, and named after the instance'
+run "echo 'service serv:%i {'                          >  $FINIT_RCSD/available/serv@.conf"
+run "echo '    descriptoin = \"Template for %i\"'      >> $FINIT_RCSD/available/serv@.conf"
+run "echo '    command     = \"serv -n\"'              >> $FINIT_RCSD/available/serv@.conf"
+run "echo '}'                                          >> $FINIT_RCSD/available/serv@.conf"
+run "initctl reload"
+
+retry 'assert_num_children 0 serv'
+assert_num_services 0 serv