From a5491b3dedc562a7a353165d50989487609d4c7e Mon Sep 17 00:00:00 2001
From: Joachim Nilsson <troglobit@gmail.com>
Date: Fri, 28 Sep 2018 06:31:28 +0200
Subject: [PATCH] Remove HOOK_SVC_START, caused more problems than it was worth

An operator wanting to monitor processes started by Finit could use the
kernel ftrace framework.  E.g. execsnoop in perf-tools.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
---
 docs/plugins.md |  5 -----
 src/plugin.c    | 15 ---------------
 src/plugin.h    |  2 --
 src/service.c   | 19 ++++---------------
 4 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/docs/plugins.md b/docs/plugins.md
index 687811c2..ef4937d5 100644
--- a/docs/plugins.md
+++ b/docs/plugins.md
@@ -110,11 +110,6 @@ Hooks
 
   **NOTE:** This hook callback gets the lost PID as argument.
 
-* `HOOK_SVC_START`: Like `HOOK_SVC_LOST`, but called when a process is
-  started.  Same caveats apply.
-
-  **NOTE:** This hook callback gets the new PID as argument.
-
 * `HOOK_RUNLEVEL_CHANGE`: Called when the user has issued a runlevel
   change.  The hook is called when services not matching the new
   runlevel have been been stopped.  When the hook has completed, Finit
diff --git a/src/plugin.c b/src/plugin.c
index 18b2be7a..f5b854ad 100644
--- a/src/plugin.c
+++ b/src/plugin.c
@@ -192,16 +192,8 @@ int plugin_exists(hook_point_t no)
 /* Some hooks are called with a fixed argument, like HOOK_SVC_LOST */
 void plugin_run_hook(hook_point_t no, void *arg)
 {
-	static int last = -1;
 	plugin_t *p, *tmp;
 
-	/*
-	 * End recursion: any plugin hook => start service => SVC start
-	 * hook => start service ... err ... wait a second
-	 */
-	if (HOOK_SVC_START == last)
-		return;
-
 	PLUGIN_ITERATOR(p, tmp) {
 		if (p->hook[no].cb) {
 			_d("Calling %s hook n:o %d (arg: %p) ...", basename(p->name), no, arg);
@@ -209,15 +201,8 @@ void plugin_run_hook(hook_point_t no, void *arg)
 		}
 	}
 
-	/* Guard against infinite recursion */
-	if (HOOK_SVC_START == no)
-		last = no;
-
 	cond_set_oneshot(hook_cond[no]);
 	service_step_all(SVC_TYPE_RUNTASK);
-
-	if (HOOK_SVC_START == no)
-		last = -1;
 }
 
 /* Regular hooks are called with the registered plugin's argument */
diff --git a/src/plugin.h b/src/plugin.h
index 4d404023..0bc183db 100644
--- a/src/plugin.h
+++ b/src/plugin.h
@@ -57,7 +57,6 @@
  *
  * - HOOK_SVC_RECONF      :: action/svc/reconf
  * - HOOK_SVC_LOST        :: action/svc/lost
- * - HOOK_SVC_START       :: action/svc/start
  * - HOOK_RUNLEVEL_CHANGE :: action/sys/runlevel
  *
  * However, the implementation did not turn out to be stable enough for
@@ -77,7 +76,6 @@
 	/* Runtime hooks, runlevel [S1-9] */			\
 	CHOOSE(HOOK_SVC_RECONF,      "nop"),			\
 	CHOOSE(HOOK_SVC_LOST,        "nop"),			\
-	CHOOSE(HOOK_SVC_START,       "nop"),			\
 	CHOOSE(HOOK_RUNLEVEL_CHANGE, "nop"),			\
 								\
 	/* Shutdown hooks, runlevel [06] */			\
diff --git a/src/service.c b/src/service.c
index 43aac6e9..1cb17b07 100644
--- a/src/service.c
+++ b/src/service.c
@@ -367,14 +367,6 @@ static int service_start(svc_t *svc)
 	if (do_progress)
 		print_result(result);
 
-	/*
-	 * Only run hook on successful start, and *after* having printed
-	 * the result, otherwise any hook tasks may overwrite it and the
-	 * result would be like double "[ OK ]" but only one service.
-	 */
-	if (!result)
-		plugin_run_hook(HOOK_SVC_START, (void *)(uintptr_t)pid);
-
 	return result;
 }
 
@@ -1044,13 +1036,6 @@ restart:
 			if (sm_is_in_teardown(&sm))
 				break;
 
-			/*
-			 * Make state transition *before* service_start(), because
-			 * of HOOK_SVC_START, which may call service_step()
-			 */
-			svc_mark_clean(svc);
-			svc_set_state(svc, SVC_RUNNING_STATE);
-
 			err = service_start(svc);
 			if (err) {
 				(*restart_cnt)++;
@@ -1059,6 +1044,10 @@ restart:
 				if (!svc_is_inetd_conn(svc))
 					break;
 			}
+
+			/* Everything went fine, clean and set state */
+			svc_mark_clean(svc);
+			svc_set_state(svc, SVC_RUNNING_STATE);
 		}
 		break;