RFC patch, 11 partsdaemon-win32

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> +int kill_async(struct async *async)
> +{
> +#ifndef WIN32
> +	return kill(async->pid, SIGTERM);
> +#else
> +	DWORD ret = 0;
> +	if (!TerminateThread(async->tid, 0))
> +		ret = error("killing thread failed: %lu", GetLastError());

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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