Re: [PATCH 3/3] git-daemon: rewrite kindergarden

From: Stephen R. van den Berg <srb@cuci.nl>
Date: Aug 13, 2008, 10:37 UTC
Message-ID: <20080813103738.GG12628@cuci.nl>
In-Reply-To: <20080813090546.GN32184@machine.or.cz>

Petr Baudis wrote:

>On Wed, Aug 13, 2008 at 10:43:31AM +0200, Stephen R. van den Berg wrote:
>I would somehow mention the string '--max-connections' in the log
>message; that is really useful when looking when some option was
>introduced.

Fixed.

>> +	struct child*next;

>struct child *next;

>> +	struct child*newborn;

>struct child *newborn;

Fixed.

>So, I can always excuse myself by mentioning that it's early in the
>morning (for me ;-) but what do you actually need the blanket for, in
>this warm digital world?

This seems to be a case of code restructuring and forgetting to optimise. There was a reason for the blanket before, but I am warming up to the idea to go without blanket in this case.

>The current for statement is *really* cryptic... What about:

Probably ok, I'll refactor it accordingly.

Show 5 quoted lines

>> -static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)
>> +static void remove_child(pid_t pid)
>>  {
>> -	struct child n;
>> +	struct child **cradle, *blanket;

Show 7 quoted lines

>> +	for (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)
>> +		if (blanket->pid == pid) {
>> +			*cradle = blanket->next;
>> +			live_children--;
>> +			free(blanket);
>> +			break;
>> +		}

>Same here. You just need a temporary variable in the innermost block.

Yes, but here using the blanket eliminates some * operators. So I'd prefer to keep it.

>> +static void kill_some_child()
>>  {
>> +	const struct child *blanket;

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

Show 6 quoted lines

>> +		for (; (next = blanket->next); blanket = next)
>> +			if (!memcmp(&blanket->address, &next->address,
>> +				   sizeof next->address)) {
>> +				kill(blanket->pid, SIGTERM);
>> +				break;
>> +			}

>I think using cradle instead of blanket in this for loop would be more
>consistent, if perhaps somewhat more morbid.

I kept the cradle for the double indirections (you have to actually reach deeper into a cradle), a blanket can be touched without reaching deep, so I'd prefer to keep a blanket here.

-- 
Sincerely,
           Stephen R. van den Berg.

"And now for something *completely* different!"