iptables-edit: iptables-edit: adds --table to iptables-restore

PATCH Applied Peter Warasin 3 participants 11 messages Nov 5, 2007 – Nov 7, 2007 netfilter

lore

Share & tools

lore

Peter Warasin

author

2007-11-05 00:13 UTC #0 lore raw

PATCH 1/3 iptables-edit: iptables-edit: adds --table to iptables-restore

Signature · 6 lines

-- 
:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: [email protected]

adds --table to iptables-restore which allows to restore only the supplied table

Signed-off-by Peter Warasin <[email protected]>

---
 iptables-restore.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

iptables-restore.c renamed from iptables-restore.c.orig +11 -1

Index: iptables-restore.c
===================================================================
@@ -33,6 +33,7 @@
 	{ "help", 0, 0, 'h' },
 	{ "noflush", 0, 0, 'n'},
 	{ "modprobe", 1, 0, 'M'},
+	{ "table", 1, 0, 'T'},
 	{ 0 }
 };

@@ -47,6 +48,7 @@
 			"	   [ --test ]\n"
 			"	   [ --help ]\n"
 			"	   [ --noflush ]\n"
+			"	   [ --table=<TABLE> ]\n"
 		        "          [ --modprobe=<command>]\n", name);
 		
 	exit(1);

@@ -123,6 +125,7 @@
 	FILE *in;
 	const char *modprobe = 0;
 	int in_table = 0, testing = 0;
+	const char *tablename = 0;
 
 	program_name = "iptables-restore";
 	program_version = IPTABLES_VERSION;

@@ -136,7 +139,7 @@
 	init_extensions();
 #endif
 
-	while ((c = getopt_long(argc, argv, "bcvthnM:", options, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, "bcvthnM:T:", options, NULL)) != -1) {
 		switch (c) {
 			case 'b':
 				binary = 1;

@@ -160,6 +163,9 @@
 			case 'M':
 				modprobe = optarg;
 				break;
+			case 'T':
+				tablename = optarg;
+				break;
 		}
 	}

@@ -212,6 +218,8 @@
 			strncpy(curtable, table, IPT_TABLE_MAXNAMELEN);
 			curtable[IPT_TABLE_MAXNAMELEN] = '\0';
 
+			if (tablename && (strcmp(tablename, table) != 0))
+				continue;
 			if (handle)
 				iptc_free(&handle);

@@ -438,6 +446,8 @@
 
 			free_argv();
 		}
+		if (tablename && (strcmp(tablename, curtable) != 0))
+			continue;
 		if (!ret) {
 			fprintf(stderr, "%s: line %u failed\n",
 					program_name, line);

begin:vcard
fn:Peter Warasin
n:;Peter Warasin
org:Endian GmbH/Srl
adr:;;Pillhof 47;Frangart/Frangarto;BZ;I-39010;Italien/Italia email;internet:[email protected]
tel;work:+39 0471 631763
tel;fax:+39 0471 631764
x-mozilla-html:FALSE
url:http://www.endian.com
version:2.1
end:vcard

Patrick McHardy

Applied

2007-11-06 00:12 UTC #1 lore raw

Quoted 2 lines · Peter Warasin

adds --table to iptables-restore which allows to restore only the supplied table

Applied, thanks Peter.

Jan Engelhardt

2007-11-07 10:52 UTC #2 lore raw

In reply to Peter Warasin

Quoted 14 lines · Peter Warasin

Date: Mon, 05 Nov 2007 01:13:53 +0100
From: Peter Warasin <[email protected]>
To: <[email protected]>
Subject: [PATCH 1/3] iptables-edit: iptables-edit: adds --table to
iptables-restore
Signature · 5 lines
:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: [email protected]

Well, please send patches inline so I can reply to something.

|adds --table to iptables-restore which allows to restore only the
|supplied table
|
|Signed-off-by: Peter Warasin <[email protected]>
|---
| iptables-restore.c |   12 +++++++++++-
| 1 file changed, 11 insertions(+), 1 deletion(-)
|
|Index: iptables-restore.c
|===================================================================
|--- iptables-restore.c.orig
|+++ iptables-restore.c
|@@ -33,6 +33,7 @@
| 	{ "help", 0, 0, 'h' },
| 	{ "noflush", 0, 0, 'n'},
| 	{ "modprobe", 1, 0, 'M'},
|+	{ "table", 1, 0, 'T'},
| 	{ 0 }
| };
| 
|@@ -47,6 +48,7 @@
| 			"	   [ --test ]\n"
| 			"	   [ --help ]\n"
| 			"	   [ --noflush ]\n"
|+			"	   [ --table=<TABLE> ]\n"
| 		        "          [ --modprobe=<command>]\n", name);
| 		
| 	exit(1);
|@@ -123,6 +125,7 @@
| 	FILE *in;
| 	const char *modprobe = 0;
| 	int in_table = 0, testing = 0;
|+	const char *tablename = 0;

