watchfrr, lib: cleanup & delay detaching

This cleans up watchfrr to be more "normal" like the other daemons in
terms of what it does in main(), i.e. using the full frr_*() call set.

Also, this changes the startup behaviour on watchfrr to stay attached on
the daemon's parent process until startup is really complete.  This
should allow removing the "watchfrr.started" hack at some point.

Signed-off-by: David Lamparter <equinox@diac24.net>

lib/libfrr.c

@@ -54,6 +54,7 @@ static char pidfile_default[512];
 static char vtypath_default[256];
 
 bool debug_memstats_at_exit = 0;
+static bool nodetach_term, nodetach_daemon;
 
 static char comb_optstr[256];
 static struct option comb_lo[64];
@@ -281,6 +282,8 @@ void frr_preinit(struct frr_daemon_info *daemon, int argc, char **argv)
 		opt_extend(&os_zclient);
 	if (!(di->flags & FRR_NO_TCPVTY))
 		opt_extend(&os_vty);
+	if (di->flags & FRR_DETACH_LATER)
+		nodetach_daemon = true;
 
 	snprintf(config_default, sizeof(config_default), "%s/%s.conf",
 		 frr_sysconfdir, di->name);
@@ -767,13 +770,16 @@ void frr_config_fork(void)
 {
 	hook_call(frr_late_init, master);
 
-	/* Don't start execution if we are in dry-run mode */
-	if (di->dryrun) {
-		frr_config_read_in(NULL);
-		exit(0);
-	}
+	if (!(di->flags & FRR_NO_CFG_PID_DRY)) {
+		/* Don't start execution if we are in dry-run mode */
+		if (di->dryrun) {
+			frr_config_read_in(NULL);
+			exit(0);
+		}
 
-	thread_add_event(master, frr_config_read_in, NULL, 0, &di->read_in);
+		thread_add_event(master, frr_config_read_in, NULL, 0,
+				 &di->read_in);
+	}
 
 	if (di->daemon_mode || di->terminal)
 		frr_daemonize();
@@ -783,7 +789,7 @@ void frr_config_fork(void)
 	pid_output(di->pid_file);
 }
 
-void frr_vty_serv(void)
+static void frr_vty_serv(void)
 {
 	/* allow explicit override of vty_path in the future
 	 * (not currently set anywhere) */
@@ -810,14 +816,22 @@ void frr_vty_serv(void)
 	vty_serv_sock(di->vty_addr, di->vty_port, di->vty_path);
 }
 
