[patch 0/4] Ipset fixes

[patch 0/4] Ipset fixes

[holger]

Hi Jozsef,

what follows are some small improvements and fixes for ipset 5.

Please take a look.  Thanks!

 /holger

-- 

[patch 1/4] ipset: show correct line numbers in restore output

[holger]

[-- Attachment #1: ipset-show-correct-line-number.diff --]
[-- Type: text/plain, Size: 779 bytes --]

When passing something like

  create foo6 hash:ip hashsize 64 family inet6
  add foo6 20a1:1234:5678::/64
  add foo6 20a1:1234:5679::/64

you get:

  ipset v5.2: Error in line 1: Syntax error: plain IP address must be supplied: 20a1:1234:5678::/64

Should be line 2 though.

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: ipset/lib/session.c
===================================================================
--- ipset.orig/lib/session.c	2011-01-05 18:59:59.000000000 +0100
+++ ipset/lib/session.c	2011-01-07 13:11:33.000000000 +0100
@@ -194,7 +194,7 @@
 
 	if (session->lineno != 0 && type == IPSET_ERROR) {
 		sprintf(session->report, "Error in line %u: ",
-			session->lineno);
+			session->lineno + 1);
 	}
 	offset = strlen(session->report);
 	

-- 

[patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[holger]

[-- Attachment #1: ipset-fix-ipv6-netmask-parsing.diff --]
[-- Type: text/plain, Size: 760 bytes --]

While the following works for AF_INET:

 ipset add foo 192.168.1.1/32

this does not work for AF_INET6:

 ipset add foo6 20a1:1:2:3:4:5:6:7/128
 ipset v5.2: Syntax error: plain IP address must be supplied: 20a1:1:2:3:4:5:6:7/128
 
Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: ipset/lib/parse.c
===================================================================
--- ipset.orig/lib/parse.c	2011-01-14 11:14:41.000000000 +0100
+++ ipset/lib/parse.c	2011-01-14 11:15:06.000000000 +0100
@@ -960,9 +960,7 @@
 		ipset_data_set(data, IPSET_OPT_FAMILY, &family);
 	}
 	
-	return family == AF_INET ? ipset_parse_ip(session, opt, str)
-				 : ipset_parse_single_ip(session, opt, str);
-
+	return ipset_parse_ip(session, opt, str);
 }
 
 /**

-- 

[patch 3/4] ipset: do session initialization once

[holger]

[-- Attachment #1: ipset-one-time-session-init.diff --]
[-- Type: text/plain, Size: 988 bytes --]

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: ipset/src/ipset.c
===================================================================
--- ipset.orig/src/ipset.c	2011-01-05 12:05:31.000000000 +0100
+++ ipset/src/ipset.c	2011-01-05 12:07:02.000000000 +0100
@@ -431,14 +431,6 @@
 	const struct ipset_commands *command;
 	const struct ipset_type *type;
 
-	/* Initialize session */
-	if (session == NULL) {
-		session = ipset_session_init(printf);
-		if (session == NULL)
-			return exit_error(OTHER_PROBLEM,
-				"Cannot initialize ipset session, aborting.");
-	}
-
 	/* Commandline parsing, somewhat similar to that of 'ip' */
 
 	/* First: parse core options */
@@ -743,5 +735,10 @@
 	ipset_type_add(&ipset_hash_ipportnet0);
 	ipset_type_add(&ipset_list_set0);
 
+	session = ipset_session_init(printf);
+	if (session == NULL)
+		return exit_error(OTHER_PROBLEM,
+						  "Cannot initialize ipset session, aborting.");
+
 	return parse_commandline(argc, argv);
 }

-- 

[patch 4/4] ipset: fix build with NDEBUG defined

[holger]

