Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2008, 20:24 UTC
Message-ID: <7vwsi6kvow.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vd4jymdfn.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 26 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> * sb/daemon (Thu Aug 14 20:02:20 2008 +0200) 4 commits
>>>  - git-daemon: rewrite kindergarden, new option --max-connections
>>>  - git-daemon: Simplify dead-children reaping logic
>>>  - git-daemon: use LOG_PID, simplify logging code
>>>  - git-daemon: call logerror() instead of error()
>>> 
>>> Can somebody who actually runs the daemon standalone comment on this 
>>> one?
>>
>> I am somewhat uneasy about running my production machine with these 
>> changes, since the last commit (the one introducing a kindergarden with 
>> cradles) is too unobvious for me.
>
> Well, I didn't ask anybody to _run_ it.
>
> I asked people who care about their daemons to comment on the change, so
> that if there are any issues in the code that I didn't see, the breakage
> gets caught before it propagates to their daemons they build from 'next'
> or 'master'.
>
> Having said that, I do agree that the kindergarden change is very involved
> (it removes more code than it adds --- the reduction of lines is somewhat
> inflated because quite a lot of comments that have become stale gets
> removed by the patch).

Ok, so I looked at the patch again (sorry, Stephen, for keeping this in a limbo for very long).

First of all, I have to say that I think this patch is reasonably well done. I do see an unnecessary check of xcalloc() return value and some style differences from what git uses, but that is only an indication of the author not very familiar with git code, and every other aspect of the change shows that this is quite competently done. The list traversal with cradle/blanket/newborn, except using perhaps too cute names for some people's tastes, makes sense.

I however do have one issue with the logic, though.

It seems that kill_some_child() will not kill anything if nobody else is coming from the same address, while the old code did kill some. Is this intended?

By the way, add_child() compares the whole "struct sockaddr_storage" in order to queue the newborn in front of an existing connection from the same address, and kill_some_child() takes advanrage of this to kill the newest connection ("We kill the newest" comment should probably be moved to add_child() to describe why the queuing is done this way). If you simplify add_child() to queue the newborn always at the front of the list, your kill_some_child() will continue to do so, so I do not see the point of the loop in add_child(). Am I mistaken?