Patchwork [09,of,10] chg: use validate to make sure the server is up to date

login
register
mail settings
Submitter Jun Wu
Date March 2, 2016, 10:44 a.m.
Message ID <f27ebf7ddfd12df9e4c0.1456915451@x1c>
Download mbox | patch
Permalink /patch/13529/
State Superseded
Commit 2f0f352d41969ddbfe9b7dd4010dbd27b893d7b6
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 2, 2016, 10:44 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1456913782 0
#      Wed Mar 02 10:16:22 2016 +0000
# Node ID f27ebf7ddfd12df9e4c087d419454504fc7f097a
# Parent  801df1c31d0c49a19fe3dca0025b393dcc292599
chg: use validate to make sure the server is up to date

This patch uses the newly added validate method to make sure the server has
loaded the up-to-date config and extensions. It will follow instructions from
the server like redirecting to another server or unlinking a socket file (
effectively stopping an out-dated server).
timeless - March 2, 2016, 2:34 p.m.
Jun Wu <quark@fb.com> wrote:
> +                               abortmsg("too long redirect path (r = %d)", r);

redirect path too long
Jun Wu - March 2, 2016, 2:38 p.m.
On 03/02/2016 02:34 PM, timeless wrote:
> redirect path too long

This is actually intentional to match the error message before (in
setcmdserveropts).
timeless - March 2, 2016, 2:54 p.m.
Unless there's this is designed to match something you can't change,
just change it everywhere :)
(You can do it in a distinct commit if it makes you feel better.)

