[patch 0/4] Ipset fixes

Hi Jozsef,

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

Please take a look.  Thanks!

 /holger

-- 

Signed-off-by: Holger Eitzenberger <redacted>

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);
 }

-- 

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 <redacted>

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);
 	

-- 

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 <redacted>

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);
 }
 
 /**

-- 

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 <redacted>

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.

-- 

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

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

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

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

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

On Tuesday 2011-01-18 21:18, Jozsef Kadlecsik wrote:

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

quoted

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.

On Tue, 18 Jan 2011, Jan Engelhardt wrote:

On Tuesday 2011-01-18 21:18, Jozsef Kadlecsik wrote:

quoted

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

quoted

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

On Tuesday 2011-01-18 21:37, Jozsef Kadlecsik wrote:

quoted

quoted

quoted

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?

On Tue, 18 Jan 2011, Jan Engelhardt wrote:

On Tuesday 2011-01-18 21:37, Jozsef Kadlecsik wrote:

quoted

quoted

quoted

quoted

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

On Tue, Jan 18, 2011 at 09:39:32PM +0100, Jan Engelhardt wrote:

On Tuesday 2011-01-18 21:37, Jozsef Kadlecsik wrote:

quoted

quoted

quoted

quoted

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.

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.