threads / patch / 25421

v4, 15 partsdaemon-win32

Subject: [PATCH v4 00/15] daemon-win32

## tl;dr

47 messages between Oct 11, 2010 and Oct 21, 2010. Diffs are folded; open one to read it.

replies: 46people: 6as markdown or json

Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · lore
OK, here's v3 of this series. Since the last version, the following
things were done:
* 01/14 has the error-path cleaned up, as suggested by Eric Sunshine
* 02/14 now supports any vararg input, and escapes "%1" to "% 1" to
  avoid a limitation ni ReportEvent(). The code has also been moved
  to compat/win32/syslog.[ch], since it's fairly self contained and
  it's not strictly speaking a mingw-specific thing.
* 05/14 now has an extended commit message, detailing the rationale
  a bit more
* 08/14 has been fixed up as suggested by Eric Sunshine
* 11/14 has been fixed up to remove code left dead by the previous
  version
* 12/14 has a better commit message, including an explanation on
  how to update the code. The sources have also been moved to
  compat/win32/sys/poll.[ch], which is where git usually looks for
  the poll-header.
* 14/14 has been split out into 14/15 and 15/15, because it
  contained some left-over changes (unsigned int -> socklen_t). The
  left-over code is 14/15.
* 15/15 have the _POSIX_VERSION-check replaced with a check for
  NO_POSIX_GOODIES. It has been changed back to opt-out, since I
  don't want to have to deal with figuring out what platforms it
  will work for or not. Currently all platforms except from Windows
  supports all posix-"goodies", but it should be easy to opt-out
  for a future plaform.
Erik Faye-Lund (11):
  inet_ntop: fix a couple of old-style decls
  mingw: use real pid
  mingw: support waitpid with pid > 0 and WNOHANG
  mingw: add kill emulation
  daemon: use run-command api for async serving
  daemon: use full buffered mode for stderr
  daemon: report connection from root-process
  mingw: import poll-emulation from gnulib
  mingw: use poll-emulation from gnulib
  daemon: use socklen_t
  daemon: opt-out on features that require posix
Martin Storsjö (1):
  Improve the mingw getaddrinfo stub to handle more use cases
Mike Pape (3):
  mingw: add network-wrappers for daemon
  mingw: implement syslog
  compat: add inet_pton and inet_ntop prototypes
 Makefile                |   23 ++-
 compat/inet_ntop.c      |   22 +--
 compat/inet_pton.c      |    8 +-
 compat/mingw.c          |  232 +++++++++++++------
 compat/mingw.h          |   41 ++--
 compat/win32/sys/poll.c |  596 +++++++++++++++++++++++++++++++++++++++++++++++
 compat/win32/sys/poll.h |   53 +++++
 compat/win32/syslog.c   |   72 ++++++
 compat/win32/syslog.h   |   20 ++
 daemon.c                |  199 +++++++++-------
 git-compat-util.h       |   11 +-
 11 files changed, 1067 insertions(+), 210 deletions(-)
 create mode 100644 compat/win32/sys/poll.c
 create mode 100644 compat/win32/sys/poll.h
 create mode 100644 compat/win32/syslog.c
 create mode 100644 compat/win32/syslog.h
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 01/15] mingw: add network-wrappers for daemon

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.

Signed-off-by: Mike Pape <dotzenlabs@gmail.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/mingw.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
 compat/mingw.h |   16 ++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletions(-)
Show changes to 2 files +59 −1

compat/mingw.c, compat/mingw.h

diff --git a/compat/mingw.c b/compat/mingw.c
index 6590f33..701a555 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1175,7 +1175,10 @@ int mingw_getnameinfo(const struct sockaddr *sa, socklen_t salen,
 int mingw_socket(int domain, int type, int protocol)
 {
 	int sockfd;
-	SOCKET s = WSASocket(domain, type, protocol, NULL, 0, 0);
+	SOCKET s;
+
+	ensure_socket_initialization();
+	s = WSASocket(domain, type, protocol, NULL, 0, 0);
 	if (s == INVALID_SOCKET) {
 		/*
 		 * WSAGetLastError() values are regular BSD error codes
@@ -1205,6 +1208,45 @@ 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) {
+		int err = errno;
+		closesocket(s2);
+		return error("unable to make a socket file descriptor: %s",
+			strerror(err));
+	}
+	return sockfd2;
+}
+
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index 83e35e8..a5bde82 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -7,6 +7,7 @@
 
 typedef int pid_t;
 typedef int uid_t;
+typedef int socklen_t;
 #define hstrerror strerror
 
 #define S_IFLNK    0120000 /* Symbolic link */
@@ -47,6 +48,9 @@ typedef int uid_t;
 #define F_SETFD 2
 #define FD_CLOEXEC 0x1
 
+#define EAFNOSUPPORT WSAEAFNOSUPPORT
+#define ECONNABORTED WSAECONNABORTED
+
 struct passwd {
 	char *pw_name;
 	char *pw_gecos;
@@ -225,6 +229,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.7.3.1.199.g72340
Jonathan Nieder· Oct 11, 2010, 22:07 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 01/15] mingw: add network-wrappers for daemon

Erik Faye-Lund wrote:
Show 6 quoted lines
> 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.

For the curious: gethostbyname(), getaddrinfo(), and getnameinfo() already call WSAStartup if it hasn't already been called. So imap-send and the libgit transport code do not suffer from the same problem. Similarly for git daemon in the !NO_IPV6 case.

In the NO_IPV6 case, git daemon does not call getaddrinfo() and --listen=<hostname> does not work (why?), so we have to hook into socket(), too.

Looks good to my winsock-ignorant eyes.
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 02/15] mingw: implement syslog

From: Mike Pape <dotzenlabs@gmail.com>

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

Strings containing "%1" gets corrupted by ReportEvent, so expand "%1" to "% 1" before reporting.

Signed-off-by: Mike Pape <dotzenlabs@gmail.com>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 Makefile              |    5 ++-
 compat/win32/syslog.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++
 compat/win32/syslog.h |   20 +++++++++++++
 daemon.c              |    2 -
 git-compat-util.h     |    1 +
 5 files changed, 96 insertions(+), 4 deletions(-)
 create mode 100644 compat/win32/syslog.c
 create mode 100644 compat/win32/syslog.h
Show changes to 5 files +96 −4

Makefile, compat/win32/syslog.c, compat/win32/syslog.h, daemon.c, git-compat-util.h

diff --git a/Makefile b/Makefile
index 1f1ce04..d9d9419 100644
--- a/Makefile
+++ b/Makefile
@@ -496,6 +496,7 @@ LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
 LIB_H += compat/win32/pthread.h
+LIB_H += compat/win32/syslog.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h
@@ -1081,7 +1082,7 @@ ifeq ($(uname_S),Windows)
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
-	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o
+	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o compat/win32/syslog.o
 	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
@@ -1131,7 +1132,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
-		compat/win32/pthread.o
+		compat/win32/pthread.o compat/win32/syslog.o
 	EXTLIBS += -lws2_32
 	PTHREAD_LIBS =
 	X = .exe
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
new file mode 100644
index 0000000..42b95a9
--- /dev/null
+++ b/compat/win32/syslog.c
@@ -0,0 +1,72 @@
+#include "../../git-compat-util.h"
+#include "../../strbuf.h"
+
+static HANDLE ms_eventlog;
+
+void openlog(const char *ident, int logopt, int facility)
+{
+	if (ms_eventlog)
+		return;
+
+	ms_eventlog = RegisterEventSourceA(NULL, ident);
+
+	if (!ms_eventlog)
+		warning("RegisterEventSource() failed: %lu", GetLastError());
+}
+
+void syslog(int priority, const char *fmt, ...)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf_expand_dict_entry dict[] = {
+		{"1", "% 1"},
+		{NULL, NULL}
+	};
+	WORD logtype;
+	char *str;
+	int str_len;
+	va_list ap;
+
+	if (!ms_eventlog)
+		return;
+
+	va_start(ap, fmt);
+	str_len = vsnprintf(NULL, 0, fmt, ap);
+	va_end(ap);
+
+	if (str_len < 0) {
+		warning("vsnprintf failed: '%s'", strerror(errno));
+		return;
+	}
+
+	str = malloc(str_len + 1);
+	va_start(ap, fmt);
+	vsnprintf(str, str_len + 1, fmt, ap);
+	va_end(ap);
+	strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
+	free(str);
+
+	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, 0, 0, NULL, 1, 0,
+	    (const char **)&sb.buf, NULL);
+
+	strbuf_release(&sb);
+}
diff --git a/compat/win32/syslog.h b/compat/win32/syslog.h
new file mode 100644
index 0000000..70daa7c
--- /dev/null
+++ b/compat/win32/syslog.h
@@ -0,0 +1,20 @@
+#ifndef SYSLOG_H
+#define SYSLOG_H
+
+#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)
+
+void openlog(const char *ident, int logopt, int facility);
+void syslog(int priority, const char *fmt, ...);
+
+#endif /* SYSLOG_H */
diff --git a/daemon.c b/daemon.c
index d6e20c6..d594375 100644
--- a/daemon.c
+++ b/daemon.c
@@ -5,8 +5,6 @@
 #include "strbuf.h"
 #include "string-list.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 2af8d3e..e192831 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -104,6 +104,7 @@
 #include <assert.h>
 #include <regex.h>
 #include <utime.h>
+#include <syslog.h>
 #ifndef __MINGW32__
 #include <sys/wait.h>
 #include <sys/poll.h>
-- 
1.7.3.1.199.g72340
Jonathan Nieder· Oct 11, 2010, 22:11 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 02/15] mingw: implement syslog

Erik Faye-Lund wrote:
> Strings containing "%1" gets corrupted by ReportEvent, so expand
> "%1" to "% 1" before reporting.

What is the symptom? Can clients trigger this, and is it worth preventing them from doing so?

Erik Faye-Lund· Oct 11, 2010, 22:28 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4 02/15] mingw: implement syslog

On Tue, Oct 12, 2010 at 12:11 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 8 quoted lines
> Erik Faye-Lund wrote:
>
>> Strings containing "%1" gets corrupted by ReportEvent, so expand
>> "%1" to "% 1" before reporting.
>
> What is the symptom?  Can clients trigger this, and is it worth
> preventing them from doing so?
>

The string gets inlined into itself (with a limit of 100 expansions) leading to string like "foo %1 bar" becoming "foo foo foo ... foo %1 bar bar bar ... bar". With our expansion, it becomes "foo % 1 bar" instead.

Jonathan Nieder· Oct 11, 2010, 22:37 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 02/15] mingw: implement syslog

Erik Faye-Lund wrote:
> The string gets inlined into itself (with a limit of 100 expansions)
> leading to string like "foo %1 bar" becoming "foo foo foo ... foo %1
> bar bar bar ... bar". With our expansion, it becomes "foo % 1 bar"
> instead.

Ah, ok. Sounds like there is no need to worry about requests for "%%1" etc. Thanks for explaining.

Maybe the symptoms and cases not covered (%2, %%1) would be worth mentioning in the log message?

Erik Faye-Lund· Oct 13, 2010, 12:36 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4 02/15] mingw: implement syslog

On Tue, Oct 12, 2010 at 12:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
> Erik Faye-Lund wrote:
>
>> The string gets inlined into itself (with a limit of 100 expansions)
>> leading to string like "foo %1 bar" becoming "foo foo foo ... foo %1
>> bar bar bar ... bar". With our expansion, it becomes "foo % 1 bar"
>> instead.
>
> Ah, ok.  Sounds like there is no need to worry about requests for "%%1"
> etc.  Thanks for explaining.
>

Actually, %%1 is a bit of a tricky one. It seems that %%1 is used to escape %1 on Windows 7, but not on earlier Windows version. I did test this on Vista an XP earlier, but I'll re-test again later and report back, in case my earlier tests were flawed.

Can %%1 occur in an IPv6 address at all? If not, I'm tempted to not handle it (unless it turns out I was wrong about %%1-escaping on Vista and XP).

> Maybe the symptoms and cases not covered (%2, %%1) would be worth
> mentioning in the log message?
>
Sorry for the late reply.

I'm sure it could be added, I'm just a bit worried about making the whole commit message too intimidating.

Something like this, perhaps?

---8<--- Strings containing "%1" gets expanded into themselves by ReportEvent, so let's expand "%1" to "% 1" before reporting, because such string can occur in IPv6-addresses. "%2" and above does not appear to be a problem, probably because ReportEvent checks the number against the wNumStrings parameter first. "%%1" is still a problem, but we don't have a code-path producing such strings. ---8<---

This is (of course), assuming my current findings are correct.
The lack of documentation on the subject is really annoying.
Eric Sunshine· Oct 13, 2010, 19:23 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 02/15] mingw: implement syslog

On 10/13/2010 08:36 AM, Erik Faye-Lund wrote:
Show 15 quoted lines
> On Tue, Oct 12, 2010 at 12:37 AM, Jonathan Nieder<jrnieder@gmail.com>  wrote:
>> Erik Faye-Lund wrote:
>>
>>> The string gets inlined into itself (with a limit of 100 expansions)
>>> leading to string like "foo %1 bar" becoming "foo foo foo ... foo %1
>>> bar bar bar ... bar". With our expansion, it becomes "foo % 1 bar"
>>> instead.
>>
>> Ah, ok.  Sounds like there is no need to worry about requests for "%%1"
>> etc.  Thanks for explaining.
>>
> Actually, %%1 is a bit of a tricky one. It seems that %%1 is used to
> escape %1 on Windows 7, but not on earlier Windows version. I did test
> this on Vista an XP earlier, but I'll re-test again later and report
> back, in case my earlier tests were flawed.

If that worked universally, escaping '%1' to '%%1' certainly would be nicer than '% 1'. (More generally, escape '%n' to '%%n', where n is a number.) It also would simplify the log message.

> Can %%1 occur in an IPv6 address at all? If not, I'm tempted to not
> handle it (unless it turns out I was wrong about %%1-escaping on Vista
> and XP).

According to sources I have studied, %%1 would be unlikely (or perhaps invalid) in IPv6 addresses.

http://en.wikipedia.org/wiki/IPv6_address#Link-local_addresses_and_zone_indices
-- ES
Pat Thoyts· Oct 13, 2010, 21:17 UTC · re: Eric Sunshine · lore

Re: [msysGit] Re: [PATCH v4 02/15] mingw: implement syslog