Use NULL instead.

| 
| 	program_name = "iptables-restore";
| 	program_version = IPTABLES_VERSION;
|@@ -136,7 +139,7 @@
| 	init_extensions();
| #endif
| 
|-	while ((c = getopt_long(argc, argv, "bcvthnM:", options, NULL)) != -1) {
|+	while ((c = getopt_long(argc, argv, "bcvthnM:T:", options, NULL)) != -1) {

Use -t instead, that would go in line with the -t from iptables.

| 		switch (c) {
| 			case 'b':
| 				binary = 1;
|@@ -160,6 +163,9 @@
| 			case 'M':
| 				modprobe = optarg;
| 				break;
|+			case 'T':
|+				tablename = optarg;
|+				break;
| 		}
| 	}
| 	
|@@ -212,6 +218,8 @@
| 			strncpy(curtable, table, IPT_TABLE_MAXNAMELEN);
| 			curtable[IPT_TABLE_MAXNAMELEN] = '\0';
| 
|+			if (tablename && (strcmp(tablename, table) != 0))
|+				continue;

drop extra ()

| 			if (handle)
| 				iptc_free(&handle);
| 
|@@ -438,6 +446,8 @@
| 
| 			free_argv();
| 		}
|+		if (tablename && (strcmp(tablename, curtable) != 0))
|+			continue;
| 		if (!ret) {
| 			fprintf(stderr, "%s: line %u failed\n",
| 					program_name, line);

same

Peter Warasin

author

2007-11-07 13:44 UTC #3 lore raw

Hi Jan

Quoted 1 line · Jan Engelhardt

Well, please send patches inline so I can reply to something.

Ok,.. I think i eventually have to upgrade my thunderbird, since i have no option for sending inline attachments :-O

Quoted 2 lines

|adds --table to iptables-restore which allows to restore only the |supplied table

This patch is already applied. Should i fix your suggestions or leave it as it is?

Quoted 4 lines

|- while ((c = getopt_long(argc, argv, "bcvthnM:", options, NULL)) != -1) {
|+ while ((c = getopt_long(argc, argv, "bcvthnM:T:", options, NULL)) != -1) {
Use -t instead, that would go in line with the -t from iptables.

-t is already --test, therefore i used -T
I suggest changing --test to --dry-run (-d), so -t will turn free for --table, what do you think?

peter

Signature · 5 lines

:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: [email protected]

Jan Engelhardt

2007-11-07 16:41 UTC #4 lore raw

Quoted 7 lines · Peter Warasin

Quoted 2 lines · Jan Engelhardt
|adds --table to iptables-restore which allows to restore only the |supplied table
This patch is already applied. Should i fix your suggestions or leave it as it is?

Try changing, it's just the Subversion thing.

Quoted 8 lines

Quoted 4 lines
|- while ((c = getopt_long(argc, argv, "bcvthnM:", options, NULL)) != -1) {
|+ while ((c = getopt_long(argc, argv, "bcvthnM:T:", options, NULL)) != -1) {
Use -t instead, that would go in line with the -t from iptables.
-t is already --test, therefore i used -T
I suggest changing --test to --dry-run (-d), so -t will turn free for --table, what do you think?

Is a dry-run option really needed? iptables-edit does not commit any tables to the kernel, so the program in itself is already in dry-run mode. If the resulting ruleset that iptables-edit will generate is not loadable (e.g. xt_somemodule missing), then iptables-restore should turn that up when trying to load the bogus ruleset.

Peter Warasin

author

2007-11-07 18:15 UTC #5 lore raw

Quoted 3 lines · Jan Engelhardt

Quoted 2 lines
This patch is already applied. Should i fix your suggestions or leave it as it is?
Try changing, it's just the Subversion thing.

Ok, i will resend later.

Quoted 1 line

Is a dry-run option really needed? iptables-edit does not commit

Attention, this is a new option of iptables-restore, not of iptables-edit. --test was already implemented within iptables-restore.
So in order to use -t instead of -T it's necessary to rename --test to for example --dry-run

peter

Signature · 5 lines

:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: [email protected]

Jan Engelhardt

2007-11-07 18:34 UTC #6 lore raw

Quoted 7 lines · Peter Warasin

Quoted 2 lines
Is a dry-run option really needed? iptables-edit does not commit
Attention, this is a new option of iptables-restore, not of iptables-edit. --test was already implemented within iptables-restore.
So in order to use -t instead of -T it's necessary to rename --test to for example --dry-run

-t means table for:
iptables
iptables-save
ip6tables
ip6tables-save
arptables
ebtables

iptables-restore having -t for test really seems out of place.

That said, iptables-restore does not have a -t, according to its help text ("iptables-restore -h") and its manpage.

Peter Warasin

author

2007-11-07 19:12 UTC #7 lore raw

Hi Jan

Quoted 5 lines · Jan Engelhardt

Quoted 2 lines · Peter Warasin
Quoted 1 line
Is a dry-run option really needed? iptables-edit does not commit
Attention, this is a new option of iptables-restore, not of iptables-edit.
-t means table for:

[..]

Quoted 1 line

iptables-restore having -t for test really seems out of place.

i agree.

Quoted 2 lines

That said, iptables-restore does not have a -t, according to its help text ("iptables-restore -h") and its manpage.

Thats not true. The current subversion code uses -t for --test. Probably you confuse with iptables-save ?

http://svn.netfilter.org/cgi-bin/viewcvs.cgi/trunk/iptables/iptables-restore.c?rev=7087&view=markup
-------- snip -----------------------------------------------

			case 't':
				testing = 1;
				break;

-------- snap -----------------------------------------------

If you want me to change it to --dry-run, i will do that, so i can use -t for --table.

peter

Signature · 5 lines

:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: [email protected]

Jan Engelhardt

2007-11-07 19:27 UTC #8 lore raw

Quoted 5 lines · Peter Warasin

Quoted 2 lines
That said, iptables-restore does not have a -t, according to its help text ("iptables-restore -h") and its manpage.
Thats not true. The current subversion code uses -t for --test. Probably you confuse with iptables-save ?

actually, overlooked it :-/

Quoted 2 lines

If you want me to change it to --dry-run, i will do that, so i can use -t for --table.

be prepared to hear the 'that breaks compatibilty' argument. Though, since it's 1.4.0, I'd go for it :p

Peter Warasin

author

2007-11-07 19:50 UTC #9 lore raw

Hi Jan

Quoted 6 lines · Jan Engelhardt

Quoted 2 lines · Peter Warasin
If you want me to change it to --dry-run, i will do that, so i can use -t for --table.
be prepared to hear the 'that breaks compatibilty' argument. Though, since it's 1.4.0, I'd go for it :p

I imagine :)
Ok, i will try.

BTW: Is somewhere some sort of coding style documentation which i can read in order to prevent such mistakes, or is it just the linux kernel coding style?

Some of the malformed style you suggested to change are due to copy&paste of current iptables code, which i used as reference in some cases, so i am little confused now.

peter

Signature · 5 lines

:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: [email protected]

Jan Engelhardt

2007-11-07 20:42 UTC #10 lore raw

Quoted 8 lines · Peter Warasin

Quoted 6 lines · Jan Engelhardt
Quoted 2 lines · Peter Warasin
If you want me to change it to --dry-run, i will do that, so i can use -t for --table.
be prepared to hear the 'that breaks compatibilty' argument. Though, since it's 1.4.0, I'd go for it :p

Quoted 6 lines

I imagine :)
Ok, i will try.
BTW: Is somewhere some sort of coding style documentation which i can read in order to prevent such mistakes, or is it just the linux kernel coding style?

LKCS is the preferred one, yes.

Quoted 3 lines

Some of the malformed style you suggested to change are due to copy&paste of current iptables code, which i used as reference in some cases, so i am little confused now.

Yup, there is a lot of crufty code. Like what, less than 1% of all active developers of a project care about styling. Unfortunately for netfilter, 1% of N is generally less than 1.