Patchwork [2,of,2] chg: do not write pidfile

login
register
mail settings
Submitter Jun Wu
Date March 10, 2016, 12:32 a.m.
Message ID <1d7ddf23a970310f5288.1457569951@x1c>
Download mbox | patch
Permalink /patch/13734/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 10, 2016, 12:32 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1457569195 0
#      Thu Mar 10 00:19:55 2016 +0000
# Node ID 1d7ddf23a970310f5288a275080061ebc7ce5762
# Parent  a0a69881240c389b0c5e84451474cde66025f6d8
chg: do not write pidfile

Current pidfile logic will only keep the pid of the newest server, which is
not very useful if we want to kill all servers, and will become outdated if
the server auto exits after being idle for too long.

Besides, the server-side pidfile writing logic runs before chgserver gets
confighash so it's not trivial to append confighash to pidfile basename like
we did for socket file.

This patch removes --pidfile from the command starting chgserver and switches
to an alternative way (unlink socket file) to stop the server.
Yuya Nishihara - March 11, 2016, 8:10 a.m.
On Thu, 10 Mar 2016 00:32:31 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1457569195 0
> #      Thu Mar 10 00:19:55 2016 +0000
> # Node ID 1d7ddf23a970310f5288a275080061ebc7ce5762
> # Parent  a0a69881240c389b0c5e84451474cde66025f6d8
> chg: do not write pidfile
> 
> Current pidfile logic will only keep the pid of the newest server, which is
> not very useful if we want to kill all servers, and will become outdated if
> the server auto exits after being idle for too long.
> 
> Besides, the server-side pidfile writing logic runs before chgserver gets
> confighash so it's not trivial to append confighash to pidfile basename like
> we did for socket file.
> 
> This patch removes --pidfile from the command starting chgserver and switches
> to an alternative way (unlink socket file) to stop the server.

I like plain old pidfile, but new approach works better and simpler. Pushed to
the clowncopter, thanks.

> +	char *resolvedpath = realpath(opts->sockname, NULL);

Added short comment why we do realpath().

> +	if (resolvedpath) {
> +		unlink(resolvedpath);
> +		free(resolvedpath);

And updated run-tests.py to unlink(sock) in place of killdaemons().

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -33,7 +33,6 @@ 
 	char sockname[UNIX_PATH_MAX];
 	char redirectsockname[UNIX_PATH_MAX];
 	char lockfile[UNIX_PATH_MAX];
-	char pidfile[UNIX_PATH_MAX];
 	size_t argsize;
 	const char **args;
 	int lockfd;
@@ -149,16 +148,12 @@ 
 	const char *basename = (envsockname) ? envsockname : sockdir;
 	const char *sockfmt = (envsockname) ? "%s" : "%s/server";
 	const char *lockfmt = (envsockname) ? "%s.lock" : "%s/lock";
-	const char *pidfmt = (envsockname) ? "%s.pid" : "%s/pid";
 	r = snprintf(opts->sockname, sizeof(opts->sockname), sockfmt, basename);
 	if (r < 0 || (size_t)r >= sizeof(opts->sockname))
 		abortmsg("too long TMPDIR or CHGSOCKNAME (r = %d)", r);
 	r = snprintf(opts->lockfile, sizeof(opts->lockfile), lockfmt, basename);
 	if (r < 0 || (size_t)r >= sizeof(opts->lockfile))
 		abortmsg("too long TMPDIR or CHGSOCKNAME (r = %d)", r);
-	r = snprintf(opts->pidfile, sizeof(opts->pidfile), pidfmt, basename);
-	if (r < 0 || (size_t)r >= sizeof(opts->pidfile))
-		abortmsg("too long TMPDIR or CHGSOCKNAME (r = %d)", r);
 }
 
 /*
@@ -215,7 +210,6 @@ 
 		"--cmdserver", "chgunix",
 		"--address", opts->sockname,
 		"--daemon-postexec", "none",
-		"--pid-file", opts->pidfile,
 		"--config", "extensions.chgserver=",
 	};
 	size_t baseargvsize = sizeof(baseargv) / sizeof(baseargv[0]);
@@ -315,21 +309,12 @@ 
 	return hgc;
 }
 
-static void killcmdserver(const struct cmdserveropts *opts, int sig)
+static void killcmdserver(const struct cmdserveropts *opts)
 {
-	FILE *fp = fopen(opts->pidfile, "r");
-	if (!fp)
-		abortmsg("cannot open %s (errno = %d)", opts->pidfile, errno);
-	int pid = 0;
-	int n = fscanf(fp, "%d", &pid);
-	fclose(fp);
-	if (n != 1 || pid <= 0)
-		abortmsg("cannot read pid from %s", opts->pidfile);
-
-	if (kill((pid_t)pid, sig) < 0) {
-		if (errno == ESRCH)
-			return;
-		abortmsg("cannot kill %d (errno = %d)", pid, errno);
+	char *resolvedpath = realpath(opts->sockname, NULL);
+	if (resolvedpath) {
+		unlink(resolvedpath);
+		free(resolvedpath);
 	}
 }
 
@@ -537,11 +522,8 @@ 
 	setcmdserverargs(&opts, argc, argv);
 
 	if (argc == 2) {
-		int sig = 0;
-		if (strcmp(argv[1], "--kill-chg-daemon") == 0)
-			sig = SIGTERM;
-		if (sig > 0) {
-			killcmdserver(&opts, sig);
+		if (strcmp(argv[1], "--kill-chg-daemon") == 0) {
+			killcmdserver(&opts);
 			return 0;
 		}
 	}