On 13 October 2010 20:23, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 32 quoted lines
> On 10/13/2010 08:36 AM, Erik Faye-Lund wrote:
>>
>> On Tue, Oct 12, 2010 at 12:37 AM, Jonathan Nieder<jrnieder@gmail.com>
>>  wrote:
>>>
>>> Erik Faye-Lund wrote:
>>>
>>>> The string gets inlined into itself (with a limit of 100 expansions)
>>>> leading to string like "foo %1 bar" becoming "foo foo foo ... foo %1
>>>> bar bar bar ... bar". With our expansion, it becomes "foo % 1 bar"
>>>> instead.
>>>
>>> Ah, ok.  Sounds like there is no need to worry about requests for "%%1"
>>> etc.  Thanks for explaining.
>>>
>> Actually, %%1 is a bit of a tricky one. It seems that %%1 is used to
>> escape %1 on Windows 7, but not on earlier Windows version. I did test
>> this on Vista an XP earlier, but I'll re-test again later and report
>> back, in case my earlier tests were flawed.
>
> If that worked universally, escaping '%1' to '%%1' certainly would be nicer
> than '% 1'. (More generally, escape '%n' to '%%n', where n is a number.) It
> also would simplify the log message.
>
>> Can %%1 occur in an IPv6 address at all? If not, I'm tempted to not
>> handle it (unless it turns out I was wrong about %%1-escaping on Vista
>> and XP).
>
> According to sources I have studied, %%1 would be unlikely (or perhaps
> invalid) in IPv6 addresses.
>
> http://en.wikipedia.org/wiki/IPv6_address#Link-local_addresses_and_zone_indices
Not on windows. Try ipconfig:
   Link-local IPv6 Address . . . . . : fe80::c9fb:7840:66f5:b2e9%13
   Default Gateway . . . . . . . . . : fe80::20c:76ff:fe1e:e00%11
and so on. Its an interface fragment or something.

However - we really don't care. You can just substitute these to spaces and no-one will care. Keep it simple.

Erik Faye-Lund· Oct 14, 2010, 00:47 UTC · re: Pat Thoyts · lore

Re: [msysGit] Re: [PATCH v4 02/15] mingw: implement syslog

On Wed, Oct 13, 2010 at 11:17 PM, Pat Thoyts <patthoyts@gmail.com> wrote:
Show 21 quoted lines
> On 13 October 2010 20:23, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On 10/13/2010 08:36 AM, Erik Faye-Lund wrote:
>>>
>>> On Tue, Oct 12, 2010 at 12:37 AM, Jonathan Nieder<jrnieder@gmail.com>
>>>  wrote:
>>>>
>>>> Erik Faye-Lund wrote:
>>>>
>>>>> The string gets inlined into itself (with a limit of 100 expansions)
>>>>> leading to string like "foo %1 bar" becoming "foo foo foo ... foo %1
>>>>> bar bar bar ... bar". With our expansion, it becomes "foo % 1 bar"
>>>>> instead.
>>>>
>>>> Ah, ok.  Sounds like there is no need to worry about requests for "%%1"
>>>> etc.  Thanks for explaining.
>>>>
>>> Actually, %%1 is a bit of a tricky one. It seems that %%1 is used to
>>> escape %1 on Windows 7, but not on earlier Windows version. I did test
>>> this on Vista an XP earlier, but I'll re-test again later and report
>>> back, in case my earlier tests were flawed.
>>

Meh. Windows XP does not escape %%1 to %1, it has the same expansion-problem as %1 does. In other words, my old assertion is still valid.

Show 21 quoted lines
>> If that worked universally, escaping '%1' to '%%1' certainly would be nicer
>> than '% 1'. (More generally, escape '%n' to '%%n', where n is a number.) It
>> also would simplify the log message.
>>
>>> Can %%1 occur in an IPv6 address at all? If not, I'm tempted to not
>>> handle it (unless it turns out I was wrong about %%1-escaping on Vista
>>> and XP).
>>
>> According to sources I have studied, %%1 would be unlikely (or perhaps
>> invalid) in IPv6 addresses.
>>
>> http://en.wikipedia.org/wiki/IPv6_address#Link-local_addresses_and_zone_indices
>
> Not on windows. Try ipconfig:
>   Link-local IPv6 Address . . . . . : fe80::c9fb:7840:66f5:b2e9%13
>   Default Gateway . . . . . . . . . : fe80::20c:76ff:fe1e:e00%11
> and so on. Its an interface fragment or something.
>
> However - we really don't care. You can just substitute these to
> spaces and no-one will care. Keep it simple.
>

Uh, none of these contain a double percent-sign. Am I misunderstanding what you're replying to?

Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 03/15] compat: add inet_pton and inet_ntop prototypes

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(-)
Show changes to 4 files +18 −6

Makefile, compat/inet_ntop.c, compat/inet_pton.c, git-compat-util.h

diff --git a/Makefile b/Makefile
index d9d9419..2aa067a 100644
--- a/Makefile
+++ b/Makefile
@@ -1398,9 +1398,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 e192831..56dce85 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -387,6 +387,14 @@ static inline void *gitmempcpy(void *dest, const void *src, size_t n)
 }
 #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);
 
 typedef void (*try_to_free_t)(size_t);
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 04/15] inet_ntop: fix a couple of old-style decls

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/inet_ntop.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)
Show changes to compat/inet_ntop.c +3 −13
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.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 05/15] mingw: use real pid

The Windows port have so far been using process handles in place of PID. However, this is not work consistent with what getpid returns.

PIDs are system-global identifiers, but process handles are local to a process. Using PIDs instead of process handles allows, for instance, a user to kill a hung process with the Task Manager, something that would have been impossible with process handles.

Change the code to use the real PID, and use OpenProcess to get a process-handle. Store the PID and the process handle in a table protected by a critical section, so we can safely close the process handle later.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/mingw.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 compat/mingw.h |   10 ++-----
 2 files changed, 72 insertions(+), 8 deletions(-)
Show changes to 2 files +72 −8

compat/mingw.c, compat/mingw.h

diff --git a/compat/mingw.c b/compat/mingw.c
index 701a555..e2e3c54 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -702,6 +702,13 @@ static int env_compare(const void *a, const void *b)
 	return strcasecmp(*ea, *eb);
 }
 
+struct {
+	pid_t pid;
+	HANDLE proc;
+} *pinfo;
+static int num_pinfo;
+CRITICAL_SECTION pinfo_cs;
+
 static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,
 			      const char *dir,
 			      int prepend_cmd, int fhin, int fhout, int fherr)
@@ -794,7 +801,23 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,
 		return -1;
 	}
 	CloseHandle(pi.hThread);
-	return (pid_t)pi.hProcess;
+
+	/*
+	 * The process ID is the human-readable identifier of the process
+	 * that we want to present in log and error messages. The handle
+	 * is not useful for this purpose. But we cannot close it, either,
+	 * because it is not possible to turn a process ID into a process
+	 * handle after the process terminated.
+	 * Keep the handle in a list for waitpid.
+	 */
+	EnterCriticalSection(&pinfo_cs);
+	num_pinfo++;
+	pinfo = xrealloc(pinfo, sizeof(*pinfo) * num_pinfo);
+	pinfo[num_pinfo - 1].pid = pi.dwProcessId;
+	pinfo[num_pinfo - 1].proc = pi.hProcess;
+	LeaveCriticalSection(&pinfo_cs);
+
+	return (pid_t)pi.dwProcessId;
 }
 
 static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
@@ -1518,6 +1541,51 @@ char *getpass(const char *prompt)
 	return strbuf_detach(&buf, NULL);
 }
 
+pid_t waitpid(pid_t pid, int *status, unsigned options)
+{
+	HANDLE h = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION,
+	    FALSE, pid);
+	if (!h) {
+		errno = ECHILD;
+		return -1;
+	}
+
+	if (options == 0) {
+		int i;
+		if (WaitForSingleObject(h, INFINITE) != WAIT_OBJECT_0) {
+			CloseHandle(h);
+			return 0;
+		}
+
+		if (status)
+			GetExitCodeProcess(h, (LPDWORD)status);
+
+		EnterCriticalSection(&pinfo_cs);
+
+		for (i = 0; i < num_pinfo; ++i)
+			if (pinfo[i].pid == pid)
+				break;
+
+		if (i < num_pinfo) {
+			CloseHandle(pinfo[i].proc);
+			memmove(pinfo + i, pinfo + i + 1,
+			    sizeof(*pinfo) * (num_pinfo - i - 1));
+			num_pinfo--;
+			pinfo = xrealloc(pinfo,
+			    sizeof(*pinfo) * num_pinfo);
+		}
+
+		LeaveCriticalSection(&pinfo_cs);
+
+		CloseHandle(h);
+		return pid;
+	}
+	CloseHandle(h);
+
+	errno = EINVAL;
+	return -1;
+}
+
 #ifndef NO_MINGW_REPLACE_READDIR
 /* MinGW readdir implementation to avoid extra lstats for Git */
 struct mingw_DIR
diff --git a/compat/mingw.h b/compat/mingw.h
index a5bde82..7c4eeea 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -140,13 +140,7 @@ static inline int mingw_unlink(const char *pathname)
 }
 #define unlink mingw_unlink
 
-static inline pid_t waitpid(pid_t pid, int *status, unsigned options)
-{
-	if (options == 0)
-		return _cwait(status, pid, 0);
-	errno = EINVAL;
-	return -1;
-}
+pid_t waitpid(pid_t pid, int *status, unsigned options);
 
 #ifndef NO_OPENSSL
 #include <openssl/ssl.h>
@@ -321,11 +315,13 @@ void free_environ(char **env);
 static int mingw_main(); \
 int main(int argc, const char **argv) \
 { \
+	extern CRITICAL_SECTION pinfo_cs; \
 	_fmode = _O_BINARY; \
 	_setmode(_fileno(stdin), _O_BINARY); \
 	_setmode(_fileno(stdout), _O_BINARY); \
 	_setmode(_fileno(stderr), _O_BINARY); \
 	argv[0] = xstrdup(_pgmptr); \
+	InitializeCriticalSection(&pinfo_cs); \
 	return mingw_main(argc, argv); \
 } \
 static int mingw_main(c,v)
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 06/15] mingw: support waitpid with pid > 0 and WNOHANG

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/mingw.c |    6 ++++++
 compat/mingw.h |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)
Show changes to 2 files +7 −0

compat/mingw.c, compat/mingw.h

diff --git a/compat/mingw.c b/compat/mingw.c
index e2e3c54..2e7c644 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1550,6 +1550,12 @@ pid_t waitpid(pid_t pid, int *status, unsigned options)
 		return -1;
 	}
 
+	if (pid > 0 && options & WNOHANG) {
+		if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0))
+			return 0;
+		options &= ~WNOHANG;
+	}
+
 	if (options == 0) {
 		int i;
 		if (WaitForSingleObject(h, INFINITE) != WAIT_OBJECT_0) {
diff --git a/compat/mingw.h b/compat/mingw.h
index 7c4eeea..379d7bf 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -140,6 +140,7 @@ static inline int mingw_unlink(const char *pathname)
 }
 #define unlink mingw_unlink
 
+#define WNOHANG 1
 pid_t waitpid(pid_t pid, int *status, unsigned options);
 
 #ifndef NO_OPENSSL
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 07/15] mingw: add kill emulation

This is a quite limited kill-emulation; it can only handle SIGTERM on positive pids. However, it's enough for git-daemon.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/mingw.c |   19 +++++++++++++++++++
 compat/mingw.h |    3 +++
 2 files changed, 22 insertions(+), 0 deletions(-)
Show changes to 2 files +22 −0

compat/mingw.c, compat/mingw.h

diff --git a/compat/mingw.c b/compat/mingw.c
index 2e7c644..21d1c2c 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -932,6 +932,25 @@ void mingw_execv(const char *cmd, char *const *argv)
 	mingw_execve(cmd, argv, environ);
 }
 
+int mingw_kill(pid_t pid, int sig)
+{
+	if (pid > 0 && sig == SIGTERM) {
+		HANDLE h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
+
+		if (TerminateProcess(h, -1)) {
+			CloseHandle(h);
+			return 0;
+		}
+
+		errno = err_win_to_posix(GetLastError());
+		CloseHandle(h);
+		return -1;
+	}
+
+	errno = EINVAL;
+	return -1;
+}
+
 static char **copy_environ(void)
 {
 	char **env;
diff --git a/compat/mingw.h b/compat/mingw.h
index 379d7bf..51fca2f 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -143,6 +143,9 @@ static inline int mingw_unlink(const char *pathname)
 #define WNOHANG 1
 pid_t waitpid(pid_t pid, int *status, unsigned options);
 
+#define kill mingw_kill
+int mingw_kill(pid_t pid, int sig);
+
 #ifndef NO_OPENSSL
 #include <openssl/ssl.h>
 static inline int mingw_SSL_set_fd(SSL *ssl, int fd)
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 08/15] daemon: use run-command api for async serving

fork() is only available on POSIX, so to support git-daemon on Windows we have to use something else.

Instead we invent the flag --serve, which is a stripped down version of --inetd-mode. We use start_command() to call git-daemon with this flag appended to serve clients.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 daemon.c |   84 +++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 42 insertions(+), 42 deletions(-)
Show changes to daemon.c +42 −42
diff --git a/daemon.c b/daemon.c
index d594375..c0da052 100644
--- a/daemon.c
+++ b/daemon.c
@@ -614,17 +614,17 @@ static unsigned int live_children;
 
 static struct child {
 	struct child *next;
-	pid_t pid;
+	struct child_process cld;
 	struct sockaddr_storage address;
 } *firstborn;
 
-static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
+static void add_child(struct child_process *cld, struct sockaddr *addr, int addrlen)
 {
 	struct child *newborn, **cradle;
 
 	newborn = xcalloc(1, sizeof(*newborn));
 	live_children++;
-	newborn->pid = pid;
+	memcpy(&newborn->cld, cld, sizeof(*cld));
 	memcpy(&newborn->address, addr, addrlen);
 	for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
 		if (!addrcmp(&(*cradle)->address, &newborn->address))
@@ -633,19 +633,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".
@@ -661,7 +648,7 @@ static void kill_some_child(void)
 
 	for (; (next = blanket->next); blanket = next)
 		if (!addrcmp(&blanket->address, &next->address)) {
-			kill(blanket->pid, SIGTERM);
+			kill(blanket->cld.pid, SIGTERM);
 			break;
 		}
 }
@@ -671,18 +658,26 @@ 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 ((pid = waitpid(blanket->cld.pid, &status, WNOHANG)) > 1) {
+			const char *dead = "";
+			if (status)
+				dead = " (with error)";
+			loginfo("[%"PRIuMAX"] Disconnected%s", (uintmax_t)pid, dead);
+
+			/* remove the child */
+			*cradle = blanket->next;
+			live_children--;
+			free(blanket);
+		} else
+			cradle = &blanket->next;
 }
 
