Skip to contentSkip to conversation

sort chains by hooknum/names

PATCH In review Olaf Rempel 5 participants 7 messages Mar 4, 2005 – Apr 19, 2005 netfilter

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 sort chains by hooknum/names

hi list

Quoted 1 line

From 1.3.0 all chain names were alphabetically sorted when listing tables.

A user-defined chain 'AAA' is listed before the buildin chain "INPUT", and "FORWARD" is listed before "INPUT".

I've created two patches to revert it to the "old" behavior:
first buildin chains, sorted by hooknum, than userdefined chains, sorted by name.

OK, why two patches?
The first one need some preconditions:
*all* buildin chains need to be "in order" and in front of *any* user chain when parsing the kernel-list.

I was not sure about this, so I made a second patch, that does not need these preconditions.

Olaf

libiptc/libiptc.c +7 -4
diff -uNr iptables-1.3.0.org/libiptc/libiptc.c iptables-1.3.0/libiptc/libiptc.c
@@ -396,10 +396,13 @@
 {
 	struct chain_head *tmp;
 
-	list_for_each_entry(tmp, &h->chains, list) {
-		if (strcmp(c->name, tmp->name) <= 0) {
-			list_add(&c->list, tmp->list.prev);
-			return;
+	/* sort only user defined chains */
+	if (!c->hooknum) {
+		list_for_each_entry(tmp, &h->chains, list) {
+			if (strcmp(c->name, tmp->name) <= 0) {
+				list_add(&c->list, tmp->list.prev);
+				return;
+			}
 		}
 	}
 
libiptc/libiptc.c +17 -4
diff -uNr iptables-1.3.0.org/libiptc/libiptc.c iptables-1.3.0/libiptc/libiptc.c
@@ -396,10 +396,23 @@
 {
 	struct chain_head *tmp;
 
-	list_for_each_entry(tmp, &h->chains, list) {
-		if (strcmp(c->name, tmp->name) <= 0) {
-			list_add(&c->list, tmp->list.prev);
-			return;
+	/* sort only user defined chains */
+	if (!c->hooknum) {
+		/* sort user-defined by name and after builtins */
+		list_for_each_entry(tmp, &h->chains, list) {
+			if (!tmp->hooknum && strcmp(c->name, tmp->name) <= 0) {
+				list_add(&c->list, tmp->list.prev);
+				return;
+			}
+		}
+	
+	} else {
+		/* sort builtins by hooknum and before user-defined */
+		list_for_each_entry(tmp, &h->chains, list) {
+			if (!tmp->hooknum || tmp->hooknum > c->hooknum) {
+				list_add(&c->list, tmp->list.prev);
+				return;
+			}
 		}
 	}
 
#1 lore raw
Quoted 8 lines · Olaf Rempel

hi list

Quoted 1 line

From 1.3.0 all chain names were alphabetically sorted when listing tables.

A user-defined chain 'AAA' is listed before the buildin chain "INPUT", and "FORWARD" is listed before "INPUT".

I've created two patches to revert it to the "old" behavior:
first buildin chains, sorted by hooknum, than userdefined chains, sorted by name.

Why is the old behaviour better than the current? Any scripts you have should be smart enough to check for '^Chain ______' to determine chain.

Phil

#2 lore raw
Quoted 7 lines · Phil Oester
Quoted 3 lines · Olaf Rempel

I've created two patches to revert it to the "old" behavior:
first buildin chains, sorted by hooknum, than userdefined chains, sorted by name.

Why is the old behaviour better than the current? Any scripts you have should be smart enough to check for '^Chain ______' to determine chain.

Unfortunately not all are, so best chance of no breakage is keeping the old behaviour. I can't remember where, but someone already reported a broken script because of this change.

Regards
Patrick

#3 lore raw
In reply to Phil Oester
Quoted 2 lines · On Fri, 4 Mar 2005 14:08:27 -0800 Phil Oester <[email protected]> wrote:

Why is the old behaviour better than the current? Any scripts you have should be smart enough to check for '^Chain ______' to determine chain.

It's not about scripts. I just think it's easier to understand:
Having the the builtins arranged like the packets traverse them, and being on top of the chains they "jump" into.

I see no advantage of changing to the new behavior, only that it confuses people that had worked for years with the old behavior.

And last but not least:
libiptc.c:565 /* FIXME: sort chains */

I read this like: not implemented yet :)

Olaf

#4 lore raw
In reply to Olaf Rempel
Quoted 4 lines · Olaf Rempel

OK, why two patches? The first one need some preconditions: *all* buildin chains need to be "in order" and in front of *any* user chain when parsing the kernel-list.

I was not sure about this, so I made a second patch, that does not need these preconditions.

I assume with "in order", you mean it relies on the current numbering of the hooks:

/* After promisc drops, checksum checks. */
#define NF_IP_PRE_ROUTING 0
/* If the packet is destined for this box. */
#define NF_IP_LOCAL_IN 1
/* If the packet is destined for another interface. */
#define NF_IP_FORWARD 2
/* Packets coming from a local process. */
#define NF_IP_LOCAL_OUT 3
/* Packets about to hit the wire. */
#define NF_IP_POST_ROUTING 4

This seems fine to me. I'm going to apply your first patch, thanks.

Regards
Patrick

#5 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Patrick McHardy on 2005-03-04 23:04 UTC:

Quoted 8 lines
Quoted 3 lines

OK, why two patches? The first one need some preconditions: *all* buildin chains need to be "in order" and in front of *any* user chain when parsing the kernel-list.

I assume with "in order", you mean it relies on the current numbering of the hooks:

This seems fine to me. I'm going to apply your first patch, thanks.

What he meant by "in order" was that when getting a list of chains from the kernel, they would be returned like this:

PREROUTING
INPUT
FORWARD
OUTPUT
POSTROUTING
USER_CHAIN_B
USER_CHAIN_A
USER_CHAIN_C
...

I.e. builtin chains first, and then user chains in random order.

This is not the case however (at least with my 2.6.11 kernel), they seem to come in a vaguely alphabetic order, with builtin chains being among the user chains like:

USER_CHAIN_A
PREROUTING
INPUT
USER_CHAIN_B
FORWARD
OUTPUT
POSTROUTING
USER_CHAIN_C
...

Thus, his second patch is necessary (at least for me) in order to get them in the order

<builtin chains sorted logically>
<user chains sorted alphabetically>

I tested switching to the second patch and the results were Good. :)

- --
- - xkr47
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFCZRrhxyF48ZTvn+4RAiNKAJ0U96HA8lxIkWUvn8+vE4zEY7jHgACeJjaD 94uZ+M79//yeo0xQi3397SA=
=GShI
-----END PGP SIGNATURE-----

#6 lore raw
In reply to Olaf Rempel
Quoted 8 lines · Olaf Rempel

hi list

Quoted 1 line

From 1.3.0 all chain names were alphabetically sorted when listing tables.

A user-defined chain 'AAA' is listed before the buildin chain "INPUT", and "FORWARD" is listed before "INPUT".

I've created two patches to revert it to the "old" behavior:
first buildin chains, sorted by hooknum, than userdefined chains, sorted by name.

Appart from the fact that it restores the old behavior (and avoids breakage of some badly written scripts or apps), can you give a single reason why the listing should be sorted?

Why not sort the output of a lot of commands (like lsof, netstat, ps, etc.) then? Hint: probably because there is no interest in wasting CPU time for this.

Herve

Signature · 4 lines
 _
(°=  Hervé Eychenne
//)
v_/_ WallFire project:  http://www.wallfire.org/