[PATCH] libipt_statistic

[PATCH] libipt_statistic

[NICOLAS BOULIANE]

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

Hi Patrick,

I think that this calculation is erroneous:
info->u.nth.count = info->u.nth.every - info->u.nth.packet;

ex: --mode nth --every 4 --packet 6
AIUI, we want to drop the 4th packet after 6 packets have pass.
Base on this example, we want to match the 10th packet.

But we are gonna match the 7th packet, wich is wrong.
   count: -3
   every: 3
   packet: 6

 -3,  -2, -1,  0,  1,  2,  (3) -- Increment
  1,   2,   3,  4,  5,  6,   7  -- nth packet

comments are welcome.
--
I think that my patch introduce space at the beginning of each line,
someone could give me a vim hint to avoid that ? thanks.
--

acidfu

[-- Attachment #2: libipt_statistic.c.patch --]
[-- Type: text/x-patch, Size: 726 bytes --]

Index: libipt_statistic.c
===================================================================
--- libipt_statistic.c	(revision 6894)
+++ libipt_statistic.c	(working copy)
@@ -113,7 +113,7 @@
 	if (flags & 0x8 && info->mode != XT_STATISTIC_MODE_NTH)
 		exit_error(PARAMETER_PROBLEM,
 			   "--packet can only be used in nth mode");
-	info->u.nth.count = info->u.nth.every - info->u.nth.packet;
+	info->u.nth.count = -info->u.nth.packet;
 }
 
 /* Prints out the matchinfo. */
@@ -156,7 +156,7 @@
 	print_match(info, "--");
 }
 
