Merge pull request #522 from firasuke/master

doc: list libcap as a dependency

doc/ChangeLog.md

@@ -153,6 +153,12 @@ All relevant changes are documented in this file.
 
 ### Fixes
 
+- Fix #512: `--` in the `command` of a block-format .conf, it was taken as the
+  start of the description, so `command = "cmd -- arg"` ran `cmd` with
+  description `arg`.  Found by Alexandru Paniș
+- Fix a command whose name starts like a service option, e.g. `logger` or
+  `pidof`.  It was taken as the `log` or `pid` option and the next argument
+  ran as the command, which also broke the `logger` example in finit.conf(5)
 - Reject a .conf file that declares the same block title twice.  The title is
   the service identity and libConfuse merges two sections sharing one, so the
   file loaded as a single service holding a mix of both declarations,

doc/build.md

@@ -15,6 +15,10 @@ and `/dev/disk/by-label/` symlinks after.  keventd is enabled by
 default, so this is a hard requirement unless you build with
 `--without-keventd`.
 
+Linux capabilities support for services is also built by default.  It needs
+[libcap][] (-lcap); the build falls back to no capabilities support when that
+library is missing, or with `--disable-libcap`.
+
 PAM session support for services is built by default.  It needs
 [Linux-PAM][] (-lpam); the build falls back to no PAM support when that
 library is missing.  A static build, `--enable-static`, disables PAM
@@ -37,6 +41,7 @@ version of the above mentioned libraries.  Currently required versions:
 - libuEv v2.2.0, or later
 - libConfuse v3.3, or later
 - libblkid, any version, when keventd is enabled
+- libcap, any version, unless `--disable-libcap` is given
 
 
 Configure
@@ -59,6 +64,10 @@ Below are a few of the main switches to configure:
   built-ins (.o files) and all external libraries, except the C library
   will be linked statically.
 
+* `--disable-libcap`: Opt out of Finit's built-in Linux capabilities support,
+  enabled by default.  Needs libcap, and falls back to disabled when it is
+  missing.  See [Linux Capabilities](config/capabilities.md)
+
 * `--disable-pam`: Opt out of Finit's built-in PAM session support,
   enabled by default, which lets a service declare `pam = "name"` and
   run inside a session set up from `/etc/pam.d/name`.  Needs libpam,
@@ -298,4 +307,5 @@ it only for debugging start up issues when Finit crashes.
 [libite]:  https://github.com/troglobit/libite
 [libConfuse]: https://github.com/libconfuse/libconfuse
 [util-linux]: https://github.com/util-linux/util-linux
+[libcap]:     https://sites.google.com/site/fullycapable/
 [Linux-PAM]:  https://github.com/linux-pam/linux-pam

doc/config/capabilities.md

@@ -169,8 +169,9 @@ ps -o user,pid,cmd -p $(pidof nginx)
 
 - Linux kernel 4.3+ (for ambient capabilities support)
 - libcap library installed
-- Finit built with `--enable-libcap`, otherwise a `capabilities` list is
-  ignored, with a warning
+- Finit built with libcap support (enabled by default unless `--disable-libcap`
+  is given or libcap was missing at build time), otherwise a `capabilities` list
+  is ignored, with a warning
 
 ## Limitations
 

keventd/builtin.c

@@ -81,6 +81,13 @@ static int builtin_kmod(struct uevent *ev, int argc, char **argv)
 
 /* ---- hwdb -------------------------------------------------------- */
 