+static char **cld_argv;
 static void handle(int incoming, struct sockaddr *addr, int addrlen)
 {
-	pid_t pid;
+	struct child_process cld = { 0 };
 
 	if (max_connections && live_children >= max_connections) {
 		kill_some_child();
@@ -695,22 +690,15 @@ 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;
-		}
-
-		add_child(pid, addr, addrlen);
-		return;
-	}
+	cld.argv = (const char **)cld_argv;
+	cld.in = incoming;
+	cld.out = dup(incoming);
 
-	dup2(incoming, 0);
-	dup2(incoming, 1);
+	if (start_command(&cld))
+		logerror("unable to fork");
+	else
+		add_child(&cld, addr, addrlen);
 	close(incoming);
-
-	exit(execute(addr));
 }
 
 static void child_handler(int signo)
@@ -991,7 +979,7 @@ int main(int argc, char **argv)
 {
 	int listen_port = 0;
 	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
-	int inetd_mode = 0;
+	int serve_mode = 0, inetd_mode = 0;
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
 	struct passwd *pass = NULL;
@@ -1017,7 +1005,12 @@ int main(int argc, char **argv)
 				continue;
 			}
 		}
+		if (!strcmp(arg, "--serve")) {
+			serve_mode = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--inetd")) {
+			serve_mode = 1;
 			inetd_mode = 1;
 			log_syslog = 1;
 			continue;
@@ -1161,12 +1154,12 @@ int main(int argc, char **argv)
 		die("base-path '%s' does not exist or is not a directory",
 		    base_path);
 
-	if (inetd_mode) {
+	if (serve_mode) {
 		struct sockaddr_storage ss;
 		struct sockaddr *peer = (struct sockaddr *)&ss;
 		socklen_t slen = sizeof(ss);
 
-		if (!freopen("/dev/null", "w", stderr))
+		if (inetd_mode && !freopen("/dev/null", "w", stderr))
 			die_errno("failed to redirect stderr to /dev/null");
 
 		if (getpeername(0, peer, &slen))
@@ -1185,5 +1178,12 @@ int main(int argc, char **argv)
 	if (pid_file)
 		store_pid(pid_file);
 
+	/* prepare argv for serving-processes */
+	cld_argv = xmalloc(sizeof (char *) * (argc + 2));
+	for (i = 0; i < argc; ++i)
+		cld_argv[i] = argv[i];
+	cld_argv[argc] = "--serve";
+	cld_argv[argc+1] = NULL;
+
 	return serve(&listen_addr, listen_port, pass, gid);
 }
-- 
1.7.3.1.199.g72340
Junio C Hamano· Oct 13, 2010, 22:47 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 08/15] daemon: use run-command api for async serving

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 26 quoted lines
> @@ -1017,7 +1005,12 @@ int main(int argc, char **argv)
>  				continue;
>  			}
>  		}
> +		if (!strcmp(arg, "--serve")) {
> +			serve_mode = 1;
> +			continue;
> +		}
>  		if (!strcmp(arg, "--inetd")) {
> +			serve_mode = 1;
>  			inetd_mode = 1;
>  			log_syslog = 1;
>  			continue;
> @@ -1161,12 +1154,12 @@ int main(int argc, char **argv)
>  		die("base-path '%s' does not exist or is not a directory",
>  		    base_path);
>  
> -	if (inetd_mode) {
> +	if (serve_mode) {
>  		struct sockaddr_storage ss;
>  		struct sockaddr *peer = (struct sockaddr *)&ss;
>  		socklen_t slen = sizeof(ss);
>  
> -		if (!freopen("/dev/null", "w", stderr))
> +		if (inetd_mode && !freopen("/dev/null", "w", stderr))
>  			die_errno("failed to redirect stderr to /dev/null");

This is not particularly a good style. Please make it more clear that we freopen in inetd mode by writing it like this:

	if (inetd_mode) {
        	if (!freopen(...))
                	die_errno(...)
	}
Erik Faye-Lund· Oct 14, 2010, 10:18 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 08/15] daemon: use run-command api for async serving

On Thu, Oct 14, 2010 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> @@ -1017,7 +1005,12 @@ int main(int argc, char **argv)
>>                               continue;
>>                       }
>>               }
>> +             if (!strcmp(arg, "--serve")) {
>> +                     serve_mode = 1;
>> +                     continue;
>> +             }
>>               if (!strcmp(arg, "--inetd")) {
>> +                     serve_mode = 1;
>>                       inetd_mode = 1;
>>                       log_syslog = 1;
>>                       continue;
>> @@ -1161,12 +1154,12 @@ int main(int argc, char **argv)
>>               die("base-path '%s' does not exist or is not a directory",
>>                   base_path);
>>
>> -     if (inetd_mode) {
>> +     if (serve_mode) {
>>               struct sockaddr_storage ss;
>>               struct sockaddr *peer = (struct sockaddr *)&ss;
>>               socklen_t slen = sizeof(ss);
>>
>> -             if (!freopen("/dev/null", "w", stderr))
>> +             if (inetd_mode && !freopen("/dev/null", "w", stderr))
>>                       die_errno("failed to redirect stderr to /dev/null");
>
> This is not particularly a good style.  Please make it more clear that we
> freopen in inetd mode by writing it like this:
>
>        if (inetd_mode) {
>                if (!freopen(...))
>                        die_errno(...)
>        }
>
>
Much nicer, yeah. Now I'm tempted to do this also:
---8<---
Show changes to daemon.c +1 −2
diff --git a/daemon.c b/daemon.c
index 7f5d72f..11a5e06 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1010,7 +1010,6 @@ int main(int argc, char **argv)
 			continue;
 		}
 		if (!strcmp(arg, "--inetd")) {
-			serve_mode = 1;
 			inetd_mode = 1;
 			log_syslog = 1;
 			continue;
@@ -1159,7 +1158,7 @@ int main(int argc, char **argv)
 			die_errno("failed to redirect stderr to /dev/null");
 	}

-	if (serve_mode) {
+	if (inetd_mode || serve_mode) {
 		struct sockaddr_storage ss;
 		struct sockaddr *peer = (struct sockaddr *)&ss;
 		socklen_t slen = sizeof(ss);
---8<---
Junio C Hamano· Oct 17, 2010, 04:43 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 08/15] daemon: use run-command api for async serving

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 19 quoted lines
> On Thu, Oct 14, 2010 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>> ...
>>> @@ -1017,7 +1005,12 @@ int main(int argc, char **argv)
>>> +             if (inetd_mode && !freopen("/dev/null", "w", stderr))
>>>                       die_errno("failed to redirect stderr to /dev/null");
>>
>> This is not particularly a good style.  Please make it more clear that we
>> freopen in inetd mode by writing it like this:
>>
>>        if (inetd_mode) {
>>                if (!freopen(...))
>>                        die_errno(...)
>>        }
>>
>>
>
> Much nicer, yeah. Now I'm tempted to do this also:
> ...
Yeah, that is much much saner.  Thanks.
Show 23 quoted lines
> ---8<---
> diff --git a/daemon.c b/daemon.c
> index 7f5d72f..11a5e06 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1010,7 +1010,6 @@ int main(int argc, char **argv)
>  			continue;
>  		}
>  		if (!strcmp(arg, "--inetd")) {
> -			serve_mode = 1;
>  			inetd_mode = 1;
>  			log_syslog = 1;
>  			continue;
> @@ -1159,7 +1158,7 @@ int main(int argc, char **argv)
>  			die_errno("failed to redirect stderr to /dev/null");
>  	}
>
> -	if (serve_mode) {
> +	if (inetd_mode || serve_mode) {
>  		struct sockaddr_storage ss;
>  		struct sockaddr *peer = (struct sockaddr *)&ss;
>  		socklen_t slen = sizeof(ss);
> ---8<---
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 09/15] daemon: use full buffered mode for stderr

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(-)
Show changes to daemon.c +4 −2
diff --git a/daemon.c b/daemon.c
index c0da052..8a44fb9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -67,12 +67,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);
 	}
 }
 
@@ -1118,7 +1120,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.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 10/15] Improve the mingw getaddrinfo stub to handle more use cases

From: Martin Storsjö <martin@martin.st>

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>
---
 compat/mingw.c |   28 +++++++++++++++++++++-------
 1 files changed, 21 insertions(+), 7 deletions(-)
Show changes to compat/mingw.c +21 −7
diff --git a/compat/mingw.c b/compat/mingw.c
index 21d1c2c..d88c0d0 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1035,19 +1035,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;
@@ -1059,14 +1062,25 @@ 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);
+	if (hints && (hints->ai_flags & AI_CANONNAME))
+		ai->ai_canonname = h ? strdup(h->h_name) : NULL;
+	else
+		ai->ai_canonname = NULL;
 
 	sin = xmalloc(ai->ai_addrlen);
 	memset(sin, 0, ai->ai_addrlen);
 	sin->sin_family = AF_INET;
+	/* Note: getaddrinfo is supposed to allow service to be a string,
+	 * which should be looked up using getservbyname. This is
+	 * currently not implemented */
 	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 if (hints && (hints->ai_flags & AI_PASSIVE))
+		sin->sin_addr.s_addr = INADDR_ANY;
+	else
+		sin->sin_addr.s_addr = INADDR_LOOPBACK;
 	ai->ai_addr = (struct sockaddr *)sin;
 	ai->ai_next = 0;
 	return 0;
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 11/15] daemon: report connection from root-process

Report incoming connections from the process that accept() the connection instead of the handling process.

This enables "Connection from"-reporting on Windows, where getpeername(0, ...) consistently fails.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 daemon.c |   72 ++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 35 insertions(+), 37 deletions(-)
Show changes to daemon.c +35 −37
diff --git a/daemon.c b/daemon.c
index 8a44fb9..1574f75 100644
--- a/daemon.c
+++ b/daemon.c
@@ -516,38 +516,11 @@ static void parse_host_arg(char *extra_args, int buflen)
 }
 
 
-static int execute(struct sockaddr *addr)
+static int execute(void)
 {
 	static char line[1000];
 	int pktlen, len, i;
 
-	if (addr) {
-		char addrbuf[256] = "";
-		int port = -1;
-
-		if (addr->sa_family == AF_INET) {
-			struct sockaddr_in *sin_addr = (void *) addr;
-			inet_ntop(addr->sa_family, &sin_addr->sin_addr, addrbuf, sizeof(addrbuf));
-			port = ntohs(sin_addr->sin_port);
-#ifndef NO_IPV6
-		} else if (addr && addr->sa_family == AF_INET6) {
-			struct sockaddr_in6 *sin6_addr = (void *) addr;
-
-			char *buf = addrbuf;
-			*buf++ = '['; *buf = '\0'; /* stpcpy() is cool */
-			inet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf, sizeof(addrbuf) - 1);
-			strcat(buf, "]");
-
-			port = ntohs(sin6_addr->sin6_port);
-#endif
-		}
-		loginfo("Connection from %s:%d", addrbuf, port);
-		setenv("REMOTE_ADDR", addrbuf, 1);
-	}
-	else {
-		unsetenv("REMOTE_ADDR");
-	}
-
 	alarm(init_timeout ? init_timeout : timeout);
 	pktlen = packet_read_line(0, line, sizeof(line));
 	alarm(0);
@@ -676,10 +649,35 @@ static void check_dead_children(void)
 			cradle = &blanket->next;
 }
 
+static char *get_addrstr(int *port, struct sockaddr *addr)
+{
+	static char addrbuf[256] = "";
+	if (addr->sa_family == AF_INET) {
+		struct sockaddr_in *sin_addr = (void *) addr;
+		inet_ntop(addr->sa_family, &sin_addr->sin_addr, addrbuf, sizeof(addrbuf));
+		*port = ntohs(sin_addr->sin_port);
+#ifndef NO_IPV6
+	} else if (addr && addr->sa_family == AF_INET6) {
+		struct sockaddr_in6 *sin6_addr = (void *) addr;
+
+		char *buf = addrbuf;
+		*buf++ = '['; *buf = '\0'; /* stpcpy() is cool */
+		inet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf, sizeof(addrbuf) - 1);
+		strcat(buf, "]");
+
+		*port = ntohs(sin6_addr->sin6_port);
+#endif
+	}
+	return addrbuf;
+}
+
 static char **cld_argv;
 static void handle(int incoming, struct sockaddr *addr, int addrlen)
 {
 	struct child_process cld = { 0 };
+	char *addrstr, envbuf[300] = "REMOTE_ADDR=";
+	char *env[] = { envbuf, NULL };
+	int port = -1;
 
 	if (max_connections && live_children >= max_connections) {
 		kill_some_child();
@@ -692,14 +690,21 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 		}
 	}
 
+	addrstr = get_addrstr(&port, addr);
+	strcat(envbuf, addrstr);
+
+	cld.env = (const char **)env;
 	cld.argv = (const char **)cld_argv;
 	cld.in = incoming;
 	cld.out = dup(incoming);
 
 	if (start_command(&cld))
 		logerror("unable to fork");
-	else
+	else {
+		loginfo("[%"PRIuMAX"] Connection from %s:%d",
+		    (uintmax_t)cld.pid, addrstr, port);
 		add_child(&cld, addr, addrlen);
+	}
 	close(incoming);
 }
 
@@ -1157,17 +1162,10 @@ int main(int argc, char **argv)
 		    base_path);
 
 	if (serve_mode) {
-		struct sockaddr_storage ss;
-		struct sockaddr *peer = (struct sockaddr *)&ss;
-		socklen_t slen = sizeof(ss);
-
 		if (inetd_mode && !freopen("/dev/null", "w", stderr))
 			die_errno("failed to redirect stderr to /dev/null");
 
-		if (getpeername(0, peer, &slen))
-			peer = NULL;
-
-		return execute(peer);
+		return execute();
 	}
 
 	if (detach) {
-- 
1.7.3.1.199.g72340
Junio C Hamano· Oct 13, 2010, 22:55 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 11/15] daemon: report connection from root-process

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 33 quoted lines
> Report incoming connections from the process that
> accept() the connection instead of the handling
> process.
>
> This enables "Connection from"-reporting on
> Windows, where getpeername(0, ...) consistently
> fails.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>  daemon.c |   72 ++++++++++++++++++++++++++++++-------------------------------
>  1 files changed, 35 insertions(+), 37 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 8a44fb9..1574f75 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -516,38 +516,11 @@ static void parse_host_arg(char *extra_args, int buflen)
>  }
>  
>  
> -static int execute(struct sockaddr *addr)
> +static int execute(void)
>  {
> -...
> -		}
> -		loginfo("Connection from %s:%d", addrbuf, port);
> -		setenv("REMOTE_ADDR", addrbuf, 1);
> ...
> +	else {
> +		loginfo("[%"PRIuMAX"] Connection from %s:%d",
> +		    (uintmax_t)cld.pid, addrstr, port);
>  		add_child(&cld, addr, addrlen);

Hmm, loginfo() calls logreport() and adds the process information as necessary to the output. Wouldn't this patch give the pid information twice?

Erik Faye-Lund· Oct 14, 2010, 10:50 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 11/15] daemon: report connection from root-process