[-- Attachment #1: ipset-fix-NDEBUG.diff --]
[-- Type: text/plain, Size: 1021 bytes --]

The usage of the gcc option -Wunused-parameter interferes badly with
the assert() macros.  In case -DNDEBUG is specified build fails with:

  cc1: warnings being treated as errors
  print.c: In function 'ipset_print_family':
  print.c:92: error: unused parameter 'opt'
  print.c: In function 'ipset_print_port':
  print.c:413: error: unused parameter 'opt'
  print.c: In function 'ipset_print_proto':

A possible fix is just to remove -Wunused, as -Wextra + -Wunused enables
-Wunused-paramter.

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: ipset/configure.ac
===================================================================
--- ipset.orig/configure.ac	2011-01-18 14:47:46.000000000 +0100
+++ ipset/configure.ac	2011-01-18 14:56:11.000000000 +0100
@@ -144,7 +144,6 @@
 AX_CFLAGS_GCC_OPTION(-Wstrict-prototypes)
 AX_CFLAGS_GCC_OPTION(-Wswitch-default)
 AX_CFLAGS_GCC_OPTION(-Wundef)
-AX_CFLAGS_GCC_OPTION(-Wunused)
 AX_CFLAGS_GCC_OPTION(-Wwrite-strings)
 
 dnl Checks for library functions.

-- 

Re: [patch 1/4] ipset: show correct line numbers in restore output

[Jozsef Kadlecsik]

Hi Holger,

First of all, thanks for the patches and reports.

On Tue, 18 Jan 2011, holger@eitzenberger.org wrote:

> When passing something like
> 
>   create foo6 hash:ip hashsize 64 family inet6
>   add foo6 20a1:1234:5678::/64
>   add foo6 20a1:1234:5679::/64
> 
> you get:
> 
>   ipset v5.2: Error in line 1: Syntax error: plain IP address must be supplied: 20a1:1234:5678::/64
> 
> Should be line 2 though.

Yes, good catch! Unfortunately your patch overwrites the correct line 
number when reported by the kernel. The proper fix was to add the missing 
session line number setting *before* the parser is called.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [patch 3/4] ipset: do session initialization once

[Jozsef Kadlecsik]

On Tue, 18 Jan 2011, holger@eitzenberger.org wrote:

> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> 
> Index: ipset/src/ipset.c
> ===================================================================
> --- ipset.orig/src/ipset.c	2011-01-05 12:05:31.000000000 +0100
> +++ ipset/src/ipset.c	2011-01-05 12:07:02.000000000 +0100
> @@ -431,14 +431,6 @@
>  	const struct ipset_commands *command;
>  	const struct ipset_type *type;
>  
> -	/* Initialize session */
> -	if (session == NULL) {
> -		session = ipset_session_init(printf);
> -		if (session == NULL)
> -			return exit_error(OTHER_PROBLEM,
> -				"Cannot initialize ipset session, aborting.");
> -	}
> -
>  	/* Commandline parsing, somewhat similar to that of 'ip' */
>  
>  	/* First: parse core options */
> @@ -743,5 +735,10 @@
>  	ipset_type_add(&ipset_hash_ipportnet0);
>  	ipset_type_add(&ipset_list_set0);
>  
> +	session = ipset_session_init(printf);
> +	if (session == NULL)
> +		return exit_error(OTHER_PROBLEM,
> +						  "Cannot initialize ipset session, aborting.");
> +
>  	return parse_commandline(argc, argv);
>  }
> 
> -- 
> 

Applied, thanks!

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[Jozsef Kadlecsik]

On Tue, 18 Jan 2011, holger@eitzenberger.org wrote:

> While the following works for AF_INET:
> 
>  ipset add foo 192.168.1.1/32
> 
> this does not work for AF_INET6:
> 
>  ipset add foo6 20a1:1:2:3:4:5:6:7/128
>  ipset v5.2: Syntax error: plain IP address must be supplied: 20a1:1:2:3:4:5:6:7/128

Yeah, the usual issue: should IPv4/32 and IPv6/128 be handled as a plain 
IPv4/v6 address when the manual says "enter a plain IPv4/v6 address" :-).

The complete fix was to add the exception to the generic IP address parser 
function.

Best regards,
Jozsef  
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [patch 4/4] ipset: fix build with NDEBUG defined

[Jozsef Kadlecsik]

On Tue, 18 Jan 2011, holger@eitzenberger.org wrote:

> The usage of the gcc option -Wunused-parameter interferes badly with
> the assert() macros.  In case -DNDEBUG is specified build fails with:
> 
>   cc1: warnings being treated as errors
>   print.c: In function 'ipset_print_family':
>   print.c:92: error: unused parameter 'opt'
>   print.c: In function 'ipset_print_port':
>   print.c:413: error: unused parameter 'opt'
>   print.c: In function 'ipset_print_proto':
> 
> A possible fix is just to remove -Wunused, as -Wextra + -Wunused enables
> -Wunused-paramter.

I chose to keep the compiler flags and add the required attribute to the 
function parameters instead.