+/*
+ * Longest lookup key.  A key built by the stock name-based evdev rule
+ * (name, phys, ev and the DMI modalias) is 280 to 300 bytes with typical
+ * DMI strings, and 250 to 265 bytes for the plain name rule.
+ */
+#define HWDB_KEY_MAX 1024
+
 /*
  * Match a single text hwdb file against the lookup key.
  * Text format: match lines at column 0, property lines indented.
@@ -106,7 +113,11 @@ static void hwdb_match_file(const char *path, const char *lookup,
 	while (fgets(line, sizeof(line), fp)) {
 		line[strcspn(line, "\n")] = '\0';
 
-		if (!line[0] || line[0] == '#') {
+		/* Comments do not end the record being read */
+		if (line[0] == '#')
+			continue;
+
+		if (!line[0]) {
 			matched  = 0;
 			in_props = 0;
 			continue;
@@ -145,9 +156,11 @@ static void hwdb_match_file(const char *path, const char *lookup,
 /*
  * Build the hwdb lookup key for input devices.
  *
- * udev hwdb files use the format:
- *   evdev:input:b{BUS}v{VENDOR}p{PRODUCT}e{VERSION}*
- * where all fields are 4-digit uppercase hex.
+ * The key is the modalias of the input device, which the rule's
+ * --lookup-prefix (evdev:, id-input:modalias:) is put in front of:
+ *   input:b{BUS}v{VENDOR}p{PRODUCT}e{VERSION}-e{CAPABILITIES...}
+ * where the IDs are 4-digit uppercase hex.  If the device has no modalias
+ * attribute, the part up to the version is built from the id/ files.
  *
  * The id/ files live under the input device directory.  For eventN nodes
  * (devpath ends in .../inputN/eventN) we strip the last component first.
@@ -156,7 +169,7 @@ static void hwdb_input_key(const struct uevent *ev, char *key, size_t len)
 {
 	char base[PATH_MAX];
 	char path[PATH_MAX];
-	char bus_s[8], vendor_s[8], product_s[8], version_s[8];
+	char bus_s[8] = "", vendor_s[8] = "", product_s[8] = "", version_s[8] = "";
 
 	if (!ev->devpath)
 		return;
@@ -180,11 +193,28 @@ static void hwdb_input_key(const struct uevent *ev, char *key, size_t len)
 		snprintf(path, sizeof(path), "/sys%s", ev->devpath);
 	}
 
-	snprintf(base, sizeof(base), "%s/id/vendor",  path); sysfs_read_file(base, vendor_s,  sizeof(vendor_s));
-	snprintf(base, sizeof(base), "%s/id/product",  path); sysfs_read_file(base, product_s, sizeof(product_s));
-	snprintf(base, sizeof(base), "%s/id/version",  path); sysfs_read_file(base, version_s, sizeof(version_s));
+	/*
+	 * Prefer the modalias of the input device.  Unlike the id/ files it
+	 * carries the capability bitmaps (-e0,1,2,4,k110,...) that some
+	 * entries match on.
+	 */
+	snprintf(base, sizeof(base), "%s/modalias", path);
+	if (!sysfs_read_file(base, key, len) && !strncmp(key, "input:", 6))
+		return;
+	key[0] = '\0';
 
-	snprintf(key, len, "evdev:input:b%04Xv%04Xp%04Xe%04X*",
+	/* Otherwise build it from the id/ files; without a full set there is no key. */
+	snprintf(base, sizeof(base), "%s/id/vendor", path);
+	if (sysfs_read_file(base, vendor_s, sizeof(vendor_s)) < 0)
+		return;
+	snprintf(base, sizeof(base), "%s/id/product", path);
+	if (sysfs_read_file(base, product_s, sizeof(product_s)) < 0)
+		return;
+	snprintf(base, sizeof(base), "%s/id/version", path);
+	if (sysfs_read_file(base, version_s, sizeof(version_s)) < 0)
+		return;
+
+	snprintf(key, len, "input:b%04Xv%04Xp%04Xe%04X*",
 		 (unsigned)strtoul(bus_s,     NULL, 16),
 		 (unsigned)strtoul(vendor_s,  NULL, 16),
 		 (unsigned)strtoul(product_s, NULL, 16),
@@ -265,24 +295,48 @@ static void hwdb_scan(const char *key, struct uevent *ev)
 	}
 }
 
+/* Insert prefix in front of key, in place; -1 if the result does not fit. */
+static int hwdb_prepend(char *key, size_t len, const char *prefix)
+{
+	size_t plen = strlen(prefix), klen = strlen(key);
+
+	if (plen + klen >= len)
+		return -1;
+	memmove(key + plen, key, klen + 1);
+	memcpy(key, prefix, plen);
+	return 0;
+}
+
 static int builtin_hwdb(struct uevent *ev, int argc, char **argv)
 {
 	const char *subsys = NULL;
 	const char *prefix = NULL;
-	char        key[256] = "";
+	const char *explicit_key = NULL;
+	char        key[HWDB_KEY_MAX] = "";
 	int         i;
 
-	/* Parse --subsystem=X and --lookup-prefix=Y */
+	/*
+	 * Parse --subsystem=X and --lookup-prefix=Y.  The first argument
+	 * that is not an option is the lookup key itself, which is how the
+	 * stock udev rules use the builtin:
+	 *
+	 *   IMPORT{builtin}="hwdb 'evdev:name:$attr{name}:$attr{[dmi/id]modalias}'"
+	 */
 	for (i = 1; i < argc; i++) {
 		if (!strncmp(argv[i], "--subsystem=", 12))
 			subsys = argv[i] + 12;
 		else if (!strncmp(argv[i], "--lookup-prefix=", 16))
 			prefix = argv[i] + 16;
+		else if (argv[i][0] != '-' && !explicit_key)
+			explicit_key = argv[i];
 	}
 	if (!subsys)
 		subsys = ev->subsystem;
 
-	if (subsys && !strcmp(subsys, "input")) {
+	if (explicit_key) {
+		if (snprintf(key, sizeof(key), "%s", explicit_key) >= (int)sizeof(key))
+			goto too_long;
+	} else if (subsys && !strcmp(subsys, "input")) {
 		hwdb_input_key(ev, key, sizeof(key));
 	} else if (subsys && (!strcmp(subsys, "usb") ||
 			      !strcmp(subsys, "usb_device"))) {
@@ -290,29 +344,30 @@ static int builtin_hwdb(struct uevent *ev, int argc, char **argv)
 	}
 
 	/* Fall back to MODALIAS / ev->modalias for pci, platform, etc. */
-	if (!key[0]) {
+	if (!key[0] && !explicit_key) {
 		const char *m = uevent_getenv(ev, "MODALIAS");
 
 		if (!m)
 			m = ev->modalias;
-		if (m)
-			snprintf(key, sizeof(key), "%s", m);
+		if (m && snprintf(key, sizeof(key), "%s", m) >= (int)sizeof(key))
+			goto too_long;
 	}
 
 	if (!key[0])
 		return 0;
 
 	/* --lookup-prefix scopes the match by prepending to the key. */
-	if (prefix && *prefix) {
-		char tmp[sizeof(key)];
-
-		snprintf(tmp, sizeof(tmp), "%s%s", prefix, key);
-		snprintf(key, sizeof(key), "%s", tmp);
-	}
+	if (prefix && *prefix && hwdb_prepend(key, sizeof(key), prefix))
+		goto too_long;
 
 	logit(LOG_DEBUG, "rules: hwdb lookup: %s", key);
 	hwdb_scan(key, ev);
 	return 0;
+
+too_long:
+	logit(LOG_WARNING, "rules: hwdb key too long (max %d bytes), lookup skipped",
+	      HWDB_KEY_MAX - 1);
+	return 0;
 }
 
 /* ---- path_id ----------------------------------------------------- */

keventd/rules.c

@@ -501,6 +501,33 @@ void rules_free(struct rule_list *list)
 
 /* ----- variable substitution ------------------------------------------- */
 
+/*
+ * Syspath of the parent device selected by the last successful
+ * KERNELS/SUBSYSTEMS/DRIVERS/ATTRS== match of the rule being evaluated.
+ * As in udev, $attr{} and %s{} fall back to this parent when the event
+ * device has no such attribute itself; the stock rules rely on it:
+ *
+ *   ATTRS{bInterfaceNumber}=="?*", SYMLINK+="...-if$attr{bInterfaceNumber}-..."
+ *
+ * Cleared at the start of every rule and again once it has been applied.
+ */
+static char matched_parent[PATH_MAX];
+
+/* Read an attribute for $attr{}/%s{}: the event device first, then the matched parent */
+static int subst_read_attr(const struct uevent *ev, const char *attr,
+			   char *buf, size_t len)
+{
+	char path[PATH_MAX];
+
+	if (ev->devpath && !sysfs_read_attr(ev->devpath, attr, buf, len))
+		return 0;
+	if (!matched_parent[0])
+		return -1;
+
+	snprintf(path, sizeof(path), "%s/%s", matched_parent, attr);
+	return sysfs_read_file(path, buf, len);
+}
+
 static void append_str(char *buf, size_t *pos, size_t bufsz, const char *s)
 {
 	size_t l;
@@ -604,8 +631,7 @@ static void subst_value(const char *src, char *buf, size_t bufsz,
 							al = sizeof(attr) - 1;
 						memcpy(attr, as, al);
 						attr[al] = 0;
-						if (!sysfs_read_attr(ev->devpath, attr,
-								     tmp, sizeof(tmp)))
+						if (!subst_read_attr(ev, attr, tmp, sizeof(tmp)))
 							append_str(buf, &pos, bufsz, tmp);
 						p = ae + 1;
 					} else {
@@ -681,7 +707,7 @@ static void subst_value(const char *src, char *buf, size_t bufsz,
 						al = sizeof(attr) - 1;
 					memcpy(attr, as, al);
 					attr[al] = 0;
-					if (!sysfs_read_attr(ev->devpath, attr, tmp, sizeof(tmp)))
+					if (!subst_read_attr(ev, attr, tmp, sizeof(tmp)))
 						append_str(buf, &pos, bufsz, tmp);
 					p = ae + 1;
 				} else {
@@ -908,6 +934,8 @@ static int parent_cb(const char *syspath, void *data)
 
 	if (pattern_match(k->value, subject, k->pat_type)) {
 		ctx->found = 1;
+		if (k->op == OP_MATCH_EQ)
+			snprintf(matched_parent, sizeof(matched_parent), "%s", syspath);
 		return 0;	/* stop walking */
 	}
 	return 1;		/* keep walking */
@@ -1101,6 +1129,8 @@ static int rule_matches(const struct rule *r, struct uevent *ev)
 {
 	int i;
 
+	matched_parent[0] = 0;
+
 	for (i = 0; i < r->nkeys; i++) {
 		const struct rule_key *k = &r->keys[i];
 
@@ -1627,6 +1657,7 @@ int rules_apply(struct rule_list *list, struct uevent *ev)
 		      uevent_action_str(ev->action), ev->devpath ?: "");
 
 		exec_rule(r, ev);
+		matched_parent[0] = 0;
 
 		/* GOTO takes effect after this rule's assignments are applied */
 		goto_label = NULL;

keventd/uevent.c

@@ -507,6 +507,62 @@ static struct devrule *find_rule(struct uevent *ev)
 	return &devrules[NELEMS(devrules) - 1];
 }
 
+/*
+ * Build the relative target of a symlink at /dev/<link> that points at the
+ * device node /dev/<node>, e.g.
+ *
+ *   link "input/by-id/usb-X-event-kbd", node "input/event17" -> "../event17"
+ *   link "disk/by-id/ata-X",            node "sda"           -> "../../sda"
+ *
+ * Both arguments are relative to /dev.  The directory components the link
+ * and the node have in common are dropped, then one "../" is emitted for
+ * each directory the link still has to climb out of.
+ */
+static int symlink_relative_target(const char *link, const char *node,
+				   char *buf, size_t len)
+{
+	const char *l = link, *n = node;
+	size_t off = 0;
+	int ret;
+
+	while (*l == '/')
+		l++;
+	while (*n == '/')
+		n++;
+
+	/* Skip leading directories shared by both paths */
+	for (;;) {
+		const char *ls = strchr(l, '/');
+		const char *ns = strchr(n, '/');
+
+		if (!ls || !ns || ls - l != ns - n || strncmp(l, n, ls - l))
+			break;
+		l = ls + 1;
+		n = ns + 1;
+		while (*l == '/')
+			l++;
+		while (*n == '/')
+			n++;
+	}
+
+	buf[0] = 0;
+	for (; (l = strchr(l, '/')); l++) {
+		while (l[1] == '/')
+			l++;
+		if (!l[1])
+			break;
+		ret = snprintf(buf + off, len - off, "../");
+		if (ret < 0 || (size_t)ret >= len - off)
+			return -1;
+		off += ret;
+	}
+
+	ret = snprintf(buf + off, len - off, "%s", n);
+	if (ret < 0 || (size_t)ret >= len - off)
+		return -1;
+	return 0;
+}
+
 /*
  * Create device node in /dev.
  */
@@ -792,7 +848,7 @@ static int disk_path_id(struct uevent *ev, char *id, size_t len)
 static int symlink_add_disk(struct uevent *ev)
 {
 	char target[PATH_MAX], link[PATH_MAX], id[256];
-	const char *name;
+	const char *name, *devtype;
 
 	if (!ev->devname)
 		return -1;
@@ -801,13 +857,22 @@ static int symlink_add_disk(struct uevent *ev)
 	name = strrchr(ev->devname, '/');
 	name = name ? name + 1 : ev->devname;
 
-	/* by-id: serial-based identifier */
+	/*
+	 * by-id: serial-based identifier.
+	 * Partitions are skipped, as their serial lives on the parent disk and would
+	 * clobber its by-id link, partitions get theirs from the udev rules instead.
+	 */
+	devtype = uevent_getenv(ev, "DEVTYPE");
+	if (devtype && !strcmp(devtype, "partition"))
+		goto path;
+
 	if (!disk_serial_id(ev, id, sizeof(id))) {
 		snprintf(target, sizeof(target), "../../%s", name);
 		snprintf(link, sizeof(link), "/dev/disk/by-id/%s", id);
 		symlink_create(target, link, ev->devpath);
 	}
 
+path:
 	/* by-path: topology-based identifier */
 	if (!disk_path_id(ev, id, sizeof(id))) {
 		snprintf(target, sizeof(target), "../../%s", name);
@@ -903,9 +968,6 @@ int symlink_add(struct uevent *ev)
 	for (i = 0; i < ev->applied.nsymlinks; i++) {
 		const char *devname, *sl;
 		char link[PATH_MAX], target[PATH_MAX];
-		size_t toff = 0;
-		int depth = 0;
-		const char *p;
 
 		sl = ev->applied.symlinks[i];
 		if (!sl || !*sl)
@@ -915,19 +977,15 @@ int symlink_add(struct uevent *ev)
 		if (!devname)
 			continue;
 
-		/* basename of devname for the symlink target */
-		const char *base = strrchr(devname, '/');
-		base = base ? base + 1 : devname;
-
-		/* Count '/' in symlink path to determine relative depth */
-		for (p = sl; *p; p++)
-			if (*p == '/')
-				depth++;
-
-		target[0] = 0;
-		for (int d = 0; d < depth; d++)
-			toff += snprintf(target + toff, sizeof(target) - toff, "../");
-		snprintf(target + toff, sizeof(target) - toff, "%s", base);
+		/*
+		 * The node lives at /dev/<devname>, which is not necessarily
+		 * directly in /dev (input/event17, dri/card0, ...), so the
+		 * target must be computed relative to the link's directory.
+		 */
+		if (symlink_relative_target(sl, devname, target, sizeof(target))) {
+			logit(LOG_WARNING, "Symlink %s -> %s: path too long", sl, devname);
+			continue;
+		}
 
 		snprintf(link, sizeof(link), "/dev/%s", sl);
 		symlink_create(target, link, ev->devpath ?: "");

src/cgroup.c

@@ -42,6 +42,9 @@
 #include "iwatch.h"
 #include "conf.h"
 #include "service.h"
+#ifdef AUTO_RELOAD
+#include "sm.h"
+#endif
 #include "log.h"
 #include "util.h"
 

src/conf.c

@@ -67,6 +67,9 @@
 #include "legacy.h"
 #include "private.h"
 #include "service.h"
+#ifdef AUTO_RELOAD
+#include "sm.h"
+#endif
 #include "helpers.h"
 #include "util.h"
 
@@ -1123,7 +1126,7 @@ static int sec_getbool(cfg_t *sec, const char *key, const char *alias)
  * The line-based format could only spell this as one stanza per
  * candidate, relying on nowarn to skip the ones that were not
  * installed.  A candidate not being there is not an error here, so
- * only the winner is looked up again by service_register().
+ * only the winner is looked up again by service_register_desc().
  *
  * A leading '-' still says a missing binary is expected, cf. systemd
  * ExecStart=-.
@@ -1440,7 +1443,7 @@ static void provides_translate(cfg_t *sec, svc_t *svc, char *file)
 /*
  * These have no legacy token, they are validated by service_set_dir()
  * and stored directly on the registered svc.  Empty means unset,
- * service_register() has already cleared the fields.
+ * service_register_desc() has already cleared the fields.
  */
 static void dirs_translate(cfg_t *sec, svc_t *svc, char *file)
 {
@@ -1725,11 +1728,9 @@ static void svc_translate(cfg_t *sec, int type, struct rlimit rlimit[], char *fi
 	/* the command and its arguments, verbatim */
 	addtok(line, sizeof(line), "%s", cmd);
 
-	if ((str = sec_getstr(sec, "description", "desc")))
-		addtok(line, sizeof(line), "-- %s", str);
-
+	str = sec_getstr(sec, "description", "desc");
 	dbg("translated: %s", line);
-	svc = service_register(type, line, rlimit, file);
+	svc = service_register_desc(type, line, str, rlimit, file);
 	if (!svc)
 		return;
 
@@ -1804,7 +1805,7 @@ static void tty_translate(cfg_t *sec, struct rlimit rlimit[], char *file)
 		addtok(line, sizeof(line), "%s", str);
 
 	dbg("translated: tty %s", line);
-	service_register(SVC_TYPE_TTY, line, rlimit, file);
+	service_register_desc(SVC_TYPE_TTY, line, NULL, rlimit, file);
 }
 
 /*

src/service.c

@@ -1599,6 +1599,15 @@ int service_restart_now(svc_t *svc)
 	if (!svc)
 		return 1;
 
+	if (svc_is_runtask(svc) && svc_is_remain(svc) &&
+	    svc->state == SVC_DONE_STATE) {
+		service_timeout_cancel(svc);
+		svc_restarting(svc);
+		service_step(svc);
+
+		return 0;
+	}
+
 	if (!svc_is_running(svc))
 		return service_start_now(svc);
 
@@ -1803,6 +1812,24 @@ static char *parse_args(char **line)
 	return NULL;
 }
 
+/*
+ * Match an option that takes a value, @opt on its own or followed by
+ * one of the characters in @sep, e.g. "log" and "log:/path".  Returns
+ * the value, or NULL, so a command like logger or pidof is not taken
+ * for an option.
+ */
+static char *is_opt(char *tok, const char *opt, const char *sep)
+{
+	size_t len = strlen(opt);
+
+	if (strncasecmp(tok, opt, len))
+		return NULL;
+	if (tok[len] && !strchr(sep, tok[len]))
+		return NULL;
+
+	return &tok[len];
+}
+
 /*
  * log:/path/to/logfile,priority:facility.level,tag:ident
  */
@@ -2147,9 +2174,10 @@ static void parse_cmdline_args(svc_t *svc, char *cmd, char **args)
 }
 
 /**
- * service_register - Register service, task or run commands
+ * service_register_desc - Register service, task or run commands
  * @type:   %SVC_TYPE_SERVICE(0), %SVC_TYPE_TASK(1), %SVC_TYPE_RUN(2)
- * @cfg:    Configuration, complete command, with -- for description text
+ * @cfg:    Configuration, complete command, without description text
+ * @desc:   Description text, or %NULL
  * @rlimit: Limits for this service/task/run, may be global limits
  * @file:   The file name service was loaded from
  *
@@ -2207,9 +2235,10 @@ static void parse_cmdline_args(svc_t *svc, char *cmd, char **args)
  * skipped on purpose -- conditional loading, bootstrap over, nowarn --
  * also returns %NULL, with @errno zero.
  */
-svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
+svc_t *service_register_desc(int type, char *cfg, const char *desc,
+			     struct rlimit rlimit[], char *file)
 {
-	char *cmd, *desc, *runlevels = NULL, *cond = NULL;
+	char *cmd, *runlevels = NULL, *cond = NULL;
 	char *username = NULL, *log = NULL, *pid = NULL;
 	char *name = NULL, *halt = NULL, *delay = NULL;
 	char *id = NULL, *env = NULL, *cgroup = NULL;
@@ -2245,24 +2274,6 @@ svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 		return NULL;
 	}
 
-	desc = strstr(line, "-- ");
-	if (desc) {
-		*desc = 0;
-		desc += 3;
-
-		while (*desc && isblank(*desc))
-			desc++;
-	} else {
-		int pos;
-
-		/* Find "--\n" to denote empty/no description */
-		pos = (int)strlen(line) - 2;
-		if (pos > 0 && !strcmp(&line[pos], "--")) {
-			line[pos] = 0;
-			desc = &line[pos];
-		}
-	}
-
 	args = line;
 	cmd = parse_args(&args);
 	if (!cmd) {
@@ -2272,6 +2283,10 @@ svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 		return NULL;
 	}
 
+	/*
+	 * Options ending in ':' can be prefix matched, the rest must match
+	 * the whole token or a command like logger is taken for one.
+	 */
 	while (cmd) {
 		char *arg;
 
@@ -2286,19 +2301,19 @@ svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 			cond = &cmd[1];
 		else if (cmd[0] == ':')	/* :ID */
 			id = &cmd[1];
-		else if (MATCH_CMD(cmd, "log", arg))
+		else if (is_opt(cmd, "log", ":"))
 			log = cmd;
-		else if (MATCH_CMD(cmd, "pid", arg))
+		else if (is_opt(cmd, "pid", ":"))
 			pid = cmd;
 		else if (MATCH_CMD(cmd, "name:", arg))
 			name = cmd;
 		else if (MATCH_CMD(cmd, "notify:", arg))
 			notify = arg;
-		else if (MATCH_CMD(cmd, "type:forking", arg))
+		else if (!strcasecmp(cmd, "type:forking"))
 			forking = 1;
-		else if (MATCH_CMD(cmd, "manual:yes", arg))
+		else if (!strcasecmp(cmd, "manual:yes"))
 			manual = 1;
-		else if (MATCH_CMD(cmd, "remain:yes", arg))
+		else if (!strcasecmp(cmd, "remain:yes"))
 			remain = 1;
 		else if (MATCH_CMD(cmd, "restart:", arg)) {
 			if (MATCH_CMD(arg, "always", arg))
@@ -2310,9 +2325,9 @@ svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 			restart_tmo = atoi(arg) * 1000;
 		else if (MATCH_CMD(cmd, "restart_sec:", arg))
 			restart_tmo = atoi(arg) * 1000;
-		else if (MATCH_CMD(cmd, "norestart", arg))
+		else if (!strcasecmp(cmd, "norestart"))
 			restart_max = 0;
-		else if (MATCH_CMD(cmd, "nowarn", arg))
+		else if (!strcasecmp(cmd, "nowarn"))
 			nowarn = 1;
 		else if (MATCH_CMD(cmd, "oncrash:", arg)) {
 			if (MATCH_CMD(arg, "reboot", arg))
@@ -2320,7 +2335,7 @@ svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 			if (MATCH_CMD(arg, "script", arg))
 				oncrash_action = SVC_ONCRASH_SCRIPT;
 		}
-		else if (MATCH_CMD(cmd, "respawn", arg))
+		else if (!strcasecmp(cmd, "respawn"))
 			respawn = 1;
 		else if (MATCH_CMD(cmd, "halt:", arg))
 			halt = arg;
@@ -2345,7 +2360,7 @@ svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 		else if (MATCH_CMD(cmd, "tty:", arg))
 			ctty = arg;
 		/* catch both cgroup: and cgroup. handled in parse_cgroup() */
-		else if (MATCH_CMD(cmd, "cgroup", arg))
+		else if ((arg = is_opt(cmd, "cgroup", ".:")))
 			cgroup = arg;
 		else if (MATCH_CMD(cmd, "conflict:", arg))
 			conflict = arg;
@@ -2730,6 +2745,45 @@ svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
 	return svc;
 }
 
+/**
+ * service_register - Register from a legacy one-liner
+ * @type:   %SVC_TYPE_SERVICE(0), %SVC_TYPE_TASK(1), %SVC_TYPE_RUN(2)
+ * @cfg:    Configuration, complete command, with -- for description text
+ * @rlimit: Limits for this service/task/run, may be global limits
+ * @file:   The file name service was loaded from
+ *
+ * The first "-- " in @cfg starts the description, the rest is handed
+ * to service_register_desc().
+ */
+svc_t *service_register(int type, char *cfg, struct rlimit rlimit[], char *file)
+{
+	char *line, *desc;
+
+	if (!cfg)
+		return service_register_desc(type, cfg, NULL, rlimit, file);
+
+	line = strdupa(cfg);
+	desc = strstr(line, "-- ");
+	if (desc) {
+		*desc = 0;
+		desc += 3;
+
+		while (*desc && isblank(*desc))
+			desc++;
+	} else {
+		int pos;
+
+		/* Find "--\n" to denote empty/no description */
+		pos = (int)strlen(line) - 2;
+		if (pos > 0 && !strcmp(&line[pos], "--")) {
+			line[pos] = 0;
+			desc = &line[pos];
+		}
+	}
+
+	return service_register_desc(type, line, desc, rlimit, file);
+}
+
 /*
  * This function is called at the end of a runlevel change or reload
  * command to delete all remnants of removed services.
@@ -3378,8 +3432,8 @@ restart:
 				svc_set_state(svc, SVC_CLEANUP_STATE);
 				service_cleanup_script(svc);
 			} else {
-				/* Unblock remain tasks after post script so they can restart */
-				if (svc_is_remain(svc))
+				/* Restart remain tasks after runlevel/reload/restart teardown. */
+				if (svc_is_remain(svc) && svc_is_restart(svc))
 					svc_unblock(svc);
 				svc_set_state(svc, SVC_HALTED_STATE);
 			}
@@ -3398,13 +3452,17 @@ restart:
 		break;
 
 	case SVC_DONE_STATE:
-		/* Remain tasks: always run post script when stopped, even if config changed */
-		if (svc_is_runtask(svc) && svc_is_remain(svc) && svc_is_stopped(svc)) {
+		/* Remain tasks run post script before stopping or restarting. */
+		if (svc_is_runtask(svc) && svc_is_remain(svc) &&
+		    (svc_is_stopped(svc) || svc_is_restart(svc))) {
 			if (svc_has_post(svc)) {
 				svc_set_state(svc, SVC_TEARDOWN_STATE);
 				service_post_script(svc);
-			} else
+			} else {
+				if (svc_is_restart(svc))
+					svc_unblock(svc);
 				svc_set_state(svc, SVC_HALTED_STATE);
+			}
 		} else if (svc_is_changed(svc))
 			svc_set_state(svc, SVC_HALTED_STATE);
 		else if (svc_is_runtask(svc) && svc_is_manual(svc) && enabled)
@@ -3727,7 +3785,7 @@ void service_runtask_clean(void)
 
 			/* Config changed or leaving runlevel: stop, run post, restart */
 			svc->once = 0;
-			svc_stop(svc);
+			svc_restarting(svc);
 			service_step(svc);
 			continue;
 		}

src/service.h

@@ -44,6 +44,8 @@ extern const struct svcdir svcdirs[NUM_SVCDIRS];
 
 int	  service_set_dir	 (svc_t *svc, const struct svcdir *sd, const char *name);
 svc_t	 *service_register	 (int type, char *line, struct rlimit rlimit[], char *file);
+svc_t	 *service_register_desc  (int type, char *line, const char *desc,
+				  struct rlimit rlimit[], char *file);
 void      service_unregister     (svc_t *svc);
 
 void      service_runtask_clean  (void);

test/Makefile.am

@@ -60,6 +60,7 @@ EXTRA_DIST		+= pre-fail.sh
 EXTRA_DIST		+= process-depends.sh
 EXTRA_DIST		+= rclocal.sh
 EXTRA_DIST		+= ready-serv.sh
+EXTRA_DIST		+= remain-task-control.sh
 EXTRA_DIST		+= restart-self.sh
 EXTRA_DIST		+= runlevel.sh
 EXTRA_DIST		+= run-restart-forever.sh
@@ -129,6 +130,7 @@ TESTS			+= pre-fail.sh
 TESTS			+= process-depends.sh
 TESTS			+= rclocal.sh
 TESTS			+= ready-serv.sh
+TESTS			+= remain-task-control.sh
 TESTS			+= restart-self.sh
 TESTS			+= runlevel.sh
 TESTS			+= sysvparts.sh

test/conf-command.sh

@@ -76,6 +76,29 @@ retry 'assert_cmd pick "serv -np -i pick"'
 assert_desc "Single command" pick
 assert_num_children 1 serv
 
+# The description has its own setting, so a -- in the command is an
+# argument to it, see issue #512.
+say 'A -- in the command is passed on to it'
+run "echo 'service pick {'                                >  $FINIT_CONF"
+run "echo '    description = \"Dashes\"'                   >> $FINIT_CONF"
+run "echo '    command     = \"serv -np -i pick -- extra\"' >> $FINIT_CONF"
+run "echo '}'                                             >> $FINIT_CONF"
+run "initctl reload"
+
+retry 'assert_cmd pick "serv -np -i pick -- extra"'
+assert_desc "Dashes" pick
+assert_num_children 1 serv
+
+# Options are matched as whole tokens, so a command that merely starts
+# like one, pidof vs. pid, is still the command.
+say 'A command named like an option is the command'
+run "echo 'task lookalike {'             >  $FINIT_CONF"
+run "echo '    command = \"pidof serv\"' >> $FINIT_CONF"
+run "echo '}'                            >> $FINIT_CONF"
+run "initctl reload"
+
+retry 'assert_cmd lookalike "pidof serv"'
+
 # A tty block takes the same list, its command is the getty to run.
 # Asserting it is loaded is enough here, a getty in the test namespace
 # has no terminal to attach to.
@@ -90,7 +113,7 @@ retry 'assert_cmd tty "serv -np -i ttypick"'
 
 # Nothing resolves, so the block is skipped the same way a single
 # missing command is.  The leading - only decides whether that is
-# quiet, service_register() bails before svc_new() either way.
+# quiet, service_register_desc() bails before svc_new() either way.
 say 'A block whose candidates are all missing is skipped'
 run "echo 'service ghost {'                                >  $FINIT_CONF"
 run "echo '    command = { \"-/no/such/one\", \"-/no/such/two\" }' >> $FINIT_CONF"

test/remain-task-control.sh

@@ -0,0 +1,76 @@
+#!/bin/sh
+
+set -eu
+
+TEST_DIR=$(dirname "$0")
+TEST_CONF=/etc/finit.d/remain-task.conf
+STATE=/run/remain-task.state
+
+test_teardown()
+{
+    run "rm -f $TEST_CONF $STATE"
+}
+
+assert_runs()
+{
+    assert "Remain task has run $1 time(s)" \
+           "$(texec initctl -j status remain-task | jq -M .restarts)" -eq "$1"
+}
+
+# shellcheck source=/dev/null
+. "$TEST_DIR/lib/setup.sh"
+
+say 'Add remain task'
+run "cat > $TEST_CONF" <<EOF
+task remain-task {
+    description       = "Remain task control"
+    runlevel          = "2345"
+    remain-after-exit = true
+    command           = "/bin/touch $STATE"
+    exec-stop-post    = "/bin/rm -f $STATE"
+}
+EOF
+run "initctl reload"
+
+retry 'assert_status remain-task done'
+assert_file_exists "$STATE"
+assert_runs 1
+
+say 'Stop remain task'
+run "initctl stop remain-task"
+retry 'assert_status remain-task stopped'
+assert "Post script removed state" \
+       "$(texec sh -c "[ ! -e '$STATE' ] && echo yes")" = yes
+assert_runs 1
+
+say 'Start remain task'
+run "initctl start remain-task"
+retry 'assert_status remain-task done'
+assert_file_exists "$STATE"
+assert_runs 2
+
+say 'Restart remain task'
+run "initctl restart remain-task"
+retry 'assert_runs 3'
+retry 'assert_status remain-task done'
+assert_file_exists "$STATE"
+
+say 'Reload changed remain task'
+run "sed -i 's/Remain task control/Updated remain task control/' $TEST_CONF"
+run "initctl reload"
+retry 'assert_runs 4'
+retry 'assert_status remain-task done'
+assert_file_exists "$STATE"
+
+say 'Leave remain task runlevel'
+run "initctl runlevel 1"
+retry 'assert_status remain-task halted'
+assert "Post script removed state" \
+       "$(texec sh -c "[ ! -e '$STATE' ] && echo yes")" = yes
+assert_runs 4
+
+say 'Re-enter remain task runlevel'
+run "initctl runlevel 2"
+retry 'assert_runs 5'
+retry 'assert_status remain-task done'
+assert_file_exists "$STATE"