[PATCH/RFC 00/11] daemon-win32

[PATCH/RFC 00/11] daemon-win32

[Erik Faye-Lund]

This is my stab at cleaning up Mike Pape's patches for git-daemon
on Windows for submission, plus some of my own.

* Patch 1-4 originate from Mike Pape, but have been cleaned up quite
  a lot by me. I hope Mike won't hate me. Credit have been retained,
  as all important code here were written by Mike. The commit
  messages were written mostly by me.

* Patch 5 is a trivial old-style function declaration fix.

* Patch 6 introduces two new functions to the run-command API,
  kill_async() and is_async_alive().

* Patch 7-8 removes the stdin/stdout redirection for the service
  functions, as redirecting won't work for the threaded version.

* Patch 9 converts the daemon-code to use the run-command API. This
  is the patch I expect to be the most controversial.

* Patch 10 is about using line-buffered mode instead of full-buffered
  mode for stderr.

* Patch 11 finally enables compilation of git-daemon on MinGW.

Let the flames begin!

Erik Faye-Lund (7):
  inet_ntop: fix a couple of old-style decls
  run-command: add kill_async() and is_async_alive()
  run-command: support input-fd
  daemon: use explicit file descriptor
  daemon: use run-command api for async serving
  daemon: use full buffered mode for stderr
  mingw: compile git-daemon

Mike Pape (4):
  mingw: add network-wrappers for daemon
  strbuf: add non-variadic function strbuf_vaddf()
  mingw: implement syslog
  compat: add inet_pton and inet_ntop prototypes

 Makefile           |    8 ++-
 compat/inet_ntop.c |   22 ++------
 compat/inet_pton.c |    8 ++-
 compat/mingw.c     |  111 +++++++++++++++++++++++++++++++++++++++++--
 compat/mingw.h     |   32 ++++++++++++
 daemon.c           |  134 ++++++++++++++++++++++++++--------------------------
 git-compat-util.h  |    9 ++++
 run-command.c      |   32 ++++++++++++-
 run-command.h      |    2 +
 strbuf.c           |   15 ++++--
 strbuf.h           |    1 +
 11 files changed, 274 insertions(+), 100 deletions(-)

[PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Erik Faye-Lund]

From: Mike Pape <dotzenlabs@gmail.com>

git-daemon requires some socket-functionality that is not yet
supported in the Windows-port. This patch adds said functionality,
and makes sure WSAStartup gets called by socket(), since it is the
first network-call in git-daemon. In addition, a check is added to
prevent WSAStartup (and WSACleanup, though atexit) from being
called more than once, since git-daemon calls both socket() and
gethostbyname().

Signed-off-by: Mike Pape <dotzenlabs@gmail.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/mingw.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 compat/mingw.h |   16 ++++++++++++++
 2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 10d6796..458021b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -983,23 +983,37 @@ char **make_augmented_environ(const char *const *vars)
 	return env;
 }
 
-/* this is the first function to call into WS_32; initialize it */
-#undef gethostbyname
-struct hostent *mingw_gethostbyname(const char *host)
+static void wsa_init(void)
 {
+	static int initialized = 0;
 	WSADATA wsa;
 
+	if (initialized)
+		return;
+
 	if (WSAStartup(MAKEWORD(2,2), &wsa))
 		die("unable to initialize winsock subsystem, error %d",
 			WSAGetLastError());
 	atexit((void(*)(void)) WSACleanup);
+	initialized = 1;
+}
+
+/* this can be the first function to call into WS_32; initialize it */
+#undef gethostbyname
+struct hostent *mingw_gethostbyname(const char *host)
+{
+	wsa_init();
 	return gethostbyname(host);
 }
 
+/* this can be the first function to call into WS_32; initialize it */
 int mingw_socket(int domain, int type, int protocol)
 {
+	SOCKET s;
 	int sockfd;
-	SOCKET s = WSASocket(domain, type, protocol, NULL, 0, 0);
+
+	wsa_init();
+	s = WSASocket(domain, type, protocol, NULL, 0, 0);
 	if (s == INVALID_SOCKET) {
 		/*
 		 * WSAGetLastError() values are regular BSD error codes
@@ -1029,6 +1043,44 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)
 	return connect(s, sa, sz);
 }
 
+#undef bind
+int mingw_bind(int sockfd, struct sockaddr *sa, size_t sz)
+{
+	SOCKET s = (SOCKET)_get_osfhandle(sockfd);
+	return bind(s, sa, sz);
+}
+
+#undef setsockopt
+int mingw_setsockopt(int sockfd, int lvl, int optname, void *optval, int optlen)
+{
+	SOCKET s = (SOCKET)_get_osfhandle(sockfd);
+	return setsockopt(s, lvl, optname, (const char*)optval, optlen);
+}
+
+#undef listen
+int mingw_listen(int sockfd, int backlog)
+{
+	SOCKET s = (SOCKET)_get_osfhandle(sockfd);
+	return listen(s, backlog);
+}
+
+#undef accept
+int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
+{
+	int sockfd2;
+
+	SOCKET s1 = (SOCKET)_get_osfhandle(sockfd1);
+	SOCKET s2 = accept(s1, sa, sz);
+
+	/* convert into a file descriptor */
+	if ((sockfd2 = _open_osfhandle(s2, O_RDWR|O_BINARY)) < 0) {
+		closesocket(s2);
+		return error("unable to make a socket file descriptor: %s",
+			strerror(errno));
+	}
+	return sockfd2;
+}
+
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index 1978f8a..f362f61 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -5,6 +5,7 @@
  */
 
 typedef int pid_t;
+typedef int socklen_t;
 #define hstrerror strerror
 
 #define S_IFLNK    0120000 /* Symbolic link */
@@ -33,6 +34,9 @@ typedef int pid_t;
 #define F_SETFD 2
 #define FD_CLOEXEC 0x1
 
+#define EAFNOSUPPORT WSAEAFNOSUPPORT
+#define ECONNABORTED WSAECONNABORTED
+
 struct passwd {
 	char *pw_name;
 	char *pw_gecos;
@@ -182,6 +186,18 @@ int mingw_socket(int domain, int type, int protocol);
 int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz);
 #define connect mingw_connect
 
+int mingw_bind(int sockfd, struct sockaddr *sa, size_t sz);
+#define bind mingw_bind
+
+int mingw_setsockopt(int sockfd, int lvl, int optname, void *optval, int optlen);
+#define setsockopt mingw_setsockopt
+
+int mingw_listen(int sockfd, int backlog);
+#define listen mingw_listen
+
+int mingw_accept(int sockfd, struct sockaddr *sa, socklen_t *sz);
+#define accept mingw_accept
+
 int mingw_rename(const char*, const char*);
 #define rename mingw_rename
 
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Erik Faye-Lund]

From: Mike Pape <dotzenlabs@gmail.com>

This patch adds strbuf_vaddf, which takes a va_list as input
instead of the variadic input that strbuf_addf takes. This
is useful for fowarding varargs to strbuf_addf.

Signed-off-by: Mike Pape <dotzenlabs@gmail.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 strbuf.c |   15 +++++++++------
 strbuf.h |    1 +
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index a6153dc..e1833fb 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -190,23 +190,18 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)
 	strbuf_setlen(sb, sb->len + len);
 }
 
-void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
+void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
 {
 	int len;
-	va_list ap;
 
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 64);
-	va_start(ap, fmt);
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-	va_end(ap);
 	if (len < 0)
 		die("your vsnprintf is broken");
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
-		va_start(ap, fmt);
 		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-		va_end(ap);
 		if (len > strbuf_avail(sb)) {
 			die("this should not happen, your snprintf is broken");
 		}