On Wed, Mar 2, 2016 at 9:38 AM, Jun Wu <quark@fb.com> wrote:
> On 03/02/2016 02:34 PM, timeless wrote:
>>
>> redirect path too long
>
>
> This is actually intentional to match the error message before (in
> setcmdserveropts).
Yuya Nishihara - March 3, 2016, 4:08 p.m.
On Wed, 2 Mar 2016 10:44:11 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456913782 0
> #      Wed Mar 02 10:16:22 2016 +0000
> # Node ID f27ebf7ddfd12df9e4c087d419454504fc7f097a
> # Parent  801df1c31d0c49a19fe3dca0025b393dcc292599
> chg: use validate to make sure the server is up to date
> 
> This patch uses the newly added validate method to make sure the server has
> loaded the up-to-date config and extensions. It will follow instructions from
> the server like redirecting to another server or unlinking a socket file (
> effectively stopping an out-dated server).
> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -31,6 +31,7 @@
>  
>  struct cmdserveropts {
>  	char sockname[UNIX_PATH_MAX];
> +	char redirectsockname[UNIX_PATH_MAX];
>  	char lockfile[UNIX_PATH_MAX];
>  	char pidfile[UNIX_PATH_MAX];
>  	size_t argsize;
> @@ -269,18 +270,24 @@
>  /* Connect to a cmdserver. Will start a new server on demand. */
>  static hgclient_t *connectcmdserver(struct cmdserveropts *opts)
>  {
> -	hgclient_t *hgc = hgc_open(opts->sockname);
> +	const char *sockname = opts->redirectsockname[0] ?
> +		opts->redirectsockname : opts->sockname;
> +	hgclient_t *hgc = hgc_open(sockname);
>  	if (hgc)
>  		return hgc;
>  
>  	lockcmdserver(opts);
> -	hgc = hgc_open(opts->sockname);
> +	hgc = hgc_open(sockname);
>  	if (hgc) {
>  		unlockcmdserver(opts);
>  		debugmsg("cmdserver is started by another process");
>  		return hgc;
>  	}
>  
> +	/* prevent us from being connected to an outdated server */
> +	if (sockname == opts->redirectsockname)
> +		unlink(opts->sockname);
> +
>  	debugmsg("start cmdserver at %s", opts->sockname);
>  
>  	pid_t pid = fork();
> @@ -447,6 +454,28 @@
>  	abortmsg("failed to prepare pager (errno = %d)", errno);
>  }
>  
> +/* Run instructions sent from the server like unlink and set redirect path */
> +static void runinstructions(struct cmdserveropts *opts, const char **insts)
> +{
> +	assert(insts);
> +	opts->redirectsockname[0] = '\0';
> +	const char **pinst;
> +	for (pinst = insts; *pinst; pinst++) {
> +		debugmsg("instruction: %s", *pinst);
> +		if (strncmp(*pinst, "unlink ", 7) == 0) {

Nit: I prefer sizeof() rather than magic number:

  static const char xxx[] = "unlink ";
  sizeof(xxx) - 1

Other than that, this and the next patch look good to me. Please resend with
the V2 series.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -31,6 +31,7 @@ 
 
 struct cmdserveropts {
 	char sockname[UNIX_PATH_MAX];
+	char redirectsockname[UNIX_PATH_MAX];
 	char lockfile[UNIX_PATH_MAX];
 	char pidfile[UNIX_PATH_MAX];
 	size_t argsize;
@@ -269,18 +270,24 @@ 
 /* Connect to a cmdserver. Will start a new server on demand. */
 static hgclient_t *connectcmdserver(struct cmdserveropts *opts)
 {
-	hgclient_t *hgc = hgc_open(opts->sockname);
+	const char *sockname = opts->redirectsockname[0] ?
+		opts->redirectsockname : opts->sockname;
+	hgclient_t *hgc = hgc_open(sockname);
 	if (hgc)
 		return hgc;
 
 	lockcmdserver(opts);
-	hgc = hgc_open(opts->sockname);
+	hgc = hgc_open(sockname);
 	if (hgc) {
 		unlockcmdserver(opts);
 		debugmsg("cmdserver is started by another process");
 		return hgc;
 	}
 
+	/* prevent us from being connected to an outdated server */
+	if (sockname == opts->redirectsockname)
+		unlink(opts->sockname);
+
 	debugmsg("start cmdserver at %s", opts->sockname);
 
 	pid_t pid = fork();
@@ -447,6 +454,28 @@ 
 	abortmsg("failed to prepare pager (errno = %d)", errno);
 }
 
+/* Run instructions sent from the server like unlink and set redirect path */
+static void runinstructions(struct cmdserveropts *opts, const char **insts)
+{
+	assert(insts);
+	opts->redirectsockname[0] = '\0';
+	const char **pinst;
+	for (pinst = insts; *pinst; pinst++) {
+		debugmsg("instruction: %s", *pinst);
+		if (strncmp(*pinst, "unlink ", 7) == 0) {
+			unlink(*pinst + 7);
+		} else if (strncmp(*pinst, "redirect ", 9) == 0) {
+			int r = snprintf(opts->redirectsockname,
+					 sizeof(opts->redirectsockname),
+					 "%s", *pinst + 9);
+			if (r < 0 || r >= (int)sizeof(opts->redirectsockname))
+				abortmsg("too long redirect path (r = %d)", r);
+		} else {
+			abortmsg("unknown instruction: %s", *pinst);
+		}
+	}
+}
+
 /*
  * Test whether the command is unsupported or not. This is not designed to
  * cover all cases. But it's fast, does not depend on the server and does
@@ -515,12 +544,21 @@ 
 		}
 	}
 
-	hgclient_t *hgc = connectcmdserver(&opts);
-	if (!hgc)
-		abortmsg("cannot open hg client");
+	hgclient_t *hgc;
+	while (1) {
+		hgc = connectcmdserver(&opts);
+		if (!hgc)
+			abortmsg("cannot open hg client");
+		hgc_setenv(hgc, envp);
+		const char **insts = hgc_validate(hgc, argv + 1, argc - 1);
+		if (insts == NULL)
+			break;
+		runinstructions(&opts, insts);
+		hgc_close(hgc);
+		free(insts);
+	}
 
 	setupsignalhandler(hgc_peerpid(hgc));
-	hgc_setenv(hgc, envp);
 	setuppager(hgc, argv + 1, argc - 1);
 	int exitcode = hgc_runcommand(hgc, argv + 1, argc - 1);
 	hgc_close(hgc);