On Thu, Oct 14, 2010 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 40 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> Report incoming connections from the process that
>> accept() the connection instead of the handling
>> process.
>>
>> This enables "Connection from"-reporting on
>> Windows, where getpeername(0, ...) consistently
>> fails.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>>  daemon.c |   72 ++++++++++++++++++++++++++++++-------------------------------
>>  1 files changed, 35 insertions(+), 37 deletions(-)
>>
>> diff --git a/daemon.c b/daemon.c
>> index 8a44fb9..1574f75 100644
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -516,38 +516,11 @@ static void parse_host_arg(char *extra_args, int buflen)
>>  }
>>
>>
>> -static int execute(struct sockaddr *addr)
>> +static int execute(void)
>>  {
>> -...
>> -             }
>> -             loginfo("Connection from %s:%d", addrbuf, port);
>> -             setenv("REMOTE_ADDR", addrbuf, 1);
>> ...
>> +     else {
>> +             loginfo("[%"PRIuMAX"] Connection from %s:%d",
>> +                 (uintmax_t)cld.pid, addrstr, port);
>>               add_child(&cld, addr, addrlen);
>
> Hmm, loginfo() calls logreport() and adds the process information as
> necessary to the output.  Wouldn't this patch give the pid information
> twice?
>

Close, but not quite. logreport() reports the current PID, while this call to loginfo reports the PID of the child process. So two non-identical PIDs are reported.

The output becomes something like this:

[6408] [3868] Connection from [::1]:55801 [3868] Extended attributes (16 bytes) exist <host=localhost> [3868] Request upload-pack for '/some-repo.git' [3868] '/some-repo.git' does not appear to be a git repository [6408] [1876] Connection from [::1]:57311 [1876] Extended attributes (16 bytes) exist <host=localhost> [1876] Request upload-pack for '/some-repo.git' [1876] '/some-repo.git' does not appear to be a git repository

Junio C Hamano· Oct 17, 2010, 04:43 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 11/15] daemon: report connection from root-process

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 8 quoted lines
>> Hmm, loginfo() calls logreport() and adds the process information as
>> necessary to the output.  Wouldn't this patch give the pid information
>> twice?
>>
>
> Close, but not quite. logreport() reports the current PID, while this
> call to loginfo reports the PID of the child process. So two
> non-identical PIDs are reported.

I know that; I was questioning if that change to the log output is really what we want. I do not deeply care myself, but people with scripts that read logs might.

Show 11 quoted lines
>
> The output becomes something like this:
>
> [6408] [3868] Connection from [::1]:55801
> [3868] Extended attributes (16 bytes) exist <host=localhost>
> [3868] Request upload-pack for '/some-repo.git'
> [3868] '/some-repo.git' does not appear to be a git repository
> [6408] [1876] Connection from [::1]:57311
> [1876] Extended attributes (16 bytes) exist <host=localhost>
> [1876] Request upload-pack for '/some-repo.git'
> [1876] '/some-repo.git' does not appear to be a git repository
Erik Faye-Lund· Oct 17, 2010, 10:18 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 11/15] daemon: report connection from root-process

On Sun, Oct 17, 2010 at 6:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> Hmm, loginfo() calls logreport() and adds the process information as
>>> necessary to the output.  Wouldn't this patch give the pid information
>>> twice?
>>>
>>
>> Close, but not quite. logreport() reports the current PID, while this
>> call to loginfo reports the PID of the child process. So two
>> non-identical PIDs are reported.
>
> I know that; I was questioning if that change to the log output is really
> what we want.  I do not deeply care myself, but people with scripts that
> read logs might.
>

I could do something like this, but then we lose the port information. Perhaps I could add a REMOTE_PORT environment variable to solve that?

Show changes to daemon.c +5 −4
diff --git a/daemon.c b/daemon.c
index 589bd04..3d18899 100644
--- a/daemon.c
+++ b/daemon.c
@@ -522,6 +522,10 @@ static int execute(void)
 {
 	static char line[1000];
 	int pktlen, len, i;
+	char *addr = getenv("REMOTE_ADDR");
+
+	if (addr)
+		loginfo("Connection from %s", addr);

 	alarm(init_timeout ? init_timeout : timeout);
 	pktlen = packet_read_line(0, line, sizeof(line));
@@ -702,11 +706,8 @@ static void handle(int incoming, struct sockaddr
*addr, socklen_t addrlen)

 	if (start_command(&cld))
 		logerror("unable to fork");
-	else {
-		loginfo("[%"PRIuMAX"] Connection from %s:%d",
-		    (uintmax_t)cld.pid, addrstr, port);
+	else
 		add_child(&cld, addr, addrlen);
-	}
 	close(incoming);
 }
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 12/15] mingw: import poll-emulation from gnulib

copy lib/poll.c and lib/poll.in.h verbatim from commit 0a05120 in git://git.savannah.gnu.org/gnulib.git to compat/win32/sys/poll.[ch]

To upgrade this code in the future, branch out from this commit, copy new versions of the files above on top, and merge back the result.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/win32/sys/poll.c |  597 +++++++++++++++++++++++++++++++++++++++++++++++
 compat/win32/sys/poll.h |   53 +++++
 2 files changed, 650 insertions(+), 0 deletions(-)
 create mode 100644 compat/win32/sys/poll.c
 create mode 100644 compat/win32/sys/poll.h
Show changes to 2 files +650 −0

compat/win32/sys/poll.c, compat/win32/sys/poll.h