+static void frr_check_detach(void)
+{
+	if (nodetach_term || nodetach_daemon)
+		return;
+
+	if (daemon_ctl_sock != -1)
+		close(daemon_ctl_sock);
+	daemon_ctl_sock = -1;
+}
+
 static void frr_terminal_close(int isexit)
 {
 	int nullfd;
 
-	if (daemon_ctl_sock != -1) {
-		close(daemon_ctl_sock);
-		daemon_ctl_sock = -1;
-	}
+	nodetach_term = false;
+	frr_check_detach();
 
 	if (!di->daemon_mode || isexit) {
 		printf("\n%s exiting\n", di->name);
@@ -881,6 +895,12 @@ out:
 	return 0;
 }
 
+void frr_detach(void)
+{
+	nodetach_daemon = false;
+	frr_check_detach();
+}
+
 void frr_run(struct thread_master *master)
 {
 	char instanceinfo[64] = "";
@@ -895,6 +915,8 @@ void frr_run(struct thread_master *master)
 		    instanceinfo, di->vty_port, di->startinfo);
 
 	if (di->terminal) {
+		nodetach_term = true;
+
 		vty_stdio(frr_terminal_close);
 		if (daemon_ctl_sock != -1) {
 			set_nonblocking(daemon_ctl_sock);
@@ -914,9 +936,7 @@ void frr_run(struct thread_master *master)
 			close(nullfd);
 		}
 
-		if (daemon_ctl_sock != -1)
-			close(daemon_ctl_sock);
-		daemon_ctl_sock = -1;
+		frr_check_detach();
 	}
 
 	/* end fixed stderr startup logging */

lib/libfrr.h

@@ -29,11 +29,21 @@
 #include "module.h"
 #include "hook.h"
 
+/* The following options disable specific command line options that
+ * are not applicable for a particular daemon.
+ */
 #define FRR_NO_PRIVSEP		(1 << 0)
 #define FRR_NO_TCPVTY		(1 << 1)
 #define FRR_LIMITED_CLI		(1 << 2)
-#define FRR_NO_CFG_PID_DRY		(1 << 3)
+#define FRR_NO_CFG_PID_DRY	(1 << 3)
 #define FRR_NO_ZCLIENT		(1 << 4)
+/* If FRR_DETACH_LATER is used, the daemon will keep its parent running
+ * until frr_detach() is called.  Normally "somedaemon -d" returns once the
+ * main event loop is reached in the daemon;  use this for extra startup bits.
+ *
+ * Does nothing if -d isn't used.
+ */
+#define FRR_DETACH_LATER	(1 << 5)
 
 struct frr_daemon_info {
 	unsigned flags;
@@ -102,10 +112,8 @@ extern struct thread_master *frr_init(void);
 DECLARE_HOOK(frr_late_init, (struct thread_master * tm), (tm))
 extern void frr_config_fork(void);
 
-extern void frr_vty_serv(void);
-
-/* note: contains call to frr_vty_serv() */
 extern void frr_run(struct thread_master *master);
+extern void frr_detach(void);
 
 extern bool frr_zclient_addr(struct sockaddr_storage *sa, socklen_t *sa_len,
 			     const char *path);

watchfrr/watchfrr.c

@@ -55,9 +55,11 @@
 
 #define PING_TOKEN	"PING"
 
+DEFINE_MGROUP(WATCHFRR, "watchfrr")
+DEFINE_MTYPE_STATIC(WATCHFRR, WATCHFRR_DAEMON, "watchfrr daemon entry")
+
 /* Needs to be global, referenced somewhere inside libfrr. */
 struct thread_master *master;
-static char pidfile_default[256];
 
 static bool watch_only = false;
 
@@ -230,7 +232,7 @@ Otherwise, the interval is doubled (but capped at the -M value).\n\n",
 		name of the daemon should be substituted.\n\
     --dry	Do not start or restart anything, just log.\n\
 -p, --pid-file	Set process identifier file name\n\
-		(default is %s).\n\
+		(default is %s/watchfrr.pid).\n\
 -b, --blank-string\n\
 		When the supplied argument string is found in any of the\n\
 		various shell command arguments (-r, -s, or -k), replace\n\
@@ -240,7 +242,7 @@ Otherwise, the interval is doubled (but capped at the -M value).\n\n",
 -h, --help	Display this help and exit\n",
 		frr_vtydir, DEFAULT_LOGLEVEL, LOG_EMERG, LOG_DEBUG, LOG_DEBUG,
 		DEFAULT_MIN_RESTART, DEFAULT_MAX_RESTART, DEFAULT_PERIOD,
-		DEFAULT_TIMEOUT, DEFAULT_RESTART_TIMEOUT, pidfile_default);
+		DEFAULT_TIMEOUT, DEFAULT_RESTART_TIMEOUT, frr_vtydir);
 }
 
 static pid_t run_background(char *shell_cmd)
@@ -609,12 +611,13 @@ static void daemon_send_ready(void)
 	if (!sent && gs.numdown == 0) {
 		FILE *fp;
 
+		zlog_notice("all daemons up, doing startup-complete notify");
+		frr_detach();
+
 		fp = fopen(DAEMON_VTY_DIR "/watchfrr.started", "w");
 		if (fp)
 			fclose(fp);
 #if defined HAVE_SYSTEMD
-		zlog_notice(
-			"Watchfrr: Notifying Systemd we are up and running");
 		systemd_send_started(master, 0);
 #endif
 		sent = 1;
@@ -961,6 +964,53 @@ static char *translate_blanks(const char *cmd, const char *blankstr)
 	return res;
 }
 
