Re: [PATCH] git-daemon: Simplify child management and associated logging by

From: Stephen R. van den Berg <srb@cuci.nl>
Date: Aug 12, 2008, 23:12 UTC
Message-ID: <20080812231210.GB15265@cuci.nl>
In-Reply-To: <20080812223224.GA4134@steel.home>

Alex Riesen wrote:

>Stephen R. van den Berg, Tue, Aug 12, 2008 23:25:35 +0200:
>> +        struct child*newborn;

>You may want to reformat the patch using tabs instead of spaces.

Tried to do it right, but apparently missed a few spots, thanks.

>> +        newborn = xmalloc(sizeof *newborn);

>The custom here is to use "sizeof(type)". Brackets and typename.

Well, as for the parens, current git source shows a ratio of 26 : 887 in your favour; apparently not everyone agrees, but most do. As for the typename, that rather uncommon, in most cases actual variables are used (with good reason).

>> +        if (newborn) {
>> +	        struct child**cradle,*blanket;

>"struct child **cradle, *blanket;" (the spaces before asterisks)

I'll be more careful.

>> +		memcpy(memset(&newborn->address, 0, sizeof newborn->address),
>> +		 addr, addrlen);

>Aren't separate calls easier to read (and type)?

Possibly, yes.

>	memset(&newborn->address, 0, sizeof(newborn->address));
>	memcpy(&newborn->address, addr, addrlen);

But it results in more machinecode in most cases. Sorry, is my builtin micro-optimiser at work.

Show 12 quoted lines

>> -static void kill_some_children(int signo, unsigned start, unsigned stop)
>> +static void kill_some_child(int signo)
>>  {
>> -	start %= MAX_CHILDREN;
>> -	stop %= MAX_CHILDREN;
>> -	while (start != stop) {
>> -		if (!(start & 3))
>> -			kill(live_child[start].pid, signo);
>> -		start = (start + 1) % MAX_CHILDREN;
>> +	const struct child *blanket;
>> +
>> +	if ((blanket = firstborn)) {

>	if (firstborn) {
>	    const struct child *blanket = firstborn;

>You don't even use blanket outside of the "if".

Good thinking. I restructured the code a few times, previously this wasn't possible.

>>  static void check_dead_children(void)
>>  {
>> +		loginfo("[%d] Disconnected%s", (int)pid, dead);

>BTW, why do you need that pid_t->int cast?

Because pid_t might be wider than int (unlikely, but possible), and then this call might crash the program.

>> @@ -1105,6 +1026,10 @@ int main(int argc, char **argv)
>>  			init_timeout = atoi(arg+15);
>>  			continue;

>> +		if (!prefixcmp(arg, "--max-connections=")) {
>> +			max_connections = atoi(arg+18);

>An error checking wouldn't go amiss. And it can't be done with atoi
>(consider strtol).

I merely copied the other argument parsing methods, didn't want to improve this, just functionally equivalent (will do fine here, IMO).

-- 
Sincerely,
           Stephen R. van den Berg.

Father's Day Special at the local clinic -- Vasectomy!