Many thanks again, Holger!

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[Jan Engelhardt]

On Tuesday 2011-01-18 21:18, Jozsef Kadlecsik wrote:
>On Tue, 18 Jan 2011, holger@eitzenberger.org wrote:
>
>> While the following works for AF_INET:
>> 
>>  ipset add foo 192.168.1.1/32
>> 
>> this does not work for AF_INET6:
>> 
>>  ipset add foo6 20a1:1:2:3:4:5:6:7/128
>>  ipset v5.2: Syntax error: plain IP address must be supplied: 20a1:1:2:3:4:5:6:7/128
>
>Yeah, the usual issue: should IPv4/32 and IPv6/128 be handled as a plain 
>IPv4/v6 address when the manual says "enter a plain IPv4/v6 address" :-).

(Assuming this was a question, heuristically based on the word order
you used:) I don't think so. iptables, resp. its modules, do not
allow that either.

Re: [patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[Jozsef Kadlecsik]

On Tue, 18 Jan 2011, Jan Engelhardt wrote:

> On Tuesday 2011-01-18 21:18, Jozsef Kadlecsik wrote:
> >On Tue, 18 Jan 2011, holger@eitzenberger.org wrote:
> >
> >> While the following works for AF_INET:
> >> 
> >>  ipset add foo 192.168.1.1/32
> >> 
> >> this does not work for AF_INET6:
> >> 
> >>  ipset add foo6 20a1:1:2:3:4:5:6:7/128
> >>  ipset v5.2: Syntax error: plain IP address must be supplied: 20a1:1:2:3:4:5:6:7/128
> >
> >Yeah, the usual issue: should IPv4/32 and IPv6/128 be handled as a plain 
> >IPv4/v6 address when the manual says "enter a plain IPv4/v6 address" :-).
> 
> (Assuming this was a question, heuristically based on the word order
> you used:) I don't think so. iptables, resp. its modules, do not
> allow that either.

I know, but the situation is a little bit more complicated: the set type 
in question works differently with IPv4 and IPv6. In the IPv4 case, a 
range of IP addresses as IPv4/prefix is accepted as input (thus 
192.168.1.1/32 too), while for IPv6, only plain IPv6 addresses are allowed 
and therefore 20a1:1:2:3:4:5:6:7/128 was rejected.

That looks really odd so I added the feature (but could not resist to add 
my comment as a pseudo-question :-).

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[Jan Engelhardt]

On Tuesday 2011-01-18 21:37, Jozsef Kadlecsik wrote:
>> >> 
>> >> this does not work for AF_INET6:
>> >> 
>> >>  ipset add foo6 20a1:1:2:3:4:5:6:7/128
>> >>  ipset v5.2: Syntax error: plain IP address must be supplied: 20a1:1:2:3:4:5:6:7/128
>> >
>> >Yeah, the usual issue: should IPv4/32 and IPv6/128 be handled as a plain 
>> >IPv4/v6 address when the manual says "enter a plain IPv4/v6 address" :-).
>> 
>> (Assuming this was a question, heuristically based on the word order
>> you used:) I don't think so. iptables, resp. its modules, do not
>> allow that either.
>
>I know, but the situation is a little bit more complicated: the set type 
>in question works differently with IPv4 and IPv6. In the IPv4 case, a 
>range of IP addresses as IPv4/prefix is accepted as input (thus 
>192.168.1.1/32 too), while for IPv6, only plain IPv6 addresses are allowed 
>and therefore 20a1:1:2:3:4:5:6:7/128 was rejected.

Is there a specific reason that there is no IPv6 net support?

Re: [patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[Jozsef Kadlecsik]

On Tue, 18 Jan 2011, Jan Engelhardt wrote:

> On Tuesday 2011-01-18 21:37, Jozsef Kadlecsik wrote:
> >> >> 
> >> >> this does not work for AF_INET6:
> >> >> 
> >> >>  ipset add foo6 20a1:1:2:3:4:5:6:7/128
> >> >>  ipset v5.2: Syntax error: plain IP address must be supplied: 20a1:1:2:3:4:5:6:7/128
> >> >
> >> >Yeah, the usual issue: should IPv4/32 and IPv6/128 be handled as a plain 
> >> >IPv4/v6 address when the manual says "enter a plain IPv4/v6 address" :-).
> >> 
> >> (Assuming this was a question, heuristically based on the word order
> >> you used:) I don't think so. iptables, resp. its modules, do not
> >> allow that either.
> >
> >I know, but the situation is a little bit more complicated: the set type 
> >in question works differently with IPv4 and IPv6. In the IPv4 case, a 
> >range of IP addresses as IPv4/prefix is accepted as input (thus 
> >192.168.1.1/32 too), while for IPv6, only plain IPv6 addresses are allowed 
> >and therefore 20a1:1:2:3:4:5:6:7/128 was rejected.
> 
> Is there a specific reason that there is no IPv6 net support?

