[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: [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

[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

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