[PATCH 0/2] git-credential-cache--daemon on Cygwin

[PATCH 0/2] git-credential-cache--daemon on Cygwin

[Ramsay Jones]

Hi Jeff,

When the 'jk/http-auth-keyring' branch was in next (and later back
in pu), I noticed that the t0300-credentials.sh test failing on cygwin.

I had a quick look, and found that unix_stream_listen() was failing to
bind() to a stale unix socket. The code looked OK to me, and should have
handled this just fine, but didn't ...

On a hunch, I found that the following "belt-n-braces" approach fixed the
the test for me:

-- >8 --
diff --git a/unix-socket.c b/unix-socket.c
index cf9890f..d974e01 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -42,7 +42,9 @@ int unix_stream_listen(const char *path)
 	fd = unix_stream_socket();
 
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+		close(fd);
 		unlink(path);
+		fd = unix_stream_socket();
 		if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
 			close(fd);
 			return -1;
-- 8< --

However, I don't particularly like the above solution, so I decided to take
a look at credential-cache--daemon to see why it was leaving a stale socket
in the first place. The following patches are the result:

    [PATCH 1/2] credential-cache--daemon.c: Don't leave stale socket on --exit
    [PATCH 2/2] credential-cache--daemon.c: unlink() a potentially stale unix socket

Note that, in serve_one_client(), the "--exit" action results in the server
exit(0)-ing immediately without close()-ing and unlink()-ing the socket.
So, patch #1 changes serve_one_client() to return a value to serve_cache_loop()
which indicates whether it's caller (serve_cache()) should exit from the main
server loop. This results in the socket being cleaned up correctly in the "--exit"
case.

Note that this does not eliminate all early-exit code paths (for example, we note
an die_errno() call), so it is still possible for the server to exit without
having cleaned up the socket, so patch #2 adds on unlink() call to the beginning of
serve_cache().

Note that each of these patches, separately and together, fix the test on cygwin.

Assuming that a modified http-auth-keyring series will make a return to pu
sometime, could you please squash these patches into (the patch corresponding to)
commit 2d6874d (credentials: add "cache" helper, 18-07-2011). Thanks!

Also, note that this series breaks the build on MinGW. The patch to fix the build
is rather simple, but the result doesn't work, of course, because winsock does
not know about AF_UNIX (or AF_LOCAL). I started to code an win32 emulation of unix
sockets, but stopped working on it when this branch dropped from next. If you
intend to re-submit this work, then I can pick this up again.

HTH.

ATB,
Ramsay Jones

Re: [PATCH 0/2] git-credential-cache--daemon on Cygwin

[Jeff King]

On Sat, Oct 22, 2011 at 06:23:25PM +0100, Ramsay Jones wrote:

> I had a quick look, and found that unix_stream_listen() was failing to
> bind() to a stale unix socket. The code looked OK to me, and should have
> handled this just fine, but didn't ...
> 
> On a hunch, I found that the following "belt-n-braces" approach fixed the
> the test for me:
> 
> -- >8 --
> diff --git a/unix-socket.c b/unix-socket.c
> index cf9890f..d974e01 100644
> --- a/unix-socket.c
> +++ b/unix-socket.c
> @@ -42,7 +42,9 @@ int unix_stream_listen(const char *path)
>  	fd = unix_stream_socket();
>  
>  	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
> +		close(fd);
>  		unlink(path);
> +		fd = unix_stream_socket();
>  		if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
>  			close(fd);
>  			return -1;
> -- 8< --

Wow, that seems horribly broken on the part of cygwin.

I think your solution to just unlink first() is the right way to go.  I
have a few comments, though so I'll respond to each of the patches
individually.

> Assuming that a modified http-auth-keyring series will make a return to pu
> sometime, could you please squash these patches into (the patch corresponding to)
> commit 2d6874d (credentials: add "cache" helper, 18-07-2011). Thanks!

I'm planning a reroll, so I'll squash them (or something similar) in.

> Also, note that this series breaks the build on MinGW. The patch to fix the build
> is rather simple, but the result doesn't work, of course, because winsock does
> not know about AF_UNIX (or AF_LOCAL). I started to code an win32 emulation of unix
> sockets, but stopped working on it when this branch dropped from next. If you
> intend to re-submit this work, then I can pick this up again.

Right. I sort of expected that, and figured we could just drop the cache
helper on mingw until somebody felt like writing such an emulation
layer.

It's definitely coming back, so if you feel like working on it, please
do. Also note that if it would be easier to have an alternate
abstraction for inter-process communication on windows, I'm open to
doing that in the cache daemon.

-Peff

Re: [PATCH 0/2] git-credential-cache--daemon on Cygwin

[Ramsay Jones]

Jeff King wrote:
> On Sat, Oct 22, 2011 at 06:23:25PM +0100, Ramsay Jones wrote:
> 
>> Assuming that a modified http-auth-keyring series will make a return to pu
>> sometime, could you please squash these patches into (the patch corresponding to)
>> commit 2d6874d (credentials: add "cache" helper, 18-07-2011). Thanks!
> 
> I'm planning a reroll, so I'll squash them (or something similar) in.

Thanks!

> It's definitely coming back, so if you feel like working on it, please
> do. Also note that if it would be easier to have an alternate
> abstraction for inter-process communication on windows, I'm open to
> doing that in the cache daemon.

My initial reaction was to use a "named pipe" (aka fifo), but on reflection,
I don't think it would be any easier; the unix socket emulation should not be too
difficult (famous last words!).  :-D

ATB,
Ramsay Jones

Re: [PATCH 0/2] git-credential-cache--daemon on Cygwin

[Jeff King]

On Thu, Oct 27, 2011 at 06:18:30PM +0100, Ramsay Jones wrote:

> > It's definitely coming back, so if you feel like working on it, please
> > do. Also note that if it would be easier to have an alternate
> > abstraction for inter-process communication on windows, I'm open to
> > doing that in the cache daemon.
> 
> My initial reaction was to use a "named pipe" (aka fifo), but on reflection,
> I don't think it would be any easier; the unix socket emulation should not be too
> difficult (famous last words!).  :-D

The problem with named pipes is that all clients share the same pipe. So
if two clients connect, their writes will be intermingled at the server,
and they will both get the responses for each other. Assuming your
platform provides atomic write() over pipes, you could design a
packet-ized protocol, and just have clients ignore packets not meant for
them. But that's getting pretty complex.

-Peff