diff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c
new file mode 100644
index 0000000..7c52cb6
--- /dev/null
+++ b/compat/win32/sys/poll.c
@@ -0,0 +1,597 @@
+/* Emulation for poll(2)
+   Contributed by Paolo Bonzini.
+
+   Copyright 2001-2003, 2006-2010 Free Software Foundation, Inc.
+
+   This file is part of gnulib.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License along
+   with this program; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+/* Tell gcc not to warn about the (nfd < 0) tests, below.  */
+#if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
+# pragma GCC diagnostic ignored "-Wtype-limits"
+#endif
+
+#include <config.h>
+#include <alloca.h>
+
+#include <sys/types.h>
+#include "poll.h"
+#include <errno.h>
+#include <limits.h>
+#include <assert.h>
+
+#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+# define WIN32_NATIVE
+# include <winsock2.h>
+# include <windows.h>
+# include <io.h>
+# include <stdio.h>
+# include <conio.h>
+#else
+# include <sys/time.h>
+# include <sys/socket.h>
+# include <sys/select.h>
+# include <unistd.h>
+#endif
+
+#ifdef HAVE_SYS_IOCTL_H
+# include <sys/ioctl.h>
+#endif
+#ifdef HAVE_SYS_FILIO_H
+# include <sys/filio.h>
+#endif
+
+#include <time.h>
+
+#ifndef INFTIM
+# define INFTIM (-1)
+#endif
+
+/* BeOS does not have MSG_PEEK.  */
+#ifndef MSG_PEEK
+# define MSG_PEEK 0
+#endif
+
+#ifdef WIN32_NATIVE
+
+#define IsConsoleHandle(h) (((long) (h) & 3) == 3)
+
+static BOOL
+IsSocketHandle (HANDLE h)
+{
+  WSANETWORKEVENTS ev;
+
+  if (IsConsoleHandle (h))
+    return FALSE;
+
+  /* Under Wine, it seems that getsockopt returns 0 for pipes too.
+     WSAEnumNetworkEvents instead distinguishes the two correctly.  */
+  ev.lNetworkEvents = 0xDEADBEEF;
+  WSAEnumNetworkEvents ((SOCKET) h, NULL, &ev);
+  return ev.lNetworkEvents != 0xDEADBEEF;
+}
+
+/* Declare data structures for ntdll functions.  */
+typedef struct _FILE_PIPE_LOCAL_INFORMATION {
+  ULONG NamedPipeType;
+  ULONG NamedPipeConfiguration;
+  ULONG MaximumInstances;
+  ULONG CurrentInstances;
+  ULONG InboundQuota;
+  ULONG ReadDataAvailable;
+  ULONG OutboundQuota;
+  ULONG WriteQuotaAvailable;
+  ULONG NamedPipeState;
+  ULONG NamedPipeEnd;
+} FILE_PIPE_LOCAL_INFORMATION, *PFILE_PIPE_LOCAL_INFORMATION;
+
+typedef struct _IO_STATUS_BLOCK
+{
+  union {
+    DWORD Status;
+    PVOID Pointer;
+  } u;
+  ULONG_PTR Information;
+} IO_STATUS_BLOCK, *PIO_STATUS_BLOCK;
+
+typedef enum _FILE_INFORMATION_CLASS {
+  FilePipeLocalInformation = 24
+} FILE_INFORMATION_CLASS, *PFILE_INFORMATION_CLASS;
+
+typedef DWORD (WINAPI *PNtQueryInformationFile)
+         (HANDLE, IO_STATUS_BLOCK *, VOID *, ULONG, FILE_INFORMATION_CLASS);
+
+# ifndef PIPE_BUF
+#  define PIPE_BUF      512
+# endif
+
+/* Compute revents values for file handle H.  If some events cannot happen
+   for the handle, eliminate them from *P_SOUGHT.  */
+
+static int
+win32_compute_revents (HANDLE h, int *p_sought)
+{
+  int i, ret, happened;
+  INPUT_RECORD *irbuffer;
+  DWORD avail, nbuffer;
+  BOOL bRet;
+  IO_STATUS_BLOCK iosb;
+  FILE_PIPE_LOCAL_INFORMATION fpli;
+  static PNtQueryInformationFile NtQueryInformationFile;
+  static BOOL once_only;
+
+  switch (GetFileType (h))
+    {
+    case FILE_TYPE_PIPE:
+      if (!once_only)
+        {
+          NtQueryInformationFile = (PNtQueryInformationFile)
+            GetProcAddress (GetModuleHandle ("ntdll.dll"),
+                            "NtQueryInformationFile");
+          once_only = TRUE;
+        }
+
+      happened = 0;
+      if (PeekNamedPipe (h, NULL, 0, NULL, &avail, NULL) != 0)
+        {
+          if (avail)
+            happened |= *p_sought & (POLLIN | POLLRDNORM);
+        }
+      else if (GetLastError () == ERROR_BROKEN_PIPE)
+        happened |= POLLHUP;
+
+      else
+        {
+          /* It was the write-end of the pipe.  Check if it is writable.
+             If NtQueryInformationFile fails, optimistically assume the pipe is
+             writable.  This could happen on Win9x, where NtQueryInformationFile
+             is not available, or if we inherit a pipe that doesn't permit
+             FILE_READ_ATTRIBUTES access on the write end (I think this should
+             not happen since WinXP SP2; WINE seems fine too).  Otherwise,
+             ensure that enough space is available for atomic writes.  */
+          memset (&iosb, 0, sizeof (iosb));
+          memset (&fpli, 0, sizeof (fpli));
+
+          if (!NtQueryInformationFile
+              || NtQueryInformationFile (h, &iosb, &fpli, sizeof (fpli),
+                                         FilePipeLocalInformation)
+              || fpli.WriteQuotaAvailable >= PIPE_BUF
+              || (fpli.OutboundQuota < PIPE_BUF &&
+                  fpli.WriteQuotaAvailable == fpli.OutboundQuota))
+            happened |= *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);
+        }
+      return happened;
+
+    case FILE_TYPE_CHAR:
+      ret = WaitForSingleObject (h, 0);
+      if (!IsConsoleHandle (h))
+        return ret == WAIT_OBJECT_0 ? *p_sought & ~(POLLPRI | POLLRDBAND) : 0;
+
+      nbuffer = avail = 0;
+      bRet = GetNumberOfConsoleInputEvents (h, &nbuffer);
+      if (bRet)
+        {
+          /* Input buffer.  */
+          *p_sought &= POLLIN | POLLRDNORM;
+          if (nbuffer == 0)
+            return POLLHUP;
+          if (!*p_sought)
+            return 0;
+
+          irbuffer = (INPUT_RECORD *) alloca (nbuffer * sizeof (INPUT_RECORD));
+          bRet = PeekConsoleInput (h, irbuffer, nbuffer, &avail);
+          if (!bRet || avail == 0)
+            return POLLHUP;
+
+          for (i = 0; i < avail; i++)
+            if (irbuffer[i].EventType == KEY_EVENT)
+              return *p_sought;
+          return 0;
+        }
+      else
+        {
+          /* Screen buffer.  */
+          *p_sought &= POLLOUT | POLLWRNORM | POLLWRBAND;
+          return *p_sought;
+        }
+
+    default:
+      ret = WaitForSingleObject (h, 0);
+      if (ret == WAIT_OBJECT_0)
+        return *p_sought & ~(POLLPRI | POLLRDBAND);
+
+      return *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);
+    }
+}
+
+/* Convert fd_sets returned by select into revents values.  */
+
+static int
+win32_compute_revents_socket (SOCKET h, int sought, long lNetworkEvents)
+{
+  int happened = 0;
+
+  if ((lNetworkEvents & (FD_READ | FD_ACCEPT | FD_CLOSE)) == FD_ACCEPT)
+    happened |= (POLLIN | POLLRDNORM) & sought;
+
+  else if (lNetworkEvents & (FD_READ | FD_ACCEPT | FD_CLOSE))
+    {
+      int r, error;
+
+      char data[64];
+      WSASetLastError (0);
+      r = recv (h, data, sizeof (data), MSG_PEEK);
+      error = WSAGetLastError ();
+      WSASetLastError (0);
+
+      if (r > 0 || error == WSAENOTCONN)
+        happened |= (POLLIN | POLLRDNORM) & sought;
+
+      /* Distinguish hung-up sockets from other errors.  */
+      else if (r == 0 || error == WSAESHUTDOWN || error == WSAECONNRESET
+               || error == WSAECONNABORTED || error == WSAENETRESET)
+        happened |= POLLHUP;
+
+      else
+        happened |= POLLERR;
+    }
+
+  if (lNetworkEvents & (FD_WRITE | FD_CONNECT))
+    happened |= (POLLOUT | POLLWRNORM | POLLWRBAND) & sought;
+
+  if (lNetworkEvents & FD_OOB)
+    happened |= (POLLPRI | POLLRDBAND) & sought;
+
+  return happened;
+}
+
+#else /* !MinGW */
+
+/* Convert select(2) returned fd_sets into poll(2) revents values.  */
+static int
+compute_revents (int fd, int sought, fd_set *rfds, fd_set *wfds, fd_set *efds)
+{
+  int happened = 0;
+  if (FD_ISSET (fd, rfds))
+    {
+      int r;
+      int socket_errno;
+
+# if defined __MACH__ && defined __APPLE__
+      /* There is a bug in Mac OS X that causes it to ignore MSG_PEEK
+         for some kinds of descriptors.  Detect if this descriptor is a
+         connected socket, a server socket, or something else using a
+         0-byte recv, and use ioctl(2) to detect POLLHUP.  */
+      r = recv (fd, NULL, 0, MSG_PEEK);
+      socket_errno = (r < 0) ? errno : 0;
+      if (r == 0 || socket_errno == ENOTSOCK)
+        ioctl (fd, FIONREAD, &r);
+# else
+      char data[64];
+      r = recv (fd, data, sizeof (data), MSG_PEEK);
+      socket_errno = (r < 0) ? errno : 0;
+# endif
+      if (r == 0)
+        happened |= POLLHUP;
+
+      /* If the event happened on an unconnected server socket,
+         that's fine. */
+      else if (r > 0 || ( /* (r == -1) && */ socket_errno == ENOTCONN))
+        happened |= (POLLIN | POLLRDNORM) & sought;
+
+      /* Distinguish hung-up sockets from other errors.  */
+      else if (socket_errno == ESHUTDOWN || socket_errno == ECONNRESET
+               || socket_errno == ECONNABORTED || socket_errno == ENETRESET)
+        happened |= POLLHUP;
+
+      else
+        happened |= POLLERR;
+    }
+
+  if (FD_ISSET (fd, wfds))
+    happened |= (POLLOUT | POLLWRNORM | POLLWRBAND) & sought;
+
+  if (FD_ISSET (fd, efds))
+    happened |= (POLLPRI | POLLRDBAND) & sought;
+
+  return happened;
+}
+#endif /* !MinGW */
+
+int
+poll (pfd, nfd, timeout)
+     struct pollfd *pfd;
+     nfds_t nfd;
+     int timeout;
+{
+#ifndef WIN32_NATIVE
+  fd_set rfds, wfds, efds;
+  struct timeval tv;
+  struct timeval *ptv;
+  int maxfd, rc;
+  nfds_t i;
+
+# ifdef _SC_OPEN_MAX
+  static int sc_open_max = -1;
+
+  if (nfd < 0
+      || (nfd > sc_open_max
+          && (sc_open_max != -1
+              || nfd > (sc_open_max = sysconf (_SC_OPEN_MAX)))))
+    {
+      errno = EINVAL;
+      return -1;
+    }
+# else /* !_SC_OPEN_MAX */
+#  ifdef OPEN_MAX
+  if (nfd < 0 || nfd > OPEN_MAX)
+    {
+      errno = EINVAL;
+      return -1;
+    }
+#  endif /* OPEN_MAX -- else, no check is needed */
+# endif /* !_SC_OPEN_MAX */
+
+  /* EFAULT is not necessary to implement, but let's do it in the
+     simplest case. */
+  if (!pfd)
+    {
+      errno = EFAULT;
+      return -1;
+    }
+
+  /* convert timeout number into a timeval structure */
+  if (timeout == 0)
+    {
+      ptv = &tv;
+      ptv->tv_sec = 0;
+      ptv->tv_usec = 0;
+    }
+  else if (timeout > 0)
+    {
+      ptv = &tv;
+      ptv->tv_sec = timeout / 1000;
+      ptv->tv_usec = (timeout % 1000) * 1000;
+    }
+  else if (timeout == INFTIM)
+    /* wait forever */
+    ptv = NULL;
+  else
+    {
+      errno = EINVAL;
+      return -1;
+    }
+
+  /* create fd sets and determine max fd */
+  maxfd = -1;
+  FD_ZERO (&rfds);
+  FD_ZERO (&wfds);
+  FD_ZERO (&efds);
+  for (i = 0; i < nfd; i++)
+    {
+      if (pfd[i].fd < 0)
+        continue;
+
+      if (pfd[i].events & (POLLIN | POLLRDNORM))
+        FD_SET (pfd[i].fd, &rfds);
+
+      /* see select(2): "the only exceptional condition detectable
+         is out-of-band data received on a socket", hence we push
+         POLLWRBAND events onto wfds instead of efds. */
+      if (pfd[i].events & (POLLOUT | POLLWRNORM | POLLWRBAND))
+        FD_SET (pfd[i].fd, &wfds);
+      if (pfd[i].events & (POLLPRI | POLLRDBAND))
+        FD_SET (pfd[i].fd, &efds);
+      if (pfd[i].fd >= maxfd
+          && (pfd[i].events & (POLLIN | POLLOUT | POLLPRI
+                               | POLLRDNORM | POLLRDBAND
+                               | POLLWRNORM | POLLWRBAND)))
+        {
+          maxfd = pfd[i].fd;
+          if (maxfd > FD_SETSIZE)
+            {
+              errno = EOVERFLOW;
+              return -1;
+            }
+        }
+    }
+
+  /* examine fd sets */
+  rc = select (maxfd + 1, &rfds, &wfds, &efds, ptv);
+  if (rc < 0)
+    return rc;
+
+  /* establish results */
+  rc = 0;
+  for (i = 0; i < nfd; i++)
+    if (pfd[i].fd < 0)
+      pfd[i].revents = 0;
+    else
+      {
+        int happened = compute_revents (pfd[i].fd, pfd[i].events,
+                                        &rfds, &wfds, &efds);
+        if (happened)
+          {
+            pfd[i].revents = happened;
+            rc++;
+          }
+      }
+
+  return rc;
+#else
+  static struct timeval tv0;
+  static HANDLE hEvent;
+  WSANETWORKEVENTS ev;
+  HANDLE h, handle_array[FD_SETSIZE + 2];
+  DWORD ret, wait_timeout, nhandles;
+  fd_set rfds, wfds, xfds;
+  BOOL poll_again;
+  MSG msg;
+  int rc = 0;
+  nfds_t i;
+
+  if (nfd < 0 || timeout < -1)
+    {
+      errno = EINVAL;
+      return -1;
+    }
+
+  if (!hEvent)
+    hEvent = CreateEvent (NULL, FALSE, FALSE, NULL);
+
+  handle_array[0] = hEvent;
+  nhandles = 1;
+  FD_ZERO (&rfds);
+  FD_ZERO (&wfds);
+  FD_ZERO (&xfds);
+
+  /* Classify socket handles and create fd sets. */
+  for (i = 0; i < nfd; i++)
+    {
+      int sought = pfd[i].events;
+      pfd[i].revents = 0;
+      if (pfd[i].fd < 0)
+        continue;
+      if (!(sought & (POLLIN | POLLRDNORM | POLLOUT | POLLWRNORM | POLLWRBAND
+                      | POLLPRI | POLLRDBAND)))
+        continue;
+
+      h = (HANDLE) _get_osfhandle (pfd[i].fd);
+      assert (h != NULL);
+      if (IsSocketHandle (h))
+        {
+          int requested = FD_CLOSE;
+
+          /* see above; socket handles are mapped onto select.  */
+          if (sought & (POLLIN | POLLRDNORM))
+            {
+              requested |= FD_READ | FD_ACCEPT;
+              FD_SET ((SOCKET) h, &rfds);
+            }
+          if (sought & (POLLOUT | POLLWRNORM | POLLWRBAND))
+            {
+              requested |= FD_WRITE | FD_CONNECT;
+              FD_SET ((SOCKET) h, &wfds);
+            }
+          if (sought & (POLLPRI | POLLRDBAND))
+            {
+              requested |= FD_OOB;
+              FD_SET ((SOCKET) h, &xfds);
+            }
+
+          if (requested)
+            WSAEventSelect ((SOCKET) h, hEvent, requested);
+        }
+      else
+        {
+          /* Poll now.  If we get an event, do not poll again.  Also,
+             screen buffer handles are waitable, and they'll block until
+             a character is available.  win32_compute_revents eliminates
+             bits for the "wrong" direction. */
+          pfd[i].revents = win32_compute_revents (h, &sought);
+          if (sought)
+            handle_array[nhandles++] = h;
+          if (pfd[i].revents)
+            timeout = 0;
+        }
+    }
+
+  if (select (0, &rfds, &wfds, &xfds, &tv0) > 0)
+    {
+      /* Do MsgWaitForMultipleObjects anyway to dispatch messages, but
+         no need to call select again.  */
+      poll_again = FALSE;
+      wait_timeout = 0;
+    }
+  else
+    {
+      poll_again = TRUE;
+      if (timeout == INFTIM)
+        wait_timeout = INFINITE;
+      else
+        wait_timeout = timeout;
+    }
+
+  for (;;)
+    {
+      ret = MsgWaitForMultipleObjects (nhandles, handle_array, FALSE,
+                                       wait_timeout, QS_ALLINPUT);
+
+      if (ret == WAIT_OBJECT_0 + nhandles)
+        {
+          /* new input of some other kind */
+          BOOL bRet;
+          while ((bRet = PeekMessage (&msg, NULL, 0, 0, PM_REMOVE)) != 0)
+            {
+              TranslateMessage (&msg);
+              DispatchMessage (&msg);
+            }
+        }
+      else
+        break;
+    }
+
+  if (poll_again)
+    select (0, &rfds, &wfds, &xfds, &tv0);
+
+  /* Place a sentinel at the end of the array.  */
+  handle_array[nhandles] = NULL;
+  nhandles = 1;
+  for (i = 0; i < nfd; i++)
+    {
+      int happened;
+
+      if (pfd[i].fd < 0)
+        continue;
+      if (!(pfd[i].events & (POLLIN | POLLRDNORM |
+                             POLLOUT | POLLWRNORM | POLLWRBAND)))
+        continue;
+
+      h = (HANDLE) _get_osfhandle (pfd[i].fd);
+      if (h != handle_array[nhandles])
+        {
+          /* It's a socket.  */
+          WSAEnumNetworkEvents ((SOCKET) h, NULL, &ev);
+          WSAEventSelect ((SOCKET) h, 0, 0);
+
+          /* If we're lucky, WSAEnumNetworkEvents already provided a way
+             to distinguish FD_READ and FD_ACCEPT; this saves a recv later.  */
+          if (FD_ISSET ((SOCKET) h, &rfds)
+              && !(ev.lNetworkEvents & (FD_READ | FD_ACCEPT)))
+            ev.lNetworkEvents |= FD_READ | FD_ACCEPT;
+          if (FD_ISSET ((SOCKET) h, &wfds))
+            ev.lNetworkEvents |= FD_WRITE | FD_CONNECT;
+          if (FD_ISSET ((SOCKET) h, &xfds))
+            ev.lNetworkEvents |= FD_OOB;
+
+          happened = win32_compute_revents_socket ((SOCKET) h, pfd[i].events,
+                                                   ev.lNetworkEvents);
+        }
+      else
+        {
+          /* Not a socket.  */
+          int sought = pfd[i].events;
+          happened = win32_compute_revents (h, &sought);
+          nhandles++;
+        }
+
+       if ((pfd[i].revents |= happened) != 0)
+        rc++;
+    }
+
+  return rc;
+#endif
+}
diff --git a/compat/win32/sys/poll.h b/compat/win32/sys/poll.h
new file mode 100644
index 0000000..b7aa59d
--- /dev/null
+++ b/compat/win32/sys/poll.h
@@ -0,0 +1,53 @@
+/* Header for poll(2) emulation
+   Contributed by Paolo Bonzini.
+
+   Copyright 2001, 2002, 2003, 2007, 2009, 2010 Free Software Foundation, Inc.
+
+   This file is part of gnulib.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License along
+   with this program; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+#ifndef _GL_POLL_H
+#define _GL_POLL_H
+
+/* fake a poll(2) environment */
+#define POLLIN      0x0001      /* any readable data available   */
+#define POLLPRI     0x0002      /* OOB/Urgent readable data      */
+#define POLLOUT     0x0004      /* file descriptor is writeable  */
+#define POLLERR     0x0008      /* some poll error occurred      */
+#define POLLHUP     0x0010      /* file descriptor was "hung up" */
+#define POLLNVAL    0x0020      /* requested events "invalid"    */
+#define POLLRDNORM  0x0040
+#define POLLRDBAND  0x0080
+#define POLLWRNORM  0x0100
+#define POLLWRBAND  0x0200
+
+struct pollfd
+{
+  int fd;                       /* which file descriptor to poll */
+  short events;                 /* events we are interested in   */
+  short revents;                /* events found on return        */
+};
+
+typedef unsigned long nfds_t;
+
+extern int poll (struct pollfd *pfd, nfds_t nfd, int timeout);
+
+/* Define INFTIM only if doing so conforms to POSIX.  */
+#if !defined (_POSIX_C_SOURCE) && !defined (_XOPEN_SOURCE)
+#define INFTIM (-1)
+#endif
+
+#endif /* _GL_POLL_H */
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 13/15] mingw: use poll-emulation from gnulib

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 Makefile                |    6 +++-
 compat/mingw.c          |   65 -----------------------------------------------
 compat/mingw.h          |   11 --------
 compat/win32/sys/poll.c |    3 +-
 git-compat-util.h       |    2 +-
 5 files changed, 6 insertions(+), 81 deletions(-)
Show changes to 5 files +6 −81

Makefile, compat/mingw.c, compat/mingw.h, compat/win32/sys/poll.c, git-compat-util.h

diff --git a/Makefile b/Makefile
index 2aa067a..46034bf 100644
--- a/Makefile
+++ b/Makefile
@@ -497,6 +497,7 @@ LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
 LIB_H += compat/win32/pthread.h
 LIB_H += compat/win32/syslog.h
+LIB_H += compat/win32/sys/poll.h
 LIB_H += csum-file.h
 LIB_H += decorate.h
 LIB_H += delta.h
@@ -1082,7 +1083,7 @@ ifeq ($(uname_S),Windows)
 	AR = compat/vcbuild/scripts/lib.pl
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
-	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o compat/win32/syslog.o
+	COMPAT_OBJS = compat/msvc.o compat/fnmatch/fnmatch.o compat/winansi.o compat/win32/pthread.o compat/win32/syslog.o compat/win32/sys/poll.o
 	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/fnmatch -Icompat/regex -Icompat/fnmatch -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
 	EXTLIBS = advapi32.lib shell32.lib wininet.lib ws2_32.lib
@@ -1132,7 +1133,8 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
-		compat/win32/pthread.o compat/win32/syslog.o
+		compat/win32/pthread.o compat/win32/syslog.o \
+		compat/win32/sys/poll.o
 	EXTLIBS += -lws2_32
 	PTHREAD_LIBS =
 	X = .exe
