Re: [PATCH] return value of conntrack protocol helper functions and a macro for stats

[Martin Josefsson <gandalf@wlug.westbo.se>]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

On Thu, 2004-08-05 at 15:34, Pablo Neira wrote:
> Hi,

Hi Pablo

I've finally had the opportunity to go through my netfilter-devel
backlog.

> This patch applies to last Harald's patch bombing. It's based on 
> Martin's idea of removing negative values in conntrack protocol helper 
> functions. Please see:
> 
> http://lists.netfilter.org/pipermail/netfilter-devel/2004-June/015769.html
> 
> Now functions packet and error return CONNTRACK_CONT to let the packet 
> continue its travel through the conntrack system. It also add a macro to 
> increase vars used for stats.

I like this patch, just a few comments below.

> @@ -303,12 +307,13 @@
>  	unsigned int insert_failed;
>  	unsigned int drop;
>  	unsigned int early_drop;
> -	unsigned int icmp_error;
> +	unsigned int error;
>  	unsigned int expect_new;
>  	unsigned int expect_create;
>  	unsigned int expect_delete;
>  };

First I wondered why you changed this.
 
> +#define CONNTRACK_STAT_INC(count) (__get_cpu_var(ip_conntrack_stat).count++)

I like the macro, it's a lot cleaner.
 
> @@ -786,41 +786,42 @@
>  	/* It may be an special packet, error, unclean...
>  	 * inverse of the return code tells to the netfilter
>  	 * core what to do with the packet. */

Remove part of the comment above?

> -	if (proto->error != NULL 
> -	    && (ret = proto->error(*pskb, &ctinfo, hooknum)) <= 0) {
> -		__get_cpu_var(ip_conntrack_stat).icmp_error++;
> -		return -ret;
> +	if (proto->error != NULL) {
> +		ret = proto->error(*pskb, &ctinfo, hooknum);
> +		if (ret != CONNTRACK_CONT) {
> +			CONNTRACK_STAT_INC(error);
> +			CONNTRACK_STAT_INC(invalid);
> +			return ret;
> +		}
>  	}

Now I see why the name was changed in the struct, it's not correct
anymore (was when I wrote it :)

This patch should be submitted before 2.6.9 is out.

I'll update the ctstat program to reflect this change. There's also some
more interesting events in the now submitted tcp-windowtracking patch
that we could use for statistics.

I've modified the ctstat program to be able to handle additional fields
in /proc/net/ip_conntrack_stat if it would be increased in the future in
order to report more stats.

I'll see if I can get ctstat distributed in the same channel as rtstat,
I thought it was distributed with iproute2 but maybe that's added in the
RH iproute2 package as I didn't see rtstat in the Debian iproute
package.
 
-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]