-static struct iptables_match statistic = { 
+static struct iptables_match statistic = {
 	.name		= "statistic",
 	.version	= IPTABLES_VERSION,
 	.size		= IPT_ALIGN(sizeof(struct xt_statistic_info)),

Re: [PATCH] libipt_statistic

[Patrick McHardy]

NICOLAS BOULIANE wrote:
> Hi Patrick,
> 
> I think that this calculation is erroneous:
> info->u.nth.count = info->u.nth.every - info->u.nth.packet;
> 
> ex: --mode nth --every 4 --packet 6
> AIUI, we want to drop the 4th packet after 6 packets have pass.
> Base on this example, we want to match the 10th packet.
> 
> But we are gonna match the 7th packet, wich is wrong.
>   count: -3
>   every: 3
>   packet: 6
> 
> -3,  -2, -1,  0,  1,  2,  (3) -- Increment
>  1,   2,   3,  4,  5,  6,   7  -- nth packet
> 
> comments are welcome.


--packet is the initial counter value and must be >= 0 and <= every - 1.

In your example, which packet do you want to drop on the second round?
The fourth or the tenth?

Re: [PATCH] libipt_statistic

[Patrick McHardy]

[Don't remove CCs please]

NICOLAS BOULIANE wrote:
> Hi Patrick,
> 
> First, there's no upper bound check to ensure that `--packet` value is n-1,
> so I'm confused.
> 
> if (string_to_number(optarg, 0, 0xFFFFFFFF,
>                         &info->u.nth.packet) == -1)


Yes, that should be fixed.

> 
> But even if I do:
>   --every 4 --packet 2
> The first time it will match the 3th packet
> and all next time it will match the 4th packet, which I dont
> understand why (technically yes, but philosophically not).
> 
> Matching pattern look like:
> 1,2,(3),4,5,6,(7),8,9,10,(11),12,13,14,(15)


It starts at zero and goes up to n - 1. So for the second
packet you would actually use --packet 1. A bit
counterintuitive, I agree, but too late to change.

Re: [PATCH] libipt_statistic

[Patrick McHardy]

Patrick McHardy wrote:
> NICOLAS BOULIANE wrote:
> 
>>But even if I do:
>>  --every 4 --packet 2
>>The first time it will match the 3th packet
>>and all next time it will match the 4th packet, which I dont
>>understand why (technically yes, but philosophically not).
>>
>>Matching pattern look like:
>>1,2,(3),4,5,6,(7),8,9,10,(11),12,13,14,(15)
> 
> 
> 
> It starts at zero and goes up to n - 1. So for the second
> packet you would actually use --packet 1. A bit
> counterintuitive, I agree, but too late to change.


BTW, if this really bothers you, you could add a new option
for --packet (maybe --start) that subtracts one from the
starting point.

Re: [PATCH] libipt_statistic

[NICOLAS BOULIANE]

On 7/2/07, Patrick McHardy <kaber@trash.net> wrote:

> It starts at zero and goes up to n - 1. So for the second
> packet you would actually use --packet 1. A bit
> counterintuitive, I agree, but too late to change.

with --every 4 --packet 2
[ count = every - packet ]

in the final check:
count = 1
every = 3
packet = 2

I don't see what is starting at zero.

--
acidfu

Re: [PATCH] libipt_statistic

[Patrick McHardy]

NICOLAS BOULIANE wrote:
> On 7/2/07, Patrick McHardy <kaber@trash.net> wrote:
>
>> It starts at zero and goes up to n - 1. So for the second
>> packet you would actually use --packet 1. A bit
>> counterintuitive, I agree, but too late to change.
>
> with --every 4 --packet 2
> [ count = every - packet ]
>
> in the final check:
> count = 1
> every = 3
> packet = 2
>
> I don't see what is starting at zero.

The count. --packet n will match the n+1th packet.

Re: [PATCH] libipt_statistic

[NICOLAS BOULIANE]

On 7/2/07, Patrick McHardy <kaber@trash.net> wrote:
> NICOLAS BOULIANE wrote:
> > On 7/2/07, Patrick McHardy <kaber@trash.net> wrote:
> >
> >> It starts at zero and goes up to n - 1. So for the second
> >> packet you would actually use --packet 1. A bit
> >> counterintuitive, I agree, but too late to change.
> >
> > with --every 4 --packet 2
> > [ count = every - packet ]
> >
> > in the final check:
> > count = 1
> > every = 3
> > packet = 2
> >
> > I don't see what is starting at zero.
>
> The count. --packet n will match the n+1th packet.
>
>

i'm not trying to bother you, but there's surely something I dont understand.

In the kernel we have:
   if (info->u.nth.count++ == info->u.nth.every)
    info->u.nth.count = 0;

--every 4 --packet 2
count = 1, every = 3, packet = 2

1th packet:
           if (1 == 3) { // no match

2th packet:
           if (2 == 3) { // no match

3th packet:
            if (3 == 3) { // match and set __count = 0__

next time we will match the 4th packet.
Is what I should expect ? or there's something wrong.

thanks,
acidfu

Re: [PATCH] libipt_statistic

[Patrick McHardy]

NICOLAS BOULIANE wrote:
> i'm not trying to bother you, but there's surely something I dont
> understand.


Don't worry :)

> In the kernel we have:
>   if (info->u.nth.count++ == info->u.nth.every)
>    info->u.nth.count = 0;
> 
> --every 4 --packet 2
> count = 1, every = 3, packet = 2
> 
> 1th packet:
>           if (1 == 3) { // no match
> 
> 2th packet:
>           if (2 == 3) { // no match
> 
> 3th packet:
>            if (3 == 3) { // match and set __count = 0__
> 
> next time we will match the 4th packet.
> Is what I should expect ? or there's something wrong.


It is. "--every 4" is the period, we'll match every 4th packet.
"--packet 2" says you want to match the third (thats the
non-obvious part) packet.

The main reason why you would --packet at all is for multiple
subsequent rules that don't abort rule traversal:

... --every 4 --packet 0 -j MARK --set-mark 1	(match 1st packet)
... --every 4 --packet 1 -j MARK --set-mark 2	(match 2nd packet)
... --every 4 --packet 2 -j MARK --set-mark 3	(match 3rd packet)
... --every 4 --packet 3 -j MARK --set-mark 4	(match 4th packet)

So for every packet only a single rule will match. You could do
something like that for load-balancing for example.

Re: [PATCH] libipt_statistic

[NICOLAS BOULIANE]

On 7/2/07, Patrick McHardy <kaber@trash.net> wrote:
> NICOLAS BOULIANE wrote:
> > i'm not trying to bother you, but there's surely something I dont
> > understand.
>
>
> Don't worry :)
>
> > In the kernel we have:
> >   if (info->u.nth.count++ == info->u.nth.every)
> >    info->u.nth.count = 0;
> >
> > --every 4 --packet 2
> > count = 1, every = 3, packet = 2
> >
> > 1th packet:
> >           if (1 == 3) { // no match
> >
> > 2th packet:
> >           if (2 == 3) { // no match
> >
> > 3th packet:
> >            if (3 == 3) { // match and set __count = 0__
> >
> > next time we will match the 4th packet.
> > Is what I should expect ? or there's something wrong.
>
>
> It is. "--every 4" is the period, we'll match every 4th packet.
> "--packet 2" says you want to match the third (thats the
> non-obvious part) packet.
>
> The main reason why you would --packet at all is for multiple
> subsequent rules that don't abort rule traversal:
>
> ... --every 4 --packet 0 -j MARK --set-mark 1   (match 1st packet)
> ... --every 4 --packet 1 -j MARK --set-mark 2   (match 2nd packet)
> ... --every 4 --packet 2 -j MARK --set-mark 3   (match 3rd packet)
> ... --every 4 --packet 3 -j MARK --set-mark 4   (match 4th packet)
>
> So for every packet only a single rule will match. You could do
> something like that for load-balancing for example.
>
>
    nth-packet / --packet

   01  02  03  04  05  06  07  08  09  10  11  12  13  14  15  16
0 x                       x                     x                        x
1       x                       x                      x                       x
2             x                      x                       x
              x
3                   x                      x                        x
                     x

Now the pattern make sense :)
thank you Patrick,

I'll write a patch for the upperbound
We can also specify --packet without --every,
side effectly --every will be set to 1, should be fix that too I think.

have some suggestions before a begin ?

-- 
acidfu

Re: [PATCH] libipt_statistic

[Patrick McHardy]

NICOLAS BOULIANE wrote:
> Now the pattern make sense :)
> thank you Patrick,
> 
> I'll write a patch for the upperbound
> We can also specify --packet without --every,
> side effectly --every will be set to 1, should be fix that too I think.


Sounds reasonable.

> 
> have some suggestions before a begin ?

No :)

Re: [PATCH] libipt_statistic

[NICOLAS BOULIANE]

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

On 7/2/07, Patrick McHardy <kaber@trash.net> wrote:
> NICOLAS BOULIANE wrote:
> > Now the pattern make sense :)
> > thank you Patrick,
> >
> > I'll write a patch for the upperbound
> > We can also specify --packet without --every,
> > side effectly --every will be set to 1, should be fix that too I think.
>
>
> Sounds reasonable.
>
> >
> > have some suggestions before a begin ?
>
> No :)
>
>
>

Signed-off-by: Nicolas Bouliane

comments are welcome,
thanks

[-- Attachment #2: libipt_statistic.c.patch --]
[-- Type: text/x-patch, Size: 744 bytes --]

Index: libipt_statistic.c
===================================================================
--- libipt_statistic.c	(revision 6898)
+++ libipt_statistic.c	(working copy)
@@ -113,6 +113,15 @@
 	if (flags & 0x8 && info->mode != XT_STATISTIC_MODE_NTH)
 		exit_error(PARAMETER_PROBLEM,
 			   "--packet can only be used in nth mode");
+	if ((flags & 0x8) && !(flags & 0x4))
+		exit_error(PARAMETER_PROBLEM,
+			   "--packet can only be used with --every");
+	/* at this point, info->u.nth.every have been decreased. */
+	if (!(info->u.nth.packet >= 0 && info->u.nth.packet <= info->u.nth.every))
+		exit_error(PARAMETER_PROBLEM,
+			  "the --packet p must be 0 <= p <= n-1");
+
+
 	info->u.nth.count = info->u.nth.every - info->u.nth.packet;
 }
 

Re: [PATCH] libipt_statistic

[Jan Engelhardt]

On Jul 2 2007 14:38, NICOLAS BOULIANE wrote:
>
> Signed-off-by: Nicolas Bouliane
>
> comments are welcome,
> thanks

inline patches preferred..

>Index: libipt_statistic.c
>===================================================================
>--- libipt_statistic.c	(revision 6898)
>+++ libipt_statistic.c	(working copy)
>@@ -113,6 +113,15 @@
> 	if (flags & 0x8 && info->mode != XT_STATISTIC_MODE_NTH)
> 		exit_error(PARAMETER_PROBLEM,
> 			   "--packet can only be used in nth mode");
>+	if ((flags & 0x8) && !(flags & 0x4))

This could perhaps be

	if ((flags & 0xC) == 0x8)


	Jan
-- 

Re: [PATCH] libipt_statistic

[NICOLAS BOULIANE]

On 7/2/07, Jan Engelhardt <jengelh@computergmbh.de> wrote:
>
> On Jul 2 2007 14:38, NICOLAS BOULIANE wrote:
> >
> > Signed-off-by: Nicolas Bouliane
> >
> > comments are welcome,
> > thanks
>
> inline patches preferred..
>
> >Index: libipt_statistic.c
> >===================================================================
> >--- libipt_statistic.c (revision 6898)
> >+++ libipt_statistic.c (working copy)
> >@@ -113,6 +113,15 @@
> >       if (flags & 0x8 && info->mode != XT_STATISTIC_MODE_NTH)
> >               exit_error(PARAMETER_PROBLEM,
> >                          "--packet can only be used in nth mode");
> >+      if ((flags & 0x8) && !(flags & 0x4))
>
> This could perhaps be
>
>         if ((flags & 0xC) == 0x8)
>
>
>         Jan
> --
>

same result, less intuitive. why should I do that ?

acidfu

Re: [PATCH] libipt_statistic

[Patrick McHardy]

NICOLAS BOULIANE wrote:
> Signed-off-by: Nicolas Bouliane


Applied, thanks Nicolas.

Re: [PATCH] libipt_statistic

[Patrick McHardy]

NICOLAS BOULIANE wrote:
> On 7/2/07, Jan Engelhardt <jengelh@computergmbh.de> wrote:
> 
>> >Index: libipt_statistic.c
>> >===================================================================
>> >--- libipt_statistic.c (revision 6898)
>> >+++ libipt_statistic.c (working copy)
>> >@@ -113,6 +113,15 @@
>> >       if (flags & 0x8 && info->mode != XT_STATISTIC_MODE_NTH)
>> >               exit_error(PARAMETER_PROBLEM,
>> >                          "--packet can only be used in nth mode");
>> >+      if ((flags & 0x8) && !(flags & 0x4))
>>
>> This could perhaps be
>>
>>         if ((flags & 0xC) == 0x8)
>>
>>
>>         Jan
>> -- 
>>
> 
> same result, less intuitive. why should I do that ?


Indeed. What would make sense is to use a couple of symbolic
constants for the flag values.