Use changed fields to fix dep solver in remove case

When told to remove services through initd_remove_recurse_deps, their
changed fields are cleared to indicate that they will no longer be
available. This is an improvement over the previous hack where the
active bits were being cleared and then restored at the end of the
function.

With the changed fields cleared, we can skip services that are currently
active, but will be going away in add_all_actives. This fixes a bug
where a service can be asked to be removed, but then added right back in
because it's currently active.

When each rc level is verified after recursion has completed, the added
services are checked to see if they are marked for removal. This fixes a
bug where a service was asked to be removed, but added back because it
was a dependency of another service. When each service is verified at
that level, it is marked to change to active.

Using the changed field clearing in remove_recurse_deps and the setting
in verify_level, we should be able to install and remove scripts just by
checking the changed field.

Finally, fixed two tests which were passing with false positives due to
these bugs.

3 files changed, 76 insertions, 45 deletions

diff --git a/lib/rdep.c b/lib/rdep.c
index 7bf441c..012d718 100644
--- a/lib/rdep.c
+++ b/lib/rdep.c
@@ -72,12 +72,11 @@ out:
 initd_list_t *initd_remove_recurse_deps(initd_list_t *pool,
 				initd_sk_t sk, const dep_t *remove)
 {
-	initd_list_t *save = NULL, *all = NULL, *chain = NULL;
-	initd_t *orig, *copy;
+	initd_list_t *rmlist = NULL, *all = NULL, *chain = NULL;
+	initd_t *ip;
 	int n;
 	dep_t *all_active = NULL;
 	char *rm;
-	initd_key_t key;
 
 	if (!pool)
 		goto out;
@@ -89,35 +88,38 @@ initd_list_t *initd_remove_recurse_deps(initd_list_t *pool,
 	if (!remove || !dep_get_num(remove))
 		goto out;
 
-	/* initialize the initd save list */
-	save = initd_list_new();
-
-	if (sk == SK_START)
-		key = KEY_ASTART;
-	else
-		key = KEY_ASTOP;
+	/* initialize the initd remove list */
+	rmlist = initd_list_new();
 
 	/* Check that the specified services to remove are in the pool.
-	 * If they are, add them to a saved list so we can clear their
-	 * active field and then restore it afterwards. */
+	 * If they are, clear their changed bits to mark them for
+	 * removal. */
 	for (n = 0; n < dep_get_num(remove); n++) {
 		rm = dep_get_dep(remove, n);
-		orig = initd_list_find_provides(pool, rm);
-		if (!orig) {
+		ip = initd_list_find_provides(pool, rm);
+		if (!ip) {
 			fprintf(stderr, "No init script provides %s\n",
 				rm);
 			goto err;
 		}
-		if (initd_is_active(orig, RC_ALL, key)) {
-			copy = initd_copy(orig);
-			initd_list_add(save, copy);
-			orig->astart = orig->astop = 0;
-		}
+
+		/* continue if already in the rmlist */
+		if (initd_list_exists_name(rmlist, ip->name))
+			continue;
+
+		/* Clear the change field to mark for removal */
+		if (sk == SK_START)
+			ip->cstart = 0;
+		else
+			ip->cstop = 0;
+
+		/* Add the initd_t to the rmlist */
+		initd_list_add(rmlist, initd_copy(ip));
 	}
 
 	/* If none of the specified services are currently active, the
-	 * save list will be empty and we can just return. */
-	if (!save->first)
+	 * rm list will be empty and we can just return. */
+	if (!rmlist->first)
 		goto out;
 
 	/* Find all the currently active services */
@@ -136,18 +138,6 @@ initd_list_t *initd_remove_recurse_deps(initd_list_t *pool,
 		goto err;
 	}
 
-	/* Restore the saved active levels */
-	for (copy = save->first; copy; copy = copy->next) {
-		orig = initd_list_find_name(pool, copy->name);
-		if (orig) {
-			orig->astart = copy->astart;
-			orig->astop = copy->astop;
-		} else {
-			fprintf(stderr, "Saved script %s not in "
-				"original pool\n", copy->name);
-		}
-	}
-
 	/* If we got here, we're successful */
 	goto out;
 err:
@@ -156,7 +146,7 @@ err:
 out:
 	initd_list_free(chain);
 	dep_free(all_active);
-	initd_list_free(save);
+	initd_list_free(rmlist);
 	return all;
 }
 
@@ -202,15 +192,25 @@ static dep_t *add_all_active(const initd_list_t *pool,
 {
 	dep_t *active;
 	initd_t *ip;
-	initd_key_t key;
+	initd_key_t key, ckey;
 
 	if (!pool)
 		return NULL;
 
 	active = dep_copy(init);
-	key = (sk == SK_START) ? KEY_ASTART : KEY_ASTOP;
+	if (sk == SK_START) {
+		key = KEY_ASTART;
+		ckey = KEY_CSTART;
+	} else {
+		key = KEY_ASTOP;
+		ckey = KEY_CSTOP;
+	}
+
 	for (ip = pool->first; ip; ip = ip->next) {
-		if (initd_is_active(ip, RC_ALL, key))
+		/* Add services if they are active on any level and
+		 * they are not marked for removal on all levels */
+		if (initd_is_active(ip, RC_ALL, key) &&
+			initd_is_active(ip, RC_ALL, ckey))
 			dep_add(active, ip->name);
 	}
 
@@ -392,6 +392,7 @@ static bool initd_list_verify_level(const initd_list_t *ord,
 	char *dstr;
 	int n;
 	bool match;
+	initd_key_t ckey;
 
 	if (!ord)
 		return false;
@@ -423,7 +424,7 @@ static bool initd_list_verify_level(const initd_list_t *ord,
 			if (!dep) {
 				if (required) {
 					fprintf(stderr,
-						"No init script "
+						"Error: No init script "
 						"provides %s\n",
 						dstr);
 					return false;
@@ -448,22 +449,47 @@ static bool initd_list_verify_level(const initd_list_t *ord,
 
 			if (!match && sk == SK_START) {
 				fprintf(stderr,
-					"%s dependency %s does not "
-					"start in level %c or sysinit\n",
+					"Error: %s dependency %s does "
+					"not start in level %c or "
+					"sysinit\n",
 					ip->name, dstr,
 					initd_rc_level_char(rc));
 				return false;
 			} else if (!match) {
 				fprintf(stderr,
-					"%s dependency %s does not stop "
-					"in level %c or halt and reboot\n",
+					"Error: %s dependency %s does "
+					"not stop in level %c or halt "
+					"and reboot\n",
 					ip->name, dstr,
 					initd_rc_level_char(rc));
 				return false;
 			}
+
+			/* Check if the required dep is marked for
+			 * removal. This happens when a script is
+			 * currently active but the changed field is
+			 * cleared. */
+			if (sk == SK_START)
+				match = (dep->astart & rc) &&
+					!(dep->cstart & rc);
+			else
+				match = (dep->astop & rc) &&
+					!(dep->cstop & rc);
+			if (match && required) {
+				fprintf(stderr,
+					"Error: %s required dependency %s"
+					" is marked for removal\n",
+					ip->name, dstr);
+				return false;
+			}
 		}
 	}
 
+	/* Mark the changed field as active for each script at this level */
+	ckey = (sk == SK_START) ? KEY_CSTART : KEY_CSTOP;
+	for (ip = ord->first; ip; ip = ip->next)
+		initd_set_rc(ip, ckey, rc);
+
 	/* If we get here, then we were successful */
 	return true;
 }
diff --git a/test/trem-deps.c b/test/trem-deps.c
index e6501d7..a4b79b1 100644
--- a/test/trem-deps.c
+++ b/test/trem-deps.c
@@ -14,8 +14,11 @@ int main(int argc, char *argv[])
 	initd_list_t *all, *startlist, *stoplist;
 	dep_t *rem = dep_new();
 
-	if (argc <= 1)
+	if (argc <= 1) {
 		dep_add(rem, "network");
+		dep_add(rem, "bar");
+		dep_add(rem, "foo");
+	}
 
 	while (argc > 1) {
 		dep_add(rem, argv[1]);
diff --git a/test/tremove.c b/test/tremove.c
index 106fe6c..6432ebb 100644
--- a/test/tremove.c
+++ b/test/tremove.c
@@ -14,8 +14,10 @@ int main(int argc, char *argv[])
 	initd_list_t *all, *startlist, *stoplist;
 	dep_t *rem = dep_new();
 
-	if (argc <= 1)
-		dep_add(rem, "mountfs");
+	if (argc <= 1) {
+		dep_add(rem, "foo");
+		dep_add(rem, "bar");
+	}
 
 	while (argc > 1) {
 		dep_add(rem, argv[1]);