diff --git a/compat/mingw.c b/compat/mingw.c
index d88c0d0..b780200 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -408,71 +408,6 @@ int pipe(int filedes[2])
 	return 0;
 }
 
-int poll(struct pollfd *ufds, unsigned int nfds, int timeout)
-{
-	int i, pending;
-
-	if (timeout >= 0) {
-		if (nfds == 0) {
-			Sleep(timeout);
-			return 0;
-		}
-		return errno = EINVAL, error("poll timeout not supported");
-	}
-
-	/* When there is only one fd to wait for, then we pretend that
-	 * input is available and let the actual wait happen when the
-	 * caller invokes read().
-	 */
-	if (nfds == 1) {
-		if (!(ufds[0].events & POLLIN))
-			return errno = EINVAL, error("POLLIN not set");
-		ufds[0].revents = POLLIN;
-		return 0;
-	}
-
-repeat:
-	pending = 0;
-	for (i = 0; i < nfds; i++) {
-		DWORD avail = 0;
-		HANDLE h = (HANDLE) _get_osfhandle(ufds[i].fd);
-		if (h == INVALID_HANDLE_VALUE)
-			return -1;	/* errno was set */
-
-		if (!(ufds[i].events & POLLIN))
-			return errno = EINVAL, error("POLLIN not set");
-
-		/* this emulation works only for pipes */
-		if (!PeekNamedPipe(h, NULL, 0, NULL, &avail, NULL)) {
-			int err = GetLastError();
-			if (err == ERROR_BROKEN_PIPE) {
-				ufds[i].revents = POLLHUP;
-				pending++;
-			} else {
-				errno = EINVAL;
-				return error("PeekNamedPipe failed,"
-					" GetLastError: %u", err);
-			}
-		} else if (avail) {
-			ufds[i].revents = POLLIN;
-			pending++;
-		} else
-			ufds[i].revents = 0;
-	}
-	if (!pending) {
-		/* The only times that we spin here is when the process
-		 * that is connected through the pipes is waiting for
-		 * its own input data to become available. But since
-		 * the process (pack-objects) is itself CPU intensive,
-		 * it will happily pick up the time slice that we are
-		 * relinquishing here.
-		 */
-		Sleep(0);
-		goto repeat;
-	}
-	return 0;
-}
-
 struct tm *gmtime_r(const time_t *timep, struct tm *result)
 {
 	/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */
diff --git a/compat/mingw.h b/compat/mingw.h
index 51fca2f..99a7467 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -59,16 +59,6 @@ struct passwd {
 
 extern char *getpass(const char *prompt);
 
-#ifndef POLLIN
-struct pollfd {
-	int fd;           /* file descriptor */
-	short events;     /* requested events */
-	short revents;    /* returned events */
-};
-#define POLLIN 1
-#define POLLHUP 2
-#endif
-
 typedef void (__cdecl *sig_handler_t)(int);
 struct sigaction {
 	sig_handler_t sa_handler;
@@ -175,7 +165,6 @@ int pipe(int filedes[2]);
 unsigned int sleep (unsigned int seconds);
 int mkstemp(char *template);
 int gettimeofday(struct timeval *tv, void *tz);
-int poll(struct pollfd *ufds, unsigned int nfds, int timeout);
 struct tm *gmtime_r(const time_t *timep, struct tm *result);
 struct tm *localtime_r(const time_t *timep, struct tm *result);
 int getpagesize(void);	/* defined in MinGW's libgcc.a */
diff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c
index 7c52cb6..c1ca0d2 100644
--- a/compat/win32/sys/poll.c
+++ b/compat/win32/sys/poll.c
@@ -24,8 +24,7 @@
 # pragma GCC diagnostic ignored "-Wtype-limits"
 #endif
 
-#include <config.h>
-#include <alloca.h>
+#include <malloc.h>
 
 #include <sys/types.h>
 #include "poll.h"
diff --git a/git-compat-util.h b/git-compat-util.h
index 56dce85..d0a1e48 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -105,9 +105,9 @@
 #include <regex.h>
 #include <utime.h>
 #include <syslog.h>
+#include <sys/poll.h>
 #ifndef __MINGW32__
 #include <sys/wait.h>
-#include <sys/poll.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <termios.h>
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 14/15] daemon: use socklen_t

---
 daemon.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to daemon.c +3 −3
diff --git a/daemon.c b/daemon.c
index 1574f75..9b97b58 100644
--- a/daemon.c
+++ b/daemon.c
@@ -593,7 +593,7 @@ static struct child {
 	struct sockaddr_storage address;
 } *firstborn;
 
-static void add_child(struct child_process *cld, struct sockaddr *addr, int addrlen)
+static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen)
 {
 	struct child *newborn, **cradle;
 
@@ -672,7 +672,7 @@ static char *get_addrstr(int *port, struct sockaddr *addr)
 }
 
 static char **cld_argv;
-static void handle(int incoming, struct sockaddr *addr, int addrlen)
+static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
 {
 	struct child_process cld = { 0 };
 	char *addrstr, envbuf[300] = "REMOTE_ADDR=";
@@ -908,7 +908,7 @@ static int service_loop(struct socketlist *socklist)
 		for (i = 0; i < socklist->nr; 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) {
-- 
1.7.3.1.199.g72340
Erik Faye-Lund· Oct 11, 2010, 21:50 UTC · re: Erik Faye-Lund · lore

[PATCH v4 15/15] daemon: opt-out on features that require posix

Windows does not supply the POSIX-functions fork(), setuuid(), setgid(), setsid() and initgroups(). Disable support for --user, --group and --detach if the NO_POSIX_GOODIES flag is set.

MinGW doesn't have prototypes and headers for inet_ntop and inet_pton, so include our implementation instead. MSVC does have, so avoid doing so there.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 Makefile |   14 +++++++++-----
 daemon.c |   37 ++++++++++++++++++++++++++++---------
 2 files changed, 37 insertions(+), 14 deletions(-)
Show changes to 2 files +37 −14

Makefile, daemon.c

diff --git a/Makefile b/Makefile
index 46034bf..53986b1 100644
--- a/Makefile
+++ b/Makefile
@@ -401,6 +401,7 @@ EXTRA_PROGRAMS =
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS += $(EXTRA_PROGRAMS)
 
+PROGRAM_OBJS += daemon.o
 PROGRAM_OBJS += fast-import.o
 PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += shell.o
@@ -1066,7 +1067,6 @@ ifeq ($(uname_S),Windows)
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1077,6 +1077,7 @@ ifeq ($(uname_S),Windows)
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	NATIVE_CRLF = YesPlease
 
 	CC = compat/vcbuild/scripts/clink.pl
@@ -1119,7 +1120,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1130,6 +1130,9 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
 	ETAGS_TARGET = ETAGS
+	NO_INET_PTON = YesPlease
+	NO_INET_NTOP = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
@@ -1249,9 +1252,6 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz
 
-ifndef NO_POSIX_ONLY_PROGRAMS
-	PROGRAM_OBJS += daemon.o
-endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
@@ -1419,6 +1419,10 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif
 
+ifdef NO_POSIX_GOODIES
+	BASIC_CFLAGS += -DNO_POSIX_GOODIES
+endif
+
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
diff --git a/daemon.c b/daemon.c
index 9b97b58..aa580f6 100644
--- a/daemon.c
+++ b/daemon.c
@@ -23,10 +23,12 @@ static const char daemon_usage[] =
 "           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
 "           [--interpolated-path=path]\n"
-"           [--reuseaddr] [--detach] [--pid-file=file]\n"
+"           [--reuseaddr] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
-"                      [--user=user [--group=group]]\n"
+#ifndef NO_POSIX_GOODIES
+"           [--detach] [--user=user [--group=group]]\n"
+#endif
 "           [directory...]";
 
 /* List of acceptable pathname prefixes */
@@ -938,6 +940,7 @@ static void sanitize_stdfds(void)
 		close(fd);
 }
 
+#ifndef NO_POSIX_GOODIES
 static void daemonize(void)
 {
 	switch (fork()) {
@@ -955,6 +958,7 @@ static void daemonize(void)
 	close(2);
 	sanitize_stdfds();
 }
+#endif
 
 static void store_pid(const char *path)
 {
@@ -965,7 +969,12 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }
 
-static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
+#ifndef NO_POSIX_GOODIES
+static struct passwd *pass;
+static gid_t gid;
+#endif
+
+static int serve(struct string_list *listen_addr, int listen_port)
 {
 	struct socketlist socklist = { NULL, 0, 0 };
 
@@ -974,10 +983,12 @@ static int serve(struct string_list *listen_addr, int listen_port, struct passwd
 		die("unable to allocate any listen sockets on port %u",
 		    listen_port);
 
+#ifndef NO_POSIX_GOODIES
 	if (pass && gid &&
 	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
 	     setuid(pass->pw_uid)))
 		die("cannot drop privileges");
+#endif
 
 	return service_loop(&socklist);
 }
@@ -987,11 +998,11 @@ int main(int argc, char **argv)
 	int listen_port = 0;
 	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
 	int serve_mode = 0, inetd_mode = 0;
-	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
+	const char *pid_file = NULL;
+#ifndef NO_POSIX_GOODIES
 	int detach = 0;
-	struct passwd *pass = NULL;
-	struct group *group;
-	gid_t gid = 0;
+	const char *user_name = NULL, *group_name = NULL;
+#endif
 	int i;
 
 	git_extract_argv0_path(argv[0]);
@@ -1080,6 +1091,7 @@ int main(int argc, char **argv)
 			pid_file = arg + 11;
 			continue;
 		}
+#ifndef NO_POSIX_GOODIES
 		if (!strcmp(arg, "--detach")) {
 			detach = 1;
 			log_syslog = 1;
@@ -1093,6 +1105,7 @@ int main(int argc, char **argv)
 			group_name = arg + 8;
 			continue;
 		}
+#endif
 		if (!prefixcmp(arg, "--enable=")) {
 			enable_service(arg + 9, 1);
 			continue;
@@ -1127,14 +1140,17 @@ int main(int argc, char **argv)
 		/* avoid splitting a message in the middle */
 		setvbuf(stderr, NULL, _IOFBF, 4096);
 
+#ifndef NO_POSIX_GOODIES
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
+#endif
 
 	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;
 
+#ifndef NO_POSIX_GOODIES
 	if (group_name && !user_name)
 		die("--group supplied without --user");
 
@@ -1146,13 +1162,14 @@ 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;
 		}
 	}
+#endif
 
 	if (strict_paths && (!ok_paths || !*ok_paths))
 		die("option --strict-paths requires a whitelist");
@@ -1168,11 +1185,13 @@ int main(int argc, char **argv)
 		return execute();
 	}
 
+#ifndef NO_POSIX_GOODIES
 	if (detach) {
 		daemonize();
 		loginfo("Ready to rumble");
 	}
 	else
+#endif
 		sanitize_stdfds();
 
 	if (pid_file)
@@ -1185,5 +1204,5 @@ int main(int argc, char **argv)
 	cld_argv[argc] = "--serve";
 	cld_argv[argc+1] = NULL;
 
-	return serve(&listen_addr, listen_port, pass, gid);
+	return serve(&listen_addr, listen_port);
 }