Call it laziness: for IPv6, the hash:ip* types does *not* accept a 
range of elements to be added/deleted in one command, expressed as

ipset add foo6 20a1:1:2:3:4:5:6:7/120

or

ipset add foo6 20A1:1:2:3:4:5:6:0-20A1:1:2:3:4:5:6:FF

For IPv4 the syntax is accepted and handled.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[Holger Eitzenberger]

On Tue, Jan 18, 2011 at 09:39:32PM +0100, Jan Engelhardt wrote:
> On Tuesday 2011-01-18 21:37, Jozsef Kadlecsik wrote:
> >> >> 
> >> >> this does not work for AF_INET6:
> >> >> 
> >> >>  ipset add foo6 20a1:1:2:3:4:5:6:7/128
> >> >>  ipset v5.2: Syntax error: plain IP address must be supplied: 20a1:1:2:3:4:5:6:7/128
> >> >
> >> >Yeah, the usual issue: should IPv4/32 and IPv6/128 be handled as a plain 
> >> >IPv4/v6 address when the manual says "enter a plain IPv4/v6 address" :-).
> >> 
> >> (Assuming this was a question, heuristically based on the word order
> >> you used:) I don't think so. iptables, resp. its modules, do not
> >> allow that either.
> >
> >I know, but the situation is a little bit more complicated: the set type 
> >in question works differently with IPv4 and IPv6. In the IPv4 case, a 
> >range of IP addresses as IPv4/prefix is accepted as input (thus 
> >192.168.1.1/32 too), while for IPv6, only plain IPv6 addresses are allowed 
> >and therefore 20a1:1:2:3:4:5:6:7/128 was rejected.
> 
> Is there a specific reason that there is no IPv6 net support?

You shouldn't use hash:ip with ranges for IPv4 too because the range
members are added individually, which is less efficient both memory
and performance wise, see:

 $ ipset create foo hash:ip hashsize 64
 $ ipset add foo 192.168.1.0/30
 $ ipset list foo
 Name: foo
 Type: hash:ip
 Header: family inet hashsize 64 maxelem 65536 
 Size in memory: 628
 References: 0
 Members:
 192.168.1.3
 192.168.1.2
 192.168.1.0
 192.168.1.1

> Call it laziness: for IPv6, the hash:ip* types does *not* accept a 
> range of elements to be added/deleted in one command, expressed as
> 
> ipset add foo6 20a1:1:2:3:4:5:6:7/120
> 
> or
> 
> ipset add foo6 20A1:1:2:3:4:5:6:0-20A1:1:2:3:4:5:6:FF
> 
> For IPv4 the syntax is accepted and handled.

Re: [patch 2/4] ipset: make IPv4 and IPv6 address handling similar

[Mr Dash Four]

> You shouldn't use hash:ip with ranges for IPv4 too because the range
> members are added individually, which is less efficient both memory
> and performance wise, see:
>
>  $ ipset create foo hash:ip hashsize 64
>  $ ipset add foo 192.168.1.0/30
>  $ ipset list foo
>  Name: foo
>  Type: hash:ip
>  Header: family inet hashsize 64 maxelem 65536 
>  Size in memory: 628
>  References: 0
>  Members:
>  192.168.1.3
>  192.168.1.2
>  192.168.1.0
>  192.168.1.1
>   
I disagree!

If I need to add the 192.168.1.0/30 then I have to execute a loop (via a 
script) and add individual elements (i.e. ipset add foo 192.168.1.0, 
ipset add foo 192.168.1.1 etc).

By specifying ipset add foo 192.168.1.0/30 I do that in one go. Even 
though I am inclined to agree that storing individual elements may not 
be the best way memory/storage wise I think performance wise (i.e. when 
the actual matching is performed) it is better matching a single IP 
address than IP range.