{"thread":{"id":"28744","subject":"[PATCH 0/2] git-credential-cache--daemon on Cygwin","startedAt":"2011-10-22T17:23:25Z","lastAt":"2011-10-27T17:41:23Z","messageCount":4,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"178164","messageId":"4EA2FC0D.5060404@ramsay1.demon.co.uk","threadId":"28744","inReplyTo":null,"subject":"[PATCH 0/2] git-credential-cache--daemon on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-10-22T17:23:25Z","receivedAt":"2011-10-22T17:23:25Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Jeff,\n\nWhen the 'jk/http-auth-keyring' branch was in next (and later back\nin pu), I noticed that the t0300-credentials.sh test failing on cygwin.\n\nI had a quick look, and found that unix_stream_listen() was failing to\nbind() to a stale unix socket. The code looked OK to me, and should have\nhandled this just fine, but didn't ...\n\nOn a hunch, I found that the following \"belt-n-braces\" approach fixed the\nthe test for me:\n\n-- >8 --\ndiff --git a/unix-socket.c b/unix-socket.c\nindex cf9890f..d974e01 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -42,7 +42,9 @@ int unix_stream_listen(const char *path)\n \tfd = unix_stream_socket();\n \n \tif (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {\n+\t\tclose(fd);\n \t\tunlink(path);\n+\t\tfd = unix_stream_socket();\n \t\tif (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {\n \t\t\tclose(fd);\n \t\t\treturn -1;\n-- 8< --\n\nHowever, I don't particularly like the above solution, so I decided to take\na look at credential-cache--daemon to see why it was leaving a stale socket\nin the first place. The following patches are the result:\n\n    [PATCH 1/2] credential-cache--daemon.c: Don't leave stale socket on --exit\n    [PATCH 2/2] credential-cache--daemon.c: unlink() a potentially stale unix socket\n\nNote that, in serve_one_client(), the \"--exit\" action results in the server\nexit(0)-ing immediately without close()-ing and unlink()-ing the socket.\nSo, patch #1 changes serve_one_client() to return a value to serve_cache_loop()\nwhich indicates whether it's caller (serve_cache()) should exit from the main\nserver loop. This results in the socket being cleaned up correctly in the \"--exit\"\ncase.\n\nNote that this does not eliminate all early-exit code paths (for example, we note\nan die_errno() call), so it is still possible for the server to exit without\nhaving cleaned up the socket, so patch #2 adds on unlink() call to the beginning of\nserve_cache().\n\nNote that each of these patches, separately and together, fix the test on cygwin.\n\nAssuming that a modified http-auth-keyring series will make a return to pu\nsometime, could you please squash these patches into (the patch corresponding to)\ncommit 2d6874d (credentials: add \"cache\" helper, 18-07-2011). Thanks!\n\nAlso, note that this series breaks the build on MinGW. The patch to fix the build\nis rather simple, but the result doesn't work, of course, because winsock does\nnot know about AF_UNIX (or AF_LOCAL). I started to code an win32 emulation of unix\nsockets, but stopped working on it when this branch dropped from next. If you\nintend to re-submit this work, then I can pick this up again.\n\nHTH.\n\nATB,\nRamsay Jones\n"},{"id":"178172","messageId":"20111022191509.GB1785@sigill.intra.peff.net","threadId":"28744","inReplyTo":"4EA2FC0D.5060404@ramsay1.demon.co.uk","subject":"Re: [PATCH 0/2] git-credential-cache--daemon on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-22T19:15:10Z","receivedAt":"2011-10-22T19:15:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 22, 2011 at 06:23:25PM +0100, Ramsay Jones wrote:\n\n> I had a quick look, and found that unix_stream_listen() was failing to\n> bind() to a stale unix socket. The code looked OK to me, and should have\n> handled this just fine, but didn't ...\n> \n> On a hunch, I found that the following \"belt-n-braces\" approach fixed the\n> the test for me:\n> \n> -- >8 --\n> diff --git a/unix-socket.c b/unix-socket.c\n> index cf9890f..d974e01 100644\n> --- a/unix-socket.c\n> +++ b/unix-socket.c\n> @@ -42,7 +42,9 @@ int unix_stream_listen(const char *path)\n>  \tfd = unix_stream_socket();\n>  \n>  \tif (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {\n> +\t\tclose(fd);\n>  \t\tunlink(path);\n> +\t\tfd = unix_stream_socket();\n>  \t\tif (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {\n>  \t\t\tclose(fd);\n>  \t\t\treturn -1;\n> -- 8< --\n\nWow, that seems horribly broken on the part of cygwin.\n\nI think your solution to just unlink first() is the right way to go.  I\nhave a few comments, though so I'll respond to each of the patches\nindividually.\n\n> Assuming that a modified http-auth-keyring series will make a return to pu\n> sometime, could you please squash these patches into (the patch corresponding to)\n> commit 2d6874d (credentials: add \"cache\" helper, 18-07-2011). Thanks!\n\nI'm planning a reroll, so I'll squash them (or something similar) in.\n\n> Also, note that this series breaks the build on MinGW. The patch to fix the build\n> is rather simple, but the result doesn't work, of course, because winsock does\n> not know about AF_UNIX (or AF_LOCAL). I started to code an win32 emulation of unix\n> sockets, but stopped working on it when this branch dropped from next. If you\n> intend to re-submit this work, then I can pick this up again.\n\nRight. I sort of expected that, and figured we could just drop the cache\nhelper on mingw until somebody felt like writing such an emulation\nlayer.\n\nIt's definitely coming back, so if you feel like working on it, please\ndo. Also note that if it would be easier to have an alternate\nabstraction for inter-process communication on windows, I'm open to\ndoing that in the cache daemon.\n\n-Peff\n"},{"id":"178360","messageId":"4EA99266.7030002@ramsay1.demon.co.uk","threadId":"28744","inReplyTo":"20111022191509.GB1785@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] git-credential-cache--daemon on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-10-27T17:18:30Z","receivedAt":"2011-10-27T17:18:30Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> On Sat, Oct 22, 2011 at 06:23:25PM +0100, Ramsay Jones wrote:\n> \n>> Assuming that a modified http-auth-keyring series will make a return to pu\n>> sometime, could you please squash these patches into (the patch corresponding to)\n>> commit 2d6874d (credentials: add \"cache\" helper, 18-07-2011). Thanks!\n> \n> I'm planning a reroll, so I'll squash them (or something similar) in.\n\nThanks!\n\n> It's definitely coming back, so if you feel like working on it, please\n> do. Also note that if it would be easier to have an alternate\n> abstraction for inter-process communication on windows, I'm open to\n> doing that in the cache daemon.\n\nMy initial reaction was to use a \"named pipe\" (aka fifo), but on reflection,\nI don't think it would be any easier; the unix socket emulation should not be too\ndifficult (famous last words!).  :-D\n\nATB,\nRamsay Jones\n"},{"id":"178364","messageId":"20111027174123.GA1967@sigill.intra.peff.net","threadId":"28744","inReplyTo":"4EA99266.7030002@ramsay1.demon.co.uk","subject":"Re: [PATCH 0/2] git-credential-cache--daemon on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-27T17:41:23Z","receivedAt":"2011-10-27T17:41:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 27, 2011 at 06:18:30PM +0100, Ramsay Jones wrote:\n\n> > It's definitely coming back, so if you feel like working on it, please\n> > do. Also note that if it would be easier to have an alternate\n> > abstraction for inter-process communication on windows, I'm open to\n> > doing that in the cache daemon.\n> \n> My initial reaction was to use a \"named pipe\" (aka fifo), but on reflection,\n> I don't think it would be any easier; the unix socket emulation should not be too\n> difficult (famous last words!).  :-D\n\nThe problem with named pipes is that all clients share the same pipe. So\nif two clients connect, their writes will be intermingled at the server,\nand they will both get the responses for each other. Assuming your\nplatform provides atomic write() over pipes, you could design a\npacket-ized protocol, and just have clients ignore packets not meant for\nthem. But that's getting pretty complex.\n\n-Peff\n"}]}