@@ -214,6 +209,14 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list va;
+	va_start(va, fmt);
+	strbuf_vaddf(sb, fmt, va);
+	va_end(va);
+}
+
 void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 		   void *context)
 {
diff --git a/strbuf.h b/strbuf.h
index d05e056..8686bcb 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -119,6 +119,7 @@ extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 
 __attribute__((format(printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 03/11] mingw: implement syslog

[Erik Faye-Lund]

From: Mike Pape <dotzenlabs@gmail.com>

Syslog does not usually exist on Windows, so we implement our own
using Window's ReportEvent mechanism.

Signed-off-by: Mike Pape <dotzenlabs@gmail.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/mingw.c    |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 compat/mingw.h    |   15 +++++++++++++++
 daemon.c          |    2 --
 git-compat-util.h |    1 +
 4 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 458021b..68116ac 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1255,6 +1255,57 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out)
 	return 0;
 }
 
+static HANDLE ms_eventlog;
+
+void openlog(const char *ident, int logopt, int facility) {
+	if (ms_eventlog)
+		return;
+	ms_eventlog = RegisterEventSourceA(NULL, ident);
+}
+
+void syslog(int priority, const char *fmt, ...) {
+	struct strbuf msg;
+	va_list va;
+	WORD logtype;
+
+	strbuf_init(&msg, 0);
+	va_start(va, fmt);
+	strbuf_vaddf(&msg, fmt, va);
+	va_end(va);
+
+	switch (priority) {
+		case LOG_EMERG:
+		case LOG_ALERT:
+		case LOG_CRIT:
+		case LOG_ERR:
+			logtype = EVENTLOG_ERROR_TYPE;
+			break;
+
+		case LOG_WARNING:
+			logtype = EVENTLOG_WARNING_TYPE;
+			break;
+
+		case LOG_NOTICE:
+		case LOG_INFO:
+		case LOG_DEBUG:
+		default:
+			logtype = EVENTLOG_INFORMATION_TYPE;
+			break;
+	}
+
+	ReportEventA(ms_eventlog,
+	    logtype,
+	    (WORD)NULL,
+	    (DWORD)NULL,
+	    NULL,
+	    1,
+	    0,
+	    (const char **)&msg.buf,
+	    NULL);
+
+	strbuf_release(&msg);
+}
+
 #undef signal
 sig_handler_t mingw_signal(int sig, sig_handler_t handler)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index f362f61..576b1a1 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -37,6 +37,19 @@ typedef int socklen_t;
 #define EAFNOSUPPORT WSAEAFNOSUPPORT
 #define ECONNABORTED WSAECONNABORTED
 
+#define LOG_PID     0x01
+
+#define LOG_EMERG   0
+#define LOG_ALERT   1
+#define LOG_CRIT    2
+#define LOG_ERR     3
+#define LOG_WARNING 4
+#define LOG_NOTICE  5
+#define LOG_INFO    6
+#define LOG_DEBUG   7
+
+#define LOG_DAEMON  (3<<3)
+
 struct passwd {
 	char *pw_name;
 	char *pw_gecos;
@@ -157,6 +170,8 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out);
 int link(const char *oldpath, const char *newpath);
 int symlink(const char *oldpath, const char *newpath);
 int readlink(const char *path, char *buf, size_t bufsiz);
+void openlog(const char *ident, int logopt, int facility);
+void syslog(int priority, const char *fmt, ...);
 
 /*
  * replacements of existing functions
diff --git a/daemon.c b/daemon.c
index 1b5ada6..07d7356 100644
--- a/daemon.c
+++ b/daemon.c
@@ -4,8 +4,6 @@
 #include "run-command.h"
 #include "strbuf.h"
 
-#include <syslog.h>
-
 #ifndef HOST_NAME_MAX
 #define HOST_NAME_MAX 256
 #endif
diff --git a/git-compat-util.h b/git-compat-util.h
index ef60803..33a8e33 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -105,6 +105,7 @@
 #include <netdb.h>
 #include <pwd.h>
 #include <inttypes.h>
+#include <syslog.h>
 #if defined(__CYGWIN__)
 #undef _XOPEN_SOURCE
 #include <grp.h>
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 04/11] compat: add inet_pton and inet_ntop prototypes

[Erik Faye-Lund]

From: Mike Pape <dotzenlabs@gmail.com>

Windows doesn't have inet_pton and inet_ntop, so
add prototypes in git-compat-util.h for them.

At the same time include git-compat-util.h in
the sources for these functions, so they use the
network-wrappers from there on Windows.

Signed-off-by: Mike Pape <dotzenlabs@gmail.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 Makefile           |    2 ++
 compat/inet_ntop.c |    6 +++---
 compat/inet_pton.c |    8 +++++---
 git-compat-util.h  |    8 ++++++++
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 70cee6d..3b01694 100644
--- a/Makefile
+++ b/Makefile
@@ -1227,9 +1227,11 @@ endif
 endif
 ifdef NO_INET_NTOP
 	LIB_OBJS += compat/inet_ntop.o
+	BASIC_CFLAGS += -DNO_INET_NTOP
 endif
 ifdef NO_INET_PTON
 	LIB_OBJS += compat/inet_pton.o
+	BASIC_CFLAGS += -DNO_INET_PTON
 endif
 
 ifdef NO_ICONV
diff --git a/compat/inet_ntop.c b/compat/inet_ntop.c
index f444982..e5b46a0 100644
--- a/compat/inet_ntop.c
+++ b/compat/inet_ntop.c
@@ -17,9 +17,9 @@
 
 #include <errno.h>
 #include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <arpa/inet.h>
+
+#include "../git-compat-util.h"
+
 #include <stdio.h>
 #include <string.h>
 
diff --git a/compat/inet_pton.c b/compat/inet_pton.c
index 4078fc0..2ec995e 100644
--- a/compat/inet_pton.c
+++ b/compat/inet_pton.c
@@ -17,9 +17,9 @@
 
 #include <errno.h>
 #include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <arpa/inet.h>
+
+#include "../git-compat-util.h"
+
 #include <stdio.h>
 #include <string.h>
 
@@ -41,7 +41,9 @@
  */
 
 static int inet_pton4(const char *src, unsigned char *dst);
+#ifndef NO_IPV6
 static int inet_pton6(const char *src, unsigned char *dst);
+#endif
 
 /* int
  * inet_pton4(src, dst)
diff --git a/git-compat-util.h b/git-compat-util.h
index 33a8e33..27fa601 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -340,6 +340,14 @@ static inline char *gitstrchrnul(const char *s, int c)
 }
 #endif
 
+#ifdef NO_INET_PTON
+int inet_pton(int af, const char *src, void *dst);
+#endif
+
+#ifdef NO_INET_NTOP
+const char *inet_ntop(int af, const void *src, char *dst, size_t size);
+#endif
+
 extern void release_pack_memory(size_t, int);
 
 extern char *xstrdup(const char *str);
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 05/11] inet_ntop: fix a couple of old-style decls

[Erik Faye-Lund]

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/inet_ntop.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/compat/inet_ntop.c b/compat/inet_ntop.c
index e5b46a0..ea249c6 100644
--- a/compat/inet_ntop.c
+++ b/compat/inet_ntop.c
@@ -50,10 +50,7 @@
  *	Paul Vixie, 1996.
  */
 static const char *
-inet_ntop4(src, dst, size)
-	const u_char *src;
-	char *dst;
-	size_t size;
+inet_ntop4(const u_char *src, char *dst, size_t size)
 {
 	static const char fmt[] = "%u.%u.%u.%u";
 	char tmp[sizeof "255.255.255.255"];
@@ -78,10 +75,7 @@ inet_ntop4(src, dst, size)
  *	Paul Vixie, 1996.
  */
 static const char *
-inet_ntop6(src, dst, size)
-	const u_char *src;
-	char *dst;
-	size_t size;
+inet_ntop6(const u_char *src, char *dst, size_t size)
 {
 	/*
 	 * Note that int32_t and int16_t need only be "at least" large enough
@@ -178,11 +172,7 @@ inet_ntop6(src, dst, size)
  *	Paul Vixie, 1996.
  */
 const char *
-inet_ntop(af, src, dst, size)
-	int af;
-	const void *src;
-	char *dst;
-	size_t size;
+inet_ntop(int af, const void *src, char *dst, size_t size)
 {
 	switch (af) {
 	case AF_INET:
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Erik Faye-Lund]

These functions will aid the Windows port of git-daemon.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 run-command.c |   27 +++++++++++++++++++++++++++
 run-command.h |    2 ++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/run-command.c b/run-command.c
index cf2d8f7..e5a0e06 100644
--- a/run-command.c
+++ b/run-command.c
@@ -373,6 +373,33 @@ int finish_async(struct async *async)
 	return ret;
 }
 
+int kill_async(struct async *async)
+{
+#ifndef WIN32
+	return kill(async->pid, SIGTERM);
+#else
+	DWORD ret = 0;
+	if (!TerminateThread(async->tid, 0))
+		ret = error("killing thread failed: %lu", GetLastError());
+	else if (!GetExitCodeThread(async->tid, &ret))
+		ret = error("cannot get thread exit code: %lu", GetLastError());
+	return ret;
+#endif
+}
+
+int is_async_alive(struct async *async)
+{
+#ifndef WIN32
+	int dummy;
+	return waitpid(async->pid, &dummy, WNOHANG);
+#else
+	int ret = WaitForSingleObject(async->tid, 0);
+	if (ret == WAIT_FAILED)
+		return error("checking thread state failed: %lu", GetLastError());
+	return ret != WAIT_OBJECT_0;
+#endif
+}
+
 int run_hook(const char *index_file, const char *name, ...)
 {
 	struct child_process hook;
diff --git a/run-command.h b/run-command.h
index fb34209..955b0bd 100644
--- a/run-command.h
+++ b/run-command.h
@@ -80,5 +80,7 @@ struct async {
 
 int start_async(struct async *async);
 int finish_async(struct async *async);
+int kill_async(struct async *async);
+int is_async_alive(struct async *async);
 
 #endif
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 07/11] run-command: support input-fd

[Erik Faye-Lund]

This patch adds the possibility to supply a non-0
file descriptor for communucation, instead of the
default-created pipe. The pipe gets duplicated, so
the caller can free it's handles.

This is usefull for async communication over sockets.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 run-command.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index e5a0e06..98771ef 100644
--- a/run-command.c
+++ b/run-command.c
@@ -327,7 +327,10 @@ int start_async(struct async *async)
 {
 	int pipe_out[2];
 
-	if (pipe(pipe_out) < 0)
+	if (async->out) {
+		pipe_out[0] = dup(async->out);
+		pipe_out[1] = dup(async->out);
+	} else if (pipe(pipe_out) < 0)
 		return error("cannot create pipe: %s", strerror(errno));
 	async->out = pipe_out[0];
 
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 08/11] daemon: use explicit file descriptor

[Erik Faye-Lund]

This patch adds support to specify an explicit file
descriotor for communication with the client, instead
of using stdin/stdout.

This will be useful for the Windows port, because it
will use threads instead of fork() to serve multiple
clients, making it impossible to reuse stdin/stdout.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 daemon.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/daemon.c b/daemon.c
index 07d7356..a0aead5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -263,7 +263,7 @@ static char *path_ok(char *directory)
 	return NULL;		/* Fallthrough. Deny by default */
 }
 
-typedef int (*daemon_service_fn)(void);
+typedef int (*daemon_service_fn)(int);
 struct daemon_service {
 	const char *name;
 	const char *config_name;
@@ -287,7 +287,7 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
-static int run_service(char *dir, struct daemon_service *service)
+static int run_service(int fd, char *dir, struct daemon_service *service)
 {
 	const char *path;
 	int enabled = service->enabled;
@@ -340,7 +340,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	 */
 	signal(SIGTERM, SIG_IGN);
 
-	return service->fn();
+	return service->fn(fd);
 }
 
 static void copy_to_log(int fd)
@@ -364,7 +364,7 @@ static void copy_to_log(int fd)
 	fclose(fp);
 }
 
-static int run_service_command(const char **argv)
+static int run_service_command(int fd, const char **argv)
 {
 	struct child_process cld;
 
@@ -372,37 +372,35 @@ static int run_service_command(const char **argv)
 	cld.argv = argv;
 	cld.git_cmd = 1;
 	cld.err = -1;
+	cld.in = cld.out = fd;
 	if (start_command(&cld))
 		return -1;
 
-	close(0);
-	close(1);
-
 	copy_to_log(cld.err);
 
 	return finish_command(&cld);
 }
 
-static int upload_pack(void)
+static int upload_pack(int fd)
 {
 	/* Timeout as string */
 	char timeout_buf[64];
 	const char *argv[] = { "upload-pack", "--strict", timeout_buf, ".", NULL };
 
 	snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout);
-	return run_service_command(argv);
+	return run_service_command(fd, argv);
 }
 
-static int upload_archive(void)
+static int upload_archive(int fd)
 {
 	static const char *argv[] = { "upload-archive", ".", NULL };
-	return run_service_command(argv);
+	return run_service_command(fd, argv);
 }
 
-static int receive_pack(void)
+static int receive_pack(int fd)
 {
 	static const char *argv[] = { "receive-pack", ".", NULL };
-	return run_service_command(argv);
+	return run_service_command(fd, argv);
 }
 
 static struct daemon_service daemon_service[] = {
@@ -532,7 +530,7 @@ static void parse_host_arg(char *extra_args, int buflen)
 }
 
 
-static int execute(struct sockaddr *addr)
+static int execute(int fd, struct sockaddr *addr)
 {
 	static char line[1000];
 	int pktlen, len, i;
@@ -565,7 +563,7 @@ static int execute(struct sockaddr *addr)
 	}
 
 	alarm(init_timeout ? init_timeout : timeout);
-	pktlen = packet_read_line(0, line, sizeof(line));
+	pktlen = packet_read_line(fd, line, sizeof(line));
 	alarm(0);
 
 	len = strlen(line);
@@ -597,7 +595,7 @@ static int execute(struct sockaddr *addr)
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(line + namelen + 5, s);
+			return run_service(fd, line + namelen + 5, s);
 		}
 	}
 
@@ -713,7 +711,7 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 	dup2(incoming, 1);
 	close(incoming);
 
-	exit(execute(addr));
+	exit(execute(0, addr));
 }
 
 static void child_handler(int signo)
@@ -1150,7 +1148,7 @@ int main(int argc, char **argv)
 		if (getpeername(0, peer, &slen))
 			peer = NULL;
 
-		return execute(peer);
+		return execute(0, peer);
 	}
 
 	if (detach) {
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 09/11] daemon: use run-command api for async serving

[Erik Faye-Lund]

fork() is only available on POSIX, so to support git-daemon
on Windows we have to use something else. Conveniently
enough, we have an API for async operation already.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 daemon.c |   79 ++++++++++++++++++++++++++++---------------------------------
 1 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/daemon.c b/daemon.c
index a0aead5..1b0e290 100644
--- a/daemon.c
+++ b/daemon.c
@@ -372,7 +372,8 @@ static int run_service_command(int fd, const char **argv)
 	cld.argv = argv;
 	cld.git_cmd = 1;
 	cld.err = -1;
-	cld.in = cld.out = fd;
+	cld.in = dup(fd);
+	cld.out = fd;
 	if (start_command(&cld))
 		return -1;
 
@@ -609,11 +610,11 @@ static unsigned int live_children;
 
 static struct child {
 	struct child *next;
-	pid_t pid;
+	struct async async;
 	struct sockaddr_storage address;
 } *firstborn;
 
-static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
+static void add_child(struct async *async, struct sockaddr *addr, int addrlen)
 {
 	struct child *newborn, **cradle;
 
@@ -623,7 +624,7 @@ static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
 	 */
 	newborn = xcalloc(1, sizeof(*newborn));
 	live_children++;
-	newborn->pid = pid;
+	memcpy(&newborn->async, async, sizeof(struct async));
 	memcpy(&newborn->address, addr, addrlen);
 	for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
 		if (!memcmp(&(*cradle)->address, &newborn->address,
@@ -633,19 +634,6 @@ static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
 	*cradle = newborn;
 }
 
-static void remove_child(pid_t pid)
-{
-	struct child **cradle, *blanket;
-
-	for (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)
-		if (blanket->pid == pid) {
-			*cradle = blanket->next;
-			live_children--;
-			free(blanket);
-			break;
-		}
-}
-
 /*
  * This gets called if the number of connections grows
  * past "max_connections".
@@ -654,7 +642,7 @@ static void remove_child(pid_t pid)
  */
 static void kill_some_child(void)
 {
-	const struct child *blanket, *next;
+	struct child *blanket, *next;
 
 	if (!(blanket = firstborn))
 		return;
@@ -662,28 +650,37 @@ static void kill_some_child(void)
 	for (; (next = blanket->next); blanket = next)
 		if (!memcmp(&blanket->address, &next->address,
 			    sizeof(next->address))) {
-			kill(blanket->pid, SIGTERM);
+			kill_async(&blanket->async);
 			break;
 		}
 }
 
 static void check_dead_children(void)
 {
-	int status;
-	pid_t pid;
-
-	while ((pid = waitpid(-1, &status, WNOHANG)) > 0) {
-		const char *dead = "";
-		remove_child(pid);
-		if (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))
-			dead = " (with error)";
-		loginfo("[%"PRIuMAX"] Disconnected%s", (uintmax_t)pid, dead);
-	}
+	struct child **cradle, *blanket;
+	for (cradle = &firstborn; (blanket = *cradle);)
+		if (!is_async_alive(&blanket->async)) {
+			*cradle = blanket->next;
+			loginfo("Disconnected\n");
+			live_children--;
+			close(blanket->async.out);
+			free(blanket);
+		} else
+			cradle = &blanket->next;
+}
+
+int async_execute(int fd, void *data)
+{
+	int ret = execute(fd, data);
+	close(fd);
+	free(data);
+	return ret;
 }
 
 static void handle(int incoming, struct sockaddr *addr, int addrlen)
 {
-	pid_t pid;
+	struct sockaddr_storage *ss;
+	struct async async = { 0 };
 
 	if (max_connections && live_children >= max_connections) {
 		kill_some_child();
@@ -696,22 +693,18 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 		}
 	}
 
-	if ((pid = fork())) {
-		close(incoming);
-		if (pid < 0) {
-			logerror("Couldn't fork %s", strerror(errno));
-			return;
-		}
+	ss = xmalloc(sizeof(*ss));
+	memcpy(ss, addr, addrlen);
 
-		add_child(pid, addr, addrlen);
-		return;
-	}
+	async.proc = async_execute;
+	async.data = ss;
+	async.out = incoming;
 
-	dup2(incoming, 0);
-	dup2(incoming, 1);
+	if (start_async(&async))
+		logerror("unable to fork");
+	else
+		add_child(&async, addr, addrlen);
 	close(incoming);
-
-	exit(execute(0, addr));
 }
 
 static void child_handler(int signo)
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 10/11] daemon: use full buffered mode for stderr