-- 
1.7.3.1.199.g72340
Junio C Hamano· Oct 13, 2010, 23:02 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 28 quoted lines
> Windows does not supply the POSIX-functions fork(), setuuid(), setgid(),
> setsid() and initgroups(). Disable support for --user, --group and
> --detach if the NO_POSIX_GOODIES flag is set.
>
> MinGW doesn't have prototypes and headers for inet_ntop and inet_pton,
> so include our implementation instead. MSVC does have, so avoid doing
> so there.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
> diff --git a/daemon.c b/daemon.c
> index 9b97b58..aa580f6 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -965,7 +969,12 @@ static void store_pid(const char *path)
>  		die_errno("failed to write pid file '%s'", path);
>  }
>  
> -static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
> +#ifndef NO_POSIX_GOODIES
> +static struct passwd *pass;
> +static gid_t gid;
> +#endif
> +
> +static int serve(struct string_list *listen_addr, int listen_port)
>  {
>  	struct socketlist socklist = { NULL, 0, 0 };
>  
This is ugly.  Why did you need to make the arguments file-scope static?
Show 10 quoted lines
> @@ -974,10 +983,12 @@ static int serve(struct string_list *listen_addr, int listen_port, struct passwd
>  		die("unable to allocate any listen sockets on port %u",
>  		    listen_port);
>  
> +#ifndef NO_POSIX_GOODIES
>  	if (pass && gid &&
>  	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
>  	     setuid(pass->pw_uid)))
>  		die("cannot drop privileges");
> +#endif

It would be cleaner to make a helper (e.g. "drop-privileges") that is a no-op on NO_POSIX_GOODIES platform, and call that without #ifdef here.

The same aversion to too many #ifdef's apply to the rest of the patch.
Erik Faye-Lund· Oct 14, 2010, 11:02 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

On Thu, Oct 14, 2010 at 1:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> Windows does not supply the POSIX-functions fork(), setuuid(), setgid(),
>> setsid() and initgroups(). Disable support for --user, --group and
>> --detach if the NO_POSIX_GOODIES flag is set.
>>
>> MinGW doesn't have prototypes and headers for inet_ntop and inet_pton,
>> so include our implementation instead. MSVC does have, so avoid doing
>> so there.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>> diff --git a/daemon.c b/daemon.c
>> index 9b97b58..aa580f6 100644
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -965,7 +969,12 @@ static void store_pid(const char *path)
>>               die_errno("failed to write pid file '%s'", path);
>>  }
>>
>> -static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>> +#ifndef NO_POSIX_GOODIES
>> +static struct passwd *pass;
>> +static gid_t gid;
>> +#endif
>> +
>> +static int serve(struct string_list *listen_addr, int listen_port)
>>  {
>>       struct socketlist socklist = { NULL, 0, 0 };
>>
>
> This is ugly.  Why did you need to make the arguments file-scope static?
>

To avoid having different signatures for the serve-function dependent on NO_POSIX_GOODIES.

Do you have any other suggestions on how to do this? Perhaps I should just move the logic in serve() to the end of main()? It's the only call-site for the function, and would remove the need for a function prototype all-together...

Show 14 quoted lines
>> @@ -974,10 +983,12 @@ static int serve(struct string_list *listen_addr, int listen_port, struct passwd
>>               die("unable to allocate any listen sockets on port %u",
>>                   listen_port);
>>
>> +#ifndef NO_POSIX_GOODIES
>>       if (pass && gid &&
>>           (initgroups(pass->pw_name, gid) || setgid (gid) ||
>>            setuid(pass->pw_uid)))
>>               die("cannot drop privileges");
>> +#endif
>
> It would be cleaner to make a helper (e.g. "drop-privileges") that is a
> no-op on NO_POSIX_GOODIES platform, and call that without #ifdef here.
>
Sure, makes sense.
> The same aversion to too many #ifdef's apply to the rest of the patch.
>

I can remove some of them, like keeping the variables in main() around, even though they'll be constant. That might cause some compile-time warnings, though.

Junio C Hamano· Oct 15, 2010, 21:16 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 16 quoted lines
>>> -static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>>> +#ifndef NO_POSIX_GOODIES
>>> +static struct passwd *pass;
>>> +static gid_t gid;
>>> +#endif
>>> +
>>> +static int serve(struct string_list *listen_addr, int listen_port)
>>>  {
>>>       struct socketlist socklist = { NULL, 0, 0 };
>>>
>>
>> This is ugly.  Why did you need to make the arguments file-scope static?
>>
>
> To avoid having different signatures for the serve-function dependent
> on NO_POSIX_GOODIES.

Why does the signature even have to be different between the two to begin with? I _think_ you have gid_t over there, although you might not have "struct passwd", in which case you can just define an empty one that your alternate implementation is not going to use anyway. This is especially true if you are making the "drop-privileges" part a helper function, no?

Erik Faye-Lund· Oct 18, 2010, 12:05 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>>> -static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>>>> +#ifndef NO_POSIX_GOODIES
>>>> +static struct passwd *pass;
>>>> +static gid_t gid;
>>>> +#endif
>>>> +
>>>> +static int serve(struct string_list *listen_addr, int listen_port)
>>>>  {
>>>>       struct socketlist socklist = { NULL, 0, 0 };
>>>>
>>>
>>> This is ugly.  Why did you need to make the arguments file-scope static?
>>>
>>
>> To avoid having different signatures for the serve-function dependent
>> on NO_POSIX_GOODIES.
>
> Why does the signature even have to be different between the two to begin
> with? I _think_ you have gid_t over there

We don't, so this is the primary reason. But also avoiding compilation-warnings is a secondary motivation.

> although you might not have
> "struct passwd", in which case you can just define an empty one that your
> alternate implementation is not going to use anyway.

We do, so this becomes a bit of a hypothetical question. But would you seriously consider pretending to have a posix-feature less ugly than inlining a function that is only used once?

(I'm going a little off-topic here, I hope that's OK) I'm not too happy with some of the pretend-really-hard-to-be-posix-magic around in the Windows-port. In fact, I have some patches to reduce posixness in some areas, while getting rid of some code in mingw.c. Would such patches be welcome, or is pretend-to-be-posix the governing portability approach? In some cases, this comes at the expense of some performance (and quite a bit of added cludge), which is a bit contradictory to the Git design IMO.

> This is especially
> true if you are making the "drop-privileges" part a helper function, no?

I don't follow this part. What exactly becomes more true by having a drop-privileges function?

Anyway, I'm pretty pleased with how this turned out after inlining serve() into main(), what do you think about this? I've also moved the reordering of usage-string into a new patch that makes inetd_mode and detach incompatible (they already are, it's just not checked for or documented).

Show changes to 2 files +46 −30

Makefile, daemon.c

diff --git a/Makefile b/Makefile
index 46034bf..53986b1 100644
--- a/Makefile
+++ b/Makefile
@@ -401,6 +401,7 @@ EXTRA_PROGRAMS =
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS += $(EXTRA_PROGRAMS)

+PROGRAM_OBJS += daemon.o
 PROGRAM_OBJS += fast-import.o
 PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += shell.o
@@ -1066,7 +1067,6 @@ ifeq ($(uname_S),Windows)
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1077,6 +1077,7 @@ ifeq ($(uname_S),Windows)
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	NATIVE_CRLF = YesPlease

 	CC = compat/vcbuild/scripts/clink.pl
@@ -1119,7 +1120,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1130,6 +1130,9 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
 	ETAGS_TARGET = ETAGS
+	NO_INET_PTON = YesPlease
+	NO_INET_NTOP = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
-Icompat/fnmatch -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
@@ -1249,9 +1252,6 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz

-ifndef NO_POSIX_ONLY_PROGRAMS
-	PROGRAM_OBJS += daemon.o
-endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
@@ -1419,6 +1419,10 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif

+ifdef NO_POSIX_GOODIES
+	BASIC_CFLAGS += -DNO_POSIX_GOODIES
+endif
+
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
diff --git a/daemon.c b/daemon.c
index b7f3874..589bd04 100644
--- a/daemon.c
+++ b/daemon.c
@@ -26,7 +26,9 @@ static const char daemon_usage[] =
 "           [--reuseaddr] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
+#ifndef NO_POSIX_GOODIES
 "                      [--detach] [--user=user [--group=group]]\n"
+#endif
 "           [directory...]";

 /* List of acceptable pathname prefixes */
@@ -938,6 +940,14 @@ static void sanitize_stdfds(void)
 		close(fd);
 }

+#ifndef NO_POSIX_GOODIES
+static void drop_privileges(struct passwd *pass, gid_t gid)
+{
+	if (initgroups(pass->pw_name, gid) || setgid (gid) ||
+	     setuid(pass->pw_uid))
+		die("cannot drop privileges");
+}
+
 static void daemonize(void)
 {
 	switch (fork()) {
@@ -955,6 +965,7 @@ static void daemonize(void)
 	close(2);
 	sanitize_stdfds();
 }
+#endif

 static void store_pid(const char *path)
 {
@@ -965,33 +976,19 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }

-static int serve(struct string_list *listen_addr, int listen_port,
struct passwd *pass, gid_t gid)
-{
-	struct socketlist socklist = { NULL, 0, 0 };
-
-	socksetup(listen_addr, listen_port, &socklist);
-	if (socklist.nr == 0)
-		die("unable to allocate any listen sockets on port %u",
-		    listen_port);
-
-	if (pass && gid &&
-	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
-	     setuid(pass->pw_uid)))
-		die("cannot drop privileges");
-
-	return service_loop(&socklist);
-}
-
 int main(int argc, char **argv)
 {
 	int listen_port = 0;
 	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
+	struct socketlist socklist = { NULL, 0, 0 };
 	int serve_mode = 0, inetd_mode = 0;
-	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
+	const char *pid_file = NULL;
+#ifndef NO_POSIX_GOODIES
+	const char *user_name = NULL, *group_name = NULL;
 	int detach = 0;
 	struct passwd *pass = NULL;
-	struct group *group;
 	gid_t gid = 0;
+#endif
 	int i;

 	git_extract_argv0_path(argv[0]);
@@ -1079,6 +1076,7 @@ int main(int argc, char **argv)
 			pid_file = arg + 11;
 			continue;
 		}
+#ifndef NO_POSIX_GOODIES
 		if (!strcmp(arg, "--detach")) {
 			detach = 1;
 			log_syslog = 1;
@@ -1092,6 +1090,7 @@ int main(int argc, char **argv)
 			group_name = arg + 8;
 			continue;
 		}
+#endif
 		if (!prefixcmp(arg, "--enable=")) {
 			enable_service(arg + 9, 1);
 			continue;
@@ -1126,14 +1125,15 @@ int main(int argc, char **argv)
 		/* avoid splitting a message in the middle */
 		setvbuf(stderr, NULL, _IOFBF, 4096);

-	if (inetd_mode && (detach || group_name || user_name))
-		die("--detach, --user and --group are incompatible with --inetd");
-
 	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;

+#ifndef NO_POSIX_GOODIES
+	if (inetd_mode && (detach || group_name || user_name))
+		die("--detach, --user and --group are incompatible with --inetd");
+
 	if (group_name && !user_name)
 		die("--group supplied without --user");

@@ -1145,13 +1145,14 @@ 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;
 		}
 	}
+#endif

 	if (strict_paths && (!ok_paths || !*ok_paths))
 		die("option --strict-paths requires a whitelist");
@@ -1168,11 +1169,13 @@ int main(int argc, char **argv)
 	if (inetd_mode || serve_mode)
 		return execute();

+#ifndef NO_POSIX_GOODIES
 	if (detach) {
 		daemonize();
 		loginfo("Ready to rumble");
 	}
 	else
+#endif
 		sanitize_stdfds();

 	if (pid_file)
@@ -1185,5 +1188,15 @@ int main(int argc, char **argv)
 	cld_argv[argc] = "--serve";
 	cld_argv[argc+1] = NULL;

-	return serve(&listen_addr, listen_port, pass, gid);
+	socksetup(&listen_addr, listen_port, &socklist);
+	if (socklist.nr == 0)
+		die("unable to allocate any listen sockets on port %u",
+		    listen_port);
+
+#ifndef NO_POSIX_GOODIES
+	if (pass && gid)
+		drop_privileges(pass, gid);
+#endif
+
+	return service_loop(&socklist);
 }
Jonathan Nieder· Oct 18, 2010, 16:31 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Hi,
A response to the general questions.
Erik Faye-Lund wrote:
> On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Why does the signature even have to be different between the two to begin
>> with? I _think_ you have gid_t over there
>
> We don't, so this is the primary reason.
Just to throw an idea out: you can also do something like
#ifndef NO_POSIX_GOODIES
struct credentials {
};
#else
struct credentials {
	struct passwd *pass;
	gid_t gid;
}
#endif
and pass a pointer to credentials around.
> But also avoiding
> compilation-warnings is a secondary motivation.
(void) gid;
works for this.
> We do, so this becomes a bit of a hypothetical question. But would you
> seriously consider pretending to have a posix-feature less ugly than
> inlining a function that is only used once?
In general, yes.

Long functions can make code much, much more difficult to read. A fake posix feature just requires some suspension of disbelief.

In this case (#ifdef-heavy main() vs opaque struct passwd), both strike me as ugly.

Show 8 quoted lines
> (I'm going a little off-topic here, I hope that's OK)
> I'm not too happy with some of the
> pretend-really-hard-to-be-posix-magic around in the Windows-port. In
> fact, I have some patches to reduce posixness in some areas, while
> getting rid of some code in mingw.c. Would such patches be welcome, or
> is pretend-to-be-posix the governing portability approach? In some
> cases, this comes at the expense of some performance (and quite a bit
> of added cludge), which is a bit contradictory to the Git design IMO.

Sometimes the best abstraction is the posix one and sometimes not. I don't think this would contradict with your planned patches, unless they introduce #ifdefs all over the place.

Show 5 quoted lines
>> This is especially
>> true if you are making the "drop-privileges" part a helper function, no?
>
> I don't follow this part. What exactly becomes more true by having a
> drop-privileges function?

(See linux-2.6.git:Documentation/SubmittingPatches, section "#ifdefs are ugly".)

The ideal: never an #ifdef within a function. (Well, the ideal is no #ifdef-s in .c files, but that's harder to take seriously.)

#ifndef HAVE_POSIX_GOODIES
static int drop_privileges(...)
{
	return error("--user and --group not supported on this platform");
}
#endif
static int drop_privileges(...)
{
	...
	do
	something
	...
}
#endif
would make serve() look like
static int serve(...)
{
	int socknum, *socklist;
	... setup socket ...
	if (want to drop privileges) {
		if (drop_privileges(...))
			return -1;
	}
	return service_loop(socknum, socklist);
}

which should be quite readable even to a person only interested in the !HAVE_POSIX_GOODIES case imho. With some code rearrangement it could be made nicer. Now compare:

static int serve(...)
{
	int socknum, *socklist;
	... setup socket ...
#ifdef HAVE_POSIX_GOODIES
	...
	do
	things
	...
#endif
	return service_loop(socknum, socklist);
}

Just my two cents. Sorry I do not have something more substantive to say.

Andreas Schwab· Oct 18, 2010, 18:13 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> Just to throw an idea out: you can also do something like
>
> #ifndef NO_POSIX_GOODIES
> struct credentials {
> };
> #else
> struct credentials {
> 	struct passwd *pass;
> 	gid_t gid;
> }
> #endif
>
> and pass a pointer to credentials around.

Empty structures are not standard C. But if you only ever use a pointer to the struct you can leave it undefined (ie. just forward-declare it).

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Jonathan Nieder· Oct 18, 2010, 18:42 UTC · re: Andreas Schwab · lore

empty structs

Andreas Schwab wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> Just to throw an idea out: you can also do something like
>>
>> #ifndef NO_POSIX_GOODIES
>> struct credentials {
>> };
>> #else
[...]
> Empty structures are not standard C.
(For those following at home:
	"A structure type describes a sequentially allocated
	 nonempty set of member objects (and, in certain
	 circumstances, an incomplete array), each of which
	 has an optionally specified name and possibly
	 distinct type.")

So I guess future patches following the pattern of v1.7.3-rc0~33^2~13 git wrapper: introduce startup_info struct, 2010-08-05) should be written like

	struct startup_info {
		char dummy;
	};
Yuck.  Thanks for the pointer.
Erik Faye-Lund· Oct 21, 2010, 21:16 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

On Mon, Oct 18, 2010 at 6:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> A response to the general questions.
Thanks!
Show 22 quoted lines
> Erik Faye-Lund wrote:
>> On Fri, Oct 15, 2010 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> Why does the signature even have to be different between the two to begin
>>> with? I _think_ you have gid_t over there
>>
>> We don't, so this is the primary reason.
>
> Just to throw an idea out: you can also do something like
>
> #ifndef NO_POSIX_GOODIES
> struct credentials {
> };
> #else
> struct credentials {
>        struct passwd *pass;
>        gid_t gid;
> }
> #endif
>
> and pass a pointer to credentials around.
>

Yes, but that structure still needs to be filled somehow. I'm not sure how this solves anything, really. Isn't it essentially another way of wrapping an ifdef around the parameters inside main() (at least when I've inlined serve() into main())?

Show 66 quoted lines
>>> This is especially
>>> true if you are making the "drop-privileges" part a helper function, no?
>>
>> I don't follow this part. What exactly becomes more true by having a
>> drop-privileges function?
>
> (See linux-2.6.git:Documentation/SubmittingPatches, section "#ifdefs
> are ugly".)
>
> The ideal: never an #ifdef within a function.  (Well, the ideal is
> no #ifdef-s in .c files, but that's harder to take seriously.)
>
> #ifndef HAVE_POSIX_GOODIES
> static int drop_privileges(...)
> {
>        return error("--user and --group not supported on this platform");
> }
> #endif
> static int drop_privileges(...)
> {
>        ...
>        do
>        something
>        ...
> }
> #endif
>
> would make serve() look like
>
> static int serve(...)
> {
>        int socknum, *socklist;
>
>        ... setup socket ...
>
>        if (want to drop privileges) {
>                if (drop_privileges(...))
>                        return -1;
>        }
>
>        return service_loop(socknum, socklist);
> }
>
> which should be quite readable even to a person only interested in the
> !HAVE_POSIX_GOODIES case imho.  With some code rearrangement it could
> be made nicer.  Now compare:
>
> static int serve(...)
> {
>        int socknum, *socklist;
>
>        ... setup socket ...
>
> #ifdef HAVE_POSIX_GOODIES
>        ...
>        do
>        things
>        ...
> #endif
>
>        return service_loop(socknum, socklist);
> }
>
> Just my two cents.  Sorry I do not have something more substantive to
> say.
>

You're leaving out the troublesome part, namely the glue between "if (user_name)" in main(), and the "want to drop privileges"-stuff in serve().

I could do a "struct credentials *cred = NULL;" in main(), and assign that inside "if (user_name)". But that'd leave a warning about unreachable code in drop_privileges(), no?

I'm also getting the feeling that I'm being hinted at to implement proper credential-dropping (ie filling out the windows-versions of the code with something that makes sense for windows), but this isn't how these things work on Windows. Daemons run as services on Windows, and what user to run a service under is a system-administrator setting. In fact, you can't even impersonate another user without having it's password.

Turning git-daemon into a service is something that can be done later. I've looked into it, and what seems to make the most sense is to have a separate mode on git-daemon (or even another program), that starts git-daemon as a subprocess. This is because of the way Windows communicates with the service, requiring a message-loop that can be terminated.

Erik Faye-Lund· Oct 21, 2010, 22:00 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

On Thu, Oct 21, 2010 at 11:16 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 84 quoted lines
> On Mon, Oct 18, 2010 at 6:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Just to throw an idea out: you can also do something like
>>
>> #ifndef NO_POSIX_GOODIES
>> struct credentials {
>> };
>> #else
>> struct credentials {
>>        struct passwd *pass;
>>        gid_t gid;
>> }
>> #endif
>>
>> and pass a pointer to credentials around.
>>
>
> Yes, but that structure still needs to be filled somehow. I'm not sure
> how this solves anything, really. Isn't it essentially another way of
> wrapping an ifdef around the parameters inside main() (at least when
> I've inlined serve() into main())?
>
>> #ifndef HAVE_POSIX_GOODIES
>> static int drop_privileges(...)
>> {
>>        return error("--user and --group not supported on this platform");
>> }
>> #endif
>> static int drop_privileges(...)
>> {
>>        ...
>>        do
>>        something
>>        ...
>> }
>> #endif
>>
>> would make serve() look like
>>
>> static int serve(...)
>> {
>>        int socknum, *socklist;
>>
>>        ... setup socket ...
>>
>>        if (want to drop privileges) {
>>                if (drop_privileges(...))
>>                        return -1;
>>        }
>>
>>        return service_loop(socknum, socklist);
>> }
>>
>> which should be quite readable even to a person only interested in the
>> !HAVE_POSIX_GOODIES case imho.  With some code rearrangement it could
>> be made nicer.  Now compare:
>>
>> static int serve(...)
>> {
>>        int socknum, *socklist;
>>
>>        ... setup socket ...
>>
>> #ifdef HAVE_POSIX_GOODIES
>>        ...
>>        do
>>        things
>>        ...
>> #endif
>>
>>        return service_loop(socknum, socklist);
>> }
>>
>> Just my two cents.  Sorry I do not have something more substantive to
>> say.
>>
>
> You're leaving out the troublesome part, namely the glue between "if
> (user_name)" in main(), and the "want to drop privileges"-stuff in
> serve().
>
> I could do a "struct credentials *cred = NULL;"  in main(), and assign
> that inside "if (user_name)". But that'd leave a warning about
> unreachable code in drop_privileges(), no?
>

OK, I did another stab at this, and this is the best I could come up with right now, what do you think?

Show changes to 2 files +63 −23

Makefile, daemon.c

diff --git a/Makefile b/Makefile
index 46034bf..53986b1 100644
--- a/Makefile
+++ b/Makefile
@@ -401,6 +401,7 @@ EXTRA_PROGRAMS =
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS += $(EXTRA_PROGRAMS)

+PROGRAM_OBJS += daemon.o
 PROGRAM_OBJS += fast-import.o
 PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += shell.o
@@ -1066,7 +1067,6 @@ ifeq ($(uname_S),Windows)
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1077,6 +1077,7 @@ ifeq ($(uname_S),Windows)
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	NATIVE_CRLF = YesPlease

 	CC = compat/vcbuild/scripts/clink.pl
@@ -1119,7 +1120,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_SVN_TESTS = YesPlease
 	NO_PERL_MAKEMAKER = YesPlease
 	RUNTIME_PREFIX = YesPlease
-	NO_POSIX_ONLY_PROGRAMS = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
@@ -1130,6 +1130,9 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
 	ETAGS_TARGET = ETAGS
+	NO_INET_PTON = YesPlease
+	NO_INET_NTOP = YesPlease
+	NO_POSIX_GOODIES = UnfortunatelyYes
 	COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat
-Icompat/fnmatch -Icompat/win32
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \
@@ -1249,9 +1252,6 @@ ifdef ZLIB_PATH
 endif
 EXTLIBS += -lz

-ifndef NO_POSIX_ONLY_PROGRAMS
-	PROGRAM_OBJS += daemon.o
-endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
@@ -1419,6 +1419,10 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif

+ifdef NO_POSIX_GOODIES
+	BASIC_CFLAGS += -DNO_POSIX_GOODIES
+endif
+
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
diff --git a/daemon.c b/daemon.c
index b7f3874..20ae9b4 100644
--- a/daemon.c
+++ b/daemon.c
@@ -26,7 +26,9 @@ static const char daemon_usage[] =
 "           [--reuseaddr] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
+#ifndef NO_POSIX_GOODIES
 "                      [--detach] [--user=user [--group=group]]\n"
+#endif
 "           [directory...]";

 /* List of acceptable pathname prefixes */
@@ -938,6 +940,33 @@ static void sanitize_stdfds(void)
 		close(fd);
 }

+#ifdef NO_POSIX_GOODIES
+
+struct credentials;
+static void drop_privileges(struct credentials *cred)
+{
+	/* nothing */
+}
+
+static void daemonize(void)
+{
+	die("--detach not supported on this platform");
+}
+
+#else
+
+struct credentials {
+	struct passwd *pass;
+	gid_t gid;
+};
+
+static void drop_privileges(struct credentials *cred)
+{
+	if (cred && initgroups(cred->pass->pw_name, cred->gid) ||
+	    setgid (cred->gid) || setuid(cred->pass->pw_uid))
+		die("cannot drop privileges");
+}
+
 static void daemonize(void)
 {
 	switch (fork()) {
@@ -955,6 +984,7 @@ static void daemonize(void)
 	close(2);
 	sanitize_stdfds();
 }
+#endif

 static void store_pid(const char *path)
 {
@@ -965,7 +995,7 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }

-static int serve(struct string_list *listen_addr, int listen_port,
struct passwd *pass, gid_t gid)
+static int serve(struct string_list *listen_addr, int listen_port,
struct credentials *cred)
 {
 	struct socketlist socklist = { NULL, 0, 0 };

@@ -974,10 +1004,7 @@ static int serve(struct string_list
*listen_addr, int listen_port, struct passwd
 		die("unable to allocate any listen sockets on port %u",
 		    listen_port);

-	if (pass && gid &&
-	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
-	     setuid(pass->pw_uid)))
-		die("cannot drop privileges");
+	drop_privileges(cred);

 	return service_loop(&socklist);
 }
@@ -989,9 +1016,7 @@ int main(int argc, char **argv)
 	int serve_mode = 0, inetd_mode = 0;
 	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;
+	struct credentials *cred = NULL;
 	int i;

 	git_extract_argv0_path(argv[0]);
@@ -1079,6 +1104,7 @@ int main(int argc, char **argv)
 			pid_file = arg + 11;
 			continue;
 		}
+#ifndef NO_POSIX_GOODIES
 		if (!strcmp(arg, "--detach")) {
 			detach = 1;
 			log_syslog = 1;
@@ -1092,6 +1118,12 @@ int main(int argc, char **argv)
 			group_name = arg + 8;
 			continue;
 		}
+#else
+		/* avoid warnings */
+		(void)user_name;
+		(void)group_name;
+		(void)detach;
+#endif
 		if (!prefixcmp(arg, "--enable=")) {
 			enable_service(arg + 9, 1);
 			continue;
@@ -1126,32 +1158,37 @@ int main(int argc, char **argv)
 		/* avoid splitting a message in the middle */
 		setvbuf(stderr, NULL, _IOFBF, 4096);

-	if (inetd_mode && (detach || group_name || user_name))
-		die("--detach, --user and --group are incompatible with --inetd");
-
 	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;

+#ifndef NO_POSIX_GOODIES
+	if (inetd_mode && (detach || group_name || user_name))
+		die("--detach, --user and --group are incompatible with --inetd");
+
 	if (group_name && !user_name)
 		die("--group supplied without --user");

 	if (user_name) {
-		pass = getpwnam(user_name);
-		if (!pass)
+		struct credentials c;
+		cred = &c;
+
+		c->pass = getpwnam(user_name);
+		if (!c->pass)
 			die("user not found - %s", user_name);

 		if (!group_name)
-			gid = pass->pw_gid;
+			c->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;
+			c->gid = group->gr_gid;
 		}
 	}
+#endif

 	if (strict_paths && (!ok_paths || !*ok_paths))
 		die("option --strict-paths requires a whitelist");
@@ -1185,5 +1222,5 @@ int main(int argc, char **argv)
 	cld_argv[argc] = "--serve";
 	cld_argv[argc+1] = NULL;

-	return serve(&listen_addr, listen_port, pass, gid);
+	return serve(&listen_addr, listen_port, cred);
 }
Jonathan Nieder· Oct 21, 2010, 22:03 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Hi Erik,
Erik Faye-Lund wrote:
> OK, I did another stab at this, and this is the best I could come up
> with right now, what do you think?
Much clearer; thanks.
Erik Faye-Lund· Oct 21, 2010, 22:04 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

On Fri, Oct 22, 2010 at 12:00 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 23 quoted lines
>        if (user_name) {
> -               pass = getpwnam(user_name);
> -               if (!pass)
> +               struct credentials c;
> +               cred = &c;
> +
> +               c->pass = getpwnam(user_name);
> +               if (!c->pass)
>                        die("user not found - %s", user_name);
>
>                if (!group_name)
> -                       gid = pass->pw_gid;
> +                       c->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;
> +                       c->gid = group->gr_gid;
>                }
>        }

Sorry for the noise, but this is clearly incorrect and won't compile. I guess replacing "c->" with "c." should do the trick :)

If I got this way, I'll obviously make sure it compiles! ;)
Junio C Hamano· Oct 21, 2010, 23:17 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Erik Faye-Lund <kusmabite@gmail.com> writes:
> OK, I did another stab at this, and this is the best I could come up
> with right now, what do you think?
Getting warmer ;-)

Similarly to drop_privileges() you can create prepare_credential that takes "struct cred *, const char *username, const char *groupname" as another helper, and make its implementation on the NO_POSIX side barf/die when called, saying that switching credentials is not supported in NO_POSIX implementation. That way, you can keep the main clean without any conditionals. Command line argument parser can (and probably should) stay the same between implementations.

Thanks.
Junio C Hamano· Oct 18, 2010, 19:26 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 8 quoted lines
> (I'm going a little off-topic here, I hope that's OK)
> I'm not too happy with some of the pretend-really-hard-to-be-posix-magic
> around in the Windows-port. In fact, I have some patches to reduce
> posixness in some areas, while getting rid of some code in
> mingw.c. Would such patches be welcome, or is pretend-to-be-posix the
> governing portability approach? In some cases, this comes at the expense
> of some performance (and quite a bit of added cludge), which is a bit
> contradictory to the Git design IMO.

If the part of the codepath you need to make conditional can be better helped by abstraction that is higher-level than POSIX, that would be a very acceptable approach. The "struct credential" idea Jonathan gave you is an example of such. The goal is not to force POSIX on Windows or make POSIX emulation on Windows more complete---that is not git's job. Just that in most of the case the level of abstraction POSIX gives has been adequate for our codebase.

Show 5 quoted lines
>> This is especially
>> true if you are making the "drop-privileges" part a helper function, no?
>
> I don't follow this part. What exactly becomes more true by having a
> drop-privileges function?

By using a bit higher level abstraction than POSIX primitives give you (e.g. initgroups(), setgid(), etc.) that does not have to depend on particular POSIX implementation details (e.g. "struct passwd", gid_t, etc.), you can make the main codepath cleaner and free of ifdefs.

Erik Faye-Lund· Oct 21, 2010, 20:37 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

On Mon, Oct 11, 2010 at 11:50 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 13 quoted lines
> @@ -1168,11 +1185,13 @@ int main(int argc, char **argv)
>                return execute();
>        }
>
> +#ifndef NO_POSIX_GOODIES
>        if (detach) {
>                daemonize();
>                loginfo("Ready to rumble");
>        }
>        else
> +#endif
>                sanitize_stdfds();
>

Does anyone know what the call to sanitize_stdfds() is good for? I tried searching the mailing list, but the discussion on the patch that introduced it seems to only discuss how to implement sanitize_stdfds(), not why...

I understand that it might be beneficial in the --detach code-path, but how can stdint, stdout or stderr be closed in this code-path?

(CC'ed Matthias, who wrote the code)
Jonathan Nieder· Oct 21, 2010, 20:39 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

Erik Faye-Lund wrote:
> I understand that it might be beneficial in the --detach code-path,
> but how can stdint, stdout or stderr be closed in this code-path?
Maybe "git daemon >&- 2>&-"?

In some situations involving setuid programs, this kind of thing can be a security problem (since fd 1 is not taken, the first open() uses that fd, so output intended for stdout goes to that file).

Erik Faye-Lund· Oct 21, 2010, 20:54 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4 15/15] daemon: opt-out on features that require posix

On Thu, Oct 21, 2010 at 10:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 11 quoted lines
> Erik Faye-Lund wrote:
>
>> I understand that it might be beneficial in the --detach code-path,
>> but how can stdint, stdout or stderr be closed in this code-path?
>
> Maybe "git daemon >&- 2>&-"?
>
> In some situations involving setuid programs, this kind of thing
> can be a security problem (since fd 1 is not taken, the first open()
> uses that fd, so output intended for stdout goes to that file).
>

This is beyond my shell-fu, but if this is supposed to not open stdin/out/err then I'm a bit puzzled. K&R explicitly states that stdin, stdout and stderr should be opened at startup in Appendix B: "When a program begins execution, the tree streams stdin, stdou and stderr are already open". There's also section 7.5, which lists redirection to files and pipes as exceptions, but not keeping them closed.

Perhaps I'm interpreting K&R a little too literary?

← back to recent threads