Re: [PATCH 1/3] xtables: tarpit: Make tarpit code generic

[Josh Hunt <johunt@akamai.com>]

On 07/08/2012 07:58 AM, Jan Engelhardt wrote:
>
> On Saturday 2012-07-07 23:20, Josh Hunt wrote:
>
>> Moves TCP header manipulation into a separate function to allow code-sharing for
>> IPv6 support.
>
> Hi Josh,
>
>
> in your patch, you seem to oversee that the tarpit_generic block that
> you are moving into a new function had "return;"s to exit from
> the tarpit_tcp function. With your change,
>
>> +	niph->tot_len = htons(nskb->len);
>> +	tcph->urg_ptr = 0;
>> +	/* Reset flags */
>> +	((u_int8_t *)tcph)[13] = 0;
>> +
>> +	tarpit_generic(oth, tcph, payload, mode);
>>
>> 	/* Adjust TCP checksum */
>> 	tcph->check = 0;
>
> The checksum code (and what follows) it will be executed in all cases,
> even though TCP RST are supposed not to get any reply per the comment.
>
> tarpit_generic would have to return a bool value propagating this
> exit style. In other words,
>
>
> static bool tarpit_generic
> {
> 	if (mode == XTTARPIT_TARPIT)
> 		/* No replies for RST, FIN , ... */
> 		if (oth->rst ...)
> 			return false;
> 	}
> 	...
> 	return true;
> }
>
> static void tarpit_tcp
> {
> 	...
> 	if (!tarpit_generic())
> 		return;
> 	tcph->check = 0;
> 	...
> }

Doh! Yep totally overlooked that. Thanks for catching. Will update and 
resend.

>
> It would also be appreciated if you could move each case
> (XTTARPIT_{TARPIT,HONEYPOT,RESET}) into their own functions in three
> patches added to the front of the set.
> (Reduces indent and makes the diffs simpler.)
>

Sure. I'll do this as well.

Thanks for the review.

Josh