[Erik Faye-Lund]

Windows doesn't support line buffered mode for file
streams, so let's just use full buffered mode with
a big buffer ("4096 should be enough for everyone")
and add explicit flushing.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 daemon.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/daemon.c b/daemon.c
index 1b0e290..130e951 100644
--- a/daemon.c
+++ b/daemon.c
@@ -66,12 +66,14 @@ static void logreport(int priority, const char *err, va_list params)
 		syslog(priority, "%s", buf);
 	} else {
 		/*
-		 * Since stderr is set to linebuffered mode, the
+		 * Since stderr is set to buffered mode, the
 		 * logging of different processes will not overlap
+		 * unless they overflow the (rather big) buffers.
 		 */
 		fprintf(stderr, "[%"PRIuMAX"] ", (uintmax_t)getpid());
 		vfprintf(stderr, err, params);
 		fputc('\n', stderr);
+		fflush(stderr);
 	}
 }
 
@@ -1094,7 +1096,7 @@ int main(int argc, char **argv)
 		set_die_routine(daemon_die);
 	} else
 		/* avoid splitting a message in the middle */
-		setvbuf(stderr, NULL, _IOLBF, 0);
+		setvbuf(stderr, NULL, _IOFBF, 4096);
 
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 11/11] mingw: compile git-daemon

[Erik Faye-Lund]

--user and --detach are disabled on Windows due to lack of
fork(), setuid(), setgid(), setsid() and initgroups().

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 Makefile       |    6 +++---
 compat/mingw.h |    1 +
 daemon.c       |   19 ++++++++++++++-----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 3b01694..406ca81 100644
--- a/Makefile
+++ b/Makefile
@@ -352,6 +352,7 @@ EXTRA_PROGRAMS =
 
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS += $(EXTRA_PROGRAMS)
+PROGRAMS += git-daemon$X
 PROGRAMS += git-fast-import$X
 PROGRAMS += git-hash-object$X
 PROGRAMS += git-imap-send$X
@@ -981,6 +982,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
 	NO_REGEX = YesPlease
 	BLK_SHA1 = YesPlease
+	NO_INET_PTON = YesPlease
+	NO_INET_NTOP = YesPlease
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	# We have GCC, so let's make use of those nice options
@@ -1079,9 +1082,6 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz
 
-ifndef NO_POSIX_ONLY_PROGRAMS
-	PROGRAMS += git-daemon$X
-endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
diff --git a/compat/mingw.h b/compat/mingw.h
index 576b1a1..1b0dd5b 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -6,6 +6,7 @@
 
 typedef int pid_t;
 typedef int socklen_t;
+typedef unsigned int gid_t;
 #define hstrerror strerror
 
 #define S_IFLNK    0120000 /* Symbolic link */
diff --git a/daemon.c b/daemon.c
index 130e951..5872ec7 100644
--- a/daemon.c
+++ b/daemon.c
@@ -616,7 +616,7 @@ static struct child {
 	struct sockaddr_storage address;
 } *firstborn;
 
-static void add_child(struct async *async, struct sockaddr *addr, int addrlen)
+static void add_child(struct async *async, struct sockaddr *addr, socklen_t addrlen)
 {
 	struct child *newborn, **cradle;
 
@@ -679,7 +679,7 @@ int async_execute(int fd, void *data)
 	return ret;
 }
 
-static void handle(int incoming, struct sockaddr *addr, int addrlen)
+static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
 {
 	struct sockaddr_storage *ss;
 	struct async async = { 0 };
@@ -884,7 +884,7 @@ static int service_loop(int socknum, int *socklist)
 		for (i = 0; i < socknum; i++) {
 			if (pfd[i].revents & POLLIN) {
 				struct sockaddr_storage ss;
-				unsigned int sslen = sizeof(ss);
+				socklen_t sslen = sizeof(ss);
 				int incoming = accept(pfd[i].fd, (struct sockaddr *)&ss, &sslen);
 				if (incoming < 0) {
 					switch (errno) {
@@ -916,6 +916,7 @@ static void sanitize_stdfds(void)
 
 static void daemonize(void)
 {
+#ifndef WIN32
 	switch (fork()) {
 		case 0:
 			break;
@@ -930,6 +931,9 @@ static void daemonize(void)
 	close(1);
 	close(2);
 	sanitize_stdfds();
+#else
+	die("--detach is not supported on Windows");
+#endif
 }
 
 static void store_pid(const char *path)
@@ -950,10 +954,12 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t
 		die("unable to allocate any listen sockets on host %s port %u",
 		    listen_addr, listen_port);
 
+#ifndef WIN32
 	if (pass && gid &&
 	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
 	     setuid(pass->pw_uid)))
 		die("cannot drop privileges");
+#endif
 
 	return service_loop(socknum, socklist);
 }
@@ -966,7 +972,6 @@ int main(int argc, char **argv)
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
 	struct passwd *pass = NULL;
-	struct group *group;
 	gid_t gid = 0;
 	int i;
 
@@ -1110,6 +1115,7 @@ int main(int argc, char **argv)
 		die("--group supplied without --user");
 
 	if (user_name) {
+#ifndef WIN32
 		pass = getpwnam(user_name);
 		if (!pass)
 			die("user not found - %s", user_name);
@@ -1117,12 +1123,15 @@ int main(int argc, char **argv)
 		if (!group_name)
 			gid = pass->pw_gid;
 		else {
-			group = getgrnam(group_name);
+			struct group *group = getgrnam(group_name);
 			if (!group)
 				die("group not found - %s", group_name);
 
 			gid = group->gr_gid;
 		}
+#else
+		die("--user is not supported on Windows");
+#endif
 	}
 
 	if (strict_paths && (!ok_paths || !*ok_paths))
-- 
1.6.5.rc2.7.g4f8d3

Re: [msysGit] [PATCH/RFC 11/11] mingw: compile git-daemon

[Johannes Sixt]

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> --- a/Makefile
> +++ b/Makefile
> @@ -352,6 +352,7 @@ EXTRA_PROGRAMS =
>
>  # ... and all the rest that could be moved out of bindir to gitexecdir
>  PROGRAMS += $(EXTRA_PROGRAMS)
> +PROGRAMS += git-daemon$X
>  PROGRAMS += git-fast-import$X
>  PROGRAMS += git-hash-object$X
>  PROGRAMS += git-imap-send$X
> @@ -981,6 +982,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))
>  	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
>  	NO_REGEX = YesPlease
>  	BLK_SHA1 = YesPlease
> +	NO_INET_PTON = YesPlease
> +	NO_INET_NTOP = YesPlease
>  	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch
>  	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
>  	# We have GCC, so let's make use of those nice options
> @@ -1079,9 +1082,6 @@ ifdef ZLIB_PATH
>  endif
>  EXTLIBS += -lz
>
> -ifndef NO_POSIX_ONLY_PROGRAMS
> -	PROGRAMS += git-daemon$X
> -endif
>  ifndef NO_OPENSSL
>  	OPENSSL_LIBSSL = -lssl
>  	ifdef OPENSSLDIR

You should remove NO_POSIX_ONLY_PROGRAMS from MSVC and MinGW sections.

-- Hannes

Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving

[Johannes Sixt]

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>  static void check_dead_children(void)
>  {
> -	int status;
> -	pid_t pid;
> -
> -	while ((pid = waitpid(-1, &status, WNOHANG)) > 0) {
> -		const char *dead = "";
> -		remove_child(pid);
> -		if (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))
> -			dead = " (with error)";
> -		loginfo("[%"PRIuMAX"] Disconnected%s", (uintmax_t)pid, dead);
> -	}
> +	struct child **cradle, *blanket;
> +	for (cradle = &firstborn; (blanket = *cradle);)
> +		if (!is_async_alive(&blanket->async)) {

This would be the right place to call finish_async(). But since we cannot 
wait, you invented is_async_alive(). But actually we are not only interested 
in whether the process is alive, but also whether it completed successfully 
so that we can add "(with error)". Would it make sense to have a function 
finish_async_nowait() instead of is_async_alive() that (1) stresses the 
start/finish symmetry and (2) can return more than just Boolean?

> +			*cradle = blanket->next;
> +			loginfo("Disconnected\n");

Here you are losing information about the pid, which is important to have in 
the syslog. The \n should be dropped.

> +	async.proc = async_execute;
> +	async.data = ss;
> +	async.out = incoming;
>
> -	dup2(incoming, 0);
> -	dup2(incoming, 1);
> +	if (start_async(&async))
> +		logerror("unable to fork");
> +	else
> +		add_child(&async, addr, addrlen);
>  	close(incoming);
> -
> -	exit(execute(0, addr));

In start_command(), the convention is that fds that are provided by the caller 
are closed by start_command() (even if there are errors). The close(incoming) 
that you leave here indicates that you are not using the same convention with 
start_async(). It would be nice to switch to the same convention.

-- Hannes

Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving

[Erik Faye-Lund]

On Fri, Nov 27, 2009 at 9:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>>  static void check_dead_children(void)
>>  {
>> -     int status;
>> -     pid_t pid;
>> -
>> -     while ((pid = waitpid(-1, &status, WNOHANG)) > 0) {
>> -             const char *dead = "";
>> -             remove_child(pid);
>> -             if (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))
>> -                     dead = " (with error)";
>> -             loginfo("[%"PRIuMAX"] Disconnected%s", (uintmax_t)pid, dead);
>> -     }
>> +     struct child **cradle, *blanket;
>> +     for (cradle = &firstborn; (blanket = *cradle);)
>> +             if (!is_async_alive(&blanket->async)) {
>
> This would be the right place to call finish_async(). But since we cannot
> wait, you invented is_async_alive(). But actually we are not only interested
> in whether the process is alive, but also whether it completed successfully
> so that we can add "(with error)". Would it make sense to have a function
> finish_async_nowait() instead of is_async_alive() that (1) stresses the
> start/finish symmetry and (2) can return more than just Boolean?
>

Yes, it does.

>> +                     *cradle = blanket->next;
>> +                     loginfo("Disconnected\n");
>
> Here you are losing information about the pid, which is important to have in
> the syslog. The \n should be dropped.
>

Yeah... I removed the pid mostly because after moving to async, there
wasn't "just a pid" any more. But if we make finish_async_nowait()
return whatever we need to report, I guess we can add the information
back somehow.

I'm not entirely sure how to make the interface, though. Any good suggestions?

>> +     async.proc = async_execute;
>> +     async.data = ss;
>> +     async.out = incoming;
>>
>> -     dup2(incoming, 0);
>> -     dup2(incoming, 1);
>> +     if (start_async(&async))
>> +             logerror("unable to fork");
>> +     else
>> +             add_child(&async, addr, addrlen);
>>       close(incoming);
>> -
>> -     exit(execute(0, addr));
>
> In start_command(), the convention is that fds that are provided by the caller
> are closed by start_command() (even if there are errors). The close(incoming)
> that you leave here indicates that you are not using the same convention with
> start_async(). It would be nice to switch to the same convention.
>

Yeah, I've fixed this for the next round.

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving

[Johannes Sixt]

On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:
> On Fri, Nov 27, 2009 at 9:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > Would it make sense to
> > have a function finish_async_nowait() instead of is_async_alive() that
> > (1) stresses the start/finish symmetry and (2) can return more than just
> > Boolean?
>...
>
> I'm not entirely sure how to make the interface, though. Any good
> suggestions?

I suggest to model finish_async_nowait() after waitpid() so that

	while ((pid = waitpid(-1, &status, WNOHANG)) > 0) ...
becomes
	while ((pid = finish_async_nowait(&some_async, &status)) > 0) ...

but where the resulting status is already "decoded", i.e. zero is success and 
non-zero is failure (including death through signal); WIFEXITED and 
WEXITSTATUS should not be applicable to status anymore.

-- Hannes

Re: [PATCH/RFC 09/11] daemon: use run-command api for async serving

[Erik Faye-Lund]

On Wed, Dec 2, 2009 at 8:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:
>> I'm not entirely sure how to make the interface, though. Any good
>> suggestions?
>
> I suggest to model finish_async_nowait() after waitpid() so that
>
>        while ((pid = waitpid(-1, &status, WNOHANG)) > 0) ...
> becomes
>        while ((pid = finish_async_nowait(&some_async, &status)) > 0) ...
>
> but where the resulting status is already "decoded", i.e. zero is success and
> non-zero is failure (including death through signal); WIFEXITED and
> WEXITSTATUS should not be applicable to status anymore.
>
> -- Hannes
>

Thanks. Implemented as suggested for the next round.

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor

[Johannes Sixt]

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> This patch adds support to specify an explicit file
> descriotor for communication with the client, instead
> of using stdin/stdout.
>
> This will be useful for the Windows port, because it
> will use threads instead of fork() to serve multiple
> clients, making it impossible to reuse stdin/stdout.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>  daemon.c |   34 ++++++++++++++++------------------
>  1 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 07d7356..a0aead5 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -263,7 +263,7 @@ static char *path_ok(char *directory)
>  	return NULL;		/* Fallthrough. Deny by default */
>  }
>
> -typedef int (*daemon_service_fn)(void);
> +typedef int (*daemon_service_fn)(int);
>  struct daemon_service {
>  	const char *name;
>  	const char *config_name;
> @@ -287,7 +287,7 @@ static int git_daemon_config(const char *var, const
> char *value, void *cb) return 0;
>  }
>
> -static int run_service(char *dir, struct daemon_service *service)
> +static int run_service(int fd, char *dir, struct daemon_service *service)
>  {
>  	const char *path;
>  	int enabled = service->enabled;
> @@ -340,7 +340,7 @@ static int run_service(char *dir, struct daemon_service
> *service) */
>  	signal(SIGTERM, SIG_IGN);
>
> -	return service->fn();
> +	return service->fn(fd);
>  }
>
>  static void copy_to_log(int fd)
> @@ -364,7 +364,7 @@ static void copy_to_log(int fd)
>  	fclose(fp);
>  }
>
> -static int run_service_command(const char **argv)
> +static int run_service_command(int fd, const char **argv)
>  {
>  	struct child_process cld;
>
> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)
>  	cld.argv = argv;
>  	cld.git_cmd = 1;
>  	cld.err = -1;
> +	cld.in = cld.out = fd;

You shouldn't do that. In fact, the next patch 9 has a hunk that correctly 
calls dup() once.

> -	close(0);
> -	close(1);

Here, stdin and stdout were closed and start_command() used both. But these 
two new calls

> +	exit(execute(0, addr));
> ...
> +		return execute(0, peer);

are the only places where a value is assigned to fd. Now it is always only 
stdin. Where does the old code initialize stdout? Shouldn't this place need a 
change, too?

-- Hannes

Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor

[Erik Faye-Lund]

Sorry for the long delay in the reply, but I'm a little low on time
these days (and I've already spent some time trying to figure out what
I was thinking - I wrote these patches a while ago).

On Thu, Nov 26, 2009 at 11:03 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)
>>       cld.argv = argv;
>>       cld.git_cmd = 1;
>>       cld.err = -1;
>> +     cld.in = cld.out = fd;
>
> You shouldn't do that. In fact, the next patch 9 has a hunk that correctly
> calls dup() once.
>

OK, as long as it works as expected, sure. But perhaps this needs a
little change (see discussion later)

>> -     close(0);
>> -     close(1);
>
> Here, stdin and stdout were closed and start_command() used both. But these
> two new calls
>
>> +     exit(execute(0, addr));
>> ...
>> +             return execute(0, peer);
>
> are the only places where a value is assigned to fd. Now it is always only
> stdin. Where does the old code initialize stdout? Shouldn't this place need a
> change, too?

The "dup2(incoming, 0)"-call in handle() is AFAICT what makes it work
to use the forked process' stdin as both stdin and stdout for the
service-process pipe (since fd 0 now becomes a pipe that is both
readable and writable). This isn't exactly a pretty mechanism, and I
guess I should rework it. At the very least, I should remove the
"dup2(incoming, 1)"-call, but I'm open to other suggestions. Perhaps I
can change this patch to do the entire socket-passing (which is
currently in the next patch)?

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor

[Erik Faye-Lund]

On Fri, Nov 27, 2009 at 3:23 PM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> Sorry for the long delay in the reply, but I'm a little low on time
> these days (and I've already spent some time trying to figure out what
> I was thinking - I wrote these patches a while ago).
>
> On Thu, Nov 26, 2009 at 11:03 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>>> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)
>>>       cld.argv = argv;
>>>       cld.git_cmd = 1;
>>>       cld.err = -1;
>>> +     cld.in = cld.out = fd;
>>
>> You shouldn't do that. In fact, the next patch 9 has a hunk that correctly
>> calls dup() once.
>>
>
> OK, as long as it works as expected, sure. But perhaps this needs a
> little change (see discussion later)
>
>>> -     close(0);
>>> -     close(1);
>>
>> Here, stdin and stdout were closed and start_command() used both. But these
>> two new calls
>>
>>> +     exit(execute(0, addr));
>>> ...
>>> +             return execute(0, peer);
>>
>> are the only places where a value is assigned to fd. Now it is always only
>> stdin. Where does the old code initialize stdout? Shouldn't this place need a
>> change, too?
>
> The "dup2(incoming, 0)"-call in handle() is AFAICT what makes it work
> to use the forked process' stdin as both stdin and stdout for the
> service-process pipe (since fd 0 now becomes a pipe that is both
> readable and writable). This isn't exactly a pretty mechanism, and I
> guess I should rework it. At the very least, I should remove the
> "dup2(incoming, 1)"-call, but I'm open to other suggestions. Perhaps I
> can change this patch to do the entire socket-passing (which is
> currently in the next patch)?
>

