[PATCH 1/2] credential-cache--daemon.c: Don't leave stale socket on --exit

[PATCH 1/2] credential-cache--daemon.c: Don't leave stale socket on --exit

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 credential-cache--daemon.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 128c5ce..ee2c15a 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -137,22 +137,22 @@ static int read_credential_request(FILE *fh, struct credential *c,
 	return 0;
 }
 
-static void serve_one_client(FILE *in, FILE *out)
+static int serve_one_client(FILE *in, FILE *out)
 {
 	struct credential c = { NULL };
 	int timeout = -1;
 	char *action = NULL;
 
 	if (read_credential_request(in, &c, &action, &timeout) < 0)
-		return;
+		return 1;
 
 	if (!action) {
 		warning("cache client didn't specify an action");
-		return;
+		return 1;
 	}
 
 	if (!strcmp(action, "exit"))
-		exit(0);
+		return 0;
 
 	if (!strcmp(action, "get")) {
 		struct credential_cache_entry *e = lookup_credential(&c);
@@ -160,27 +160,27 @@ static void serve_one_client(FILE *in, FILE *out)
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
 		}
-		return;
+		return 1;
 	}
 
 	if (!strcmp(action, "erase")) {
 		remove_credential(&c);
-		return;
+		return 1;
 	}
 
 	if (!strcmp(action, "store")) {
 		if (timeout < 0) {
 			warning("cache client didn't specify a timeout");
-			return;
+			return 1;
 		}
 
 		remove_credential(&c);
 		cache_credential(&c, timeout);
-		return;
+		return 1;
 	}
 
 	warning("cache client sent unknown action: %s", action);
-	return;
+	return 1;
 }
 
 static int serve_cache_loop(int fd)
@@ -201,7 +201,7 @@ static int serve_cache_loop(int fd)
 	}
 
 	if (pfd.revents & POLLIN) {
-		int client, client2;
+		int client, client2, ret;
 		FILE *in, *out;
 
 		client = accept(fd, NULL, NULL);
@@ -218,9 +218,10 @@ static int serve_cache_loop(int fd)
 
 		in = xfdopen(client, "r");
 		out = xfdopen(client2, "w");
-		serve_one_client(in, out);
+		ret = serve_one_client(in, out);
 		fclose(in);
 		fclose(out);
+		return ret;
 	}
 	return 1;
 }
-- 
1.7.7

Re: [PATCH 1/2] credential-cache--daemon.c: Don't leave stale socket on --exit

[Jeff King]

On Sat, Oct 22, 2011 at 06:24:51PM +0100, Ramsay Jones wrote:

> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  credential-cache--daemon.c |   23 ++++++++++++-----------
>  1 files changed, 12 insertions(+), 11 deletions(-)

Looks sane, and I'll probably squash it in. Alternatively, we could also
set a signal/exit handler to clean up our socket when we die. That would
also cover the error exit cases.

In either case, I think we need to handle stale sockets better. They
will happen eventually due to power loss or kill -9, anyway.

-Peff

Re: [PATCH 1/2] credential-cache--daemon.c: Don't leave stale socket on --exit

[Ramsay Jones]

Jeff King wrote:
> On Sat, Oct 22, 2011 at 06:24:51PM +0100, Ramsay Jones wrote:
> 
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> ---
>>  credential-cache--daemon.c |   23 ++++++++++++-----------
>>  1 files changed, 12 insertions(+), 11 deletions(-)
> 
> Looks sane, and I'll probably squash it in. Alternatively, we could also
> set a signal/exit handler to clean up our socket when we die. That would
> also cover the error exit cases.

I considered this, *very* briefly, but decided it wasn't worth the effort
or complexity.

> In either case, I think we need to handle stale sockets better. They
> will happen eventually due to power loss or kill -9, anyway.

Indeed, hence patch #2. ;-)

I suspect that, given the current code, the *vast* majority of stale sockets
would be (somewhat gratuitously) created by the --exit action - hence this
patch. :-P

ATB,
Ramsay Jones

Re: [PATCH 1/2] credential-cache--daemon.c: Don't leave stale socket on --exit

[Jeff King]

On Thu, Oct 27, 2011 at 06:29:46PM +0100, Ramsay Jones wrote:

> Jeff King wrote:
> > On Sat, Oct 22, 2011 at 06:24:51PM +0100, Ramsay Jones wrote:
> > 
> >> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> >> ---
> >>  credential-cache--daemon.c |   23 ++++++++++++-----------
> >>  1 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > Looks sane, and I'll probably squash it in. Alternatively, we could also
> > set a signal/exit handler to clean up our socket when we die. That would
> > also cover the error exit cases.
> 
> I considered this, *very* briefly, but decided it wasn't worth the effort
> or complexity.

Actually, I think with the sigchain code, it would only end up as a few
lines. I'll probably take a look when I re-roll.

Thanks for your patches.

-Peff