+static void watchfrr_init(int argc, char **argv)
+{
+	const char *special = "zebra";
+	int i;
+	struct daemon *dmn, **add = &gs.daemons;
+	char alldaemons[512] = "", *p = alldaemons;
+
+	for (i = optind; i < argc; i++) {
+		dmn = XCALLOC(MTYPE_WATCHFRR_DAEMON, sizeof(*dmn));
+
+		dmn->name = dmn->restart.name = argv[i];
+		dmn->state = DAEMON_INIT;
+		gs.numdaemons++;
+		gs.numdown++;
+		dmn->fd = -1;
+		dmn->t_wakeup = NULL;
+		thread_add_timer_msec(master, wakeup_init, dmn,
+				      100 + (random() % 900),
+				      &dmn->t_wakeup);
+		dmn->restart.interval = gs.min_restart_interval;
+		*add = dmn;
+		add = &dmn->next;
+
+		if (!strcmp(dmn->name, special))
+			gs.special = dmn;
+	}
+
+	if (!gs.daemons) {
+		fprintf(stderr,
+			"Must specify one or more daemons to monitor.\n\n");
+		frr_help_exit(1);
+	}
+	if (!watch_only && !gs.special) {
+		fprintf(stderr, "\"%s\" daemon must be in daemon lists\n\n",
+			special);
+		frr_help_exit(1);
+	}
+
+	for (dmn = gs.daemons; dmn; dmn = dmn->next) {
+		snprintf(p, alldaemons + sizeof(alldaemons) - p, "%s%s",
+			 (p == alldaemons) ? "" : " ", dmn->name);
+		p += strlen(p);
+	}
+	zlog_notice("%s %s watching [%s]%s", progname, FRR_VERSION, alldaemons,
+		    watch_only ? ", monitor mode" : "");
+}
+
 struct zebra_privs_t watchfrr_privs = {
 #ifdef VTY_GROUP
 	.vty_group = VTY_GROUP,
@@ -984,7 +1034,8 @@ static struct quagga_signal_t watchfrr_signals[] = {
 
 FRR_DAEMON_INFO(watchfrr, WATCHFRR,
 		.flags = FRR_NO_PRIVSEP | FRR_NO_TCPVTY | FRR_LIMITED_CLI
-			 | FRR_NO_CFG_PID_DRY | FRR_NO_ZCLIENT,
+			 | FRR_NO_CFG_PID_DRY | FRR_NO_ZCLIENT
+			 | FRR_DETACH_LATER,
 
 		.printhelp = printhelp,
 		.copyright = "Copyright 2004 Andrew J. Schorr",
@@ -999,13 +1050,8 @@ FRR_DAEMON_INFO(watchfrr, WATCHFRR,
 int main(int argc, char **argv)
 {
 	int opt;
-	const char *pidfile = pidfile_default;
-	const char *special = "zebra";
 	const char *blankstr = NULL;
 
-	snprintf(pidfile_default, sizeof(pidfile_default), "%s/watchfrr.pid",
-		 frr_vtydir);
-
 	frr_preinit(&watchfrr_di, argc, argv);
 	progname = watchfrr_di.progname;
 
@@ -1087,7 +1133,7 @@ int main(int argc, char **argv)
 			gs.period = 1000 * period;
 		} break;
 		case 'p':
-			pidfile = optarg;
+			watchfrr_di.pid_file = optarg;
 			break;
 		case 'r':
 			if (!valid_command(optarg)) {
@@ -1167,99 +1213,18 @@ int main(int argc, char **argv)
 
 	master = frr_init();
 	watchfrr_error_init();
-
-	zlog_set_level(ZLOG_DEST_MONITOR, ZLOG_DISABLED);
-	if (watchfrr_di.daemon_mode) {
-		zlog_set_level(ZLOG_DEST_SYSLOG, MIN(gs.loglevel, LOG_DEBUG));
-		if (daemon(0, 0) < 0) {
-			fprintf(stderr, "Watchfrr daemon failed: %s",
-				strerror(errno));
-			exit(1);
-		}
-	} else
-		zlog_set_level(ZLOG_DEST_STDOUT, MIN(gs.loglevel, LOG_DEBUG));
-
+	watchfrr_init(argc, argv);
 	watchfrr_vty_init();
 
-	frr_vty_serv();
+	frr_config_fork();
 
-	{
-		int i;
-		struct daemon *tail = NULL;
+	zlog_set_level(ZLOG_DEST_MONITOR, ZLOG_DISABLED);
+	if (watchfrr_di.daemon_mode)
+		zlog_set_level(ZLOG_DEST_SYSLOG, MIN(gs.loglevel, LOG_DEBUG));
+	else
+		zlog_set_level(ZLOG_DEST_STDOUT, MIN(gs.loglevel, LOG_DEBUG));
 
-		for (i = optind; i < argc; i++) {
-			struct daemon *dmn;
-
-			if (!(dmn = (struct daemon *)calloc(1, sizeof(*dmn)))) {
-				fprintf(stderr, "calloc(1,%u) failed: %s\n",
-					(unsigned int)sizeof(*dmn),
-					safe_strerror(errno));
-				return 1;
-			}
-			dmn->name = dmn->restart.name = argv[i];
-			dmn->state = DAEMON_INIT;
-			gs.numdaemons++;
-			gs.numdown++;
-			dmn->fd = -1;
-			dmn->t_wakeup = NULL;
-			thread_add_timer_msec(master, wakeup_init, dmn,
-					      100 + (random() % 900),
-					      &dmn->t_wakeup);
-			dmn->restart.interval = gs.min_restart_interval;
-			if (tail)
-				tail->next = dmn;
-			else
-				gs.daemons = dmn;
-			tail = dmn;
-
-			if (!strcmp(dmn->name, special))
-				gs.special = dmn;
-		}
-	}
-	if (!gs.daemons) {
-		flog_err(EC_WATCHFRR_UNEXPECTED_DAEMONS,
-			  "Must specify one or more daemons to monitor.");
-		frr_help_exit(1);
-	}
-	if (!watch_only && !gs.special) {
-		flog_err(EC_WATCHFRR_UNEXPECTED_DAEMONS,
-			  "\"%s\" daemon must be in daemon lists", special);
-		frr_help_exit(1);
-	}
-
-	/* Make sure we're not already running. */
-	pid_output(pidfile);
-
-	/* Announce which daemons are being monitored. */
-	{
-		struct daemon *dmn;
-		size_t len = 0;
-
-		for (dmn = gs.daemons; dmn; dmn = dmn->next)
-			len += strlen(dmn->name) + 1;
-
-		{
-			char buf[len + 1];
-			char *p = buf;
-
-			for (dmn = gs.daemons; dmn; dmn = dmn->next) {
-				if (p != buf)
-					*p++ = ' ';
-				strcpy(p, dmn->name);
-				p += strlen(p);
-			}
-			zlog_notice("%s %s watching [%s]%s", progname,
-				    FRR_VERSION, buf,
-				    watch_only ? ", monitor mode" : "");
-		}
-	}
-
-	{
-		struct thread thread;
-
-		while (thread_fetch(master, &thread))
-			thread_call(&thread);
-	}
+	frr_run(master);
 
 	systemd_send_stopping();
 	/* Not reached. */

watchfrr/watchfrr.h

@@ -21,6 +21,10 @@
 #ifndef FRR_WATCHFRR_H
 #define FRR_WATCHFRR_H
 
+#include "lib/memory.h"
+
+DECLARE_MGROUP(WATCHFRR)
+
 extern void watchfrr_vty_init(void);
 
 extern pid_t integrated_write_pid;