Something along these lines?

---8<---
diff --git a/daemon.c b/daemon.c
index a0aead5..8774ed5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -372,7 +372,8 @@ static int run_service_command(int fd, const char **argv)
 	cld.argv = argv;
 	cld.git_cmd = 1;
 	cld.err = -1;
-	cld.in = cld.out = fd;
+	cld.in = dup(fd);
+	cld.out = fd;
 	if (start_command(&cld))
 		return -1;

@@ -707,11 +708,7 @@ static void handle(int incoming, struct sockaddr
*addr, int addrlen)
 		return;
 	}

-	dup2(incoming, 0);
-	dup2(incoming, 1);
-	close(incoming);
-
-	exit(execute(0, addr));
+	exit(execute(incoming, addr));
 }

 static void child_handler(int signo)
---8<---

When I think more about it, I might've broken the inetd-mode as it
should communicate over stdin and stdout (not just stdin as it would
try to do now)... I don't know the inetd internals, but this frightens
me a bit.

So perhaps I'll need to provide support for two unidirectional file
descriptors after all?

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor

[Johannes Sixt]

On Freitag, 27. November 2009, Erik Faye-Lund wrote:
> On Fri, Nov 27, 2009 at 3:23 PM, Erik Faye-Lund
> <kusmabite@googlemail.com> wrote:
> > At the very least, I should remove the
> > "dup2(incoming, 1)"-call, but I'm open to other suggestions. Perhaps I
> > can change this patch to do the entire socket-passing (which is
> > currently in the next patch)?

No, an infrastructure change in a separate patch is good.

> Something along these lines?
>
> ---8<---
> -	cld.in = cld.out = fd;
> +	cld.in = dup(fd);
> +	cld.out = fd;
>...
> -	dup2(incoming, 0);
> -	dup2(incoming, 1);
> -	close(incoming);
> -
> -	exit(execute(0, addr));
> +	exit(execute(incoming, addr));
> ---8<---

Yes, this looks very good.

> When I think more about it, I might've broken the inetd-mode as it
> should communicate over stdin and stdout (not just stdin as it would
> try to do now)... I don't know the inetd internals, but this frightens
> me a bit.

Do we need inetd mode on Windows? At one time a looked for a inetd-like 
service, but couldn't find one.

-- Hannes

Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor

[Johannes Sixt]

On Freitag, 27. November 2009, Johannes Sixt wrote:
> On Freitag, 27. November 2009, Erik Faye-Lund wrote:
> > When I think more about it, I might've broken the inetd-mode as it
> > should communicate over stdin and stdout (not just stdin as it would
> > try to do now)... I don't know the inetd internals, but this frightens
> > me a bit.
>
> Do we need inetd mode on Windows? At one time a looked for a inetd-like
> service, but couldn't find one.

How foolish of me. This affects all platforms. Of course it is an important to 
keep inetd mode.

-- Hannes

Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor

[Erik Faye-Lund]

On Fri, Nov 27, 2009 at 9:28 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Freitag, 27. November 2009, Johannes Sixt wrote:
>> On Freitag, 27. November 2009, Erik Faye-Lund wrote:
>> > When I think more about it, I might've broken the inetd-mode as it
>> > should communicate over stdin and stdout (not just stdin as it would
>> > try to do now)... I don't know the inetd internals, but this frightens
>> > me a bit.
>>
>> Do we need inetd mode on Windows? At one time a looked for a inetd-like
>> service, but couldn't find one.
>
> How foolish of me. This affects all platforms. Of course it is an important to
> keep inetd mode.
>
> -- Hannes
>

I believe I've fixed this for the next round, but I haven't been able
to test the inetd-stuff, since I don't have a working
test-environment.

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd

[Johannes Sixt]

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> This patch adds the possibility to supply a non-0
> file descriptor for communucation, instead of the
> default-created pipe. The pipe gets duplicated, so
> the caller can free it's handles.
>
> This is usefull for async communication over sockets.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>  run-command.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index e5a0e06..98771ef 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -327,7 +327,10 @@ int start_async(struct async *async)
>  {
>  	int pipe_out[2];
>
> -	if (pipe(pipe_out) < 0)
> +	if (async->out) {
> +		pipe_out[0] = dup(async->out);
> +		pipe_out[1] = dup(async->out);
> +	} else if (pipe(pipe_out) < 0)
>  		return error("cannot create pipe: %s", strerror(errno));
>  	async->out = pipe_out[0];

Hm. If async->out != 0:

	pipe_out[0] = dup(async->out);
	async->out = pipe_out[0];

This is confusing.

Moreover, you are assigning (a dup of) the same fd to the writable end. This 
assumes a bi-directional channel. I don't yet know what I should think about 
this (haven't studied the later patches, yet).

It would be great if you could add a few words to 
Documentation/technical/api-runcommand.txt.

-- Hannes

Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd

[Erik Faye-Lund]

On Thu, Nov 26, 2009 at 10:53 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>> @@ -327,7 +327,10 @@ int start_async(struct async *async)
>>  {
>>       int pipe_out[2];
>>
>> -     if (pipe(pipe_out) < 0)
>> +     if (async->out) {
>> +             pipe_out[0] = dup(async->out);
>> +             pipe_out[1] = dup(async->out);
>> +     } else if (pipe(pipe_out) < 0)
>>               return error("cannot create pipe: %s", strerror(errno));
>>       async->out = pipe_out[0];
>
> Hm. If async->out != 0:
>
>        pipe_out[0] = dup(async->out);
>        async->out = pipe_out[0];
>
> This is confusing.

What do you find confusing about it? The idea is to use a provided
bi-directional fd instead of a pipe if async->out is non-zero. The
currently defined rules for async is that async->out must be zero
(since the structure should be zero-initialized).

> Moreover, you are assigning (a dup of) the same fd to the writable end. This
> assumes a bi-directional channel. I don't yet know what I should think about
> this (haven't studied the later patches, yet).
>

Indeed it does. Do we want to extend it to support a set of
unidirectional channels instead?

> It would be great if you could add a few words to
> Documentation/technical/api-runcommand.txt.
>

Ah, yes. I know I should update the documentation and all, I'm just
usually really bad (*cough* lazy *cough*) at documenting stuff. But
I'll give it a go and if people hate what I write, they can suggest
changes.

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd

[Johannes Sixt]

On Freitag, 27. November 2009, Erik Faye-Lund wrote:
> On Thu, Nov 26, 2009 at 10:53 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> >> @@ -327,7 +327,10 @@ int start_async(struct async *async)
> >>  {
> >>       int pipe_out[2];
> >>
> >> -     if (pipe(pipe_out) < 0)
> >> +     if (async->out) {
> >> +             pipe_out[0] = dup(async->out);
> >> +             pipe_out[1] = dup(async->out);
> >> +     } else if (pipe(pipe_out) < 0)
> >>               return error("cannot create pipe: %s", strerror(errno));
> >>       async->out = pipe_out[0];
> >
> > Hm. If async->out != 0:
> >
> >        pipe_out[0] = dup(async->out);
> >        async->out = pipe_out[0];
> >
> > This is confusing.
>
> What do you find confusing about it? The idea is to use a provided
> bi-directional fd instead of a pipe if async->out is non-zero. The
> currently defined rules for async is that async->out must be zero
> (since the structure should be zero-initialized).

It is just the code structure that is confusing. It should be

	if (async->out) {
		/* fd was provided */
		do all that is needed in this case
	} else {
		/* fd was requested */
		do all for this other case
	}
	/* nothing to do anymore here */

(Of course, this should only replace the part that is cited above, not the 
whole function.)

> > Moreover, you are assigning (a dup of) the same fd to the writable end.
> > This assumes a bi-directional channel. I don't yet know what I should
> > think about this (haven't studied the later patches, yet).
>
> Indeed it does. Do we want to extend it to support a set of
> unidirectional channels instead?

Yes, I think so. We could pass a regular int fd[2] array around with the clear 
definition that both can be closed independently, i.e. one must be a dup() of 
the other. struct async would also have such an array.

Speaking of dup(): The underlying function is DuplicateHandle(), and its 
documentation says:

"You should not use DuplicateHandle to duplicate handles to the following 
objects: ... o Sockets. ... use WSADuplicateSocket."

But then the docs of WSADuplicateSocket() talk only about duplicating a socket 
to a separate process. Perhaps DuplicateHandle() of a socket within the same 
process Just Works?

-- Hannes

Re: [PATCH/RFC 07/11] run-command: support input-fd

[Erik Faye-Lund]

On Fri, Nov 27, 2009 at 9:14 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Freitag, 27. November 2009, Erik Faye-Lund wrote:
>> What do you find confusing about it? The idea is to use a provided
>> bi-directional fd instead of a pipe if async->out is non-zero. The
>> currently defined rules for async is that async->out must be zero
>> (since the structure should be zero-initialized).
>
> It is just the code structure that is confusing. It should be
>
>        if (async->out) {
>                /* fd was provided */
>                do all that is needed in this case
>        } else {
>                /* fd was requested */
>                do all for this other case
>        }
>        /* nothing to do anymore here */
>
> (Of course, this should only replace the part that is cited above, not the
> whole function.)
>

OK. I've reimplemented the change for the next round, taking this into account.

>> Indeed it does. Do we want to extend it to support a set of
>> unidirectional channels instead?
>
> Yes, I think so. We could pass a regular int fd[2] array around with the clear
> definition that both can be closed independently, i.e. one must be a dup() of
> the other. struct async would also have such an array.
>

OK. This has been included for the next round. Instead of an array,
I've tried to be consistent with start_command, and used two
variables, "in" and "out".

> Speaking of dup(): The underlying function is DuplicateHandle(), and its
> documentation says:
>
> "You should not use DuplicateHandle to duplicate handles to the following
> objects: ... o Sockets. ... use WSADuplicateSocket."
>
> But then the docs of WSADuplicateSocket() talk only about duplicating a socket
> to a separate process. Perhaps DuplicateHandle() of a socket within the same
> process Just Works?
>

It seems the rest of the Windows-world depends on DuplicateHandle()
working for sockets, so I'm not too worried. I can't find anything
documentation(1) for _dup, and I don't think we have our own
dup()-implementation.

(1) http://msdn.microsoft.com/en-us/library/8syseb29(VS.71).aspx

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Johannes Sixt]

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> +int kill_async(struct async *async)
> +{
> +#ifndef WIN32
> +	return kill(async->pid, SIGTERM);
> +#else
> +	DWORD ret = 0;
> +	if (!TerminateThread(async->tid, 0))
> +		ret = error("killing thread failed: %lu", GetLastError());

Ugh! Did you read the documentation of TerminateThread()?

We need to kill processes/threads when we detect that there are too many 
connections. But TerminateThread() is such a dangerous function that we 
cannot pretend that everything is good, and we continue to accept 
connections.

Unless we find a different solution, I would prefer to punt and die instead.

> +	else if (!GetExitCodeThread(async->tid, &ret))
> +		ret = error("cannot get thread exit code: %lu", GetLastError());

What should the exit code be good for? The return value of this function can 
only be -1 (failure, could not kill) or 0 (success, process killed).

-- Hannes

Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Erik Faye-Lund]

On Thu, Nov 26, 2009 at 10:46 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>> +int kill_async(struct async *async)
>> +{
>> +#ifndef WIN32
>> +     return kill(async->pid, SIGTERM);
>> +#else
>> +     DWORD ret = 0;
>> +     if (!TerminateThread(async->tid, 0))
>> +             ret = error("killing thread failed: %lu", GetLastError());
>
> Ugh! Did you read the documentation of TerminateThread()?
>
> We need to kill processes/threads when we detect that there are too many
> connections. But TerminateThread() is such a dangerous function that we
> cannot pretend that everything is good, and we continue to accept
> connections.
>

Ouch, this is nasty. Something else needs to be done.

> Unless we find a different solution, I would prefer to punt and die instead.
>

Do you really think it's better to unconditionally take down the
entire process with an error, instead of having a relatively small
chance of stuff blowing up without any sensible error? I'm not 100%
convinced - but let's hope we'll find a proper fix.

>> +     else if (!GetExitCodeThread(async->tid, &ret))
>> +             ret = error("cannot get thread exit code: %lu", GetLastError());
>
> What should the exit code be good for? The return value of this function can
> only be -1 (failure, could not kill) or 0 (success, process killed).
>

I thought wait_or_whine() returned the exit-code, so I wanted to be
somewhat consistent. But even if it does (haven't checked), it's
probably not be worth it - I'll remove this for the next iteration.

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Johannes Sixt]

On Freitag, 27. November 2009, Erik Faye-Lund wrote:
> Do you really think it's better to unconditionally take down the
> entire process with an error, instead of having a relatively small
> chance of stuff blowing up without any sensible error? I'm not 100%
> convinced - but let's hope we'll find a proper fix.

"relatively small chance of stuff blowing up"? The docs of 
TerminateThread: "... the kernel32 state for the thread's process could be 
inconsistent." That's scary if we are talking about a process that should run 
for days or weeks without interruption.

The reason why we are killing a thread is to prevent keeping lots of 
connections open (to the same IP address). There are two situations to take 
care of:

1. We are in a lengthy computation without paying attention to the socket.

2. The client does not send or accept data for a long time.

Case 1 could happen if upload-pack is "counting objects" on a large 
repository. We would need some way to kill upload-pack. Since it is a 
separate process anyway, we could use TerminateProcess().

Case 2 could be achieved by using setsockopt() with SO_RCVTIMEO and 
SO_SNDTIMEO and a tiny timeout. But notice that we would set a timeout in one 
thread while another thread is waiting in ReadFile() or WriteFile(). Would 
that work?

-- Hannes

Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Erik Faye-Lund]

On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Freitag, 27. November 2009, Erik Faye-Lund wrote:
>> Do you really think it's better to unconditionally take down the
>> entire process with an error, instead of having a relatively small
>> chance of stuff blowing up without any sensible error? I'm not 100%
>> convinced - but let's hope we'll find a proper fix.
>
> "relatively small chance of stuff blowing up"? The docs of
> TerminateThread: "... the kernel32 state for the thread's process could be
> inconsistent." That's scary if we are talking about a process that should run
> for days or weeks without interruption.
>

I think there's a misunderstanding here. I thought your suggestion was
to simply call die(), which would take down the main process. After
reading this explanation, I think you're talking about giving an error
and rejecting the connection instead. Which makes more sense than to
risk crashing the main-process, indeed.

> The reason why we are killing a thread is to prevent keeping lots of
> connections open (to the same IP address). There are two situations to take
> care of:
>
> 1. We are in a lengthy computation without paying attention to the socket.
>
> 2. The client does not send or accept data for a long time.
>
> Case 1 could happen if upload-pack is "counting objects" on a large
> repository. We would need some way to kill upload-pack. Since it is a
> separate process anyway, we could use TerminateProcess().
>

Makes sense. I'll play around a bit with this and see what I come up with.

> Case 2 could be achieved by using setsockopt() with SO_RCVTIMEO and
> SO_SNDTIMEO and a tiny timeout. But notice that we would set a timeout in one
> thread while another thread is waiting in ReadFile() or WriteFile(). Would
> that work?
>

I think it should work fine, but I won't give you a guarantee ;)
Perhaps we should have a configurable global max timeout, and just set
that on all sockets? Or does this open for DDOS attacks?

Anyway, thanks for the sanity :)

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Johannes Sixt]

On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:
> On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > "relatively small chance of stuff blowing up"? The docs of
> > TerminateThread: "... the kernel32 state for the thread's process could
> > be inconsistent." That's scary if we are talking about a process that
> > should run for days or weeks without interruption.
>
> I think there's a misunderstanding here. I thought your suggestion was
> to simply call die(), which would take down the main process. After
> reading this explanation, I think you're talking about giving an error
> and rejecting the connection instead. Which makes more sense than to
> risk crashing the main-process, indeed.

Just rejecting a connection is certainly the simplest do to keep the daemon 
process alive. But the server can be DoS-ed from a single source IP.

Currently git-daemon can only be DDoS-ed because there is a maximum number of 
connections, which are not closed if all of them originate from different 
IPs.

> > Case 2 could be achieved by using setsockopt() with SO_RCVTIMEO and
> > SO_SNDTIMEO and a tiny timeout. But notice that we would set a timeout in
> > one thread while another thread is waiting in ReadFile() or WriteFile().
> > Would that work?
>
> I think it should work fine, but I won't give you a guarantee ;)
> Perhaps we should have a configurable global max timeout, and just set
> that on all sockets? Or does this open for DDOS attacks?

I'm sure that there is a global timeout already, but it is in the order of 
minutes, which is too long. Here I mean it to be set to zero or one 
milli-second so that the connection closes right away - as if the process on 
the server side had been killed.

Hm - perhaps it is possible to really *close* the socket while some other 
thread waits in ReadFile() or WriteFile()?

-- Hannes

Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Erik Faye-Lund]

On Wed, Dec 2, 2009 at 8:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:
>> On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> > "relatively small chance of stuff blowing up"? The docs of
>> > TerminateThread: "... the kernel32 state for the thread's process could
>> > be inconsistent." That's scary if we are talking about a process that
>> > should run for days or weeks without interruption.
>>
>> I think there's a misunderstanding here. I thought your suggestion was
>> to simply call die(), which would take down the main process. After
>> reading this explanation, I think you're talking about giving an error
>> and rejecting the connection instead. Which makes more sense than to
>> risk crashing the main-process, indeed.
>
> Just rejecting a connection is certainly the simplest do to keep the daemon
> process alive. But the server can be DoS-ed from a single source IP.
>
> Currently git-daemon can only be DDoS-ed because there is a maximum number of
> connections, which are not closed if all of them originate from different
> IPs.
>

After some testing I've found that git-daemon can very much be DoS-ed
from a single IP in it's current form. This is for two reasons:
1) The clever xcalloc + memcmp trick has a fault; the port for each
connection is different, so there will never be a match. I have a
patch[1] for this that I plan to send out soon.
2) Even with this patch the effect of the DoS-protection is kind of
limited. This is because it's a child process of the fork()'d process
again that does all the heavy lifting, and kill(pid, SIGHUP) doesn't
kill child processes. So, the connection gets to continue the action
until upload-pack (or whatever the current command is) finish. This
might be quite lengthy.

As I said, I have a patch for 1), but I don't quite know how to fix
2). Perhaps this is a good use for process groups? I'm a Windows-guy;
my POSIX isn't exactly super-awesome...

I found these issues during my latest effort to port git-daemon to
Windows. I managed to get this to work fine on Windows, by
implementing a kill(x, SIGTERM) that terminated child-processes
(because I was under the impression that this was what happened... I
guess daemon.c lead me to believe that).

[1]: http://repo.or.cz/w/git/kusma.git/commit/b1d286d32f42c57b90a1db9b7b8d6775a5d1ad7b

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Erik Faye-Lund]

On Sat, Jan 9, 2010 at 1:49 AM, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Wed, Dec 2, 2009 at 8:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:
>>> On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>> > "relatively small chance of stuff blowing up"? The docs of
>>> > TerminateThread: "... the kernel32 state for the thread's process could
>>> > be inconsistent." That's scary if we are talking about a process that
>>> > should run for days or weeks without interruption.
>>>
>>> I think there's a misunderstanding here. I thought your suggestion was
>>> to simply call die(), which would take down the main process. After
>>> reading this explanation, I think you're talking about giving an error
>>> and rejecting the connection instead. Which makes more sense than to
>>> risk crashing the main-process, indeed.
>>
>> Just rejecting a connection is certainly the simplest do to keep the daemon
>> process alive. But the server can be DoS-ed from a single source IP.
>>
>> Currently git-daemon can only be DDoS-ed because there is a maximum number of
>> connections, which are not closed if all of them originate from different
>> IPs.
>>
>
> After some testing I've found that git-daemon can very much be DoS-ed
> from a single IP in it's current form. This is for two reasons:
> 1) The clever xcalloc + memcmp trick has a fault; the port for each
> connection is different, so there will never be a match. I have a
> patch[1] for this that I plan to send out soon.
> 2) Even with this patch the effect of the DoS-protection is kind of
> limited. This is because it's a child process of the fork()'d process
> again that does all the heavy lifting, and kill(pid, SIGHUP) doesn't
> kill child processes. So, the connection gets to continue the action
> until upload-pack (or whatever the current command is) finish. This
> might be quite lengthy.
>

Actually, this isn't the end of the story, there's an additional issue
that happens if 1) doesn't:
In commit 02d57da (Be slightly smarter about git-daemon client
shutdown), Linus adds the following hunk:

@@ -26,6 +26,12 @@ static int upload(char *dir, int dirlen)
            access("HEAD", R_OK))
                return -1;

+       /*
+        * We'll ignore SIGTERM from now on, we have a
+        * good client.
+        */
+       signal(SIGTERM, SIG_IGN);
+
        /* git-upload-pack only ever reads stuff, so this is safe */
        execlp("git-upload-pack", "git-upload-pack", ".", NULL);
        return -1;

This is fine, because he also makes sure to first try to kill with
SIGTERM, and then fall back to SIGKILL if that failed. However,
Stephen later adds commit 3bd62c2 ("git-daemon: rewrite kindergarden,
new option --max-connections"), and here he leaves the hunk quoted
above, but removes the SIGKILL code-path. The consequence is that the
forked process doesn't die at all any more.

IMO, Stephen did kind-of right in removing the SIGKILL code-path,
since we don't kill just a random child anymore. But leaving the
SIGTERM-ignoring on looks like a bug to me.

Now, if that was fixed alone, I think we'd suffer even worse, due to
2) - since the child processes aren't killed, we'd end up allowing
more connections than the value of max_connections - upload-pack would
gladly continue in the background. Some quick testing showed that the
following patch solved the issue for me. I'm not very happy about it,
since I'm working on porting the code to Windows, and we don't have
the same process-group concept on windows... Oh well.

diff --git a/daemon.c b/daemon.c
index 918e560..bc6874c 100644
--- a/daemon.c
+++ b/daemon.c
@@ -291,12 +291,6 @@ static int run_service(char *dir, struct
daemon_service *service)
                return -1;
        }

