[patch 0/6] Ipset patches

[patch 0/6] Ipset patches

[Holger Eitzenberger]

Hi Jozsef,

what follows are some fixes and improvements for Ipset, please
consider.

This time please keep the 'Signed-off-by' line if applying one of
those.

Thanks.

 /holger

[patch 1/6] ipset: turn Set name[] into a const pointer

[Holger Eitzenberger]

[-- Attachment #1: ipset-set_type-name-pointer.diff --]
[-- Type: text/plain, Size: 2005 bytes --]

Also check for the name length.

Note that passing errno values back is not done consistently at
various place, as there are some functions which set errno manually,
others pass -errno back.  I use the -errno approach here, as it is
slightly shorter.

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

Index: ipset/include/libipset/types.h
===================================================================
--- ipset.orig/include/libipset/types.h
+++ ipset/include/libipset/types.h
@@ -70,7 +70,7 @@ struct ipset_elem {
  * but for the readability the full list is supported.
   */
 struct ipset_type {
-	char name[IPSET_MAXNAMELEN];		/* type name */
+	const char *name;
 	uint8_t revision;			/* revision number */
 	uint8_t family;				/* supported family */
 	uint8_t dimension;			/* elem dimension */
Index: ipset/lib/types.c
===================================================================
--- ipset.orig/lib/types.c
+++ ipset/lib/types.c
@@ -441,13 +441,15 @@ ipset_type_add(struct ipset_type *type)
 
 	assert(type);
 
+	if (strlen(type->name) > IPSET_MAXNAMELEN - 1)
+		return -EINVAL;
+
 	/* Add to the list: higher revision numbers first */
 	for (t = typelist, prev = NULL; t != NULL; t = t->next) {
 		if (STREQ(t->name, type->name)) {
-			if (t->revision == type->revision) {
-				errno = EEXIST;
-				return -1;
-			} else if (t->revision < type->revision) {
+			if (t->revision == type->revision)
+				return -EEXIST;
+			else if (t->revision < type->revision) {
 				type->next = t;
 				if (prev)
 					prev->next = type;
@@ -457,10 +459,9 @@ ipset_type_add(struct ipset_type *type)
 			}
 		}
 		if (t->next != NULL && STREQ(t->next->name, type->name)) {
-			if (t->next->revision == type->revision) {
-				errno = EEXIST;
-				return -1;
-			} else if (t->next->revision < type->revision) {
+			if (t->next->revision == type->revision)
+				return -EEXIST;
+			else if (t->next->revision < type->revision) {
 				type->next = t->next;
 				t->next = type;
 				return 0;

[patch 2/6] ipset: fix the Netlink sequence number

[Holger Eitzenberger]

[-- Attachment #1: ipset-netlink-sequence.diff --]
[-- Type: text/plain, Size: 999 bytes --]

Do not use time() as a Netlink sequence number for each message,
as otherwise the same seq number will be used when sending
another message in the same second.  Instead use time() just for
initialization, then increment per message.

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

Index: ipset/lib/mnl.c
===================================================================
--- ipset.orig/lib/mnl.c
+++ ipset/lib/mnl.c
@@ -75,7 +75,7 @@ ipset_mnl_fill_hdr(struct ipset_handle *
 	nlh->nlmsg_flags = cmdflags[cmd - 1];
 	if (envflags & IPSET_ENV_EXIST)
 	    	nlh->nlmsg_flags &=  ~NLM_F_EXCL;
-	nlh->nlmsg_seq = handle->seq = time(NULL);
+	nlh->nlmsg_seq = ++handle->seq;
 
 	nfg = mnl_nlmsg_put_extra_header(nlh, sizeof(struct nfgenmsg));
 	nfg->nfgen_family = AF_INET;
@@ -135,6 +135,7 @@ ipset_mnl_init(mnl_cb_t *cb_ctl, void *d
 	handle->portid = mnl_socket_get_portid(handle->h);
 	handle->cb_ctl = cb_ctl;
 	handle->data = data;
+	handle->seq = time(NULL);
 	
 	return handle;
 

[patch 3/6] ipset: pass ipset_arg argument pointer

[Holger Eitzenberger]

[-- Attachment #1: ipset-pass-arg.diff --]
[-- Type: text/plain, Size: 3030 bytes --]

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

Index: ipset/src/ipset.c
===================================================================
--- ipset.orig/src/ipset.c
+++ ipset/src/ipset.c
@@ -208,7 +208,6 @@ call_parser(int *argc, char *argv[], con
 {
 	int i = 1, ret = 0;
 	const struct ipset_arg *arg;
-	const char *optstr;
 	
 	/* Currently CREATE and ADT may have got additional arguments */
 	if (!args)
@@ -221,7 +220,6 @@ call_parser(int *argc, char *argv[], con
 				i++;
 				continue;
 			}
-			optstr = argv[i];
 			/* Shift off matched option */
 			D("match %s", arg->name[0]);
 			ipset_shift_argv(argc, argv, i);
@@ -236,10 +234,7 @@ call_parser(int *argc, char *argv[], con
 				/* Fall through */
 			case IPSET_OPTIONAL_ARG:
 				if (i + 1 <= *argc) {
-					ret = ipset_call_parser(session,
-							arg->parse,
-							optstr, arg->opt,
-							argv[i]);
+					ret = ipset_call_parser(session, arg, argv[i]);
 					if (ret < 0)
 						return ret;
 					ipset_shift_argv(argc, argv, i);
@@ -247,10 +242,7 @@ call_parser(int *argc, char *argv[], con
 				}
 				/* Fall through */
 			default:
-				ret = ipset_call_parser(session,
-							arg->parse,
-							optstr, arg->opt,
-							optstr);
+				ret = ipset_call_parser(session, arg, argv[1]);
 				if (ret < 0)
 					return ret;
 			}
Index: ipset/include/libipset/parse.h
===================================================================
--- ipset.orig/include/libipset/parse.h
+++ ipset/include/libipset/parse.h
@@ -17,6 +17,7 @@
 #define IPSET_PROTO_SEPARATOR	":"
 
 struct ipset_session;
+struct ipset_arg;
 
 typedef int (*ipset_parsefn)(struct ipset_session *s,
 			     enum ipset_opt opt, const char *str);
@@ -84,8 +85,8 @@ extern int ipset_parse_ignored(struct ip
 extern int ipset_parse_elem(struct ipset_session *session,
                             enum ipset_opt opt, const char *str);
 extern int ipset_call_parser(struct ipset_session *session,
-			     ipset_parsefn parse, const char *optstr,
-			     enum ipset_opt optional, const char *str);
+							 const struct ipset_arg *arg,
+							 const char *str);
 
 /* Compatibility parser functions */
 extern int ipset_parse_iptimeout(struct ipset_session *session,
Index: ipset/lib/parse.c
===================================================================
--- ipset.orig/lib/parse.c
+++ ipset/lib/parse.c
@@ -1416,15 +1416,14 @@ ipset_parse_ignored(struct ipset_session
  */
 int
 ipset_call_parser(struct ipset_session *session,
-		  ipset_parsefn parse, const char *optstr,
-		  enum ipset_opt opt, const char *str)
+				  const struct ipset_arg *arg,
+				  const char *str)
 {
 	if (ipset_data_flags_test(ipset_session_data(session),
-				  IPSET_FLAG(opt)))
-		syntax_err("%s already specified", optstr);
+				  IPSET_FLAG(arg->opt)))
+		syntax_err("%s already specified", arg->name[0]);
 
-	return parse(session, opt, parse == ipset_parse_ignored
-				   ? optstr : str);
+	return arg->parse(session, arg->opt, str);
 }
 
 #define parse_elem(s, t, d, str)					\

[patch 4/6] ipset: avoid the unnecessary argv[] loop

[Holger Eitzenberger]

[-- Attachment #1: ipset-remove-unnecessary-argv-loop.diff --]
[-- Type: text/plain, Size: 3397 bytes --]

After stripping off the global options there simply has to follow
a command name, there is no other syntax possible.  Therefore the
argv[] loop is unnecessary.

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

Index: ipset/src/ipset.c
===================================================================
--- ipset.orig/src/ipset.c
+++ ipset/src/ipset.c
@@ -468,61 +468,57 @@ parse_commandline(int argc, char *argv[]
 
 	/* Second: parse command */
 	for (command = ipset_commands;
-	     command->cmd && cmd == IPSET_CMD_NONE;
+		 argc > 1 && command->cmd && cmd == IPSET_CMD_NONE;
 	     command++) {
-		for (i = 1; i < argc; ) {
-			if (!ipset_match_cmd(argv[1], command->name)) {
-				i++;
-				continue;
-			}
-			if (restore_line != 0
-			    && (command->cmd == IPSET_CMD_RESTORE
-			    	|| command->cmd == IPSET_CMD_VERSION
-			    	|| command->cmd == IPSET_CMD_HELP))
+		if (!ipset_match_cmd(argv[1], command->name))
+			continue;
+
+		if (restore_line != 0
+		    && (command->cmd == IPSET_CMD_RESTORE
+		    	|| command->cmd == IPSET_CMD_VERSION
+		    	|| command->cmd == IPSET_CMD_HELP))
+			return exit_error(PARAMETER_PROBLEM,
+				"Command `%s' is invalid "
+				"in restore mode.",
+				command->name[0]);
+		if (interactive && command->cmd == IPSET_CMD_RESTORE) {
+			printf("Restore command ignored "
+			       "in interactive mode\n");
+			return 0;
+		}
+
+		/* Shift off matched command arg */
+		ipset_shift_argv(&argc, argv, 1);
+		cmd = command->cmd;
+		switch (command->has_arg) {
+		case IPSET_MANDATORY_ARG:
+		case IPSET_MANDATORY_ARG2:
+			if (argc < 2)
 				return exit_error(PARAMETER_PROBLEM,
-					"Command `%s' is invalid "
-					"in restore mode.",
+					"Missing mandatory argument "
+					"to command %s",
 					command->name[0]);
-				if (interactive
-				    && command->cmd == IPSET_CMD_RESTORE) {
-					printf("Restore command ignored "
-					       "in interactive mode\n");
-				return 0;
-			}
-
-			/* Shift off matched command arg */
-			ipset_shift_argv(&argc, argv, i);
-			cmd = command->cmd;
-			switch (command->has_arg) {
-			case IPSET_MANDATORY_ARG:
-			case IPSET_MANDATORY_ARG2:
-				if (i + 1 > argc)
-					return exit_error(PARAMETER_PROBLEM,
-						"Missing mandatory argument "
-						"to command %s",
-						command->name[0]);
-				/* Fall through */
-			case IPSET_OPTIONAL_ARG:
-				arg0 = argv[i];
-				if (i + 1 <= argc)
-					/* Shift off first arg */
-					ipset_shift_argv(&argc, argv, i);
-				break;
-			default:
-				break;
-			}
-			if (command->has_arg == IPSET_MANDATORY_ARG2) {
-				if (i + 1 > argc)
-					return exit_error(PARAMETER_PROBLEM,
-						"Missing second mandatory "
-						"argument to command %s",
-						command->name[0]);
-				arg1 = argv[i];
-				/* Shift off second arg */
-				ipset_shift_argv(&argc, argv, i);
-			}
+			/* Fall through */
+		case IPSET_OPTIONAL_ARG:
+			arg0 = argv[1];
+			if (argc >= 2)
+				/* Shift off first arg */
+				ipset_shift_argv(&argc, argv, 1);
 			break;
+		default:
+			break;
+		}
+		if (command->has_arg == IPSET_MANDATORY_ARG2) {
+			if (argc < 2)
+				return exit_error(PARAMETER_PROBLEM,
+					"Missing second mandatory "
+					"argument to command %s",
+					command->name[0]);
+			arg1 = argv[1];
+			/* Shift off second arg */
+			ipset_shift_argv(&argc, argv, 1);
 		}
+		break;
 	}
 
 	/* Third: catch interactive mode, handle help, version */

[patch 5/6] ipset: improve command argument parsing

[Holger Eitzenberger]

[-- Attachment #1: ipset-improve-cmd-argument-parsing.diff --]
[-- Type: text/plain, Size: 2669 bytes --]

The number of comparisons for a matching a command name can be
made smaller by just checking on argv[1].

As an example consider the following 'create' arguments 'hashsize',
'family' and 'timeout'.  When having the command
 
 create foo hash:ip timeout 60 family inet hashsize 64

it compares without this patch:

 strcmp("timeout", "hashsize")
 strcmp("64", "hashsize")
 strcmp("family", "hashsize")
 strcmp("inet", "hashsize")
 strcmp("hashsize", "hashsize")

It is worse in practice, as 'create' has more arguments than this.

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

Index: ipset/src/ipset.c
===================================================================
--- ipset.orig/src/ipset.c
+++ ipset/src/ipset.c
@@ -206,38 +206,35 @@ restore(char *argv0)
 static int
 call_parser(int *argc, char *argv[], const struct ipset_arg *args)
 {
-	int i = 1, ret = 0;
+	int ret = 0;
 	const struct ipset_arg *arg;
 	
 	/* Currently CREATE and ADT may have got additional arguments */
-	if (!args)
-		goto done;
-	for (arg = args; arg->opt; arg++) {
-		for (i = 1; i < *argc; ) {
-			D("argc: %u, i: %u: %s vs %s",
-			  *argc, i, argv[i], arg->name[0]);
-			if (!(ipset_match_option(argv[i], arg->name))) {
-				i++;
+	if (!args && *argc > 1)
+		goto err_unknown;
+	while (*argc > 1) {
+		for (arg = args; arg->opt; arg++) {
+			D("argc: %u, %s vs %s", *argc, argv[1], arg->name[0]);
+			if (!(ipset_match_option(argv[1], arg->name)))
 				continue;
-			}
+
 			/* Shift off matched option */
 			D("match %s", arg->name[0]);
-			ipset_shift_argv(argc, argv, i);
-			D("argc: %u, i: %u", *argc, i);
+			ipset_shift_argv(argc, argv, 1);
 			switch (arg->has_arg) {
 			case IPSET_MANDATORY_ARG:
-				if (i + 1 > *argc)
+				if (*argc < 2)
 					return exit_error(PARAMETER_PROBLEM,
 						"Missing mandatory argument "
 						"of option `%s'",
 						arg->name[0]);
 				/* Fall through */
 			case IPSET_OPTIONAL_ARG:
-				if (i + 1 <= *argc) {
-					ret = ipset_call_parser(session, arg, argv[i]);
+				if (*argc >= 2) {
+					ret = ipset_call_parser(session, arg, argv[1]);
 					if (ret < 0)
 						return ret;
-					ipset_shift_argv(argc, argv, i);
+					ipset_shift_argv(argc, argv, 1);
 					break;
 				}
 				/* Fall through */
@@ -246,14 +243,18 @@ call_parser(int *argc, char *argv[], con
 				if (ret < 0)
 					return ret;
 			}
+
+			break;
 		}
+
+		if (!arg->opt)
+			goto err_unknown;
 	}
-done:
-	if (i < *argc)
-		return exit_error(PARAMETER_PROBLEM,
-				  "Unknown argument: `%s'",
-				  argv[i]);
+
 	return ret;
+
+err_unknown:
+	return exit_error(PARAMETER_PROBLEM, "Unknown argument: `%s'", argv[1]);
 }
 
 static enum ipset_adt

[patch 6/6] ipset: fix spelling error

[Holger Eitzenberger]

[-- Attachment #1: ipset-fix-spelling-error.diff --]
[-- Type: text/plain, Size: 945 bytes --]

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

Index: ipset/src/errcode.c
===================================================================
--- ipset.orig/src/errcode.c
+++ ipset/src/errcode.c
@@ -30,14 +30,14 @@ static const struct ipset_errcode_table
 	{ EEXIST, IPSET_CMD_CREATE,
 	  "Set cannot be created: set with the same name already exists" },
 	{ IPSET_ERR_FIND_TYPE, 0,
-	  "Kernel error received: set type does not supported" },
+	  "Kernel error received: set type not supported" },
 	{ IPSET_ERR_MAX_SETS, 0,
 	  "Kernel error received: maximal number of sets reached, "
 	  "cannot create more." },
 	{ IPSET_ERR_INVALID_NETMASK, 0,
 	  "The value of the netmask parameter is invalid" },
 	{ IPSET_ERR_INVALID_FAMILY, 0,
-	  "The protocol family not supported by the set type" },
+	  "Potocol family not supported by the set type" },
 
 	/* DESTROY specific error codes */
 	{ IPSET_ERR_BUSY, IPSET_CMD_DESTROY,

Re: [patch 1/6] ipset: turn Set name[] into a const pointer

[Jozsef Kadlecsik]

On Mon, 24 Jan 2011, Holger Eitzenberger wrote:

> Also check for the name length.
> 
> Note that passing errno values back is not done consistently at
> various place, as there are some functions which set errno manually,
> others pass -errno back.  I use the -errno approach here, as it is
> slightly shorter.
> 
> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Applied, thanks 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/6] ipset: fix the Netlink sequence number

[Jozsef Kadlecsik]

On Mon, 24 Jan 2011, Holger Eitzenberger wrote:

> Do not use time() as a Netlink sequence number for each message,
> as otherwise the same seq number will be used when sending
> another message in the same second.  Instead use time() just for
> initialization, then increment per message.
> 
> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Yes, you are right, also applied.

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/6] ipset: pass ipset_arg argument pointer

[Jozsef Kadlecsik]

On Mon, 24 Jan 2011, Holger Eitzenberger wrote:

> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> 
> Index: ipset/src/ipset.c
> ===================================================================
> --- ipset.orig/src/ipset.c
> +++ ipset/src/ipset.c
> @@ -208,7 +208,6 @@ call_parser(int *argc, char *argv[], con
>  {
>  	int i = 1, ret = 0;
>  	const struct ipset_arg *arg;
> -	const char *optstr;
>  	
>  	/* Currently CREATE and ADT may have got additional arguments */
>  	if (!args)
> @@ -221,7 +220,6 @@ call_parser(int *argc, char *argv[], con
>  				i++;
>  				continue;
>  			}
> -			optstr = argv[i];
>  			/* Shift off matched option */
>  			D("match %s", arg->name[0]);
>  			ipset_shift_argv(argc, argv, i);
> @@ -236,10 +234,7 @@ call_parser(int *argc, char *argv[], con
>  				/* Fall through */
>  			case IPSET_OPTIONAL_ARG:
>  				if (i + 1 <= *argc) {
> -					ret = ipset_call_parser(session,
> -							arg->parse,
> -							optstr, arg->opt,
> -							argv[i]);
> +					ret = ipset_call_parser(session, arg, argv[i]);
>  					if (ret < 0)
>  						return ret;
>  					ipset_shift_argv(argc, argv, i);
> @@ -247,10 +242,7 @@ call_parser(int *argc, char *argv[], con
>  				}
>  				/* Fall through */
>  			default:
> -				ret = ipset_call_parser(session,
> -							arg->parse,
> -							optstr, arg->opt,
> -							optstr);
> +				ret = ipset_call_parser(session, arg, argv[1]);

We can't do this, because then the flag-style options (no arg) cannot be 
parsed. So I cannot apply this patch and the next two ones based on it.

>  				if (ret < 0)
>  					return ret;
>  			}
> Index: ipset/include/libipset/parse.h
> ===================================================================
> --- ipset.orig/include/libipset/parse.h
> +++ ipset/include/libipset/parse.h
> @@ -17,6 +17,7 @@
>  #define IPSET_PROTO_SEPARATOR	":"
>  
>  struct ipset_session;
> +struct ipset_arg;
>  
>  typedef int (*ipset_parsefn)(struct ipset_session *s,
>  			     enum ipset_opt opt, const char *str);
> @@ -84,8 +85,8 @@ extern int ipset_parse_ignored(struct ip
>  extern int ipset_parse_elem(struct ipset_session *session,
>                              enum ipset_opt opt, const char *str);
>  extern int ipset_call_parser(struct ipset_session *session,
> -			     ipset_parsefn parse, const char *optstr,
> -			     enum ipset_opt optional, const char *str);
> +							 const struct ipset_arg *arg,
> +							 const char *str);
>  
>  /* Compatibility parser functions */
>  extern int ipset_parse_iptimeout(struct ipset_session *session,
> Index: ipset/lib/parse.c
> ===================================================================
> --- ipset.orig/lib/parse.c
> +++ ipset/lib/parse.c
> @@ -1416,15 +1416,14 @@ ipset_parse_ignored(struct ipset_session
>   */
>  int
>  ipset_call_parser(struct ipset_session *session,
> -		  ipset_parsefn parse, const char *optstr,
> -		  enum ipset_opt opt, const char *str)
> +				  const struct ipset_arg *arg,
> +				  const char *str)
>  {
>  	if (ipset_data_flags_test(ipset_session_data(session),
> -				  IPSET_FLAG(opt)))
> -		syntax_err("%s already specified", optstr);
> +				  IPSET_FLAG(arg->opt)))
> +		syntax_err("%s already specified", arg->name[0]);
>  
> -	return parse(session, opt, parse == ipset_parse_ignored
> -				   ? optstr : str);
> +	return arg->parse(session, arg->opt, str);
>  }
>  
>  #define parse_elem(s, t, d, str)					\

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 6/6] ipset: fix spelling error

[Jozsef Kadlecsik]

On Mon, 24 Jan 2011, Holger Eitzenberger wrote:

> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> 
> Index: ipset/src/errcode.c
> ===================================================================
> --- ipset.orig/src/errcode.c
> +++ ipset/src/errcode.c
> @@ -30,14 +30,14 @@ static const struct ipset_errcode_table
>  	{ EEXIST, IPSET_CMD_CREATE,
>  	  "Set cannot be created: set with the same name already exists" },
>  	{ IPSET_ERR_FIND_TYPE, 0,
> -	  "Kernel error received: set type does not supported" },
> +	  "Kernel error received: set type not supported" },
>  	{ IPSET_ERR_MAX_SETS, 0,
>  	  "Kernel error received: maximal number of sets reached, "
>  	  "cannot create more." },
>  	{ IPSET_ERR_INVALID_NETMASK, 0,
>  	  "The value of the netmask parameter is invalid" },
>  	{ IPSET_ERR_INVALID_FAMILY, 0,
> -	  "The protocol family not supported by the set type" },
> +	  "Potocol family not supported by the set type" },
>  
>  	/* DESTROY specific error codes */
>  	{ IPSET_ERR_BUSY, IPSET_CMD_DESTROY,
> 
> --

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 6/6] ipset: fix spelling error

[Ferenc Wagner]

"Holger Eitzenberger" <holger@eitzenberger.org> writes:

> -	  "The protocol family not supported by the set type" },
> +	  "Potocol family not supported by the set type" },

I guess you meat Protocol instead.
-- 
Feri.

Re: [patch 6/6] ipset: fix spelling error

[Jozsef Kadlecsik]

On Wed, 26 Jan 2011, Ferenc Wagner wrote:

> "Holger Eitzenberger" <holger@eitzenberger.org> writes:
> 
> > -	  "The protocol family not supported by the set type" },
> > +	  "Potocol family not supported by the set type" },
> 
> I guess you meat Protocol instead.

Fixed, 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 3/6] ipset: pass ipset_arg argument pointer

[Jozsef Kadlecsik]

Hi Holger,

On Tue, 25 Jan 2011, Jozsef Kadlecsik wrote:

> On Mon, 24 Jan 2011, Holger Eitzenberger wrote:
> 
> > Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> > 
> > Index: ipset/src/ipset.c
> > ===================================================================
> > --- ipset.orig/src/ipset.c
> > +++ ipset/src/ipset.c
> > @@ -208,7 +208,6 @@ call_parser(int *argc, char *argv[], con
> >  {
> >  	int i = 1, ret = 0;
> >  	const struct ipset_arg *arg;
> > -	const char *optstr;
> >  	
> >  	/* Currently CREATE and ADT may have got additional arguments */
> >  	if (!args)
> > @@ -221,7 +220,6 @@ call_parser(int *argc, char *argv[], con
> >  				i++;
> >  				continue;
> >  			}
> > -			optstr = argv[i];
> >  			/* Shift off matched option */
> >  			D("match %s", arg->name[0]);
> >  			ipset_shift_argv(argc, argv, i);
> > @@ -236,10 +234,7 @@ call_parser(int *argc, char *argv[], con
> >  				/* Fall through */
> >  			case IPSET_OPTIONAL_ARG:
> >  				if (i + 1 <= *argc) {
> > -					ret = ipset_call_parser(session,
> > -							arg->parse,
> > -							optstr, arg->opt,
> > -							argv[i]);
> > +					ret = ipset_call_parser(session, arg, argv[i]);
> >  					if (ret < 0)
> >  						return ret;
> >  					ipset_shift_argv(argc, argv, i);
> > @@ -247,10 +242,7 @@ call_parser(int *argc, char *argv[], con
> >  				}
> >  				/* Fall through */
> >  			default:
> > -				ret = ipset_call_parser(session,
> > -							arg->parse,
> > -							optstr, arg->opt,
> > -							optstr);
> > +				ret = ipset_call_parser(session, arg, argv[1]);
> 
> We can't do this, because then the flag-style options (no arg) cannot be 
> parsed. So I cannot apply this patch and the next two ones based on it.

If optstr is kept, then that can be passed to the parser in the default 
branch. Could you rewrite your patch that way and resend to me (and the 
dependent ones too)?

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/6] ipset: pass ipset_arg argument pointer

[Jozsef Kadlecsik]

On Thu, 27 Jan 2011, Jozsef Kadlecsik wrote:

> Hi Holger,
> 
> On Tue, 25 Jan 2011, Jozsef Kadlecsik wrote:
> 
> > On Mon, 24 Jan 2011, Holger Eitzenberger wrote:
> > 
> > > Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> > > 
> > > Index: ipset/src/ipset.c
> > > ===================================================================
> > > --- ipset.orig/src/ipset.c
> > > +++ ipset/src/ipset.c
> > > @@ -208,7 +208,6 @@ call_parser(int *argc, char *argv[], con
> > >  {
> > >  	int i = 1, ret = 0;
> > >  	const struct ipset_arg *arg;
> > > -	const char *optstr;
> > >  	
> > >  	/* Currently CREATE and ADT may have got additional arguments */
> > >  	if (!args)
> > > @@ -221,7 +220,6 @@ call_parser(int *argc, char *argv[], con
> > >  				i++;
> > >  				continue;
> > >  			}
> > > -			optstr = argv[i];
> > >  			/* Shift off matched option */
> > >  			D("match %s", arg->name[0]);
> > >  			ipset_shift_argv(argc, argv, i);
> > > @@ -236,10 +234,7 @@ call_parser(int *argc, char *argv[], con
> > >  				/* Fall through */
> > >  			case IPSET_OPTIONAL_ARG:
> > >  				if (i + 1 <= *argc) {
> > > -					ret = ipset_call_parser(session,
> > > -							arg->parse,
> > > -							optstr, arg->opt,
> > > -							argv[i]);
> > > +					ret = ipset_call_parser(session, arg, argv[i]);
> > >  					if (ret < 0)
> > >  						return ret;
> > >  					ipset_shift_argv(argc, argv, i);
> > > @@ -247,10 +242,7 @@ call_parser(int *argc, char *argv[], con
> > >  				}
> > >  				/* Fall through */
> > >  			default:
> > > -				ret = ipset_call_parser(session,
> > > -							arg->parse,
> > > -							optstr, arg->opt,
> > > -							optstr);
> > > +				ret = ipset_call_parser(session, arg, argv[1]);
> > 
> > We can't do this, because then the flag-style options (no arg) cannot be 
> > parsed. So I cannot apply this patch and the next two ones based on it.
> 
> If optstr is kept, then that can be passed to the parser in the default 
> branch. Could you rewrite your patch that way and resend to me (and the 
> dependent ones too)?

I re-added the missing bits and applied your patches. 

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