{"thread":{"id":"21755","subject":"[PATCH/RFC 00/11] daemon-win32","startedAt":"2009-11-26T00:44:09Z","lastAt":"2010-01-10T17:06:52Z","messageCount":54,"participants":["Erik Faye-Lund","Junio C Hamano","Martin Storsjö","Paolo Bonzini","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"128399","messageId":"1259196260-3064-1-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":null,"subject":"[PATCH/RFC 00/11] daemon-win32","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:09Z","receivedAt":"2009-11-26T00:44:09Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"This is my stab at cleaning up Mike Pape's patches for git-daemon\non Windows for submission, plus some of my own.\n\n* Patch 1-4 originate from Mike Pape, but have been cleaned up quite\n a lot by me. I hope Mike won't hate me. Credit have been retained,\n as all important code here were written by Mike. The commit\n messages were written mostly by me.\n\n* Patch 5 is a trivial old-style function declaration fix.\n\n* Patch 6 introduces two new functions to the run-command API,\n kill_async() and is_async_alive().\n\n* Patch 7-8 removes the stdin/stdout redirection for the service\n functions, as redirecting won't work for the threaded version.\n\n* Patch 9 converts the daemon-code to use the run-command API. This\n is the patch I expect to be the most controversial.\n\n* Patch 10 is about using line-buffered mode instead of full-buffered\n mode for stderr.\n\n* Patch 11 finally enables compilation of git-daemon on MinGW.\n\nLet the flames begin!\n\nErik Faye-Lund (7):\n inet_ntop: fix a couple of old-style decls\n run-command: add kill_async() and is_async_alive()\n run-command: support input-fd\n daemon: use explicit file descriptor\n daemon: use run-command api for async serving\n daemon: use full buffered mode for stderr\n mingw: compile git-daemon\n\nMike Pape (4):\n mingw: add network-wrappers for daemon\n strbuf: add non-variadic function strbuf_vaddf()\n mingw: implement syslog\n compat: add inet_pton and inet_ntop prototypes\n\n Makefile | 8 ++-\n compat/inet_ntop.c | 22 ++------\n compat/inet_pton.c | 8 ++-\n compat/mingw.c | 111 +++++++++++++++++++++++++++++++++++++++++--\n compat/mingw.h | 32 ++++++++++++\n daemon.c | 134 ++++++++++++++++++++++++++--------------------------\n git-compat-util.h | 9 ++++\n run-command.c | 32 ++++++++++++-\n run-command.h | 2 +\n strbuf.c | 15 ++++--\n strbuf.h | 1 +\n 11 files changed, 274 insertions(+), 100 deletions(-)\n"},{"id":"128400","messageId":"1259196260-3064-2-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:10Z","receivedAt":"2009-11-26T00:44:10Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Mike Pape \n\ngit-daemon requires some socket-functionality that is not yet\nsupported in the Windows-port. This patch adds said functionality,\nand makes sure WSAStartup gets called by socket(), since it is the\nfirst network-call in git-daemon. In addition, a check is added to\nprevent WSAStartup (and WSACleanup, though atexit) from being\ncalled more than once, since git-daemon calls both socket() and\ngethostbyname().\n\nSigned-off-by: Mike Pape \nSigned-off-by: Erik Faye-Lund \n---\n compat/mingw.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++---\n compat/mingw.h | 16 ++++++++++++++\n 2 files changed, 72 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 10d6796..458021b 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -983,23 +983,37 @@ char **make_augmented_environ(const char *const *vars)\n \treturn env;\n }\n \n-/* this is the first function to call into WS_32; initialize it */\n-#undef gethostbyname\n-struct hostent *mingw_gethostbyname(const char *host)\n+static void wsa_init(void)\n {\n+\tstatic int initialized = 0;\n \tWSADATA wsa;\n \n+\tif (initialized)\n+\t\treturn;\n+\n \tif (WSAStartup(MAKEWORD(2,2), &wsa))\n \t\tdie(\"unable to initialize winsock subsystem, error %d\",\n \t\t\tWSAGetLastError());\n \tatexit((void(*)(void)) WSACleanup);\n+\tinitialized = 1;\n+}\n+\n+/* this can be the first function to call into WS_32; initialize it */\n+#undef gethostbyname\n+struct hostent *mingw_gethostbyname(const char *host)\n+{\n+\twsa_init();\n \treturn gethostbyname(host);\n }\n \n+/* this can be the first function to call into WS_32; initialize it */\n int mingw_socket(int domain, int type, int protocol)\n {\n+\tSOCKET s;\n \tint sockfd;\n-\tSOCKET s = WSASocket(domain, type, protocol, NULL, 0, 0);\n+\n+\twsa_init();\n+\ts = WSASocket(domain, type, protocol, NULL, 0, 0);\n \tif (s == INVALID_SOCKET) {\n \t\t/*\n \t\t * WSAGetLastError() values are regular BSD error codes\n@@ -1029,6 +1043,44 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)\n \treturn connect(s, sa, sz);\n }\n \n+#undef bind\n+int mingw_bind(int sockfd, struct sockaddr *sa, size_t sz)\n+{\n+\tSOCKET s = (SOCKET)_get_osfhandle(sockfd);\n+\treturn bind(s, sa, sz);\n+}\n+\n+#undef setsockopt\n+int mingw_setsockopt(int sockfd, int lvl, int optname, void *optval, int optlen)\n+{\n+\tSOCKET s = (SOCKET)_get_osfhandle(sockfd);\n+\treturn setsockopt(s, lvl, optname, (const char*)optval, optlen);\n+}\n+\n+#undef listen\n+int mingw_listen(int sockfd, int backlog)\n+{\n+\tSOCKET s = (SOCKET)_get_osfhandle(sockfd);\n+\treturn listen(s, backlog);\n+}\n+\n+#undef accept\n+int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)\n+{\n+\tint sockfd2;\n+\n+\tSOCKET s1 = (SOCKET)_get_osfhandle(sockfd1);\n+\tSOCKET s2 = accept(s1, sa, sz);\n+\n+\t/* convert into a file descriptor */\n+\tif ((sockfd2 = _open_osfhandle(s2, O_RDWR|O_BINARY)) < 0) {\n+\t\tclosesocket(s2);\n+\t\treturn error(\"unable to make a socket file descriptor: %s\",\n+\t\t\tstrerror(errno));\n+\t}\n+\treturn sockfd2;\n+}\n+\n #undef rename\n int mingw_rename(const char *pold, const char *pnew)\n {\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 1978f8a..f362f61 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -5,6 +5,7 @@\n */\n \n typedef int pid_t;\n+typedef int socklen_t;\n #define hstrerror strerror\n \n #define S_IFLNK 0120000 /* Symbolic link */\n@@ -33,6 +34,9 @@ typedef int pid_t;\n #define F_SETFD 2\n #define FD_CLOEXEC 0x1\n \n+#define EAFNOSUPPORT WSAEAFNOSUPPORT\n+#define ECONNABORTED WSAECONNABORTED\n+\n struct passwd {\n \tchar *pw_name;\n \tchar *pw_gecos;\n@@ -182,6 +186,18 @@ int mingw_socket(int domain, int type, int protocol);\n int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz);\n #define connect mingw_connect\n \n+int mingw_bind(int sockfd, struct sockaddr *sa, size_t sz);\n+#define bind mingw_bind\n+\n+int mingw_setsockopt(int sockfd, int lvl, int optname, void *optval, int optlen);\n+#define setsockopt mingw_setsockopt\n+\n+int mingw_listen(int sockfd, int backlog);\n+#define listen mingw_listen\n+\n+int mingw_accept(int sockfd, struct sockaddr *sa, socklen_t *sz);\n+#define accept mingw_accept\n+\n int mingw_rename(const char*, const char*);\n #define rename mingw_rename\n \n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128401","messageId":"1259196260-3064-3-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-2-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:11Z","receivedAt":"2009-11-26T00:44:11Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Mike Pape \n\nThis patch adds strbuf_vaddf, which takes a va_list as input\ninstead of the variadic input that strbuf_addf takes. This\nis useful for fowarding varargs to strbuf_addf.\n\nSigned-off-by: Mike Pape \nSigned-off-by: Erik Faye-Lund \n---\n strbuf.c | 15 +++++++++------\n strbuf.h | 1 +\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex a6153dc..e1833fb 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -190,23 +190,18 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)\n \tstrbuf_setlen(sb, sb->len + len);\n }\n \n-void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n+void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n {\n \tint len;\n-\tva_list ap;\n \n \tif (!strbuf_avail(sb))\n \t\tstrbuf_grow(sb, 64);\n-\tva_start(ap, fmt);\n \tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n-\tva_end(ap);\n \tif (len < 0)\n \t\tdie(\"your vsnprintf is broken\");\n \tif (len > strbuf_avail(sb)) {\n \t\tstrbuf_grow(sb, len);\n-\t\tva_start(ap, fmt);\n \t\tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n-\t\tva_end(ap);\n \t\tif (len > strbuf_avail(sb)) {\n \t\t\tdie(\"this should not happen, your snprintf is broken\");\n \t\t}\n@@ -214,6 +209,14 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n \tstrbuf_setlen(sb, sb->len + len);\n }\n \n+void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n+{\n+\tva_list va;\n+\tva_start(va, fmt);\n+\tstrbuf_vaddf(sb, fmt, va);\n+\tva_end(va);\n+}\n+\n void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n \t\t void *context)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex d05e056..8686bcb 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -119,6 +119,7 @@ extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n \n __attribute__((format(printf,2,3)))\n extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n \n extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n /* XXX: if read fails, any partial read is undone */\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128403","messageId":"1259196260-3064-4-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-3-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 03/11] mingw: implement syslog","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:12Z","receivedAt":"2009-11-26T00:44:12Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Mike Pape \n\nSyslog does not usually exist on Windows, so we implement our own\nusing Window's ReportEvent mechanism.\n\nSigned-off-by: Mike Pape \nSigned-off-by: Erik Faye-Lund \n---\n compat/mingw.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++\n compat/mingw.h | 15 +++++++++++++++\n daemon.c | 2 --\n git-compat-util.h | 1 +\n 4 files changed, 67 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 458021b..68116ac 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1255,6 +1255,57 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n \treturn 0;\n }\n \n+static HANDLE ms_eventlog;\n+\n+void openlog(const char *ident, int logopt, int facility) {\n+\tif (ms_eventlog)\n+\t\treturn;\n+\tms_eventlog = RegisterEventSourceA(NULL, ident);\n+}\n+\n+void syslog(int priority, const char *fmt, ...) {\n+\tstruct strbuf msg;\n+\tva_list va;\n+\tWORD logtype;\n+\n+\tstrbuf_init(&msg, 0);\n+\tva_start(va, fmt);\n+\tstrbuf_vaddf(&msg, fmt, va);\n+\tva_end(va);\n+\n+\tswitch (priority) {\n+\t\tcase LOG_EMERG:\n+\t\tcase LOG_ALERT:\n+\t\tcase LOG_CRIT:\n+\t\tcase LOG_ERR:\n+\t\t\tlogtype = EVENTLOG_ERROR_TYPE;\n+\t\t\tbreak;\n+\n+\t\tcase LOG_WARNING:\n+\t\t\tlogtype = EVENTLOG_WARNING_TYPE;\n+\t\t\tbreak;\n+\n+\t\tcase LOG_NOTICE:\n+\t\tcase LOG_INFO:\n+\t\tcase LOG_DEBUG:\n+\t\tdefault:\n+\t\t\tlogtype = EVENTLOG_INFORMATION_TYPE;\n+\t\t\tbreak;\n+\t}\n+\n+\tReportEventA(ms_eventlog,\n+\t logtype,\n+\t (WORD)NULL,\n+\t (DWORD)NULL,\n+\t NULL,\n+\t 1,\n+\t 0,\n+\t (const char **)&msg.buf,\n+\t NULL);\n+\n+\tstrbuf_release(&msg);\n+}\n+\n #undef signal\n sig_handler_t mingw_signal(int sig, sig_handler_t handler)\n {\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex f362f61..576b1a1 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -37,6 +37,19 @@ typedef int socklen_t;\n #define EAFNOSUPPORT WSAEAFNOSUPPORT\n #define ECONNABORTED WSAECONNABORTED\n \n+#define LOG_PID 0x01\n+\n+#define LOG_EMERG 0\n+#define LOG_ALERT 1\n+#define LOG_CRIT 2\n+#define LOG_ERR 3\n+#define LOG_WARNING 4\n+#define LOG_NOTICE 5\n+#define LOG_INFO 6\n+#define LOG_DEBUG 7\n+\n+#define LOG_DAEMON (3<<3)\n+\n struct passwd {\n \tchar *pw_name;\n \tchar *pw_gecos;\n@@ -157,6 +170,8 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out);\n int link(const char *oldpath, const char *newpath);\n int symlink(const char *oldpath, const char *newpath);\n int readlink(const char *path, char *buf, size_t bufsiz);\n+void openlog(const char *ident, int logopt, int facility);\n+void syslog(int priority, const char *fmt, ...);\n \n /*\n * replacements of existing functions\ndiff --git a/daemon.c b/daemon.c\nindex 1b5ada6..07d7356 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -4,8 +4,6 @@\n #include \"run-command.h\"\n #include \"strbuf.h\"\n \n-#include \n-\n #ifndef HOST_NAME_MAX\n #define HOST_NAME_MAX 256\n #endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ef60803..33a8e33 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -105,6 +105,7 @@\n #include \n #include \n #include \n+#include \n #if defined(__CYGWIN__)\n #undef _XOPEN_SOURCE\n #include \n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128402","messageId":"1259196260-3064-5-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-4-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 04/11] compat: add inet_pton and inet_ntop prototypes","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:13Z","receivedAt":"2009-11-26T00:44:13Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Mike Pape \n\nWindows doesn't have inet_pton and inet_ntop, so\nadd prototypes in git-compat-util.h for them.\n\nAt the same time include git-compat-util.h in\nthe sources for these functions, so they use the\nnetwork-wrappers from there on Windows.\n\nSigned-off-by: Mike Pape \nSigned-off-by: Erik Faye-Lund \n---\n Makefile | 2 ++\n compat/inet_ntop.c | 6 +++---\n compat/inet_pton.c | 8 +++++---\n git-compat-util.h | 8 ++++++++\n 4 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 70cee6d..3b01694 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1227,9 +1227,11 @@ endif\n endif\n ifdef NO_INET_NTOP\n \tLIB_OBJS += compat/inet_ntop.o\n+\tBASIC_CFLAGS += -DNO_INET_NTOP\n endif\n ifdef NO_INET_PTON\n \tLIB_OBJS += compat/inet_pton.o\n+\tBASIC_CFLAGS += -DNO_INET_PTON\n endif\n \n ifdef NO_ICONV\ndiff --git a/compat/inet_ntop.c b/compat/inet_ntop.c\nindex f444982..e5b46a0 100644\n--- a/compat/inet_ntop.c\n+++ b/compat/inet_ntop.c\n@@ -17,9 +17,9 @@\n \n #include \n #include \n-#include \n-#include \n-#include \n+\n+#include \"../git-compat-util.h\"\n+\n #include \n #include \n \ndiff --git a/compat/inet_pton.c b/compat/inet_pton.c\nindex 4078fc0..2ec995e 100644\n--- a/compat/inet_pton.c\n+++ b/compat/inet_pton.c\n@@ -17,9 +17,9 @@\n \n #include \n #include \n-#include \n-#include \n-#include \n+\n+#include \"../git-compat-util.h\"\n+\n #include \n #include \n \n@@ -41,7 +41,9 @@\n */\n \n static int inet_pton4(const char *src, unsigned char *dst);\n+#ifndef NO_IPV6\n static int inet_pton6(const char *src, unsigned char *dst);\n+#endif\n \n /* int\n * inet_pton4(src, dst)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 33a8e33..27fa601 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -340,6 +340,14 @@ static inline char *gitstrchrnul(const char *s, int c)\n }\n #endif\n \n+#ifdef NO_INET_PTON\n+int inet_pton(int af, const char *src, void *dst);\n+#endif\n+\n+#ifdef NO_INET_NTOP\n+const char *inet_ntop(int af, const void *src, char *dst, size_t size);\n+#endif\n+\n extern void release_pack_memory(size_t, int);\n \n extern char *xstrdup(const char *str);\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128404","messageId":"1259196260-3064-6-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-5-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 05/11] inet_ntop: fix a couple of old-style decls","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:14Z","receivedAt":"2009-11-26T00:44:14Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Signed-off-by: Erik Faye-Lund \n---\n compat/inet_ntop.c | 16 +++-------------\n 1 files changed, 3 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/inet_ntop.c b/compat/inet_ntop.c\nindex e5b46a0..ea249c6 100644\n--- a/compat/inet_ntop.c\n+++ b/compat/inet_ntop.c\n@@ -50,10 +50,7 @@\n *\tPaul Vixie, 1996.\n */\n static const char *\n-inet_ntop4(src, dst, size)\n-\tconst u_char *src;\n-\tchar *dst;\n-\tsize_t size;\n+inet_ntop4(const u_char *src, char *dst, size_t size)\n {\n \tstatic const char fmt[] = \"%u.%u.%u.%u\";\n \tchar tmp[sizeof \"255.255.255.255\"];\n@@ -78,10 +75,7 @@ inet_ntop4(src, dst, size)\n *\tPaul Vixie, 1996.\n */\n static const char *\n-inet_ntop6(src, dst, size)\n-\tconst u_char *src;\n-\tchar *dst;\n-\tsize_t size;\n+inet_ntop6(const u_char *src, char *dst, size_t size)\n {\n \t/*\n \t * Note that int32_t and int16_t need only be \"at least\" large enough\n@@ -178,11 +172,7 @@ inet_ntop6(src, dst, size)\n *\tPaul Vixie, 1996.\n */\n const char *\n-inet_ntop(af, src, dst, size)\n-\tint af;\n-\tconst void *src;\n-\tchar *dst;\n-\tsize_t size;\n+inet_ntop(int af, const void *src, char *dst, size_t size)\n {\n \tswitch (af) {\n \tcase AF_INET:\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128405","messageId":"1259196260-3064-7-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-6-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:15Z","receivedAt":"2009-11-26T00:44:15Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"These functions will aid the Windows port of git-daemon.\n\nSigned-off-by: Erik Faye-Lund \n---\n run-command.c | 27 +++++++++++++++++++++++++++\n run-command.h | 2 ++\n 2 files changed, 29 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..e5a0e06 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -373,6 +373,33 @@ int finish_async(struct async *async)\n \treturn ret;\n }\n \n+int kill_async(struct async *async)\n+{\n+#ifndef WIN32\n+\treturn kill(async->pid, SIGTERM);\n+#else\n+\tDWORD ret = 0;\n+\tif (!TerminateThread(async->tid, 0))\n+\t\tret = error(\"killing thread failed: %lu\", GetLastError());\n+\telse if (!GetExitCodeThread(async->tid, &ret))\n+\t\tret = error(\"cannot get thread exit code: %lu\", GetLastError());\n+\treturn ret;\n+#endif\n+}\n+\n+int is_async_alive(struct async *async)\n+{\n+#ifndef WIN32\n+\tint dummy;\n+\treturn waitpid(async->pid, &dummy, WNOHANG);\n+#else\n+\tint ret = WaitForSingleObject(async->tid, 0);\n+\tif (ret == WAIT_FAILED)\n+\t\treturn error(\"checking thread state failed: %lu\", GetLastError());\n+\treturn ret != WAIT_OBJECT_0;\n+#endif\n+}\n+\n int run_hook(const char *index_file, const char *name, ...)\n {\n \tstruct child_process hook;\ndiff --git a/run-command.h b/run-command.h\nindex fb34209..955b0bd 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -80,5 +80,7 @@ struct async {\n \n int start_async(struct async *async);\n int finish_async(struct async *async);\n+int kill_async(struct async *async);\n+int is_async_alive(struct async *async);\n \n #endif\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128406","messageId":"1259196260-3064-8-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-7-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 07/11] run-command: support input-fd","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:16Z","receivedAt":"2009-11-26T00:44:16Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"This patch adds the possibility to supply a non-0\nfile descriptor for communucation, instead of the\ndefault-created pipe. The pipe gets duplicated, so\nthe caller can free it's handles.\n\nThis is usefull for async communication over sockets.\n\nSigned-off-by: Erik Faye-Lund \n---\n run-command.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex e5a0e06..98771ef 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -327,7 +327,10 @@ int start_async(struct async *async)\n {\n \tint pipe_out[2];\n \n-\tif (pipe(pipe_out) < 0)\n+\tif (async->out) {\n+\t\tpipe_out[0] = dup(async->out);\n+\t\tpipe_out[1] = dup(async->out);\n+\t} else if (pipe(pipe_out) < 0)\n \t\treturn error(\"cannot create pipe: %s\", strerror(errno));\n \tasync->out = pipe_out[0];\n \n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128407","messageId":"1259196260-3064-9-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-8-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 08/11] daemon: use explicit file descriptor","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:17Z","receivedAt":"2009-11-26T00:44:17Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"This patch adds support to specify an explicit file\ndescriotor for communication with the client, instead\nof using stdin/stdout.\n\nThis will be useful for the Windows port, because it\nwill use threads instead of fork() to serve multiple\nclients, making it impossible to reuse stdin/stdout.\n\nSigned-off-by: Erik Faye-Lund \n---\n daemon.c | 34 ++++++++++++++++------------------\n 1 files changed, 16 insertions(+), 18 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 07d7356..a0aead5 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -263,7 +263,7 @@ static char *path_ok(char *directory)\n \treturn NULL;\t\t/* Fallthrough. Deny by default */\n }\n \n-typedef int (*daemon_service_fn)(void);\n+typedef int (*daemon_service_fn)(int);\n struct daemon_service {\n \tconst char *name;\n \tconst char *config_name;\n@@ -287,7 +287,7 @@ static int git_daemon_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n-static int run_service(char *dir, struct daemon_service *service)\n+static int run_service(int fd, char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n \tint enabled = service->enabled;\n@@ -340,7 +340,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \t */\n \tsignal(SIGTERM, SIG_IGN);\n \n-\treturn service->fn();\n+\treturn service->fn(fd);\n }\n \n static void copy_to_log(int fd)\n@@ -364,7 +364,7 @@ static void copy_to_log(int fd)\n \tfclose(fp);\n }\n \n-static int run_service_command(const char **argv)\n+static int run_service_command(int fd, const char **argv)\n {\n \tstruct child_process cld;\n \n@@ -372,37 +372,35 @@ static int run_service_command(const char **argv)\n \tcld.argv = argv;\n \tcld.git_cmd = 1;\n \tcld.err = -1;\n+\tcld.in = cld.out = fd;\n \tif (start_command(&cld))\n \t\treturn -1;\n \n-\tclose(0);\n-\tclose(1);\n-\n \tcopy_to_log(cld.err);\n \n \treturn finish_command(&cld);\n }\n \n-static int upload_pack(void)\n+static int upload_pack(int fd)\n {\n \t/* Timeout as string */\n \tchar timeout_buf[64];\n \tconst char *argv[] = { \"upload-pack\", \"--strict\", timeout_buf, \".\", NULL };\n \n \tsnprintf(timeout_buf, sizeof timeout_buf, \"--timeout=%u\", timeout);\n-\treturn run_service_command(argv);\n+\treturn run_service_command(fd, argv);\n }\n \n-static int upload_archive(void)\n+static int upload_archive(int fd)\n {\n \tstatic const char *argv[] = { \"upload-archive\", \".\", NULL };\n-\treturn run_service_command(argv);\n+\treturn run_service_command(fd, argv);\n }\n \n-static int receive_pack(void)\n+static int receive_pack(int fd)\n {\n \tstatic const char *argv[] = { \"receive-pack\", \".\", NULL };\n-\treturn run_service_command(argv);\n+\treturn run_service_command(fd, argv);\n }\n \n static struct daemon_service daemon_service[] = {\n@@ -532,7 +530,7 @@ static void parse_host_arg(char *extra_args, int buflen)\n }\n \n \n-static int execute(struct sockaddr *addr)\n+static int execute(int fd, struct sockaddr *addr)\n {\n \tstatic char line[1000];\n \tint pktlen, len, i;\n@@ -565,7 +563,7 @@ static int execute(struct sockaddr *addr)\n \t}\n \n \talarm(init_timeout ? init_timeout : timeout);\n-\tpktlen = packet_read_line(0, line, sizeof(line));\n+\tpktlen = packet_read_line(fd, line, sizeof(line));\n \talarm(0);\n \n \tlen = strlen(line);\n@@ -597,7 +595,7 @@ static int execute(struct sockaddr *addr)\n \t\t\t * Note: The directory here is probably context sensitive,\n \t\t\t * and might depend on the actual service being performed.\n \t\t\t */\n-\t\t\treturn run_service(line + namelen + 5, s);\n+\t\t\treturn run_service(fd, line + namelen + 5, s);\n \t\t}\n \t}\n \n@@ -713,7 +711,7 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \tdup2(incoming, 1);\n \tclose(incoming);\n \n-\texit(execute(addr));\n+\texit(execute(0, addr));\n }\n \n static void child_handler(int signo)\n@@ -1150,7 +1148,7 @@ int main(int argc, char **argv)\n \t\tif (getpeername(0, peer, &slen))\n \t\t\tpeer = NULL;\n \n-\t\treturn execute(peer);\n+\t\treturn execute(0, peer);\n \t}\n \n \tif (detach) {\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128408","messageId":"1259196260-3064-10-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-9-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 09/11] daemon: use run-command api for async serving","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:18Z","receivedAt":"2009-11-26T00:44:18Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"fork() is only available on POSIX, so to support git-daemon\non Windows we have to use something else. Conveniently\nenough, we have an API for async operation already.\n\nSigned-off-by: Erik Faye-Lund \n---\n daemon.c | 79 ++++++++++++++++++++++++++++---------------------------------\n 1 files changed, 36 insertions(+), 43 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex a0aead5..1b0e290 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -372,7 +372,8 @@ static int run_service_command(int fd, const char **argv)\n \tcld.argv = argv;\n \tcld.git_cmd = 1;\n \tcld.err = -1;\n-\tcld.in = cld.out = fd;\n+\tcld.in = dup(fd);\n+\tcld.out = fd;\n \tif (start_command(&cld))\n \t\treturn -1;\n \n@@ -609,11 +610,11 @@ static unsigned int live_children;\n \n static struct child {\n \tstruct child *next;\n-\tpid_t pid;\n+\tstruct async async;\n \tstruct sockaddr_storage address;\n } *firstborn;\n \n-static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n+static void add_child(struct async *async, struct sockaddr *addr, int addrlen)\n {\n \tstruct child *newborn, **cradle;\n \n@@ -623,7 +624,7 @@ static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n \t */\n \tnewborn = xcalloc(1, sizeof(*newborn));\n \tlive_children++;\n-\tnewborn->pid = pid;\n+\tmemcpy(&newborn->async, async, sizeof(struct async));\n \tmemcpy(&newborn->address, addr, addrlen);\n \tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n \t\tif (!memcmp(&(*cradle)->address, &newborn->address,\n@@ -633,19 +634,6 @@ static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n \t*cradle = newborn;\n }\n \n-static void remove_child(pid_t pid)\n-{\n-\tstruct child **cradle, *blanket;\n-\n-\tfor (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)\n-\t\tif (blanket->pid == pid) {\n-\t\t\t*cradle = blanket->next;\n-\t\t\tlive_children--;\n-\t\t\tfree(blanket);\n-\t\t\tbreak;\n-\t\t}\n-}\n-\n /*\n * This gets called if the number of connections grows\n * past \"max_connections\".\n@@ -654,7 +642,7 @@ static void remove_child(pid_t pid)\n */\n static void kill_some_child(void)\n {\n-\tconst struct child *blanket, *next;\n+\tstruct child *blanket, *next;\n \n \tif (!(blanket = firstborn))\n \t\treturn;\n@@ -662,28 +650,37 @@ static void kill_some_child(void)\n \tfor (; (next = blanket->next); blanket = next)\n \t\tif (!memcmp(&blanket->address, &next->address,\n \t\t\t sizeof(next->address))) {\n-\t\t\tkill(blanket->pid, SIGTERM);\n+\t\t\tkill_async(&blanket->async);\n \t\t\tbreak;\n \t\t}\n }\n \n static void check_dead_children(void)\n {\n-\tint status;\n-\tpid_t pid;\n-\n-\twhile ((pid = waitpid(-1, &status, WNOHANG)) > 0) {\n-\t\tconst char *dead = \"\";\n-\t\tremove_child(pid);\n-\t\tif (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))\n-\t\t\tdead = \" (with error)\";\n-\t\tloginfo(\"[%\"PRIuMAX\"] Disconnected%s\", (uintmax_t)pid, dead);\n-\t}\n+\tstruct child **cradle, *blanket;\n+\tfor (cradle = &firstborn; (blanket = *cradle);)\n+\t\tif (!is_async_alive(&blanket->async)) {\n+\t\t\t*cradle = blanket->next;\n+\t\t\tloginfo(\"Disconnected\\n\");\n+\t\t\tlive_children--;\n+\t\t\tclose(blanket->async.out);\n+\t\t\tfree(blanket);\n+\t\t} else\n+\t\t\tcradle = &blanket->next;\n+}\n+\n+int async_execute(int fd, void *data)\n+{\n+\tint ret = execute(fd, data);\n+\tclose(fd);\n+\tfree(data);\n+\treturn ret;\n }\n \n static void handle(int incoming, struct sockaddr *addr, int addrlen)\n {\n-\tpid_t pid;\n+\tstruct sockaddr_storage *ss;\n+\tstruct async async = { 0 };\n \n \tif (max_connections && live_children >= max_connections) {\n \t\tkill_some_child();\n@@ -696,22 +693,18 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \t\t}\n \t}\n \n-\tif ((pid = fork())) {\n-\t\tclose(incoming);\n-\t\tif (pid < 0) {\n-\t\t\tlogerror(\"Couldn't fork %s\", strerror(errno));\n-\t\t\treturn;\n-\t\t}\n+\tss = xmalloc(sizeof(*ss));\n+\tmemcpy(ss, addr, addrlen);\n \n-\t\tadd_child(pid, addr, addrlen);\n-\t\treturn;\n-\t}\n+\tasync.proc = async_execute;\n+\tasync.data = ss;\n+\tasync.out = incoming;\n \n-\tdup2(incoming, 0);\n-\tdup2(incoming, 1);\n+\tif (start_async(&async))\n+\t\tlogerror(\"unable to fork\");\n+\telse\n+\t\tadd_child(&async, addr, addrlen);\n \tclose(incoming);\n-\n-\texit(execute(0, addr));\n }\n \n static void child_handler(int signo)\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128409","messageId":"1259196260-3064-11-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-10-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 10/11] daemon: use full buffered mode for stderr","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:19Z","receivedAt":"2009-11-26T00:44:19Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Windows doesn't support line buffered mode for file\nstreams, so let's just use full buffered mode with\na big buffer (\"4096 should be enough for everyone\")\nand add explicit flushing.\n\nSigned-off-by: Erik Faye-Lund \n---\n daemon.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 1b0e290..130e951 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -66,12 +66,14 @@ static void logreport(int priority, const char *err, va_list params)\n \t\tsyslog(priority, \"%s\", buf);\n \t} else {\n \t\t/*\n-\t\t * Since stderr is set to linebuffered mode, the\n+\t\t * Since stderr is set to buffered mode, the\n \t\t * logging of different processes will not overlap\n+\t\t * unless they overflow the (rather big) buffers.\n \t\t */\n \t\tfprintf(stderr, \"[%\"PRIuMAX\"] \", (uintmax_t)getpid());\n \t\tvfprintf(stderr, err, params);\n \t\tfputc('\\n', stderr);\n+\t\tfflush(stderr);\n \t}\n }\n \n@@ -1094,7 +1096,7 @@ int main(int argc, char **argv)\n \t\tset_die_routine(daemon_die);\n \t} else\n \t\t/* avoid splitting a message in the middle */\n-\t\tsetvbuf(stderr, NULL, _IOLBF, 0);\n+\t\tsetvbuf(stderr, NULL, _IOFBF, 4096);\n \n \tif (inetd_mode && (group_name || user_name))\n \t\tdie(\"--user and --group are incompatible with --inetd\");\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128410","messageId":"1259196260-3064-12-git-send-email-kusmabite@gmail.com","threadId":"21755","inReplyTo":"1259196260-3064-11-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 11/11] mingw: compile git-daemon","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T00:44:20Z","receivedAt":"2009-11-26T00:44:20Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"--user and --detach are disabled on Windows due to lack of\nfork(), setuid(), setgid(), setsid() and initgroups().\n\nSigned-off-by: Erik Faye-Lund \n---\n Makefile | 6 +++---\n compat/mingw.h | 1 +\n daemon.c | 19 ++++++++++++++-----\n 3 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 3b01694..406ca81 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -352,6 +352,7 @@ EXTRA_PROGRAMS =\n \n # ... and all the rest that could be moved out of bindir to gitexecdir\n PROGRAMS += $(EXTRA_PROGRAMS)\n+PROGRAMS += git-daemon$X\n PROGRAMS += git-fast-import$X\n PROGRAMS += git-hash-object$X\n PROGRAMS += git-imap-send$X\n@@ -981,6 +982,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n \tNO_REGEX = YesPlease\n \tBLK_SHA1 = YesPlease\n+\tNO_INET_PTON = YesPlease\n+\tNO_INET_NTOP = YesPlease\n \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch\n \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n \t# We have GCC, so let's make use of those nice options\n@@ -1079,9 +1082,6 @@ ifdef ZLIB_PATH\n endif\n EXTLIBS += -lz\n \n-ifndef NO_POSIX_ONLY_PROGRAMS\n-\tPROGRAMS += git-daemon$X\n-endif\n ifndef NO_OPENSSL\n \tOPENSSL_LIBSSL = -lssl\n \tifdef OPENSSLDIR\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 576b1a1..1b0dd5b 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -6,6 +6,7 @@\n \n typedef int pid_t;\n typedef int socklen_t;\n+typedef unsigned int gid_t;\n #define hstrerror strerror\n \n #define S_IFLNK 0120000 /* Symbolic link */\ndiff --git a/daemon.c b/daemon.c\nindex 130e951..5872ec7 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -616,7 +616,7 @@ static struct child {\n \tstruct sockaddr_storage address;\n } *firstborn;\n \n-static void add_child(struct async *async, struct sockaddr *addr, int addrlen)\n+static void add_child(struct async *async, struct sockaddr *addr, socklen_t addrlen)\n {\n \tstruct child *newborn, **cradle;\n \n@@ -679,7 +679,7 @@ int async_execute(int fd, void *data)\n \treturn ret;\n }\n \n-static void handle(int incoming, struct sockaddr *addr, int addrlen)\n+static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n {\n \tstruct sockaddr_storage *ss;\n \tstruct async async = { 0 };\n@@ -884,7 +884,7 @@ static int service_loop(int socknum, int *socklist)\n \t\tfor (i = 0; i < socknum; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n \t\t\t\tstruct sockaddr_storage ss;\n-\t\t\t\tunsigned int sslen = sizeof(ss);\n+\t\t\t\tsocklen_t sslen = sizeof(ss);\n \t\t\t\tint incoming = accept(pfd[i].fd, (struct sockaddr *)&ss, &sslen);\n \t\t\t\tif (incoming < 0) {\n \t\t\t\t\tswitch (errno) {\n@@ -916,6 +916,7 @@ static void sanitize_stdfds(void)\n \n static void daemonize(void)\n {\n+#ifndef WIN32\n \tswitch (fork()) {\n \t\tcase 0:\n \t\t\tbreak;\n@@ -930,6 +931,9 @@ static void daemonize(void)\n \tclose(1);\n \tclose(2);\n \tsanitize_stdfds();\n+#else\n+\tdie(\"--detach is not supported on Windows\");\n+#endif\n }\n \n static void store_pid(const char *path)\n@@ -950,10 +954,12 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t\n \t\tdie(\"unable to allocate any listen sockets on host %s port %u\",\n \t\t listen_addr, listen_port);\n \n+#ifndef WIN32\n \tif (pass && gid &&\n \t (initgroups(pass->pw_name, gid) || setgid (gid) ||\n \t setuid(pass->pw_uid)))\n \t\tdie(\"cannot drop privileges\");\n+#endif\n \n \treturn service_loop(socknum, socklist);\n }\n@@ -966,7 +972,6 @@ int main(int argc, char **argv)\n \tconst char *pid_file = NULL, *user_name = NULL, *group_name = NULL;\n \tint detach = 0;\n \tstruct passwd *pass = NULL;\n-\tstruct group *group;\n \tgid_t gid = 0;\n \tint i;\n \n@@ -1110,6 +1115,7 @@ int main(int argc, char **argv)\n \t\tdie(\"--group supplied without --user\");\n \n \tif (user_name) {\n+#ifndef WIN32\n \t\tpass = getpwnam(user_name);\n \t\tif (!pass)\n \t\t\tdie(\"user not found - %s\", user_name);\n@@ -1117,12 +1123,15 @@ int main(int argc, char **argv)\n \t\tif (!group_name)\n \t\t\tgid = pass->pw_gid;\n \t\telse {\n-\t\t\tgroup = getgrnam(group_name);\n+\t\t\tstruct group *group = getgrnam(group_name);\n \t\t\tif (!group)\n \t\t\t\tdie(\"group not found - %s\", group_name);\n \n \t\t\tgid = group->gr_gid;\n \t\t}\n+#else\n+\t\tdie(\"--user is not supported on Windows\");\n+#endif\n \t}\n \n \tif (strict_paths && (!ok_paths || !*ok_paths))\n-- \n1.6.5.rc2.7.g4f8d3\n"},{"id":"128414","messageId":"7vskc2ksnn.fsf@alter.siamese.dyndns.org","threadId":"21755","inReplyTo":"1259196260-3064-3-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-26T00:59:24Z","receivedAt":"2009-11-26T00:59:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n> {\n> \tint len;\n> \n> \tif (!strbuf_avail(sb))\n> \t\tstrbuf_grow(sb, 64);\n> \tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n> \tif (len < 0)\n> \t\tdie(\"your vsnprintf is broken\");\n> \tif (len > strbuf_avail(sb)) {\n> \t\tstrbuf_grow(sb, len);\n> \t\tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n> \t\tif (len > strbuf_avail(sb)) {\n> \t\t\tdie(\"this should not happen, your snprintf is broken\");\n> \t\t}\n\nHmm, I would have expected to see va_copy() somewhere in the patch text.\nIs it safe to reuse ap like this in two separate invocations of\nvsnprintf()?\n"},{"id":"128444","messageId":"alpine.DEB.2.00.0911261015140.14228@cone.home.martin.st","threadId":"21755","inReplyTo":"1259196260-3064-2-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Martin Storsjö","fromEmail":"martin@martin.st","sentAt":"2009-11-26T08:24:08Z","receivedAt":"2009-11-26T08:24:08Z","isPatch":true,"sender":{"key":"martin@martin.st","avatar":"https://avatars.githubusercontent.com/u/69727?v=4"},"body":"Hi,\n\nFirst of all, great that you're working on adding daemon support for \nwindows!\n\nOn Thu, 26 Nov 2009, Erik Faye-Lund wrote:\n\n> +static void wsa_init(void)\n> {\n> +\tstatic int initialized = 0;\n> \tWSADATA wsa;\n> \n> +\tif (initialized)\n> +\t\treturn;\n> +\n> \tif (WSAStartup(MAKEWORD(2,2), &wsa))\n> \t\tdie(\"unable to initialize winsock subsystem, error %d\",\n> \t\t\tWSAGetLastError());\n> \tatexit((void(*)(void)) WSACleanup);\n> +\tinitialized = 1;\n> +}\n\nSomething similar to this was merged into master recently as part of my \nmingw/ipv6 patches, so by rebasing your patch on top of that, this patch \nwill probably get a bit smaller.\n\nAlso, the getaddrinfo-compatibility wrappers perhaps may need some minor \nupdates to handle the use cases needed for setting up listening sockets.\n\n// Martin\n"},{"id":"128448","messageId":"40aa078e0911260238rd0c90cag126709d1de5f50de@mail.gmail.com","threadId":"21755","inReplyTo":"7vskc2ksnn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T10:38:49Z","receivedAt":"2009-11-26T10:38:49Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n>>  {\n>>       int len;\n>>\n>>       if (!strbuf_avail(sb))\n>>               strbuf_grow(sb, 64);\n>>       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n>>       if (len < 0)\n>>               die(\"your vsnprintf is broken\");\n>>       if (len > strbuf_avail(sb)) {\n>>               strbuf_grow(sb, len);\n>>               len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n>>               if (len > strbuf_avail(sb)) {\n>>                       die(\"this should not happen, your snprintf is broken\");\n>>               }\n>\n> Hmm, I would have expected to see va_copy() somewhere in the patch text.\n> Is it safe to reuse ap like this in two separate invocations of\n> vsnprintf()?\n>\n\nI think your expectation is well justified, this seems to be a\nportability-bug waiting to happen. Sorry for missing this prior to\nsending out - on Windows this is known to work, and this function is\ncurrently only used from the Windows implementation of syslog.\n\nHow kosher is it to use va_copy in the git-core, considering that it's\nC99? A quick grep reveals only one occurrence of va_copy in the\nsource, and that's in compat/winansi.c. Searching the history of next\nreveals that Alex Riesen (CC'd) already removed one occurrence\n(4bf5383), so I'm starting to get slightly scared it might not be OK.\n\nIn practice it seems that something like the following works\nportably-enough for many applications, dunno if it's something we'll\nbe happy with:\n#ifndef va_copy\n#define va_copy(a,b) ((a) = (b))\n#endif\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128449","messageId":"alpine.DEB.2.00.0911261241590.14228@cone.home.martin.st","threadId":"21755","inReplyTo":"alpine.DEB.2.00.0911261015140.14228@cone.home.martin.st","subject":"[PATCH] Improve the mingw getaddrinfo stub to handle more use cases","fromName":"Martin Storsjö","fromEmail":"martin@martin.st","sentAt":"2009-11-26T10:43:44Z","receivedAt":"2009-11-26T10:43:44Z","isPatch":true,"sender":{"key":"martin@martin.st","avatar":"https://avatars.githubusercontent.com/u/69727?v=4"},"body":"Allow the node parameter to be null, which is used for getting\nthe default bind address.\n\nAlso allow the hints parameter to be null, to improve standard\nconformance of the stub implementation a little.\n\nSigned-off-by: Martin Storsjo \n---\n\nThis patch adds support for the getaddrinfo parameters used by git-daemon, \nas mentioned earlier.\n\n compat/mingw.c | 20 +++++++++++++-------\n 1 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0653560..17d1314 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -913,19 +913,22 @@ static int WSAAPI getaddrinfo_stub(const char *node, const char *service,\n \t\t\t\t const struct addrinfo *hints,\n \t\t\t\t struct addrinfo **res)\n {\n-\tstruct hostent *h = gethostbyname(node);\n+\tstruct hostent *h = NULL;\n \tstruct addrinfo *ai;\n \tstruct sockaddr_in *sin;\n \n-\tif (!h)\n-\t\treturn WSAGetLastError();\n+\tif (node) {\n+\t\th = gethostbyname(node);\n+\t\tif (!h)\n+\t\t\treturn WSAGetLastError();\n+\t}\n \n \tai = xmalloc(sizeof(struct addrinfo));\n \t*res = ai;\n \tai->ai_flags = 0;\n \tai->ai_family = AF_INET;\n-\tai->ai_socktype = hints->ai_socktype;\n-\tswitch (hints->ai_socktype) {\n+\tai->ai_socktype = hints ? hints->ai_socktype : 0;\n+\tswitch (ai->ai_socktype) {\n \tcase SOCK_STREAM:\n \t\tai->ai_protocol = IPPROTO_TCP;\n \t\tbreak;\n@@ -937,14 +940,17 @@ static int WSAAPI getaddrinfo_stub(const char *node, const char *service,\n \t\tbreak;\n \t}\n \tai->ai_addrlen = sizeof(struct sockaddr_in);\n-\tai->ai_canonname = strdup(h->h_name);\n+\tai->ai_canonname = h ? strdup(h->h_name) : NULL;\n \n \tsin = xmalloc(ai->ai_addrlen);\n \tmemset(sin, 0, ai->ai_addrlen);\n \tsin->sin_family = AF_INET;\n \tif (service)\n \t\tsin->sin_port = htons(atoi(service));\n-\tsin->sin_addr = *(struct in_addr *)h->h_addr;\n+\tif (h)\n+\t\tsin->sin_addr = *(struct in_addr *)h->h_addr;\n+\telse\n+\t\tsin->sin_addr.s_addr = INADDR_ANY;\n \tai->ai_addr = (struct sockaddr *)sin;\n \tai->ai_next = 0;\n \treturn 0;\n-- \n1.6.4.4\n"},{"id":"128450","messageId":"40aa078e0911260246j47fa36d5t421de7c1d07d5cca@mail.gmail.com","threadId":"21755","inReplyTo":"alpine.DEB.2.00.0911261015140.14228@cone.home.martin.st","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T10:46:43Z","receivedAt":"2009-11-26T10:46:43Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Nov 26, 2009 at 9:24 AM, Martin Storsjö wrote:\n> Hi,\n>\n> First of all, great that you're working on adding daemon support for\n> windows!\n\nThanks. I meant to send this out a couple of weeks ago, but I wasn't\nable to find the time until now.\n\nAlso, I wouldn't have come this far without going tired of it without\nMike's patches, so some credit should go to him for doing good initial\nwork!\n\n> On Thu, 26 Nov 2009, Erik Faye-Lund wrote:\n>\n>> +static void wsa_init(void)\n>>  {\n>> +     static int initialized = 0;\n>>       WSADATA wsa;\n>>\n>> +     if (initialized)\n>> +             return;\n>> +\n>>       if (WSAStartup(MAKEWORD(2,2), &wsa))\n>>               die(\"unable to initialize winsock subsystem, error %d\",\n>>                       WSAGetLastError());\n>>       atexit((void(*)(void)) WSACleanup);\n>> +     initialized = 1;\n>> +}\n>\n> Something similar to this was merged into master recently as part of my\n> mingw/ipv6 patches, so by rebasing your patch on top of that, this patch\n> will probably get a bit smaller.\n\nYeah, I saw your patches, and realized that I needed to rebase my work\nat some point, but none of the repos I usually pull from seems to\ncontain the patches yet. Rebasing will be a requirement before this\ncan be applied for sure.\n\n>\n> Also, the getaddrinfo-compatibility wrappers perhaps may need some minor\n> updates to handle the use cases needed for setting up listening sockets.\n\nI expect you're referring to IPv6 support in the wrappers this patch\nadds? Unfortunately IPv6 isn't something I'm very familiar with, but\nI'll give it a go unless someone else provides some patches...\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128451","messageId":"alpine.DEB.2.00.0911261250180.14228@cone.home.martin.st","threadId":"21755","inReplyTo":"40aa078e0911260246j47fa36d5t421de7c1d07d5cca@mail.gmail.com","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Martin Storsjö","fromEmail":"martin@martin.st","sentAt":"2009-11-26T11:03:37Z","receivedAt":"2009-11-26T11:03:37Z","isPatch":true,"sender":{"key":"martin@martin.st","avatar":"https://avatars.githubusercontent.com/u/69727?v=4"},"body":"On Thu, 26 Nov 2009, Erik Faye-Lund wrote:\n\n> Yeah, I saw your patches, and realized that I needed to rebase my work\n> at some point, but none of the repos I usually pull from seems to\n> contain the patches yet. Rebasing will be a requirement before this\n> can be applied for sure.\n\nOk, great! I tried applying it on the latest master, and it wasn't too \nmuch of work.\n\n> > Also, the getaddrinfo-compatibility wrappers perhaps may need some minor\n> > updates to handle the use cases needed for setting up listening sockets.\n> \n> I expect you're referring to IPv6 support in the wrappers this patch\n> adds? Unfortunately IPv6 isn't something I'm very familiar with, but\n> I'll give it a go unless someone else provides some patches...\n\nNo, sorry for being unclear.\n\nWhen IPv6 is enabled, name lookups go through getaddrinfo instead of \ngethostbyname. Since getaddrinfo isn't available on Win2k (and switching \nbetween getaddrinfo/gethostbyname happens at compile time when IPv6 is \nenabled), we have to provide a small getaddrinfo stub, implemented in \nterms of gethostbyname. This currently implements only parts of the \ngetaddrinfo interface - enough for the way getaddrinfo was used this far.\n\ngit-daemon uses getaddrinfo in a slightly different way (for setting up \nlistening sockets), and thus uses parameters that our current getaddrinfo \nstub doesn't support. The patch I sent to this thread a moment ago adds \nsupport for the way git-daemon uses getaddrinfo.\n\n\nI tested this patch series on top of the latest master, with IPv6 support, \nand found a slight problem caused by the IPv6 support. If IPv6 isn't \nenabled, git-daemon always listens on one single socket, otherwise it may \nlisten on two separate sockets, one for v4 and one for v6.\n\nThis causes problems with the mingw poll() replacement, which has a \nspecial case for polling one single fd - otherwise it tries to use some \nemulation that currently only works for pipes. I didn't try to make any \nproper fix for this though. I tested git-daemon by hacking it to listen on \nonly one of the sockets, and that worked well for both v4 and v6.\n\n\nSo, in addition to the getaddrinfo patch I sent, the mingw poll() \nreplacement needs some updates to handle polling multiple sockets. Except \nfrom that, things seem to work, at a quick glance.\n\n// Martin\n"},{"id":"128453","messageId":"helns7$mha$1@ger.gmane.org","threadId":"21755","inReplyTo":"40aa078e0911260238rd0c90cag126709d1de5f50de@mail.gmail.com","subject":"Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2009-11-26T11:13:11Z","receivedAt":"2009-11-26T11:13:11Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 11/26/2009 11:38 AM, Erik Faye-Lund wrote:\n> In practice it seems that something like the following works\n> portably-enough for many applications, dunno if it's something we'll\n> be happy with:\n> #ifndef va_copy\n> #define va_copy(a,b) ((a) = (b))\n> #endif\n\nYes, this is correct.\n\nPaolo\n"},{"id":"128487","messageId":"7vbpip86q5.fsf@alter.siamese.dyndns.org","threadId":"21755","inReplyTo":"40aa078e0911260238rd0c90cag126709d1de5f50de@mail.gmail.com","subject":"Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-26T18:46:10Z","receivedAt":"2009-11-26T18:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano wrote:\n>> Erik Faye-Lund writes:\n>>\n>>> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n>>>  {\n>>>       int len;\n>>>\n>>>       if (!strbuf_avail(sb))\n>>>               strbuf_grow(sb, 64);\n>>>       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n>>>       if (len < 0)\n>>>               die(\"your vsnprintf is broken\");\n>>>       if (len > strbuf_avail(sb)) {\n>>>               strbuf_grow(sb, len);\n>>>               len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\n>>>               if (len > strbuf_avail(sb)) {\n>>>                       die(\"this should not happen, your snprintf is broken\");\n>>>               }\n>>\n>> Hmm, I would have expected to see va_copy() somewhere in the patch text.\n>> Is it safe to reuse ap like this in two separate invocations of\n>> vsnprintf()?\n>\n> I think your expectation is well justified, this seems to be a\n> portability-bug waiting to happen. Sorry for missing this prior to\n> sending out - on Windows this is known to work, and this function is\n> currently only used from the Windows implementation of syslog.\n>\n> How kosher is it to use va_copy in the git-core, considering that it's\n> C99? A quick grep reveals only one occurrence of va_copy in the\n> source, and that's in compat/winansi.c. Searching the history of next\n> reveals that Alex Riesen (CC'd) already removed one occurrence\n> (4bf5383), so I'm starting to get slightly scared it might not be OK.\n\nWe tend to avoid C99 features and it saved us in a few occasions. Recent\nMSVC port revealed that we still had a handful of decl-after-statments but\nluckily the necessary fix-ups were minimal because I have been reasonably\ncareful to reject patches that add it long before MSVC port happened.\n\n> In practice it seems that something like the following works\n> portably-enough for many applications, dunno if it's something we'll\n> be happy with:\n> #ifndef va_copy\n> #define va_copy(a,b) ((a) = (b))\n> #endif\n\nSince an obvious implementation of va_list would be to make it a pointer\ninto the stack frame, doing the above would work on many systems. On\nesoteric systems that needs something different (e.g. where va_list is\nimplemented as a size-1 array of pointers, va_copy(a,b) needs to be an\nassignment (*(a) = *(b))), people can add compatibility macro later.\n\nHistorically some systems that do have a suitable implementation had it\nunder the name __va_copy() instead, so it would have been better to define\nit as something like:\n\n #ifndef va_copy\n # ifdef __va_copy\n # define va_copy(a,b) __va_copy(a,b)\n # else\n # /* fallback for the most obvious implementation of va_list */\n # define va_copy(a,b) ((a) = (b))\n # endif\n #endif\n\nBut I do not know it still matters in practice anymore.\n"},{"id":"128498","messageId":"200911262104.25087.j6t@kdbg.org","threadId":"21755","inReplyTo":"1259196260-3064-1-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC 00/11] daemon-win32","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-26T20:04:24Z","receivedAt":"2009-11-26T20:04:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> This is my stab at cleaning up Mike Pape's patches for git-daemon\n> on Windows for submission, plus some of my own.\n\nPlease rebase the series on top of current master. I'd appreciate if you could \nmake it available to pull from somewhere. I've a version here:\n\n git://repo.or.cz/git/mingw/j6t.git ef/mingw-daemon\n\n(including Martin's getaddrinfo updates) but the resulting git-daemon crashes \nimmediately after there is a connection.\n\n-- Hannes\n"},{"id":"128502","messageId":"200911262223.22777.j6t@kdbg.org","threadId":"21755","inReplyTo":"1259196260-3064-4-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-26T21:23:22Z","receivedAt":"2009-11-26T21:23:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> +void syslog(int priority, const char *fmt, ...) {\n\nStyle: Brace goes to next line.\n\n> +\tstruct strbuf msg;\n> +\tva_list va;\n> +\tWORD logtype;\n> +\n> +\tstrbuf_init(&msg, 0);\n> +\tva_start(va, fmt);\n> +\tstrbuf_vaddf(&msg, fmt, va);\n> +\tva_end(va);\n\nI would\n\n\tconst char* arg;\n\tif (strcmp(fmt, \"%s\"))\n\t\tdie(\"format string of syslog() not implemented\")\n\tva_start(va, fmt);\n\targ = va_arg(va, char*);\n\tva_end(va);\n\nbecause we have exactly one invocation of syslog(), which passes \"%s\" as \nformat string. Then strbuf_vaddf is not needed. Or even simpler: declare the \nfunction as\n\nvoid syslog(int priority, const char *fmt, const char*arg);\n\n> +\tReportEventA(ms_eventlog,\n> +\t logtype,\n> +\t (WORD)NULL,\n> +\t (DWORD)NULL,\n> +\t NULL,\n> +\t 1,\n> +\t 0,\n> +\t (const char **)&msg.buf,\n> +\t NULL);\n\nWhy do you pass NULL and apply casts? The first one gives a warning.\n\nDid you read this paragraph in the documentation?\nhttp://msdn.microsoft.com/en-us/library/aa363679(VS.85).aspx\n\n\"Note that the string that you log cannot contain %n, where n is an integer \nvalue (for example, %1) because the event viewer treats it as an insertion \nstring. ...\"\n\nHow are the chances that this condition applies to our use of the function?\n\n-- Hannes\n"},{"id":"128504","messageId":"200911262246.13342.j6t@kdbg.org","threadId":"21755","inReplyTo":"1259196260-3064-7-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-26T21:46:13Z","receivedAt":"2009-11-26T21:46:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> +int kill_async(struct async *async)\n> +{\n> +#ifndef WIN32\n> +\treturn kill(async->pid, SIGTERM);\n> +#else\n> +\tDWORD ret = 0;\n> +\tif (!TerminateThread(async->tid, 0))\n> +\t\tret = error(\"killing thread failed: %lu\", GetLastError());\n\nUgh! Did you read the documentation of TerminateThread()?\n\nWe need to kill processes/threads when we detect that there are too many \nconnections. But TerminateThread() is such a dangerous function that we \ncannot pretend that everything is good, and we continue to accept \nconnections.\n\nUnless we find a different solution, I would prefer to punt and die instead.\n\n> +\telse if (!GetExitCodeThread(async->tid, &ret))\n> +\t\tret = error(\"cannot get thread exit code: %lu\", GetLastError());\n\nWhat should the exit code be good for? The return value of this function can \nonly be -1 (failure, could not kill) or 0 (success, process killed).\n\n-- Hannes\n"},{"id":"128506","messageId":"200911262253.59641.j6t@kdbg.org","threadId":"21755","inReplyTo":"1259196260-3064-8-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-26T21:53:59Z","receivedAt":"2009-11-26T21:53:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> This patch adds the possibility to supply a non-0\n> file descriptor for communucation, instead of the\n> default-created pipe. The pipe gets duplicated, so\n> the caller can free it's handles.\n>\n> This is usefull for async communication over sockets.\n>\n> Signed-off-by: Erik Faye-Lund \n> ---\n> run-command.c | 5 ++++-\n> 1 files changed, 4 insertions(+), 1 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index e5a0e06..98771ef 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -327,7 +327,10 @@ int start_async(struct async *async)\n> {\n> \tint pipe_out[2];\n>\n> -\tif (pipe(pipe_out) < 0)\n> +\tif (async->out) {\n> +\t\tpipe_out[0] = dup(async->out);\n> +\t\tpipe_out[1] = dup(async->out);\n> +\t} else if (pipe(pipe_out) < 0)\n> \t\treturn error(\"cannot create pipe: %s\", strerror(errno));\n> \tasync->out = pipe_out[0];\n\nHm. If async->out != 0:\n\n\tpipe_out[0] = dup(async->out);\n\tasync->out = pipe_out[0];\n\nThis is confusing.\n\nMoreover, you are assigning (a dup of) the same fd to the writable end. This \nassumes a bi-directional channel. I don't yet know what I should think about \nthis (haven't studied the later patches, yet).\n\nIt would be great if you could add a few words to \nDocumentation/technical/api-runcommand.txt.\n\n-- Hannes\n"},{"id":"128510","messageId":"200911262303.57228.j6t@kdbg.org","threadId":"21755","inReplyTo":"1259196260-3064-9-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-26T22:03:57Z","receivedAt":"2009-11-26T22:03:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> This patch adds support to specify an explicit file\n> descriotor for communication with the client, instead\n> of using stdin/stdout.\n>\n> This will be useful for the Windows port, because it\n> will use threads instead of fork() to serve multiple\n> clients, making it impossible to reuse stdin/stdout.\n>\n> Signed-off-by: Erik Faye-Lund \n> ---\n> daemon.c | 34 ++++++++++++++++------------------\n> 1 files changed, 16 insertions(+), 18 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 07d7356..a0aead5 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -263,7 +263,7 @@ static char *path_ok(char *directory)\n> \treturn NULL;\t\t/* Fallthrough. Deny by default */\n> }\n>\n> -typedef int (*daemon_service_fn)(void);\n> +typedef int (*daemon_service_fn)(int);\n> struct daemon_service {\n> \tconst char *name;\n> \tconst char *config_name;\n> @@ -287,7 +287,7 @@ static int git_daemon_config(const char *var, const\n> char *value, void *cb) return 0;\n> }\n>\n> -static int run_service(char *dir, struct daemon_service *service)\n> +static int run_service(int fd, char *dir, struct daemon_service *service)\n> {\n> \tconst char *path;\n> \tint enabled = service->enabled;\n> @@ -340,7 +340,7 @@ static int run_service(char *dir, struct daemon_service\n> *service) */\n> \tsignal(SIGTERM, SIG_IGN);\n>\n> -\treturn service->fn();\n> +\treturn service->fn(fd);\n> }\n>\n> static void copy_to_log(int fd)\n> @@ -364,7 +364,7 @@ static void copy_to_log(int fd)\n> \tfclose(fp);\n> }\n>\n> -static int run_service_command(const char **argv)\n> +static int run_service_command(int fd, const char **argv)\n> {\n> \tstruct child_process cld;\n>\n> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)\n> \tcld.argv = argv;\n> \tcld.git_cmd = 1;\n> \tcld.err = -1;\n> +\tcld.in = cld.out = fd;\n\nYou shouldn't do that. In fact, the next patch 9 has a hunk that correctly \ncalls dup() once.\n\n> -\tclose(0);\n> -\tclose(1);\n\nHere, stdin and stdout were closed and start_command() used both. But these \ntwo new calls\n\n> +\texit(execute(0, addr));\n> ...\n> +\t\treturn execute(0, peer);\n\nare the only places where a value is assigned to fd. Now it is always only \nstdin. Where does the old code initialize stdout? Shouldn't this place need a \nchange, too?\n\n-- Hannes\n"},{"id":"128516","messageId":"40aa078e0911261537r40b19dffqf019848dcad23fef@mail.gmail.com","threadId":"21755","inReplyTo":"7vbpip86q5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-26T23:37:27Z","receivedAt":"2009-11-26T23:37:27Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Nov 26, 2009 at 7:46 PM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>> In practice it seems that something like the following works\n>> portably-enough for many applications, dunno if it's something we'll\n>> be happy with:\n>> #ifndef va_copy\n>> #define va_copy(a,b) ((a) = (b))\n>> #endif\n>\n> Since an obvious implementation of va_list would be to make it a pointer\n> into the stack frame, doing the above would work on many systems.  On\n> esoteric systems that needs something different (e.g. where va_list is\n> implemented as a size-1 array of pointers, va_copy(a,b) needs to be an\n> assignment (*(a) = *(b))), people can add compatibility macro later.\n>\n> Historically some systems that do have a suitable implementation had it\n> under the name __va_copy() instead, so it would have been better to define\n> it as something like:\n>\n>    #ifndef va_copy\n>    # ifdef __va_copy\n>    # define va_copy(a,b) __va_copy(a,b)\n>    # else\n>    # /* fallback for the most obvious implementation of va_list */\n>    # define va_copy(a,b) ((a) = (b))\n>    # endif\n>    #endif\n>\n> But I do not know it still matters in practice anymore.\n\nPerhaps I can do one better: use memcpy instead of standard\nassignment. The Autoconf manual[1] suggests that it's more portable.\nSomething like this:\n\n#ifndef va_copy\n# ifdef __va_copy\n# define va_copy(a,b) __va_copy(a,b)\n# else\n# define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))\n# endif\n#endif\n\nI'll add this to git-compat-util.h this for the next round unless\nsomeone yells really loud at me.\n\n*[1] http://www.gnu.org/software/hello/manual/autoconf/Function-Portability.html#index-g_t_0040code_007bva_005fcopy_007d-357\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128535","messageId":"4B0F7B34.4090908@viscovery.net","threadId":"21755","inReplyTo":"40aa078e0911261537r40b19dffqf019848dcad23fef@mail.gmail.com","subject":"Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-11-27T07:09:40Z","receivedAt":"2009-11-27T07:09:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Erik Faye-Lund schrieb:\n> Perhaps I can do one better: use memcpy instead of standard\n> assignment. The Autoconf manual[1] suggests that it's more portable.\n> Something like this:\n> \n> #ifndef va_copy\n> # ifdef __va_copy\n> # define va_copy(a,b) __va_copy(a,b)\n> # else\n> # define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))\n> # endif\n> #endif\n> \n> I'll add this to git-compat-util.h this for the next round unless\n> someone yells really loud at me.\n\nAs I said elsewhere in the thread, I do not see enough reason to add\nstrbuf_vaddf() only for a syslog() emulation in the first place.\n\n-- Hannes\n"},{"id":"128541","messageId":"40aa078e0911270009u7569cfe5gb250092c8d2c0eac@mail.gmail.com","threadId":"21755","inReplyTo":"200911262223.22777.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-27T08:09:11Z","receivedAt":"2009-11-27T08:09:11Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt wrote:\n> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n>> +     struct strbuf msg;\n>> +     va_list va;\n>> +     WORD logtype;\n>> +\n>> +     strbuf_init(&msg, 0);\n>> +     va_start(va, fmt);\n>> +     strbuf_vaddf(&msg, fmt, va);\n>> +     va_end(va);\n>\n> I would\n>\n>        const char* arg;\n>        if (strcmp(fmt, \"%s\"))\n>                die(\"format string of syslog() not implemented\")\n>        va_start(va, fmt);\n>        arg = va_arg(va, char*);\n>        va_end(va);\n>\n> because we have exactly one invocation of syslog(), which passes \"%s\" as\n> format string. Then strbuf_vaddf is not needed. Or even simpler: declare the\n> function as\n>\n> void syslog(int priority, const char *fmt, const char*arg);\n\nAfter reading this again, I agree that this is the best solution. I'll\nupdate for the next iteration.\n\n>> +     ReportEventA(ms_eventlog,\n>> +         logtype,\n>> +         (WORD)NULL,\n>> +         (DWORD)NULL,\n>> +         NULL,\n>> +         1,\n>> +         0,\n>> +         (const char **)&msg.buf,\n>> +         NULL);\n>\n> Why do you pass NULL and apply casts? The first one gives a warning.\n\nI'll remove the casts for the next iteration.\n\n>\n> Did you read this paragraph in the documentation?\n> http://msdn.microsoft.com/en-us/library/aa363679(VS.85).aspx\n>\n> \"Note that the string that you log cannot contain %n, where n is an integer\n> value (for example, %1) because the event viewer treats it as an insertion\n> string. ...\"\n>\n> How are the chances that this condition applies to our use of the function?\n>\n\nUgh, increasingly high since we're adding IPv6 support, I guess.\nPerhaps some form of escaping needs to be done?\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128563","messageId":"40aa078e0911270623m1a06890cmd2d46b3d9e216769@mail.gmail.com","threadId":"21755","inReplyTo":"200911262303.57228.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-27T14:23:02Z","receivedAt":"2009-11-27T14:23:02Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Sorry for the long delay in the reply, but I'm a little low on time\nthese days (and I've already spent some time trying to figure out what\nI was thinking - I wrote these patches a while ago).\n\nOn Thu, Nov 26, 2009 at 11:03 PM, Johannes Sixt wrote:\n> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n>> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)\n>>       cld.argv = argv;\n>>       cld.git_cmd = 1;\n>>       cld.err = -1;\n>> +     cld.in = cld.out = fd;\n>\n> You shouldn't do that. In fact, the next patch 9 has a hunk that correctly\n> calls dup() once.\n>\n\nOK, as long as it works as expected, sure. But perhaps this needs a\nlittle change (see discussion later)\n\n>> -     close(0);\n>> -     close(1);\n>\n> Here, stdin and stdout were closed and start_command() used both. But these\n> two new calls\n>\n>> +     exit(execute(0, addr));\n>> ...\n>> +             return execute(0, peer);\n>\n> are the only places where a value is assigned to fd. Now it is always only\n> stdin. Where does the old code initialize stdout? Shouldn't this place need a\n> change, too?\n\nThe \"dup2(incoming, 0)\"-call in handle() is AFAICT what makes it work\nto use the forked process' stdin as both stdin and stdout for the\nservice-process pipe (since fd 0 now becomes a pipe that is both\nreadable and writable). This isn't exactly a pretty mechanism, and I\nguess I should rework it. At the very least, I should remove the\n\"dup2(incoming, 1)\"-call, but I'm open to other suggestions. Perhaps I\ncan change this patch to do the entire socket-passing (which is\ncurrently in the next patch)?\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128566","messageId":"40aa078e0911270639n1de36517w5fdf6ef38e931b19@mail.gmail.com","threadId":"21755","inReplyTo":"200911262253.59641.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-27T14:39:39Z","receivedAt":"2009-11-27T14:39:39Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Nov 26, 2009 at 10:53 PM, Johannes Sixt wrote:\n> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n>> @@ -327,7 +327,10 @@ int start_async(struct async *async)\n>>  {\n>>       int pipe_out[2];\n>>\n>> -     if (pipe(pipe_out) < 0)\n>> +     if (async->out) {\n>> +             pipe_out[0] = dup(async->out);\n>> +             pipe_out[1] = dup(async->out);\n>> +     } else if (pipe(pipe_out) < 0)\n>>               return error(\"cannot create pipe: %s\", strerror(errno));\n>>       async->out = pipe_out[0];\n>\n> Hm. If async->out != 0:\n>\n>        pipe_out[0] = dup(async->out);\n>        async->out = pipe_out[0];\n>\n> This is confusing.\n\nWhat do you find confusing about it? The idea is to use a provided\nbi-directional fd instead of a pipe if async->out is non-zero. The\ncurrently defined rules for async is that async->out must be zero\n(since the structure should be zero-initialized).\n\n> Moreover, you are assigning (a dup of) the same fd to the writable end. This\n> assumes a bi-directional channel. I don't yet know what I should think about\n> this (haven't studied the later patches, yet).\n>\n\nIndeed it does. Do we want to extend it to support a set of\nunidirectional channels instead?\n\n> It would be great if you could add a few words to\n> Documentation/technical/api-runcommand.txt.\n>\n\nAh, yes. I know I should update the documentation and all, I'm just\nusually really bad (*cough* lazy *cough*) at documenting stuff. But\nI'll give it a go and if people hate what I write, they can suggest\nchanges.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128574","messageId":"40aa078e0911270746x55946f52qd76dc4f9443aebc6@mail.gmail.com","threadId":"21755","inReplyTo":"40aa078e0911270623m1a06890cmd2d46b3d9e216769@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-27T15:46:49Z","receivedAt":"2009-11-27T15:46:49Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Nov 27, 2009 at 3:23 PM, Erik Faye-Lund\n wrote:\n> Sorry for the long delay in the reply, but I'm a little low on time\n> these days (and I've already spent some time trying to figure out what\n> I was thinking - I wrote these patches a while ago).\n>\n> On Thu, Nov 26, 2009 at 11:03 PM, Johannes Sixt wrote:\n>> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n>>> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)\n>>>       cld.argv = argv;\n>>>       cld.git_cmd = 1;\n>>>       cld.err = -1;\n>>> +     cld.in = cld.out = fd;\n>>\n>> You shouldn't do that. In fact, the next patch 9 has a hunk that correctly\n>> calls dup() once.\n>>\n>\n> OK, as long as it works as expected, sure. But perhaps this needs a\n> little change (see discussion later)\n>\n>>> -     close(0);\n>>> -     close(1);\n>>\n>> Here, stdin and stdout were closed and start_command() used both. But these\n>> two new calls\n>>\n>>> +     exit(execute(0, addr));\n>>> ...\n>>> +             return execute(0, peer);\n>>\n>> are the only places where a value is assigned to fd. Now it is always only\n>> stdin. Where does the old code initialize stdout? Shouldn't this place need a\n>> change, too?\n>\n> The \"dup2(incoming, 0)\"-call in handle() is AFAICT what makes it work\n> to use the forked process' stdin as both stdin and stdout for the\n> service-process pipe (since fd 0 now becomes a pipe that is both\n> readable and writable). This isn't exactly a pretty mechanism, and I\n> guess I should rework it. At the very least, I should remove the\n> \"dup2(incoming, 1)\"-call, but I'm open to other suggestions. Perhaps I\n> can change this patch to do the entire socket-passing (which is\n> currently in the next patch)?\n>\n\nSomething along these lines?\n\n---8<---\ndiff --git a/daemon.c b/daemon.c\nindex a0aead5..8774ed5 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -372,7 +372,8 @@ static int run_service_command(int fd, const char **argv)\n \tcld.argv = argv;\n \tcld.git_cmd = 1;\n \tcld.err = -1;\n-\tcld.in = cld.out = fd;\n+\tcld.in = dup(fd);\n+\tcld.out = fd;\n \tif (start_command(&cld))\n \t\treturn -1;\n\n@@ -707,11 +708,7 @@ static void handle(int incoming, struct sockaddr\n*addr, int addrlen)\n \t\treturn;\n \t}\n\n-\tdup2(incoming, 0);\n-\tdup2(incoming, 1);\n-\tclose(incoming);\n-\n-\texit(execute(0, addr));\n+\texit(execute(incoming, addr));\n }\n\n static void child_handler(int signo)\n---8<---\n\nWhen I think more about it, I might've broken the inetd-mode as it\nshould communicate over stdin and stdout (not just stdin as it would\ntry to do now)... I don't know the inetd internals, but this frightens\nme a bit.\n\nSo perhaps I'll need to provide support for two unidirectional file\ndescriptors after all?\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128575","messageId":"40aa078e0911270804i1a828ea6we1611047d37869f7@mail.gmail.com","threadId":"21755","inReplyTo":"200911262246.13342.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-11-27T16:04:31Z","receivedAt":"2009-11-27T16:04:31Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Nov 26, 2009 at 10:46 PM, Johannes Sixt wrote:\n> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n>> +int kill_async(struct async *async)\n>> +{\n>> +#ifndef WIN32\n>> +     return kill(async->pid, SIGTERM);\n>> +#else\n>> +     DWORD ret = 0;\n>> +     if (!TerminateThread(async->tid, 0))\n>> +             ret = error(\"killing thread failed: %lu\", GetLastError());\n>\n> Ugh! Did you read the documentation of TerminateThread()?\n>\n> We need to kill processes/threads when we detect that there are too many\n> connections. But TerminateThread() is such a dangerous function that we\n> cannot pretend that everything is good, and we continue to accept\n> connections.\n>\n\nOuch, this is nasty. Something else needs to be done.\n\n> Unless we find a different solution, I would prefer to punt and die instead.\n>\n\nDo you really think it's better to unconditionally take down the\nentire process with an error, instead of having a relatively small\nchance of stuff blowing up without any sensible error? I'm not 100%\nconvinced - but let's hope we'll find a proper fix.\n\n>> +     else if (!GetExitCodeThread(async->tid, &ret))\n>> +             ret = error(\"cannot get thread exit code: %lu\", GetLastError());\n>\n> What should the exit code be good for? The return value of this function can\n> only be -1 (failure, could not kill) or 0 (success, process killed).\n>\n\nI thought wait_or_whine() returned the exit-code, so I wanted to be\nsomewhat consistent. But even if it does (haven't checked), it's\nprobably not be worth it - I'll remove this for the next iteration.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"128593","messageId":"200911272023.58262.j6t@kdbg.org","threadId":"21755","inReplyTo":"40aa078e0911270009u7569cfe5gb250092c8d2c0eac@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-27T19:23:58Z","receivedAt":"2009-11-27T19:23:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n> On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt wrote:\n> > I would\n> >\n> >        const char* arg;\n> >        if (strcmp(fmt, \"%s\"))\n> >                die(\"format string of syslog() not implemented\")\n> >        va_start(va, fmt);\n> >        arg = va_arg(va, char*);\n> >        va_end(va);\n> >\n> > because we have exactly one invocation of syslog(), which passes \"%s\" as\n> > format string. Then strbuf_vaddf is not needed. Or even simpler: declare\n> > the function as\n> >\n> > void syslog(int priority, const char *fmt, const char*arg);\n>\n> After reading this again, I agree that this is the best solution. I'll\n> update for the next iteration.\n\nExcept that you shouldn't die like I proposed because here we are already in \nthe die_routine, no?\n\n> > \"Note that the string that you log cannot contain %n, where n is an\n> > integer value (for example, %1) because the event viewer treats it as an\n> > insertion string. ...\"\n> >\n> > How are the chances that this condition applies to our use of the\n> > function?\n>\n> Ugh, increasingly high since we're adding IPv6 support, I guess.\n> Perhaps some form of escaping needs to be done?\n\nI think so, but actually I have no clue.\n\n-- Hannes\n"},{"id":"128596","messageId":"200911272059.25934.j6t@kdbg.org","threadId":"21755","inReplyTo":"40aa078e0911270804i1a828ea6we1611047d37869f7@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-27T19:59:25Z","receivedAt":"2009-11-27T19:59:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n> Do you really think it's better to unconditionally take down the\n> entire process with an error, instead of having a relatively small\n> chance of stuff blowing up without any sensible error? I'm not 100%\n> convinced - but let's hope we'll find a proper fix.\n\n\"relatively small chance of stuff blowing up\"? The docs of \nTerminateThread: \"... the kernel32 state for the thread's process could be \ninconsistent.\" That's scary if we are talking about a process that should run \nfor days or weeks without interruption.\n\nThe reason why we are killing a thread is to prevent keeping lots of \nconnections open (to the same IP address). There are two situations to take \ncare of:\n\n1. We are in a lengthy computation without paying attention to the socket.\n\n2. The client does not send or accept data for a long time.\n\nCase 1 could happen if upload-pack is \"counting objects\" on a large \nrepository. We would need some way to kill upload-pack. Since it is a \nseparate process anyway, we could use TerminateProcess().\n\nCase 2 could be achieved by using setsockopt() with SO_RCVTIMEO and \nSO_SNDTIMEO and a tiny timeout. But notice that we would set a timeout in one \nthread while another thread is waiting in ReadFile() or WriteFile(). Would \nthat work?\n\n-- Hannes\n"},{"id":"128599","messageId":"200911272114.13107.j6t@kdbg.org","threadId":"21755","inReplyTo":"40aa078e0911270639n1de36517w5fdf6ef38e931b19@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-27T20:14:13Z","receivedAt":"2009-11-27T20:14:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n> On Thu, Nov 26, 2009 at 10:53 PM, Johannes Sixt wrote:\n> > On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> >> @@ -327,7 +327,10 @@ int start_async(struct async *async)\n> >>  {\n> >>       int pipe_out[2];\n> >>\n> >> -     if (pipe(pipe_out) < 0)\n> >> +     if (async->out) {\n> >> +             pipe_out[0] = dup(async->out);\n> >> +             pipe_out[1] = dup(async->out);\n> >> +     } else if (pipe(pipe_out) < 0)\n> >>               return error(\"cannot create pipe: %s\", strerror(errno));\n> >>       async->out = pipe_out[0];\n> >\n> > Hm. If async->out != 0:\n> >\n> >        pipe_out[0] = dup(async->out);\n> >        async->out = pipe_out[0];\n> >\n> > This is confusing.\n>\n> What do you find confusing about it? The idea is to use a provided\n> bi-directional fd instead of a pipe if async->out is non-zero. The\n> currently defined rules for async is that async->out must be zero\n> (since the structure should be zero-initialized).\n\nIt is just the code structure that is confusing. It should be\n\n\tif (async->out) {\n\t\t/* fd was provided */\n\t\tdo all that is needed in this case\n\t} else {\n\t\t/* fd was requested */\n\t\tdo all for this other case\n\t}\n\t/* nothing to do anymore here */\n\n(Of course, this should only replace the part that is cited above, not the \nwhole function.)\n\n> > Moreover, you are assigning (a dup of) the same fd to the writable end.\n> > This assumes a bi-directional channel. I don't yet know what I should\n> > think about this (haven't studied the later patches, yet).\n>\n> Indeed it does. Do we want to extend it to support a set of\n> unidirectional channels instead?\n\nYes, I think so. We could pass a regular int fd[2] array around with the clear \ndefinition that both can be closed independently, i.e. one must be a dup() of \nthe other. struct async would also have such an array.\n\nSpeaking of dup(): The underlying function is DuplicateHandle(), and its \ndocumentation says:\n\n\"You should not use DuplicateHandle to duplicate handles to the following \nobjects: ... o Sockets. ... use WSADuplicateSocket.\"\n\nBut then the docs of WSADuplicateSocket() talk only about duplicating a socket \nto a separate process. Perhaps DuplicateHandle() of a socket within the same \nprocess Just Works?\n\n-- Hannes\n"},{"id":"128601","messageId":"200911272123.45163.j6t@kdbg.org","threadId":"21755","inReplyTo":"40aa078e0911270746x55946f52qd76dc4f9443aebc6@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-27T20:23:45Z","receivedAt":"2009-11-27T20:23:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n> On Fri, Nov 27, 2009 at 3:23 PM, Erik Faye-Lund\n> wrote:\n> > At the very least, I should remove the\n> > \"dup2(incoming, 1)\"-call, but I'm open to other suggestions. Perhaps I\n> > can change this patch to do the entire socket-passing (which is\n> > currently in the next patch)?\n\nNo, an infrastructure change in a separate patch is good.\n\n> Something along these lines?\n>\n> ---8<---\n> -\tcld.in = cld.out = fd;\n> +\tcld.in = dup(fd);\n> +\tcld.out = fd;\n>...\n> -\tdup2(incoming, 0);\n> -\tdup2(incoming, 1);\n> -\tclose(incoming);\n> -\n> -\texit(execute(0, addr));\n> +\texit(execute(incoming, addr));\n> ---8<---\n\nYes, this looks very good.\n\n> When I think more about it, I might've broken the inetd-mode as it\n> should communicate over stdin and stdout (not just stdin as it would\n> try to do now)... I don't know the inetd internals, but this frightens\n> me a bit.\n\nDo we need inetd mode on Windows? At one time a looked for a inetd-like \nservice, but couldn't find one.\n\n-- Hannes\n"},{"id":"128602","messageId":"200911272128.48974.j6t@kdbg.org","threadId":"21755","inReplyTo":"200911272123.45163.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-27T20:28:48Z","receivedAt":"2009-11-27T20:28:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 27. November 2009, Johannes Sixt wrote:\n> On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n> > When I think more about it, I might've broken the inetd-mode as it\n> > should communicate over stdin and stdout (not just stdin as it would\n> > try to do now)... I don't know the inetd internals, but this frightens\n> > me a bit.\n>\n> Do we need inetd mode on Windows? At one time a looked for a inetd-like\n> service, but couldn't find one.\n\nHow foolish of me. This affects all platforms. Of course it is an important to \nkeep inetd mode.\n\n-- Hannes\n"},{"id":"128605","messageId":"200911272159.38757.j6t@kdbg.org","threadId":"21755","inReplyTo":"1259196260-3064-10-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-27T20:59:38Z","receivedAt":"2009-11-27T20:59:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> static void check_dead_children(void)\n> {\n> -\tint status;\n> -\tpid_t pid;\n> -\n> -\twhile ((pid = waitpid(-1, &status, WNOHANG)) > 0) {\n> -\t\tconst char *dead = \"\";\n> -\t\tremove_child(pid);\n> -\t\tif (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))\n> -\t\t\tdead = \" (with error)\";\n> -\t\tloginfo(\"[%\"PRIuMAX\"] Disconnected%s\", (uintmax_t)pid, dead);\n> -\t}\n> +\tstruct child **cradle, *blanket;\n> +\tfor (cradle = &firstborn; (blanket = *cradle);)\n> +\t\tif (!is_async_alive(&blanket->async)) {\n\nThis would be the right place to call finish_async(). But since we cannot \nwait, you invented is_async_alive(). But actually we are not only interested \nin whether the process is alive, but also whether it completed successfully \nso that we can add \"(with error)\". Would it make sense to have a function \nfinish_async_nowait() instead of is_async_alive() that (1) stresses the \nstart/finish symmetry and (2) can return more than just Boolean?\n\n> +\t\t\t*cradle = blanket->next;\n> +\t\t\tloginfo(\"Disconnected\\n\");\n\nHere you are losing information about the pid, which is important to have in \nthe syslog. The \\n should be dropped.\n\n> +\tasync.proc = async_execute;\n> +\tasync.data = ss;\n> +\tasync.out = incoming;\n>\n> -\tdup2(incoming, 0);\n> -\tdup2(incoming, 1);\n> +\tif (start_async(&async))\n> +\t\tlogerror(\"unable to fork\");\n> +\telse\n> +\t\tadd_child(&async, addr, addrlen);\n> \tclose(incoming);\n> -\n> -\texit(execute(0, addr));\n\nIn start_command(), the convention is that fds that are provided by the caller \nare closed by start_command() (even if there are errors). The close(incoming) \nthat you leave here indicates that you are not using the same convention with \nstart_async(). It would be nice to switch to the same convention.\n\n-- Hannes\n"},{"id":"128608","messageId":"200911272217.22271.j6t@kdbg.org","threadId":"21755","inReplyTo":"1259196260-3064-12-git-send-email-kusmabite@gmail.com","subject":"Re: [msysGit] [PATCH/RFC 11/11] mingw: compile git-daemon","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-11-27T21:17:22Z","receivedAt":"2009-11-27T21:17:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -352,6 +352,7 @@ EXTRA_PROGRAMS =\n>\n> # ... and all the rest that could be moved out of bindir to gitexecdir\n> PROGRAMS += $(EXTRA_PROGRAMS)\n> +PROGRAMS += git-daemon$X\n> PROGRAMS += git-fast-import$X\n> PROGRAMS += git-hash-object$X\n> PROGRAMS += git-imap-send$X\n> @@ -981,6 +982,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))\n> \tOBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo\n> \tNO_REGEX = YesPlease\n> \tBLK_SHA1 = YesPlease\n> +\tNO_INET_PTON = YesPlease\n> +\tNO_INET_NTOP = YesPlease\n> \tCOMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch\n> \tCOMPAT_CFLAGS += -DSTRIP_EXTENSION=\\\".exe\\\"\n> \t# We have GCC, so let's make use of those nice options\n> @@ -1079,9 +1082,6 @@ ifdef ZLIB_PATH\n> endif\n> EXTLIBS += -lz\n>\n> -ifndef NO_POSIX_ONLY_PROGRAMS\n> -\tPROGRAMS += git-daemon$X\n> -endif\n> ifndef NO_OPENSSL\n> \tOPENSSL_LIBSSL = -lssl\n> \tifdef OPENSSLDIR\n\nYou should remove NO_POSIX_ONLY_PROGRAMS from MSVC and MinGW sections.\n\n-- Hannes\n"},{"id":"129000","messageId":"40aa078e0912020501v9378c37l106e1e23b5e7b43d@mail.gmail.com","threadId":"21755","inReplyTo":"alpine.DEB.2.00.0911261250180.14228@cone.home.martin.st","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-02T13:01:00Z","receivedAt":"2009-12-02T13:01:00Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"(CC'ing Hannes, as he seems to be the original author of our poll()-emulation)\n\nOn Thu, Nov 26, 2009 at 12:03 PM, Martin Storsjö wrote:\n> On Thu, 26 Nov 2009, Erik Faye-Lund wrote:\n>\n>> Yeah, I saw your patches, and realized that I needed to rebase my work\n>> at some point, but none of the repos I usually pull from seems to\n>> contain the patches yet. Rebasing will be a requirement before this\n>> can be applied for sure.\n>\n> Ok, great! I tried applying it on the latest master, and it wasn't too\n> much of work.\n\nI've finally gotten around to doing the same now, thanks for the heads up :)\n\n> This causes problems with the mingw poll() replacement, which has a\n> special case for polling one single fd - otherwise it tries to use some\n> emulation that currently only works for pipes. I didn't try to make any\n> proper fix for this though. I tested git-daemon by hacking it to listen on\n> only one of the sockets, and that worked well for both v4 and v6.\n>\n>\n> So, in addition to the getaddrinfo patch I sent, the mingw poll()\n> replacement needs some updates to handle polling multiple sockets. Except\n> from that, things seem to work, at a quick glance.\n>\n\nThanks a lot for helping out! I'm seeing the same issue here when\nrunning on Vista:\n$ ./git-daemon.exe --base-path=/c/Users/Erik/src/ --export-all\nerror: PeekNamedPipe failed, GetLastError: 1\n[3656] Poll failed, resuming: Invalid argument\nerror: PeekNamedPipe failed, GetLastError: 1\n[3656] Poll failed, resuming: Invalid argument\n...\n\nIf I hack the poll()-emulation to never enter the PeekNamedPipe\ncode-path seems to make it work with IPv6 here, but this is obviously\nnot the correct solution as it breaks pipe-polling:\n--->8---\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -360,7 +360,7 @@ int poll(struct pollfd *ufds, unsigned int nfds, int timeout\n * input is available and let the actual wait happen when the\n * caller invokes read().\n */\n- if (nfds == 1) {\n+ if (1) {\n if (!(ufds[0].events & POLLIN))\n return errno = EINVAL, error(\"POLLIN not set\");\n ufds[0].revents = POLLIN;\n--->8---\n\nI'm not very familiar with poll(), but if I understand the man-pages\ncorrectly it's waiting for events on file descriptors, and is in our\ncase used to check for incoming connections, right? If so, I see three\npossible ways forward: (1) extending our poll()-emulation to handle\nmultiple sockets, (2) change daemon.c to check one socket at the time,\nand (3) using select() instead of poll().\n\n(1) seems like the \"correct\" but tricky thing to do, (2) like the\n\"easy\" but nasty thing to do. However, (3) strikes me as the least\ndangerous thing to do ;)\n\nFor (1), there's also a WSAPoll() function in Windows, but I'm not\nsure how to figure out if an fd is a socket or a pipe. There's also\nWaitForMultipleObjects.\n\nHere's a quick stab at (3). It seems to work fine under Windows. Not\ntested on other platforms, though.\n\n--->8---\n--- a/daemon.c\n+++ b/daemon.c\n@@ -812,26 +812,22 @@ static int socksetup(char *listen_addr, int listen_port, i\nnt **socklist_p)\n\n static int service_loop(int socknum, int *socklist)\n {\n- struct pollfd *pfd;\n- int i;\n-\n- pfd = xcalloc(socknum, sizeof(struct pollfd));\n-\n- for (i = 0; i < socknum; i++) {\n- pfd[i].fd = socklist[i];\n- pfd[i].events = POLLIN;\n- }\n-\n signal(SIGCHLD, child_handler);\n\n for (;;) {\n int i;\n+ fd_set fds;\n+ struct timeval timeout = { 0, 0 };\n\n check_dead_children();\n\n- if (poll(pfd, socknum, -1) < 0) {\n+ FD_ZERO(&fds);\n+ for (i = 0; i < socknum; i++)\n+ FD_SET(socklist[i], &fds);\n+\n+ if (select(0, &fds, NULL, NULL, &timeout) > 0) {\n if (errno != EINTR) {\n- logerror(\"Poll failed, resuming: %s\",\n+ logerror(\"select() failed, resuming: %s\",\n strerror(errno));\n sleep(1);\n }\n@@ -839,10 +835,10 @@ static int service_loop(int socknum, int *socklist)\n }\n\n for (i = 0; i < socknum; i++) {\n- if (pfd[i].revents & POLLIN) {\n+ if (FD_ISSET(socklist[i], &fds)) {\n struct sockaddr_storage ss;\n socklen_t sslen = sizeof(ss);\n- int incoming = accept(pfd[i].fd,\n(struct sockaddr *)&ss, &sslen);\n+ int incoming = accept(socklist[i],\n(struct sockaddr *)&ss, &sslen);\n if (incoming < 0) {\n switch (errno) {\n case EAGAIN:\n@@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)\n }\n }\n handle(incoming, (struct sockaddr *)&ss, sslen);\n\n+ break;\n }\n }\n }\n--->8---\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129001","messageId":"alpine.DEB.2.00.0912021511310.5582@cone.home.martin.st","threadId":"21755","inReplyTo":"40aa078e0912020501v9378c37l106e1e23b5e7b43d@mail.gmail.com","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Martin Storsjö","fromEmail":"martin@martin.st","sentAt":"2009-12-02T13:21:59Z","receivedAt":"2009-12-02T13:21:59Z","isPatch":true,"sender":{"key":"martin@martin.st","avatar":"https://avatars.githubusercontent.com/u/69727?v=4"},"body":"On Wed, 2 Dec 2009, Erik Faye-Lund wrote:\n\n> I'm not very familiar with poll(), but if I understand the man-pages\n> correctly it's waiting for events on file descriptors, and is in our\n> case used to check for incoming connections, right? If so, I see three\n> possible ways forward: (1) extending our poll()-emulation to handle\n> multiple sockets, (2) change daemon.c to check one socket at the time,\n> and (3) using select() instead of poll().\n> \n> (1) seems like the \"correct\" but tricky thing to do, (2) like the\n> \"easy\" but nasty thing to do. However, (3) strikes me as the least\n> dangerous thing to do ;)\n\nGenerally, poll is a better API than select, since select is limited to fd \nvalues up to (about) 1000, depending on the implementation. (This is due \nto the fact that fd_set is a fixed size bit set with a bit for each \npossible fd.)\n\nBut since we're only doing select on the handful of sockets that were \nopened initially in the process (so these should receive low numbers), \nthis should only be a theoretical concern.\n\n> For (1), there's also a WSAPoll() function in Windows, but I'm not\n> sure how to figure out if an fd is a socket or a pipe. There's also\n> WaitForMultipleObjects.\n> \n> Here's a quick stab at (3). It seems to work fine under Windows. Not\n> tested on other platforms, though.\n\nA few comments below, just by reading through this, didn't test it yet:\n\n> --->8---\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -812,26 +812,22 @@ static int socksetup(char *listen_addr, int listen_port, i\n> nt **socklist_p)\n> \n> static int service_loop(int socknum, int *socklist)\n> {\n> - struct pollfd *pfd;\n> - int i;\n> -\n> - pfd = xcalloc(socknum, sizeof(struct pollfd));\n> -\n> - for (i = 0; i < socknum; i++) {\n> - pfd[i].fd = socklist[i];\n> - pfd[i].events = POLLIN;\n> - }\n> -\n> signal(SIGCHLD, child_handler);\n> \n> for (;;) {\n> int i;\n> + fd_set fds;\n> + struct timeval timeout = { 0, 0 };\n> \n> check_dead_children();\n> \n> - if (poll(pfd, socknum, -1) < 0) {\n> + FD_ZERO(&fds);\n> + for (i = 0; i < socknum; i++)\n> + FD_SET(socklist[i], &fds);\n> +\n> + if (select(0, &fds, NULL, NULL, &timeout) > 0) {\n> if (errno != EINTR) {\n\nThe first parameter to select should be max(all fds set in the fd_set) + \n1, this should be trivial enough to determine in the loop above where the \nfd:s are added with FD_SET.\n\nYou're calling select() with a zero timeout - I'd guess this chews quite a \nbit of cpu just looping around doing nothing? If the last parameter would \nbe set to NULL, it would wait indefinitely, just like the previous poll \nloop did.\n\n> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)\n> }\n> }\n> handle(incoming, (struct sockaddr *)&ss, sslen);\n> \n> + break;\n\nWhat's this good for?\n\nOther than that, this looks quite good to me, at a first glance.\n\n// Martin\n"},{"id":"129002","messageId":"40aa078e0912020549s792eb3ffi93b2cfdc6b7219dd@mail.gmail.com","threadId":"21755","inReplyTo":"alpine.DEB.2.00.0912021511310.5582@cone.home.martin.st","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-02T13:49:19Z","receivedAt":"2009-12-02T13:49:19Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 2, 2009 at 2:21 PM, Martin Storsjö wrote:\n> On Wed, 2 Dec 2009, Erik Faye-Lund wrote:\n>\n>> I'm not very familiar with poll(), but if I understand the man-pages\n>> correctly it's waiting for events on file descriptors, and is in our\n>> case used to check for incoming connections, right? If so, I see three\n>> possible ways forward: (1) extending our poll()-emulation to handle\n>> multiple sockets, (2) change daemon.c to check one socket at the time,\n>> and (3) using select() instead of poll().\n>>\n>> (1) seems like the \"correct\" but tricky thing to do, (2) like the\n>> \"easy\" but nasty thing to do. However, (3) strikes me as the least\n>> dangerous thing to do ;)\n>\n> Generally, poll is a better API than select, since select is limited to fd\n> values up to (about) 1000, depending on the implementation. (This is due\n> to the fact that fd_set is a fixed size bit set with a bit for each\n> possible fd.)\n>\n> But since we're only doing select on the handful of sockets that were\n> opened initially in the process (so these should receive low numbers),\n> this should only be a theoretical concern.\n>\n\nYeah. In our case, I guess it's a maximum of two times the number of\nnetwork adapters installed, so I think we should be relatively safe.\n\n>\n>> --->8---\n>> --- a/daemon.c\n>> +++ b/daemon.c\n>> @@ -812,26 +812,22 @@ static int socksetup(char *listen_addr, int listen_port, i\n>> nt **socklist_p)\n>>\n>>  static int service_loop(int socknum, int *socklist)\n>>  {\n>> -       struct pollfd *pfd;\n>> -       int i;\n>> -\n>> -       pfd = xcalloc(socknum, sizeof(struct pollfd));\n>> -\n>> -       for (i = 0; i < socknum; i++) {\n>> -               pfd[i].fd = socklist[i];\n>> -               pfd[i].events = POLLIN;\n>> -       }\n>> -\n>>         signal(SIGCHLD, child_handler);\n>>\n>>         for (;;) {\n>>                 int i;\n>> +               fd_set fds;\n>> +               struct timeval timeout = { 0, 0 };\n>>\n>>                 check_dead_children();\n>>\n>> -               if (poll(pfd, socknum, -1) < 0) {\n>> +               FD_ZERO(&fds);\n>> +               for (i = 0; i < socknum; i++)\n>> +                       FD_SET(socklist[i], &fds);\n>> +\n>> +               if (select(0, &fds, NULL, NULL, &timeout) > 0) {\n>>                         if (errno != EINTR) {\n>\n> The first parameter to select should be max(all fds set in the fd_set) +\n> 1, this should be trivial enough to determine in the loop above where the\n> fd:s are added with FD_SET.\n>\n\nYeah, thanks for pointing out. I did a little bit of searching, and it\nseems like \"most other people passes zero, and it just works\".\nHowever, we should be nice to systems where it doesn't \"just work\", so\nI'll fix this before sending it out.\n\n> You're calling select() with a zero timeout - I'd guess this chews quite a\n> bit of cpu just looping around doing nothing? If the last parameter would\n> be set to NULL, it would wait indefinitely, just like the previous poll\n> loop did.\n>\n\nAh, yes. That's much nicer!\n\n>> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)\n>>                                         }\n>>                                 }\n>>                                 handle(incoming, (struct sockaddr *)&ss, sslen);\n>>\n>> +                               break;\n>\n> What's this good for?\n>\n\nWhen I clone git://localhost/some-repo, select() returns a fd-set with\nboth the IPv4 and IPv6 fds. After accept()'ing the first one, the\nsecond call to accept() hangs. I solved this by accepting only the\nfirst connection I got; the second one should be accepted in the next\nround of the service loop (if still available).\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129005","messageId":"40aa078e0912020711v6dcebba1g511e6cacbbed16be@mail.gmail.com","threadId":"21755","inReplyTo":"40aa078e0912020549s792eb3ffi93b2cfdc6b7219dd@mail.gmail.com","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-02T15:11:23Z","receivedAt":"2009-12-02T15:11:23Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 2, 2009 at 2:49 PM, Erik Faye-Lund wrote:\n> On Wed, Dec 2, 2009 at 2:21 PM, Martin Storsjö wrote:\n>> On Wed, 2 Dec 2009, Erik Faye-Lund wrote:\n>>> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)\n>>>                                         }\n>>>                                 }\n>>>                                 handle(incoming, (struct sockaddr *)&ss, sslen);\n>>>\n>>> +                               break;\n>>\n>> What's this good for?\n>>\n>\n> When I clone git://localhost/some-repo, select() returns a fd-set with\n> both the IPv4 and IPv6 fds. After accept()'ing the first one, the\n> second call to accept() hangs. I solved this by accepting only the\n> first connection I got; the second one should be accepted in the next\n> round of the service loop (if still available).\n>\n\nActually, it's no good - my code is broken. FD_SET() and FD_ISSET()\nneeds wrapping to call _get_osfhandle() on the socket. Since this is\nnot needed on Linux, the code ran just fine there. But on Windows,\nselect() failed, but due to the following bug, it wasn't picked up:\n+ if (select(0, &fds, NULL, NULL, &timeout) > 0) {\n\nchanging the comparison around, revealed that select hadn't done\nanything so fds wasn't modified at all! Thus, I wrongly interpreted it\nas if both sockets had an awaiting connection, making IPv6 connections\nwork (since they are on the first socket), but IPv4 not.\n\nI've corrected this locally, and I'll include the fixed patch in the next round.\n\nThanks for pointing out the issue, causing me to second guess my \"cure\" :)\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129007","messageId":"40aa078e0912020745o4b72342fm722a944621cfda5@mail.gmail.com","threadId":"21755","inReplyTo":"200911272159.38757.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-02T15:45:03Z","receivedAt":"2009-12-02T15:45:03Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Nov 27, 2009 at 9:59 PM, Johannes Sixt wrote:\n> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:\n>>  static void check_dead_children(void)\n>>  {\n>> -     int status;\n>> -     pid_t pid;\n>> -\n>> -     while ((pid = waitpid(-1, &status, WNOHANG)) > 0) {\n>> -             const char *dead = \"\";\n>> -             remove_child(pid);\n>> -             if (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))\n>> -                     dead = \" (with error)\";\n>> -             loginfo(\"[%\"PRIuMAX\"] Disconnected%s\", (uintmax_t)pid, dead);\n>> -     }\n>> +     struct child **cradle, *blanket;\n>> +     for (cradle = &firstborn; (blanket = *cradle);)\n>> +             if (!is_async_alive(&blanket->async)) {\n>\n> This would be the right place to call finish_async(). But since we cannot\n> wait, you invented is_async_alive(). But actually we are not only interested\n> in whether the process is alive, but also whether it completed successfully\n> so that we can add \"(with error)\". Would it make sense to have a function\n> finish_async_nowait() instead of is_async_alive() that (1) stresses the\n> start/finish symmetry and (2) can return more than just Boolean?\n>\n\nYes, it does.\n\n>> +                     *cradle = blanket->next;\n>> +                     loginfo(\"Disconnected\\n\");\n>\n> Here you are losing information about the pid, which is important to have in\n> the syslog. The \\n should be dropped.\n>\n\nYeah... I removed the pid mostly because after moving to async, there\nwasn't \"just a pid\" any more. But if we make finish_async_nowait()\nreturn whatever we need to report, I guess we can add the information\nback somehow.\n\nI'm not entirely sure how to make the interface, though. Any good suggestions?\n\n>> +     async.proc = async_execute;\n>> +     async.data = ss;\n>> +     async.out = incoming;\n>>\n>> -     dup2(incoming, 0);\n>> -     dup2(incoming, 1);\n>> +     if (start_async(&async))\n>> +             logerror(\"unable to fork\");\n>> +     else\n>> +             add_child(&async, addr, addrlen);\n>>       close(incoming);\n>> -\n>> -     exit(execute(0, addr));\n>\n> In start_command(), the convention is that fds that are provided by the caller\n> are closed by start_command() (even if there are errors). The close(incoming)\n> that you leave here indicates that you are not using the same convention with\n> start_async(). It would be nice to switch to the same convention.\n>\n\nYeah, I've fixed this for the next round.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129009","messageId":"40aa078e0912020757i3b63ef6eh71c3d4d99047f1f2@mail.gmail.com","threadId":"21755","inReplyTo":"200911272059.25934.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-02T15:57:01Z","receivedAt":"2009-12-02T15:57:01Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt wrote:\n> On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n>> Do you really think it's better to unconditionally take down the\n>> entire process with an error, instead of having a relatively small\n>> chance of stuff blowing up without any sensible error? I'm not 100%\n>> convinced - but let's hope we'll find a proper fix.\n>\n> \"relatively small chance of stuff blowing up\"? The docs of\n> TerminateThread: \"... the kernel32 state for the thread's process could be\n> inconsistent.\" That's scary if we are talking about a process that should run\n> for days or weeks without interruption.\n>\n\nI think there's a misunderstanding here. I thought your suggestion was\nto simply call die(), which would take down the main process. After\nreading this explanation, I think you're talking about giving an error\nand rejecting the connection instead. Which makes more sense than to\nrisk crashing the main-process, indeed.\n\n> The reason why we are killing a thread is to prevent keeping lots of\n> connections open (to the same IP address). There are two situations to take\n> care of:\n>\n> 1. We are in a lengthy computation without paying attention to the socket.\n>\n> 2. The client does not send or accept data for a long time.\n>\n> Case 1 could happen if upload-pack is \"counting objects\" on a large\n> repository. We would need some way to kill upload-pack. Since it is a\n> separate process anyway, we could use TerminateProcess().\n>\n\nMakes sense. I'll play around a bit with this and see what I come up with.\n\n> Case 2 could be achieved by using setsockopt() with SO_RCVTIMEO and\n> SO_SNDTIMEO and a tiny timeout. But notice that we would set a timeout in one\n> thread while another thread is waiting in ReadFile() or WriteFile(). Would\n> that work?\n>\n\nI think it should work fine, but I won't give you a guarantee ;)\nPerhaps we should have a configurable global max timeout, and just set\nthat on all sockets? Or does this open for DDOS attacks?\n\nAnyway, thanks for the sanity :)\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129042","messageId":"200912022012.08905.j6t@kdbg.org","threadId":"21755","inReplyTo":"40aa078e0912020745o4b72342fm722a944621cfda5@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-02T19:12:08Z","receivedAt":"2009-12-02T19:12:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:\n> On Fri, Nov 27, 2009 at 9:59 PM, Johannes Sixt wrote:\n> > Would it make sense to\n> > have a function finish_async_nowait() instead of is_async_alive() that\n> > (1) stresses the start/finish symmetry and (2) can return more than just\n> > Boolean?\n>...\n>\n> I'm not entirely sure how to make the interface, though. Any good\n> suggestions?\n\nI suggest to model finish_async_nowait() after waitpid() so that\n\n\twhile ((pid = waitpid(-1, &status, WNOHANG)) > 0) ...\nbecomes\n\twhile ((pid = finish_async_nowait(&some_async, &status)) > 0) ...\n\nbut where the resulting status is already \"decoded\", i.e. zero is success and \nnon-zero is failure (including death through signal); WIFEXITED and \nWEXITSTATUS should not be applicable to status anymore.\n\n-- Hannes\n"},{"id":"129047","messageId":"200912022027.23344.j6t@kdbg.org","threadId":"21755","inReplyTo":"40aa078e0912020757i3b63ef6eh71c3d4d99047f1f2@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-02T19:27:23Z","receivedAt":"2009-12-02T19:27:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:\n> On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt wrote:\n> > \"relatively small chance of stuff blowing up\"? The docs of\n> > TerminateThread: \"... the kernel32 state for the thread's process could\n> > be inconsistent.\" That's scary if we are talking about a process that\n> > should run for days or weeks without interruption.\n>\n> I think there's a misunderstanding here. I thought your suggestion was\n> to simply call die(), which would take down the main process. After\n> reading this explanation, I think you're talking about giving an error\n> and rejecting the connection instead. Which makes more sense than to\n> risk crashing the main-process, indeed.\n\nJust rejecting a connection is certainly the simplest do to keep the daemon \nprocess alive. But the server can be DoS-ed from a single source IP.\n\nCurrently git-daemon can only be DDoS-ed because there is a maximum number of \nconnections, which are not closed if all of them originate from different \nIPs.\n\n> > Case 2 could be achieved by using setsockopt() with SO_RCVTIMEO and\n> > SO_SNDTIMEO and a tiny timeout. But notice that we would set a timeout in\n> > one thread while another thread is waiting in ReadFile() or WriteFile().\n> > Would that work?\n>\n> I think it should work fine, but I won't give you a guarantee ;)\n> Perhaps we should have a configurable global max timeout, and just set\n> that on all sockets? Or does this open for DDOS attacks?\n\nI'm sure that there is a global timeout already, but it is in the order of \nminutes, which is too long. Here I mean it to be set to zero or one \nmilli-second so that the connection closes right away - as if the process on \nthe server side had been killed.\n\nHm - perhaps it is possible to really *close* the socket while some other \nthread waits in ReadFile() or WriteFile()?\n\n-- Hannes\n"},{"id":"129048","messageId":"200912022034.09266.j6t@kdbg.org","threadId":"21755","inReplyTo":"40aa078e0912020501v9378c37l106e1e23b5e7b43d@mail.gmail.com","subject":"Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-02T19:34:09Z","receivedAt":"2009-12-02T19:34:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:\n> I'm not very familiar with poll(), but if I understand the man-pages\n> correctly it's waiting for events on file descriptors, and is in our\n> case used to check for incoming connections, right? If so, I see three\n> possible ways forward: (1) extending our poll()-emulation to handle\n> multiple sockets, (2) change daemon.c to check one socket at the time,\n> and (3) using select() instead of poll().\n>\n> (1) seems like the \"correct\" but tricky thing to do, (2) like the\n> \"easy\" but nasty thing to do. However, (3) strikes me as the least\n> dangerous thing to do ;)\n>\n> For (1), there's also a WSAPoll() function in Windows, but I'm not\n> sure how to figure out if an fd is a socket or a pipe. There's also\n> WaitForMultipleObjects.\n\nGetFileType() returns FILE_TYPE_PIPE for both pipes and sockets. But once you \nknow this, you can use getsockopt(): If it succeeds, it is a socket, and in \nthis case, assume that poll() was called from git-daemon, i.e. all polled-for \nfds are sockets and you can select().\n\n-- Hannes\n"},{"id":"129532","messageId":"40aa078e0912080536o21101f18p15c6c708b1cb6c14@mail.gmail.com","threadId":"21755","inReplyTo":"200912022012.08905.j6t@kdbg.org","subject":"Re: [PATCH/RFC 09/11] daemon: use run-command api for async serving","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-08T13:36:49Z","receivedAt":"2009-12-08T13:36:49Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 2, 2009 at 8:12 PM, Johannes Sixt wrote:\n> On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:\n>> I'm not entirely sure how to make the interface, though. Any good\n>> suggestions?\n>\n> I suggest to model finish_async_nowait() after waitpid() so that\n>\n>        while ((pid = waitpid(-1, &status, WNOHANG)) > 0) ...\n> becomes\n>        while ((pid = finish_async_nowait(&some_async, &status)) > 0) ...\n>\n> but where the resulting status is already \"decoded\", i.e. zero is success and\n> non-zero is failure (including death through signal); WIFEXITED and\n> WEXITSTATUS should not be applicable to status anymore.\n>\n> -- Hannes\n>\n\nThanks. Implemented as suggested for the next round.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129533","messageId":"40aa078e0912080538p16a99a8br647dbd42e18e1cde@mail.gmail.com","threadId":"21755","inReplyTo":"200911272128.48974.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-08T13:38:56Z","receivedAt":"2009-12-08T13:38:56Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Nov 27, 2009 at 9:28 PM, Johannes Sixt wrote:\n> On Freitag, 27. November 2009, Johannes Sixt wrote:\n>> On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n>> > When I think more about it, I might've broken the inetd-mode as it\n>> > should communicate over stdin and stdout (not just stdin as it would\n>> > try to do now)... I don't know the inetd internals, but this frightens\n>> > me a bit.\n>>\n>> Do we need inetd mode on Windows? At one time a looked for a inetd-like\n>> service, but couldn't find one.\n>\n> How foolish of me. This affects all platforms. Of course it is an important to\n> keep inetd mode.\n>\n> -- Hannes\n>\n\nI believe I've fixed this for the next round, but I haven't been able\nto test the inetd-stuff, since I don't have a working\ntest-environment.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129535","messageId":"40aa078e0912080546v544451c6yd3a3b15cb05a08ed@mail.gmail.com","threadId":"21755","inReplyTo":"200911272114.13107.j6t@kdbg.org","subject":"Re: [PATCH/RFC 07/11] run-command: support input-fd","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-08T13:46:19Z","receivedAt":"2009-12-08T13:46:19Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Nov 27, 2009 at 9:14 PM, Johannes Sixt wrote:\n> On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n>> What do you find confusing about it? The idea is to use a provided\n>> bi-directional fd instead of a pipe if async->out is non-zero. The\n>> currently defined rules for async is that async->out must be zero\n>> (since the structure should be zero-initialized).\n>\n> It is just the code structure that is confusing. It should be\n>\n>        if (async->out) {\n>                /* fd was provided */\n>                do all that is needed in this case\n>        } else {\n>                /* fd was requested */\n>                do all for this other case\n>        }\n>        /* nothing to do anymore here */\n>\n> (Of course, this should only replace the part that is cited above, not the\n> whole function.)\n>\n\nOK. I've reimplemented the change for the next round, taking this into account.\n\n>> Indeed it does. Do we want to extend it to support a set of\n>> unidirectional channels instead?\n>\n> Yes, I think so. We could pass a regular int fd[2] array around with the clear\n> definition that both can be closed independently, i.e. one must be a dup() of\n> the other. struct async would also have such an array.\n>\n\nOK. This has been included for the next round. Instead of an array,\nI've tried to be consistent with start_command, and used two\nvariables, \"in\" and \"out\".\n\n> Speaking of dup(): The underlying function is DuplicateHandle(), and its\n> documentation says:\n>\n> \"You should not use DuplicateHandle to duplicate handles to the following\n> objects: ... o Sockets. ... use WSADuplicateSocket.\"\n>\n> But then the docs of WSADuplicateSocket() talk only about duplicating a socket\n> to a separate process. Perhaps DuplicateHandle() of a socket within the same\n> process Just Works?\n>\n\nIt seems the rest of the Windows-world depends on DuplicateHandle()\nworking for sockets, so I'm not too worried. I can't find anything\ndocumentation(1) for _dup, and I don't think we have our own\ndup()-implementation.\n\n(1) http://msdn.microsoft.com/en-us/library/8syseb29(VS.71).aspx\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"129538","messageId":"40aa078e0912080601p3d73f8b4k469ba5229c931575@mail.gmail.com","threadId":"21755","inReplyTo":"200911272023.58262.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-08T14:01:16Z","receivedAt":"2009-12-08T14:01:16Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Nov 27, 2009 at 8:23 PM, Johannes Sixt wrote:\n> On Freitag, 27. November 2009, Erik Faye-Lund wrote:\n>> On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt wrote:\n>> > I would\n>> >\n>> >        const char* arg;\n>> >        if (strcmp(fmt, \"%s\"))\n>> >                die(\"format string of syslog() not implemented\")\n>> >        va_start(va, fmt);\n>> >        arg = va_arg(va, char*);\n>> >        va_end(va);\n>> >\n>> > because we have exactly one invocation of syslog(), which passes \"%s\" as\n>> > format string. Then strbuf_vaddf is not needed. Or even simpler: declare\n>> > the function as\n>> >\n>> > void syslog(int priority, const char *fmt, const char*arg);\n>>\n>> After reading this again, I agree that this is the best solution. I'll\n>> update for the next iteration.\n>\n> Except that you shouldn't die like I proposed because here we are already in\n> the die_routine, no?\n>\n\nMight be. Either way, I think loosing a log-entry is better than\ntaking down the server. I'm just doing a warning() and return. If\nwe're lucky, the warning gets somewhere. If not, well, something\nhappened and no one was around to see it.\n\n>> > \"Note that the string that you log cannot contain %n, where n is an\n>> > integer value (for example, %1) because the event viewer treats it as an\n>> > insertion string. ...\"\n>> >\n>> > How are the chances that this condition applies to our use of the\n>> > function?\n>>\n>> Ugh, increasingly high since we're adding IPv6 support, I guess.\n>> Perhaps some form of escaping needs to be done?\n>\n> I think so, but actually I have no clue.\n>\n\nBah, according to Microsoft Support (1), there's no simple way to\nescape this. I'm tempted to leave this bug in, and rather try to fix\nthe symptoms when/if they start popping up. Unless someone else comes\nup with something better, that is.\n\n(1) http://support.microsoft.com/kb/934640\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"131153","messageId":"40aa078e1001081649h5cb767d5t880110d923418300@mail.gmail.com","threadId":"21755","inReplyTo":"200912022027.23344.j6t@kdbg.org","subject":"Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-01-09T00:49:47Z","receivedAt":"2010-01-09T00:49:47Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 2, 2009 at 8:27 PM, Johannes Sixt wrote:\n> On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:\n>> On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt wrote:\n>> > \"relatively small chance of stuff blowing up\"? The docs of\n>> > TerminateThread: \"... the kernel32 state for the thread's process could\n>> > be inconsistent.\" That's scary if we are talking about a process that\n>> > should run for days or weeks without interruption.\n>>\n>> I think there's a misunderstanding here. I thought your suggestion was\n>> to simply call die(), which would take down the main process. After\n>> reading this explanation, I think you're talking about giving an error\n>> and rejecting the connection instead. Which makes more sense than to\n>> risk crashing the main-process, indeed.\n>\n> Just rejecting a connection is certainly the simplest do to keep the daemon\n> process alive. But the server can be DoS-ed from a single source IP.\n>\n> Currently git-daemon can only be DDoS-ed because there is a maximum number of\n> connections, which are not closed if all of them originate from different\n> IPs.\n>\n\nAfter some testing I've found that git-daemon can very much be DoS-ed\nfrom a single IP in it's current form. This is for two reasons:\n1) The clever xcalloc + memcmp trick has a fault; the port for each\nconnection is different, so there will never be a match. I have a\npatch[1] for this that I plan to send out soon.\n2) Even with this patch the effect of the DoS-protection is kind of\nlimited. This is because it's a child process of the fork()'d process\nagain that does all the heavy lifting, and kill(pid, SIGHUP) doesn't\nkill child processes. So, the connection gets to continue the action\nuntil upload-pack (or whatever the current command is) finish. This\nmight be quite lengthy.\n\nAs I said, I have a patch for 1), but I don't quite know how to fix\n2). Perhaps this is a good use for process groups? I'm a Windows-guy;\nmy POSIX isn't exactly super-awesome...\n\nI found these issues during my latest effort to port git-daemon to\nWindows. I managed to get this to work fine on Windows, by\nimplementing a kill(x, SIGTERM) that terminated child-processes\n(because I was under the impression that this was what happened... I\nguess daemon.c lead me to believe that).\n\n[1]: http://repo.or.cz/w/git/kusma.git/commit/b1d286d32f42c57b90a1db9b7b8d6775a5d1ad7b\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"131233","messageId":"40aa078e1001100906v2ea6fb7eo93b6fd63a167ef19@mail.gmail.com","threadId":"21755","inReplyTo":"40aa078e1001081649h5cb767d5t880110d923418300@mail.gmail.com","subject":"Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-01-10T17:06:52Z","receivedAt":"2010-01-10T17:06:52Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Sat, Jan 9, 2010 at 1:49 AM, Erik Faye-Lund wrote:\n> On Wed, Dec 2, 2009 at 8:27 PM, Johannes Sixt wrote:\n>> On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:\n>>> On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt wrote:\n>>> > \"relatively small chance of stuff blowing up\"? The docs of\n>>> > TerminateThread: \"... the kernel32 state for the thread's process could\n>>> > be inconsistent.\" That's scary if we are talking about a process that\n>>> > should run for days or weeks without interruption.\n>>>\n>>> I think there's a misunderstanding here. I thought your suggestion was\n>>> to simply call die(), which would take down the main process. After\n>>> reading this explanation, I think you're talking about giving an error\n>>> and rejecting the connection instead. Which makes more sense than to\n>>> risk crashing the main-process, indeed.\n>>\n>> Just rejecting a connection is certainly the simplest do to keep the daemon\n>> process alive. But the server can be DoS-ed from a single source IP.\n>>\n>> Currently git-daemon can only be DDoS-ed because there is a maximum number of\n>> connections, which are not closed if all of them originate from different\n>> IPs.\n>>\n>\n> After some testing I've found that git-daemon can very much be DoS-ed\n> from a single IP in it's current form. This is for two reasons:\n> 1) The clever xcalloc + memcmp trick has a fault; the port for each\n> connection is different, so there will never be a match. I have a\n> patch[1] for this that I plan to send out soon.\n> 2) Even with this patch the effect of the DoS-protection is kind of\n> limited. This is because it's a child process of the fork()'d process\n> again that does all the heavy lifting, and kill(pid, SIGHUP) doesn't\n> kill child processes. So, the connection gets to continue the action\n> until upload-pack (or whatever the current command is) finish. This\n> might be quite lengthy.\n>\n\nActually, this isn't the end of the story, there's an additional issue\nthat happens if 1) doesn't:\nIn commit 02d57da (Be slightly smarter about git-daemon client\nshutdown), Linus adds the following hunk:\n\n@@ -26,6 +26,12 @@ static int upload(char *dir, int dirlen)\n access(\"HEAD\", R_OK))\n return -1;\n\n+ /*\n+ * We'll ignore SIGTERM from now on, we have a\n+ * good client.\n+ */\n+ signal(SIGTERM, SIG_IGN);\n+\n /* git-upload-pack only ever reads stuff, so this is safe */\n execlp(\"git-upload-pack\", \"git-upload-pack\", \".\", NULL);\n return -1;\n\nThis is fine, because he also makes sure to first try to kill with\nSIGTERM, and then fall back to SIGKILL if that failed. However,\nStephen later adds commit 3bd62c2 (\"git-daemon: rewrite kindergarden,\nnew option --max-connections\"), and here he leaves the hunk quoted\nabove, but removes the SIGKILL code-path. The consequence is that the\nforked process doesn't die at all any more.\n\nIMO, Stephen did kind-of right in removing the SIGKILL code-path,\nsince we don't kill just a random child anymore. But leaving the\nSIGTERM-ignoring on looks like a bug to me.\n\nNow, if that was fixed alone, I think we'd suffer even worse, due to\n2) - since the child processes aren't killed, we'd end up allowing\nmore connections than the value of max_connections - upload-pack would\ngladly continue in the background. Some quick testing showed that the\nfollowing patch solved the issue for me. I'm not very happy about it,\nsince I'm working on porting the code to Windows, and we don't have\nthe same process-group concept on windows... Oh well.\n\ndiff --git a/daemon.c b/daemon.c\nindex 918e560..bc6874c 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -291,12 +291,6 @@ static int run_service(char *dir, struct\ndaemon_service *service)\n return -1;\n }\n\n- /*\n- * We'll ignore SIGTERM from now on, we have a\n- * good client.\n- */\n- signal(SIGTERM, SIG_IGN);\n-\n return service->fn();\n }\n\n@@ -633,7 +627,8 @@ static void kill_some_child(void)\n\n for (; (next = blanket->next); blanket = next)\n if (!addrcmp(&blanket->address, &next->address)) {\n- kill(blanket->pid, SIGTERM);\n+ kill(-blanket->pid, SIGTERM);\n break;\n }\n }\n@@ -676,7 +671,8 @@ static void handle(int incoming, struct sockaddr\n*addr, int addrlen)\n\n add_child(pid, addr, addrlen);\n return;\n- }\n+ } else\n+ setpgid(0, 0);\n\n dup2(incoming, 0);\n dup2(incoming, 1);\n\n-- \nErik \"kusma\" Faye-Lund\n"}]}