-       /*
-        * We'll ignore SIGTERM from now on, we have a
-        * good client.
-        */
-       signal(SIGTERM, SIG_IGN);
-
        return service->fn();
 }

@@ -633,7 +627,8 @@ static void kill_some_child(void)

        for (; (next = blanket->next); blanket = next)
                if (!addrcmp(&blanket->address, &next->address)) {
-                       kill(blanket->pid, SIGTERM);
+                       kill(-blanket->pid, SIGTERM);
                        break;
                }
 }
@@ -676,7 +671,8 @@ static void handle(int incoming, struct sockaddr
*addr, int addrlen)

                add_child(pid, addr, addrlen);
                return;
-       }
+       } else
+               setpgid(0, 0);

        dup2(incoming, 0);
        dup2(incoming, 1);

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog

[Johannes Sixt]

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> +void syslog(int priority, const char *fmt, ...) {

Style: Brace goes to next line.

> +	struct strbuf msg;
> +	va_list va;
> +	WORD logtype;
> +
> +	strbuf_init(&msg, 0);
> +	va_start(va, fmt);
> +	strbuf_vaddf(&msg, fmt, va);
> +	va_end(va);

I would

	const char* arg;
	if (strcmp(fmt, "%s"))
		die("format string of syslog() not implemented")
	va_start(va, fmt);
	arg = va_arg(va, char*);
	va_end(va);

because we have exactly one invocation of syslog(), which passes "%s" as 
format string. Then strbuf_vaddf is not needed. Or even simpler: declare the 
function as

void syslog(int priority, const char *fmt, const char*arg);

> +	ReportEventA(ms_eventlog,
> +	    logtype,
> +	    (WORD)NULL,
> +	    (DWORD)NULL,
> +	    NULL,
> +	    1,
> +	    0,
> +	    (const char **)&msg.buf,
> +	    NULL);

Why do you pass NULL and apply casts? The first one gives a warning.

Did you read this paragraph in the documentation?
http://msdn.microsoft.com/en-us/library/aa363679(VS.85).aspx

"Note that the string that you log cannot contain %n, where n is an integer 
value (for example, %1) because the event viewer treats it as an insertion 
string. ..."

How are the chances that this condition applies to our use of the function?

-- Hannes

Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog

[Erik Faye-Lund]

On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>> +     struct strbuf msg;
>> +     va_list va;
>> +     WORD logtype;
>> +
>> +     strbuf_init(&msg, 0);
>> +     va_start(va, fmt);
>> +     strbuf_vaddf(&msg, fmt, va);
>> +     va_end(va);
>
> I would
>
>        const char* arg;
>        if (strcmp(fmt, "%s"))
>                die("format string of syslog() not implemented")
>        va_start(va, fmt);
>        arg = va_arg(va, char*);
>        va_end(va);
>
> because we have exactly one invocation of syslog(), which passes "%s" as
> format string. Then strbuf_vaddf is not needed. Or even simpler: declare the
> function as
>
> void syslog(int priority, const char *fmt, const char*arg);

After reading this again, I agree that this is the best solution. I'll
update for the next iteration.

>> +     ReportEventA(ms_eventlog,
>> +         logtype,
>> +         (WORD)NULL,
>> +         (DWORD)NULL,
>> +         NULL,
>> +         1,
>> +         0,
>> +         (const char **)&msg.buf,
>> +         NULL);
>
> Why do you pass NULL and apply casts? The first one gives a warning.

I'll remove the casts for the next iteration.

>
> Did you read this paragraph in the documentation?
> http://msdn.microsoft.com/en-us/library/aa363679(VS.85).aspx
>
> "Note that the string that you log cannot contain %n, where n is an integer
> value (for example, %1) because the event viewer treats it as an insertion
> string. ..."
>
> How are the chances that this condition applies to our use of the function?
>

Ugh, increasingly high since we're adding IPv6 support, I guess.
Perhaps some form of escaping needs to be done?

-- 
Erik "kusma" Faye-Lund

Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog

[Johannes Sixt]

On Freitag, 27. November 2009, Erik Faye-Lund wrote:
> On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > I would
> >
> >        const char* arg;
> >        if (strcmp(fmt, "%s"))
> >                die("format string of syslog() not implemented")
> >        va_start(va, fmt);
> >        arg = va_arg(va, char*);
> >        va_end(va);
> >
> > because we have exactly one invocation of syslog(), which passes "%s" as
> > format string. Then strbuf_vaddf is not needed. Or even simpler: declare
> > the function as
> >
> > void syslog(int priority, const char *fmt, const char*arg);
>
> After reading this again, I agree that this is the best solution. I'll
> update for the next iteration.

Except that you shouldn't die like I proposed because here we are already in 
the die_routine, no?

> > "Note that the string that you log cannot contain %n, where n is an
> > integer value (for example, %1) because the event viewer treats it as an
> > insertion string. ..."
> >
> > How are the chances that this condition applies to our use of the
> > function?
>
> Ugh, increasingly high since we're adding IPv6 support, I guess.
> Perhaps some form of escaping needs to be done?

I think so, but actually I have no clue.

-- Hannes

Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog

[Erik Faye-Lund]

On Fri, Nov 27, 2009 at 8:23 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Freitag, 27. November 2009, Erik Faye-Lund wrote:
>> On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> > I would
>> >
>> >        const char* arg;
>> >        if (strcmp(fmt, "%s"))
>> >                die("format string of syslog() not implemented")
>> >        va_start(va, fmt);
>> >        arg = va_arg(va, char*);
>> >        va_end(va);
>> >
>> > because we have exactly one invocation of syslog(), which passes "%s" as
>> > format string. Then strbuf_vaddf is not needed. Or even simpler: declare
>> > the function as
>> >
>> > void syslog(int priority, const char *fmt, const char*arg);
>>
>> After reading this again, I agree that this is the best solution. I'll
>> update for the next iteration.
>
> Except that you shouldn't die like I proposed because here we are already in
> the die_routine, no?
>

Might be. Either way, I think loosing a log-entry is better than
taking down the server. I'm just doing a warning() and return. If
we're lucky, the warning gets somewhere. If not, well, something
happened and no one was around to see it.

>> > "Note that the string that you log cannot contain %n, where n is an
>> > integer value (for example, %1) because the event viewer treats it as an
>> > insertion string. ..."
>> >
>> > How are the chances that this condition applies to our use of the
>> > function?
>>
>> Ugh, increasingly high since we're adding IPv6 support, I guess.
>> Perhaps some form of escaping needs to be done?
>
> I think so, but actually I have no clue.
>

Bah, according to Microsoft Support (1), there's no simple way to
escape this. I'm tempted to leave this bug in, and rather try to fix
the symptoms when/if they start popping up. Unless someone else comes
up with something better, that is.

(1) http://support.microsoft.com/kb/934640

-- 
Erik "kusma" Faye-Lund

Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Junio C Hamano]

Erik Faye-Lund <kusmabite@googlemail.com> writes:

> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
>  {
>  	int len;
>  
>  	if (!strbuf_avail(sb))
>  		strbuf_grow(sb, 64);
>  	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>  	if (len < 0)
>  		die("your vsnprintf is broken");
>  	if (len > strbuf_avail(sb)) {
>  		strbuf_grow(sb, len);
>  		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>  		if (len > strbuf_avail(sb)) {
>  			die("this should not happen, your snprintf is broken");
>  		}

Hmm, I would have expected to see va_copy() somewhere in the patch text.
Is it safe to reuse ap like this in two separate invocations of
vsnprintf()?

Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Erik Faye-Lund]

On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>
>> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
>>  {
>>       int len;
>>
>>       if (!strbuf_avail(sb))
>>               strbuf_grow(sb, 64);
>>       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>       if (len < 0)
>>               die("your vsnprintf is broken");
>>       if (len > strbuf_avail(sb)) {
>>               strbuf_grow(sb, len);
>>               len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>               if (len > strbuf_avail(sb)) {
>>                       die("this should not happen, your snprintf is broken");
>>               }
>
> Hmm, I would have expected to see va_copy() somewhere in the patch text.
> Is it safe to reuse ap like this in two separate invocations of
> vsnprintf()?
>

I think your expectation is well justified, this seems to be a
portability-bug waiting to happen. Sorry for missing this prior to
sending out - on Windows this is known to work, and this function is
currently only used from the Windows implementation of syslog.

How kosher is it to use va_copy in the git-core, considering that it's
C99? A quick grep reveals only one occurrence of va_copy in the
source, and that's in compat/winansi.c. Searching the history of next
reveals that Alex Riesen (CC'd) already removed one occurrence
(4bf5383), so I'm starting to get slightly scared it might not be OK.

In practice it seems that something like the following works
portably-enough for many applications, dunno if it's something we'll
be happy with:
#ifndef va_copy
#define va_copy(a,b) ((a) = (b))
#endif

-- 
Erik "kusma" Faye-Lund

Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Paolo Bonzini]

On 11/26/2009 11:38 AM, Erik Faye-Lund wrote:
> In practice it seems that something like the following works
> portably-enough for many applications, dunno if it's something we'll
> be happy with:
> #ifndef va_copy
> #define va_copy(a,b) ((a) = (b))
> #endif

Yes, this is correct.

Paolo

Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Junio C Hamano]

Erik Faye-Lund <kusmabite@googlemail.com> writes:

> On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>>
>>> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
>>>  {
>>>       int len;
>>>
>>>       if (!strbuf_avail(sb))
>>>               strbuf_grow(sb, 64);
>>>       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>>       if (len < 0)
>>>               die("your vsnprintf is broken");
>>>       if (len > strbuf_avail(sb)) {
>>>               strbuf_grow(sb, len);
>>>               len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>>               if (len > strbuf_avail(sb)) {
>>>                       die("this should not happen, your snprintf is broken");
>>>               }
>>
>> Hmm, I would have expected to see va_copy() somewhere in the patch text.
>> Is it safe to reuse ap like this in two separate invocations of
>> vsnprintf()?
>
> I think your expectation is well justified, this seems to be a
> portability-bug waiting to happen. Sorry for missing this prior to
> sending out - on Windows this is known to work, and this function is
> currently only used from the Windows implementation of syslog.
>
> How kosher is it to use va_copy in the git-core, considering that it's
> C99? A quick grep reveals only one occurrence of va_copy in the
> source, and that's in compat/winansi.c. Searching the history of next
> reveals that Alex Riesen (CC'd) already removed one occurrence
> (4bf5383), so I'm starting to get slightly scared it might not be OK.

We tend to avoid C99 features and it saved us in a few occasions.  Recent
MSVC port revealed that we still had a handful of decl-after-statments but
luckily the necessary fix-ups were minimal because I have been reasonably
careful to reject patches that add it long before MSVC port happened.

> In practice it seems that something like the following works
> portably-enough for many applications, dunno if it's something we'll
> be happy with:
> #ifndef va_copy
> #define va_copy(a,b) ((a) = (b))
> #endif

Since an obvious implementation of va_list would be to make it a pointer
into the stack frame, doing the above would work on many systems.  On
esoteric systems that needs something different (e.g. where va_list is
implemented as a size-1 array of pointers, va_copy(a,b) needs to be an
assignment (*(a) = *(b))), people can add compatibility macro later.

Historically some systems that do have a suitable implementation had it
under the name __va_copy() instead, so it would have been better to define
it as something like:

    #ifndef va_copy
    # ifdef __va_copy
    # define va_copy(a,b) __va_copy(a,b)
    # else
    # /* fallback for the most obvious implementation of va_list */
    # define va_copy(a,b) ((a) = (b))
    # endif
    #endif

But I do not know it still matters in practice anymore.

Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Erik Faye-Lund]

On Thu, Nov 26, 2009 at 7:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>> In practice it seems that something like the following works
>> portably-enough for many applications, dunno if it's something we'll
>> be happy with:
>> #ifndef va_copy
>> #define va_copy(a,b) ((a) = (b))
>> #endif
>
> Since an obvious implementation of va_list would be to make it a pointer
> into the stack frame, doing the above would work on many systems.  On
> esoteric systems that needs something different (e.g. where va_list is
> implemented as a size-1 array of pointers, va_copy(a,b) needs to be an
> assignment (*(a) = *(b))), people can add compatibility macro later.
>
> Historically some systems that do have a suitable implementation had it
> under the name __va_copy() instead, so it would have been better to define
> it as something like:
>
>    #ifndef va_copy
>    # ifdef __va_copy
>    # define va_copy(a,b) __va_copy(a,b)
>    # else
>    # /* fallback for the most obvious implementation of va_list */
>    # define va_copy(a,b) ((a) = (b))
>    # endif
>    #endif
>
> But I do not know it still matters in practice anymore.

Perhaps I can do one better: use memcpy instead of standard
assignment. The Autoconf manual[1] suggests that it's more portable.
Something like this:

#ifndef va_copy
# ifdef __va_copy
#  define va_copy(a,b) __va_copy(a,b)
# else
#  define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))
# endif
#endif

I'll add this to git-compat-util.h this for the next round unless
someone yells really loud at me.

*[1] http://www.gnu.org/software/hello/manual/autoconf/Function-Portability.html#index-g_t_0040code_007bva_005fcopy_007d-357
-- 
Erik "kusma" Faye-Lund

Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Johannes Sixt]

Erik Faye-Lund schrieb:
> Perhaps I can do one better: use memcpy instead of standard
> assignment. The Autoconf manual[1] suggests that it's more portable.
> Something like this:
> 
> #ifndef va_copy
> # ifdef __va_copy
> #  define va_copy(a,b) __va_copy(a,b)
> # else
> #  define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))
> # endif
> #endif
> 
> I'll add this to git-compat-util.h this for the next round unless
> someone yells really loud at me.

As I said elsewhere in the thread, I do not see enough reason to add
strbuf_vaddf() only for a syslog() emulation in the first place.

-- Hannes

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Martin Storsjö]

Hi,

First of all, great that you're working on adding daemon support for 
windows!

On Thu, 26 Nov 2009, Erik Faye-Lund wrote:

> +static void wsa_init(void)
>  {
> +	static int initialized = 0;
>  	WSADATA wsa;
>  
> +	if (initialized)
> +		return;
> +
>  	if (WSAStartup(MAKEWORD(2,2), &wsa))
>  		die("unable to initialize winsock subsystem, error %d",
>  			WSAGetLastError());
>  	atexit((void(*)(void)) WSACleanup);
> +	initialized = 1;
> +}

Something similar to this was merged into master recently as part of my 
mingw/ipv6 patches, so by rebasing your patch on top of that, this patch 
will probably get a bit smaller.

Also, the getaddrinfo-compatibility wrappers perhaps may need some minor 
updates to handle the use cases needed for setting up listening sockets.

// Martin

[PATCH] Improve the mingw getaddrinfo stub to handle more use cases

[Martin Storsjö]

Allow the node parameter to be null, which is used for getting
the default bind address.

Also allow the hints parameter to be null, to improve standard
conformance of the stub implementation a little.

Signed-off-by: Martin Storsjo <martin@martin.st>
---

This patch adds support for the getaddrinfo parameters used by git-daemon, 
as mentioned earlier.

 compat/mingw.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 0653560..17d1314 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -913,19 +913,22 @@ static int WSAAPI getaddrinfo_stub(const char *node, const char *service,
 				   const struct addrinfo *hints,
 				   struct addrinfo **res)
 {
-	struct hostent *h = gethostbyname(node);
+	struct hostent *h = NULL;
 	struct addrinfo *ai;
 	struct sockaddr_in *sin;
 
-	if (!h)
-		return WSAGetLastError();
+	if (node) {
+		h = gethostbyname(node);
+		if (!h)
+			return WSAGetLastError();
+	}
 
 	ai = xmalloc(sizeof(struct addrinfo));
 	*res = ai;
 	ai->ai_flags = 0;
 	ai->ai_family = AF_INET;
-	ai->ai_socktype = hints->ai_socktype;
-	switch (hints->ai_socktype) {
+	ai->ai_socktype = hints ? hints->ai_socktype : 0;
+	switch (ai->ai_socktype) {
 	case SOCK_STREAM:
 		ai->ai_protocol = IPPROTO_TCP;
 		break;
@@ -937,14 +940,17 @@ static int WSAAPI getaddrinfo_stub(const char *node, const char *service,
 		break;
 	}
 	ai->ai_addrlen = sizeof(struct sockaddr_in);
-	ai->ai_canonname = strdup(h->h_name);
+	ai->ai_canonname = h ? strdup(h->h_name) : NULL;
 
 	sin = xmalloc(ai->ai_addrlen);
 	memset(sin, 0, ai->ai_addrlen);
 	sin->sin_family = AF_INET;
 	if (service)
 		sin->sin_port = htons(atoi(service));
-	sin->sin_addr = *(struct in_addr *)h->h_addr;
+	if (h)
+		sin->sin_addr = *(struct in_addr *)h->h_addr;
+	else
+		sin->sin_addr.s_addr = INADDR_ANY;
 	ai->ai_addr = (struct sockaddr *)sin;
 	ai->ai_next = 0;
 	return 0;
-- 
1.6.4.4

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Erik Faye-Lund]

On Thu, Nov 26, 2009 at 9:24 AM, Martin Storsjö <martin@martin.st> wrote:
> Hi,
>
> First of all, great that you're working on adding daemon support for
> windows!

Thanks. I meant to send this out a couple of weeks ago, but I wasn't
able to find the time until now.

Also, I wouldn't have come this far without going tired of it without
Mike's patches, so some credit should go to him for doing good initial
work!

> On Thu, 26 Nov 2009, Erik Faye-Lund wrote:
>
>> +static void wsa_init(void)
>>  {
>> +     static int initialized = 0;
>>       WSADATA wsa;
>>
>> +     if (initialized)
>> +             return;
>> +
>>       if (WSAStartup(MAKEWORD(2,2), &wsa))
>>               die("unable to initialize winsock subsystem, error %d",
>>                       WSAGetLastError());
>>       atexit((void(*)(void)) WSACleanup);
>> +     initialized = 1;
>> +}
>
> Something similar to this was merged into master recently as part of my
> mingw/ipv6 patches, so by rebasing your patch on top of that, this patch
> will probably get a bit smaller.

Yeah, I saw your patches, and realized that I needed to rebase my work
at some point, but none of the repos I usually pull from seems to
contain the patches yet. Rebasing will be a requirement before this
can be applied for sure.

>
> Also, the getaddrinfo-compatibility wrappers perhaps may need some minor
> updates to handle the use cases needed for setting up listening sockets.

I expect you're referring to IPv6 support in the wrappers this patch
adds? Unfortunately IPv6 isn't something I'm very familiar with, but
I'll give it a go unless someone else provides some patches...

-- 
Erik "kusma" Faye-Lund

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Martin Storsjö]

On Thu, 26 Nov 2009, Erik Faye-Lund wrote:

> Yeah, I saw your patches, and realized that I needed to rebase my work
> at some point, but none of the repos I usually pull from seems to
> contain the patches yet. Rebasing will be a requirement before this
> can be applied for sure.

Ok, great! I tried applying it on the latest master, and it wasn't too 
much of work.

> > Also, the getaddrinfo-compatibility wrappers perhaps may need some minor
> > updates to handle the use cases needed for setting up listening sockets.
> 
> I expect you're referring to IPv6 support in the wrappers this patch
> adds? Unfortunately IPv6 isn't something I'm very familiar with, but
> I'll give it a go unless someone else provides some patches...

No, sorry for being unclear.

When IPv6 is enabled, name lookups go through getaddrinfo instead of 
gethostbyname. Since getaddrinfo isn't available on Win2k (and switching 
between getaddrinfo/gethostbyname happens at compile time when IPv6 is 
enabled), we have to provide a small getaddrinfo stub, implemented in 
terms of gethostbyname. This currently implements only parts of the 
getaddrinfo interface - enough for the way getaddrinfo was used this far.

git-daemon uses getaddrinfo in a slightly different way (for setting up 
listening sockets), and thus uses parameters that our current getaddrinfo 
stub doesn't support. The patch I sent to this thread a moment ago adds 
support for the way git-daemon uses getaddrinfo.


I tested this patch series on top of the latest master, with IPv6 support, 
and found a slight problem caused by the IPv6 support. If IPv6 isn't 
enabled, git-daemon always listens on one single socket, otherwise it may 
listen on two separate sockets, one for v4 and one for v6.

This causes problems with the mingw poll() replacement, which has a 
special case for polling one single fd - otherwise it tries to use some 
emulation that currently only works for pipes. I didn't try to make any 
proper fix for this though. I tested git-daemon by hacking it to listen on 
only one of the sockets, and that worked well for both v4 and v6.


So, in addition to the getaddrinfo patch I sent, the mingw poll() 
replacement needs some updates to handle polling multiple sockets. Except 
from that, things seem to work, at a quick glance.

// Martin

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Erik Faye-Lund]

(CC'ing Hannes, as he seems to be the original author of our poll()-emulation)

On Thu, Nov 26, 2009 at 12:03 PM, Martin Storsjö <martin@martin.st> wrote:
> On Thu, 26 Nov 2009, Erik Faye-Lund wrote:
>
>> Yeah, I saw your patches, and realized that I needed to rebase my work
>> at some point, but none of the repos I usually pull from seems to
>> contain the patches yet. Rebasing will be a requirement before this
>> can be applied for sure.
>
> Ok, great! I tried applying it on the latest master, and it wasn't too
> much of work.

I've finally gotten around to doing the same now, thanks for the heads up :)

> This causes problems with the mingw poll() replacement, which has a
> special case for polling one single fd - otherwise it tries to use some
> emulation that currently only works for pipes. I didn't try to make any
> proper fix for this though. I tested git-daemon by hacking it to listen on
> only one of the sockets, and that worked well for both v4 and v6.
>
>
> So, in addition to the getaddrinfo patch I sent, the mingw poll()
> replacement needs some updates to handle polling multiple sockets. Except
> from that, things seem to work, at a quick glance.
>

Thanks a lot for helping out! I'm seeing the same issue here when
running on Vista:
$ ./git-daemon.exe --base-path=/c/Users/Erik/src/ --export-all
error: PeekNamedPipe failed, GetLastError: 1
[3656] Poll failed, resuming: Invalid argument
error: PeekNamedPipe failed, GetLastError: 1
[3656] Poll failed, resuming: Invalid argument
...

If I hack the poll()-emulation to never enter the PeekNamedPipe
code-path seems to make it work with IPv6 here, but this is obviously
not the correct solution as it breaks pipe-polling:
--->8---
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -360,7 +360,7 @@ int poll(struct pollfd *ufds, unsigned int nfds, int timeout
         * input is available and let the actual wait happen when the
         * caller invokes read().
         */
-       if (nfds == 1) {
+       if (1) {
                if (!(ufds[0].events & POLLIN))
                        return errno = EINVAL, error("POLLIN not set");
                ufds[0].revents = POLLIN;
--->8---

I'm not very familiar with poll(), but if I understand the man-pages
correctly it's waiting for events on file descriptors, and is in our
case used to check for incoming connections, right? If so, I see three
possible ways forward: (1) extending our poll()-emulation to handle
multiple sockets, (2) change daemon.c to check one socket at the time,
and (3) using select() instead of poll().

(1) seems like the "correct" but tricky thing to do, (2) like the
"easy" but nasty thing to do. However, (3) strikes me as the least
dangerous thing to do ;)

For (1), there's also a WSAPoll() function in Windows, but I'm not
sure how to figure out if an fd is a socket or a pipe. There's also
WaitForMultipleObjects.

Here's a quick stab at (3). It seems to work fine under Windows. Not
tested on other platforms, though.

--->8---
--- a/daemon.c
+++ b/daemon.c
@@ -812,26 +812,22 @@ static int socksetup(char *listen_addr, int listen_port, i
nt **socklist_p)

 static int service_loop(int socknum, int *socklist)
 {
-       struct pollfd *pfd;
-       int i;
-
-       pfd = xcalloc(socknum, sizeof(struct pollfd));
-
-       for (i = 0; i < socknum; i++) {
-               pfd[i].fd = socklist[i];
-               pfd[i].events = POLLIN;
-       }
-
        signal(SIGCHLD, child_handler);

        for (;;) {
                int i;
+               fd_set fds;
+               struct timeval timeout = { 0, 0 };

                check_dead_children();

-               if (poll(pfd, socknum, -1) < 0) {
+               FD_ZERO(&fds);
+               for (i = 0; i < socknum; i++)
+                       FD_SET(socklist[i], &fds);
+
+               if (select(0, &fds, NULL, NULL, &timeout) > 0) {
                        if (errno != EINTR) {
-                               logerror("Poll failed, resuming: %s",
+                               logerror("select() failed, resuming: %s",
                                      strerror(errno));
                                sleep(1);
                        }
@@ -839,10 +835,10 @@ static int service_loop(int socknum, int *socklist)
                }

                for (i = 0; i < socknum; i++) {
-                       if (pfd[i].revents & POLLIN) {
+                       if (FD_ISSET(socklist[i], &fds)) {
                                struct sockaddr_storage ss;
                                socklen_t sslen = sizeof(ss);
-                               int incoming = accept(pfd[i].fd,
(struct sockaddr *)&ss, &sslen);
+                               int incoming = accept(socklist[i],
(struct sockaddr *)&ss, &sslen);
                                if (incoming < 0) {
                                        switch (errno) {
                                        case EAGAIN:
@@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)
                                        }
                                }
                                handle(incoming, (struct sockaddr *)&ss, sslen);

+                               break;
                        }
                }
        }
--->8---

-- 
Erik "kusma" Faye-Lund

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Martin Storsjö]

On Wed, 2 Dec 2009, Erik Faye-Lund wrote:

> I'm not very familiar with poll(), but if I understand the man-pages
> correctly it's waiting for events on file descriptors, and is in our
> case used to check for incoming connections, right? If so, I see three
> possible ways forward: (1) extending our poll()-emulation to handle
> multiple sockets, (2) change daemon.c to check one socket at the time,
> and (3) using select() instead of poll().
> 
> (1) seems like the "correct" but tricky thing to do, (2) like the
> "easy" but nasty thing to do. However, (3) strikes me as the least
> dangerous thing to do ;)

Generally, poll is a better API than select, since select is limited to fd 
values up to (about) 1000, depending on the implementation. (This is due 
to the fact that fd_set is a fixed size bit set with a bit for each 
possible fd.)

But since we're only doing select on the handful of sockets that were 
opened initially in the process (so these should receive low numbers), 
this should only be a theoretical concern.

> For (1), there's also a WSAPoll() function in Windows, but I'm not
> sure how to figure out if an fd is a socket or a pipe. There's also
> WaitForMultipleObjects.
> 
> Here's a quick stab at (3). It seems to work fine under Windows. Not
> tested on other platforms, though.

A few comments below, just by reading through this, didn't test it yet:

> --->8---
> --- a/daemon.c
> +++ b/daemon.c
> @@ -812,26 +812,22 @@ static int socksetup(char *listen_addr, int listen_port, i
> nt **socklist_p)
> 
>  static int service_loop(int socknum, int *socklist)
>  {
> -       struct pollfd *pfd;
> -       int i;
> -
> -       pfd = xcalloc(socknum, sizeof(struct pollfd));
> -
> -       for (i = 0; i < socknum; i++) {
> -               pfd[i].fd = socklist[i];
> -               pfd[i].events = POLLIN;
> -       }
> -
>         signal(SIGCHLD, child_handler);
> 
>         for (;;) {
>                 int i;
> +               fd_set fds;
> +               struct timeval timeout = { 0, 0 };
> 
>                 check_dead_children();
> 
> -               if (poll(pfd, socknum, -1) < 0) {
> +               FD_ZERO(&fds);
> +               for (i = 0; i < socknum; i++)
> +                       FD_SET(socklist[i], &fds);
> +
> +               if (select(0, &fds, NULL, NULL, &timeout) > 0) {
>                         if (errno != EINTR) {

The first parameter to select should be max(all fds set in the fd_set) + 
1, this should be trivial enough to determine in the loop above where the 
fd:s are added with FD_SET.

You're calling select() with a zero timeout - I'd guess this chews quite a 
bit of cpu just looping around doing nothing? If the last parameter would 
be set to NULL, it would wait indefinitely, just like the previous poll 
loop did.

> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)
>                                         }
>                                 }
>                                 handle(incoming, (struct sockaddr *)&ss, sslen);
> 
> +                               break;

What's this good for?

Other than that, this looks quite good to me, at a first glance.

// Martin

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Erik Faye-Lund]

On Wed, Dec 2, 2009 at 2:21 PM, Martin Storsjö <martin@martin.st> wrote:
> On Wed, 2 Dec 2009, Erik Faye-Lund wrote:
>
>> I'm not very familiar with poll(), but if I understand the man-pages
>> correctly it's waiting for events on file descriptors, and is in our
>> case used to check for incoming connections, right? If so, I see three
>> possible ways forward: (1) extending our poll()-emulation to handle
>> multiple sockets, (2) change daemon.c to check one socket at the time,
>> and (3) using select() instead of poll().
>>
>> (1) seems like the "correct" but tricky thing to do, (2) like the
>> "easy" but nasty thing to do. However, (3) strikes me as the least
>> dangerous thing to do ;)
>
> Generally, poll is a better API than select, since select is limited to fd
> values up to (about) 1000, depending on the implementation. (This is due
> to the fact that fd_set is a fixed size bit set with a bit for each
> possible fd.)
>
> But since we're only doing select on the handful of sockets that were
> opened initially in the process (so these should receive low numbers),
> this should only be a theoretical concern.
>

Yeah. In our case, I guess it's a maximum of two times the number of
network adapters installed, so I think we should be relatively safe.

>
>> --->8---
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -812,26 +812,22 @@ static int socksetup(char *listen_addr, int listen_port, i
>> nt **socklist_p)
>>
>>  static int service_loop(int socknum, int *socklist)
>>  {
>> -       struct pollfd *pfd;
>> -       int i;
>> -
>> -       pfd = xcalloc(socknum, sizeof(struct pollfd));
>> -
>> -       for (i = 0; i < socknum; i++) {
>> -               pfd[i].fd = socklist[i];
>> -               pfd[i].events = POLLIN;
>> -       }
>> -
>>         signal(SIGCHLD, child_handler);
>>
>>         for (;;) {
>>                 int i;
>> +               fd_set fds;
>> +               struct timeval timeout = { 0, 0 };
>>
>>                 check_dead_children();
>>
>> -               if (poll(pfd, socknum, -1) < 0) {
>> +               FD_ZERO(&fds);
>> +               for (i = 0; i < socknum; i++)
>> +                       FD_SET(socklist[i], &fds);
>> +
>> +               if (select(0, &fds, NULL, NULL, &timeout) > 0) {
>>                         if (errno != EINTR) {
>
> The first parameter to select should be max(all fds set in the fd_set) +
> 1, this should be trivial enough to determine in the loop above where the
> fd:s are added with FD_SET.
>

Yeah, thanks for pointing out. I did a little bit of searching, and it
seems like "most other people passes zero, and it just works".
However, we should be nice to systems where it doesn't "just work", so
I'll fix this before sending it out.

> You're calling select() with a zero timeout - I'd guess this chews quite a
> bit of cpu just looping around doing nothing? If the last parameter would
> be set to NULL, it would wait indefinitely, just like the previous poll
> loop did.
>

Ah, yes. That's much nicer!

>> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)
>>                                         }
>>                                 }
>>                                 handle(incoming, (struct sockaddr *)&ss, sslen);
>>
>> +                               break;
>
> What's this good for?
>

When I clone git://localhost/some-repo, select() returns a fd-set with
both the IPv4 and IPv6 fds. After accept()'ing the first one, the
second call to accept() hangs. I solved this by accepting only the
first connection I got; the second one should be accepted in the next
round of the service loop (if still available).

-- 
Erik "kusma" Faye-Lund

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Erik Faye-Lund]

On Wed, Dec 2, 2009 at 2:49 PM, Erik Faye-Lund <kusmabite@googlemail.com> wrote:
> On Wed, Dec 2, 2009 at 2:21 PM, Martin Storsjö <martin@martin.st> wrote:
>> On Wed, 2 Dec 2009, Erik Faye-Lund wrote:
>>> @@ -854,6 +850,7 @@ static int service_loop(int socknum, int *socklist)
>>>                                         }
>>>                                 }
>>>                                 handle(incoming, (struct sockaddr *)&ss, sslen);
>>>
>>> +                               break;
>>
>> What's this good for?
>>
>
> When I clone git://localhost/some-repo, select() returns a fd-set with
> both the IPv4 and IPv6 fds. After accept()'ing the first one, the
> second call to accept() hangs. I solved this by accepting only the
> first connection I got; the second one should be accepted in the next
> round of the service loop (if still available).
>

Actually, it's no good - my code is broken. FD_SET() and FD_ISSET()
needs wrapping to call _get_osfhandle() on the socket. Since this is
not needed on Linux, the code ran just fine there. But on Windows,
select() failed, but due to the following bug, it wasn't picked up:
+               if (select(0, &fds, NULL, NULL, &timeout) > 0) {

changing the comparison around, revealed that select hadn't done
anything so fds wasn't modified at all! Thus, I wrongly interpreted it
as if both sockets had an awaiting connection, making IPv6 connections
work (since they are on the first socket), but IPv4 not.

I've corrected this locally, and I'll include the fixed patch in the next round.

Thanks for pointing out the issue, causing me to second guess my "cure" :)

-- 
Erik "kusma" Faye-Lund

Re: [PATCH/RFC 01/11] mingw: add network-wrappers for daemon

[Johannes Sixt]

On Mittwoch, 2. Dezember 2009, Erik Faye-Lund wrote:
> I'm not very familiar with poll(), but if I understand the man-pages
> correctly it's waiting for events on file descriptors, and is in our
> case used to check for incoming connections, right? If so, I see three
> possible ways forward: (1) extending our poll()-emulation to handle
> multiple sockets, (2) change daemon.c to check one socket at the time,
> and (3) using select() instead of poll().
>
> (1) seems like the "correct" but tricky thing to do, (2) like the
> "easy" but nasty thing to do. However, (3) strikes me as the least
> dangerous thing to do ;)
>
> For (1), there's also a WSAPoll() function in Windows, but I'm not
> sure how to figure out if an fd is a socket or a pipe. There's also
> WaitForMultipleObjects.

GetFileType() returns FILE_TYPE_PIPE for both pipes and sockets. But once you 
know this, you can use getsockopt(): If it succeeds, it is a socket, and in 
this case, assume that poll() was called from git-daemon, i.e. all polled-for 
fds are sockets and you can select().

-- Hannes

Re: [msysGit] [PATCH/RFC 00/11] daemon-win32

[Johannes Sixt]

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> This is my stab at cleaning up Mike Pape's patches for git-daemon
> on Windows for submission, plus some of my own.

Please rebase the series on top of current master. I'd appreciate if you could 
make it available to pull from somewhere. I've a version here:

  git://repo.or.cz/git/mingw/j6t.git ef/mingw-daemon

(including Martin's getaddrinfo updates) but the resulting git-daemon crashes 
immediately after there is a connection.

-- Hannes

[PATCH/RFC 00/11] daemon-win32

[Erik Faye-Lund]

This is my stab at cleaning up Mike Pape's patches for git-daemon
on Windows for submission, plus some of my own.

* Patch 1-4 originate from Mike Pape, but have been cleaned up quite
  a lot by me. I hope Mike won't hate me. Credit have been retained,
  as all important code here were written by Mike. The commit
  messages were written mostly by me.

* Patch 5 is a trivial old-style function declaration fix.

* Patch 6 introduces two new functions to the run-command API,
  kill_async() and is_async_alive().

* Patch 7-8 removes the stdin/stdout redirection for the service
  functions, as redirecting won't work for the threaded version.

* Patch 9 converts the daemon-code to use the run-command API. This
  is the patch I expect to be the most controversial.

* Patch 10 is about using line-buffered mode instead of full-buffered
  mode for stderr.

* Patch 11 finally enables compilation of git-daemon on MinGW.

Let the flames begin!

Erik Faye-Lund (7):
  inet_ntop: fix a couple of old-style decls
  run-command: add kill_async() and is_async_alive()
  run-command: support input-fd
  daemon: use explicit file descriptor
  daemon: use run-command api for async serving
  daemon: use full buffered mode for stderr
  mingw: compile git-daemon

Mike Pape (4):
  mingw: add network-wrappers for daemon
  strbuf: add non-variadic function strbuf_vaddf()
  mingw: implement syslog
  compat: add inet_pton and inet_ntop prototypes

 Makefile           |    8 ++-
 compat/inet_ntop.c |   22 ++------
 compat/inet_pton.c |    8 ++-
 compat/mingw.c     |  111 +++++++++++++++++++++++++++++++++++++++++--
 compat/mingw.h     |   32 ++++++++++++
 daemon.c           |  134 ++++++++++++++++++++++++++--------------------------
 git-compat-util.h  |    9 ++++
 run-command.c      |   32 ++++++++++++-
 run-command.h      |    2 +
 strbuf.c           |   15 ++++--
 strbuf.h           |    1 +
 11 files changed, 274 insertions(+), 100 deletions(-)

Re: [PATCH/RFC 00/11] daemon-win32

[Erik Faye-Lund]

Awww, sorry people. I managed to send this one to the wrong
msysgit-account. I'll repost, as I think it's important to get that
list involved as well.

On Thu, Nov 26, 2009 at 1:39 AM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> This is my stab at cleaning up Mike Pape's patches for git-daemon
> on Windows for submission, plus some of my own.
>
> * Patch 1-4 originate from Mike Pape, but have been cleaned up quite
>  a lot by me. I hope Mike won't hate me. Credit have been retained,
>  as all important code here were written by Mike. The commit
>  messages were written mostly by me.
>
> * Patch 5 is a trivial old-style function declaration fix.
>
> * Patch 6 introduces two new functions to the run-command API,
>  kill_async() and is_async_alive().
>
> * Patch 7-8 removes the stdin/stdout redirection for the service
>  functions, as redirecting won't work for the threaded version.
>
> * Patch 9 converts the daemon-code to use the run-command API. This
>  is the patch I expect to be the most controversial.
>
> * Patch 10 is about using line-buffered mode instead of full-buffered
>  mode for stderr.
>
> * Patch 11 finally enables compilation of git-daemon on MinGW.
>
> Let the flames begin!
>
> Erik Faye-Lund (7):
>  inet_ntop: fix a couple of old-style decls
>  run-command: add kill_async() and is_async_alive()
>  run-command: support input-fd
>  daemon: use explicit file descriptor
>  daemon: use run-command api for async serving
>  daemon: use full buffered mode for stderr
>  mingw: compile git-daemon
>
> Mike Pape (4):
>  mingw: add network-wrappers for daemon
>  strbuf: add non-variadic function strbuf_vaddf()
>  mingw: implement syslog
>  compat: add inet_pton and inet_ntop prototypes
>
>  Makefile           |    8 ++-
>  compat/inet_ntop.c |   22 ++------
>  compat/inet_pton.c |    8 ++-
>  compat/mingw.c     |  111 +++++++++++++++++++++++++++++++++++++++++--
>  compat/mingw.h     |   32 ++++++++++++
>  daemon.c           |  134 ++++++++++++++++++++++++++--------------------------
>  git-compat-util.h  |    9 ++++
>  run-command.c      |   32 ++++++++++++-
>  run-command.h      |    2 +
>  strbuf.c           |   15 ++++--
>  strbuf.h           |    1 +
>  11 files changed, 274 insertions(+), 100 deletions(-)
>
>



-- 
Erik "kusma" Faye-Lund