[patch 0/2] Ipset patches

[patch 0/2] Ipset patches

[holger]

Hi Jozsef,

what follows are two small patches which remove unneeded argv[]
loops when parsing the ipset commands.  They are in particular no
change in command line handling.

The command line handling can still be improved, as e. g.

 $ ipset -o xml create foo hash:ip
 $ ipset -s add foo 192.168.1.1

both work, but the options are just ignored.  And they don't
make much sense for either 'create' or 'add'.

Also we should consider to make the argument handling a little
more strict, as e. g.

 $ ipset -o xml list foo
 $ ipset list -o xml foo
 $ ipset list foo -o xml

all work.  By removing some of those possibilities it should even
get simpler code wise.

 /holger

-- 

[patch 1/2] ipset: remove the unneeded argvloop

[holger]

[-- Attachment #1: ipset-enforce-command-order.diff --]
[-- Type: text/plain, Size: 3471 bytes --]

Remove the inner argv[] loop.

It is not a change functionality.

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

Index: ipset/src/ipset.c
===================================================================
--- ipset.orig/src/ipset.c	2011-01-19 21:14:59.000000000 +0100
+++ ipset/src/ipset.c	2011-01-19 21:17:55.000000000 +0100
@@ -425,7 +425,6 @@
 {
 	int ret = 0;
 	enum ipset_cmd cmd = IPSET_CMD_NONE;
-	int i;
 	char *arg0 = NULL, *arg1 = NULL, *c;
 	const struct ipset_envopts *opt;
 	const struct ipset_commands *command;
@@ -476,61 +475,58 @@
 
 	/* 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++;
+		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,
+
+		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;
-			}
+		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)
+		/* 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,
-						"Missing mandatory argument "
-						"to command %s",
-						command->name[0]);
+									  "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);
-			}
+		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 2/2] ipset: remove unneeded argvloop

[holger]

[-- Attachment #1: ipset-enforce-command-argument-order.diff --]
[-- Type: text/plain, Size: 2821 bytes --]

It is not a change of functionality.

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

Index: ipset/src/ipset.c
===================================================================
--- ipset.orig/src/ipset.c	2011-01-19 12:11:34.000000000 +0100
+++ ipset/src/ipset.c	2011-01-19 14:05:10.000000000 +0100
@@ -206,61 +206,56 @@
 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;
 	const char *optstr;
 	
 	/* 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++;
-				continue;
-			}
-			optstr = argv[i];
-			/* Shift off matched option */
-			D("match %s", arg->name[0]);
-			ipset_shift_argv(argc, argv, i);
-			D("argc: %u, i: %u", *argc, i);
-			switch (arg->has_arg) {
-			case IPSET_MANDATORY_ARG:
-				if (i + 1 > *argc)
-					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->parse,
-							optstr, arg->opt,
-							argv[i]);
-					if (ret < 0)
-						return ret;
-					ipset_shift_argv(argc, argv, i);
-					break;
-				}
-				/* Fall through */
-			default:
+	for (arg = args; *argc > 1 && arg->opt; arg++) {
+		D("argc: %u: %s vs %s", *argc, argv[1], arg->name[0]);
+		if (!(ipset_match_option(argv[1], arg->name)))
+			continue;
+
+		optstr = argv[1];
+		/* Shift off matched option */
+		D("match %s", arg->name[0]);
+		ipset_shift_argv(argc, argv, 1);
+		D("argc: %u", *argc);
+		switch (arg->has_arg) {
+		case IPSET_MANDATORY_ARG:
+			if (*argc < 2)
+				return exit_error(PARAMETER_PROBLEM,
+								  "Missing mandatory argument "
+								  "of option `%s'",
+								  arg->name[0]);
+			/* Fall through */
+		case IPSET_OPTIONAL_ARG:
+			if (*argc >= 2) {
 				ret = ipset_call_parser(session,
-							arg->parse,
-							optstr, arg->opt,
-							optstr);
+										arg->parse,
+										optstr, arg->opt,
+										argv[1]);
 				if (ret < 0)
 					return ret;
+				ipset_shift_argv(argc, argv, 1);
+				break;
 			}
+			/* Fall through */
+		default:
+			ret = ipset_call_parser(session,
+									arg->parse,
+									optstr, arg->opt,
+									optstr);
+			if (ret < 0)
+				return ret;
 		}
 	}
 done:
-	if (i < *argc)
-		return exit_error(PARAMETER_PROBLEM,
-				  "Unknown argument: `%s'",
-				  argv[i]);
+	if (*argc > 1)
+		return exit_error(PARAMETER_PROBLEM, "Unknown argument: `%s'",
+						  argv[1]);
 	return ret;
 }
 

-- 

Re: [patch 0/2] Ipset patches

[Jozsef Kadlecsik]

Hi Holger,

On Wed, 19 Jan 2011, holger@eitzenberger.org wrote:

> what follows are two small patches which remove unneeded argv[]
> loops when parsing the ipset commands.  They are in particular no
> change in command line handling.
> 
> The command line handling can still be improved, as e. g.
> 
>  $ ipset -o xml create foo hash:ip
>  $ ipset -s add foo 192.168.1.1
> 
> both work, but the options are just ignored.  And they don't
> make much sense for either 'create' or 'add'.
> 
> Also we should consider to make the argument handling a little
> more strict, as e. g.
> 
>  $ ipset -o xml list foo
>  $ ipset list -o xml foo
>  $ ipset list foo -o xml
> 
> all work.  By removing some of those possibilities it should even
> get simpler code wise.

Thank you indeed, after fixing the issues raised by Patrick and Eric, I'll 
work on the userspace part and 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