RFC: net/netfilter reorganization

RFC: net/netfilter reorganization

[Patrick McHardy]

We've talked about reorganizing net/netfilter in a smaller group
during the workshop. Its currently quite crowded in there,
additionally the IPVS guys are moving their code under
net/netfilter/ipvs in 2.6.28.

Some suggestions for a new directory structure:

net/netfilter/xtables
net/netfilter/{ct, conntrack, nfct, ...}
net/netfilter/nftables for my new stuff

We could go further by splitting up xtables and conntrack,
parts of this structure would be mirrored in net/ipv4/netfilter
and net/ipv6/netfilter:

net/netfilter/xtables/matches
net/netfilter/xtables/targets
net/netfilter/ct/protocols
net/netfilter/ct/helpers

I'm not sure whether this is too much since 4 directory
levels seem a bit extreme.

Since the amount of core code is increasing too, one more
thought was

net/netfilter/core

containing core.c, netfilter.c, logging and queuing stuff and
everything netlink related that doesn't fall in a more specific
area.

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Sunday 2008-10-05 08:34, Patrick McHardy wrote:

> We've talked about reorganizing net/netfilter in a smaller group
> during the workshop. Its currently quite crowded in there,
> additionally the IPVS guys are moving their code under
> net/netfilter/ipvs in 2.6.28.
>
> Some suggestions for a new directory structure:
>
> net/netfilter/xtables
> net/netfilter/{ct, conntrack, nfct, ...}
> net/netfilter/nftables for my new stuff

Yes definitely welcomed.

> We could go further by splitting up xtables and conntrack,

That's just what you proposed! Or at least that is what I think of
Netfilter. Xtables and Conntrack are already separated, simply
because each one can be used without the other. (Minus things like
xt_conntrack, which I think belong to Xtables.)

> parts of this structure would be mirrored in net/ipv4/netfilter
> and net/ipv6/netfilter:
>
> net/netfilter/xtables/matches
> net/netfilter/xtables/targets
> net/netfilter/ct/protocols
> net/netfilter/ct/helpers
>
> I'm not sure whether this is too much since 4 directory
> levels seem a bit extreme.

If you take that structure _and_ mirror it in ipv4, you get:
net/ipv4/netfilter/xtables/matches/ which is already 5 :-)

> Since the amount of core code is increasing too, one more
> thought was
>
> net/netfilter/core
>
> containing core.c, netfilter.c, logging and queuing stuff and
> everything netlink related that doesn't fall in a more specific
> area.

I think the core stuff can remain in net/netfilter/. Not because
it is growing, but because other subsystems do just that.
Like, there is fs/ (with the core code) and fs/<component>/ for
anything not-so-core-y.

What I really certainly would like to see is that we somehow get rid
of the oddness of naming L3 trackers and L4 trackers. Right now we
have (two examples)

-rw-r--r-- 1 15924 2008-09-09 20:34 nf_conntrack_ftp.c
-rw-r--r-- 1 43369 2008-09-09 20:33 nf_conntrack_proto_tcp.c

and in everybody's mind, FTP is a protocol too of course, “so why
does not ftp have a _proto part, and why does TCP?” one might ask.
We could just remove the _proto part (also helpful to reduce the
name length), as there is no clash between L3 and L4 trackers
except perhaps for proto_generic and l3proto_generic. Hm?

One thing I have done in my git realms is just call my branches
"nfct_foo" simply because I was too lazy to type "nf_conntrack_foo".
Maybe we could do the same for our files? Maybe we can even get rid
of the nf_conntrack_/nfct_ prefix at all when they are moved into
net/netfilter/conntrack/, but still have it in their .ko files. IOW:
net/netfilter/conntrack/ftp.c produces
net/netfilter/conntrack/nfct_ftp.ko. How about it?

core.c (remains)
nf_conntrack_acct.c            -> conntrack/acct.c     
nf_conntrack_amanda.c          -> conntrack/amanda.c
nf_conntrack_core.c            -> conntrack/core.c
...
nf_log.c (remains)
nfnetlink.c (remains)
nfnetlink_log.c (remains)
nfnetlink_queue.c (...)
nf_queue.c
nf_sockopt.c
x_tables.c              -> xtables/x_tables.c
xt_CLASSIFY.c           -> xtables/CLASSIFY.c (or just keep xt_ prefix?)
xt_comment.c            -> ...
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: net/netfilter reorganization

[Patrick McHardy]

Jan Engelhardt wrote:
> On Sunday 2008-10-05 08:34, Patrick McHardy wrote:
> 
>> Some suggestions for a new directory structure:
>>
>> net/netfilter/xtables
>> net/netfilter/{ct, conntrack, nfct, ...}
>> net/netfilter/nftables for my new stuff
> 
> Yes definitely welcomed.
> 
>> We could go further by splitting up xtables and conntrack,
> 
> That's just what you proposed! Or at least that is what I think of
> Netfilter. Xtables and Conntrack are already separated, simply
> because each one can be used without the other. (Minus things like
> xt_conntrack, which I think belong to Xtables.)

Yes, confusing wording. I meant splitting them up internally,
i.e. xtables => matches/targets etc, as described below. And
its only about the directory structure of course, how things
work together at runtime remains the same.

>> Since the amount of core code is increasing too, one more
>> thought was
>>
>> net/netfilter/core
>>
>> containing core.c, netfilter.c, logging and queuing stuff and
>> everything netlink related that doesn't fall in a more specific
>> area.
> 
> I think the core stuff can remain in net/netfilter/. Not because
> it is growing, but because other subsystems do just that.
> Like, there is fs/ (with the core code) and fs/<component>/ for
> anything not-so-core-y.

I agree that its more logical to keep the core stuff at the
netfilter root.

> What I really certainly would like to see is that we somehow get rid
> of the oddness of naming L3 trackers and L4 trackers. Right now we
> have (two examples)
> 
> -rw-r--r-- 1 15924 2008-09-09 20:34 nf_conntrack_ftp.c
> -rw-r--r-- 1 43369 2008-09-09 20:33 nf_conntrack_proto_tcp.c
> 
> and in everybody's mind, FTP is a protocol too of course, “so why
> does not ftp have a _proto part, and why does TCP?” one might ask.
> We could just remove the _proto part (also helpful to reduce the
> name length), as there is no clash between L3 and L4 trackers
> except perhaps for proto_generic and l3proto_generic. Hm?

Agreed, this is a good opportunity to fix that as well.
The _proto prefix becomes redundant if we use

net/netfilter/ct/protocols

anyway.

> One thing I have done in my git realms is just call my branches
> "nfct_foo" simply because I was too lazy to type "nf_conntrack_foo".
> Maybe we could do the same for our files? Maybe we can even get rid
> of the nf_conntrack_/nfct_ prefix at all when they are moved into
> net/netfilter/conntrack/, but still have it in their .ko files. IOW:
> net/netfilter/conntrack/ftp.c produces
> net/netfilter/conntrack/nfct_ftp.ko. How about it?

Its a quite verbose naming scheme currently, I agree. I like
some common prefixes for related things, even if slightly
redundant, but using nfct_ instead of nf_conntrack_ sounds
fine to me.

> core.c (remains)
> nf_conntrack_acct.c            -> conntrack/acct.c     
> nf_conntrack_amanda.c          -> conntrack/amanda.c
> nf_conntrack_core.c            -> conntrack/core.c
> ...
> nf_log.c (remains)
> nfnetlink.c (remains)
> nfnetlink_log.c (remains)
> nfnetlink_queue.c (...)
> nf_queue.c
> nf_sockopt.c
> x_tables.c              -> xtables/x_tables.c
> xt_CLASSIFY.c           -> xtables/CLASSIFY.c (or just keep xt_ prefix?)
> xt_comment.c            -> ...


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Sunday 2008-10-05 10:02, Patrick McHardy wrote:
>
> Yes, confusing wording. I meant splitting them up internally,
> i.e. xtables => matches/targets etc, as described below. And
> its only about the directory structure of course, how things
> work together at runtime remains the same.

I would not want to split matches and targets. Yes I am aware that we
have these guys wanting to store it on case-ignorant filesystems, but
we would still have all the include files in a conflicting position.

net/netfilter/xtables/matches is already over my personal
preferred limit of 3. If at all, I think they should get
{pre|post}fixed à la tg_connmark.c (target) and mt_connmark.c (or
connmark_{tg,mt}.c, whichever makes 'em happy).

Also the idea of merging files for the benefit of reduces in-memory
size was floating. I have revisited the koredux branch since you said
on-disk size ain't important, so the exact _memory_ statistic is:

One extension per module:

Single blob:
	Module                  Size  Used by
	xtables_ext            94312  0 (on-disk size 160K)

All blobs that xtables_ext encompasses:
	Module                  Size  Used by
	ip6t_mh                 2176  0 
	ip6t_hl                 2176  0 
	ip6t_eui64              2240  0 
	ip6t_HL                 2432  0 
	ipt_ttl                 2112  0 
	ipt_ecn                 2496  0 
	ipt_ah                  2176  0 
	ipt_addrtype            3136  0 
	ipt_ULOG                9096  0 
	ipt_TTL                 2496  0 
	ipt_REJECT              3712  0 
	ipt_LOG                 6400  0 
	ipt_ECN                 3072  0 
	xt_u32                  2816  0 
	xt_time                 3136  0 
	xt_tcpudp               3840  0 
	xt_tcpmss               2624  0 
	xt_string               2752  0 
	xt_statistic            2368  0 
	xt_sctp                 3456  0 
	xt_recent              11232  0 
	xt_realm                1984  0 
	xt_rateest              3136  0 
	xt_quota                2304  0 
	xt_policy               3648  0 
	xt_pkttype              2304  0 
	xt_physdev              2960  0 
	xt_owner                3328  0 
	xt_multiport            3840  0 
	xt_mark                 2496  0 
	xt_mac                  2240  0 
	xt_limit                2752  0 
	xt_iprange              2944  0 
	xt_hashlimit           12960  0 
	xt_esp                  2432  0 
	xt_dccp                 3464  0 
	xt_comment              2176  0 
	xt_dscp                 3264  0 
	xt_TRACE                1984  0 
	xt_TCPOPTSTRIP          3136  0 
	xt_TCPMSS               4992  0 
	xt_SECMARK              3012  0 
	xt_RATEEST              4676  1 xt_rateest
	xt_NFQUEUE              2304  0 
	xt_NFLOG                2368  0 
	xt_MARK                 3136  0 
	xt_DSCP                 4160  0 
	xt_CLASSIFY             2048  0 
	total                 165992  (on disk about 1037K)

That's a 43% reduction in memory.

>> -rw-r--r-- 1 15924 2008-09-09 20:34 nf_conntrack_ftp.c
>> -rw-r--r-- 1 43369 2008-09-09 20:33 nf_conntrack_proto_tcp.c
>> 
>> and in everybody's mind, FTP is a protocol too of course, “so why
>> does not ftp have a _proto part, and why does TCP?” one might ask.
>> We could just remove the _proto part (also helpful to reduce the
>> name length), as there is no clash between L3 and L4 trackers
>> except perhaps for proto_generic and l3proto_generic. Hm?
>
> Agreed, this is a good opportunity to fix that as well.
> The _proto prefix becomes redundant if we use
>
> net/netfilter/ct/protocols
>
> anyway.

It does not really solve anything IMHO. Protocols? FTP is one, so hm,
same confusion. This also got my anti-favorite 4-depth :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: net/netfilter reorganization

[Patrick McHardy]

Jan Engelhardt wrote:
> On Sunday 2008-10-05 10:02, Patrick McHardy wrote:
>> Yes, confusing wording. I meant splitting them up internally,
>> i.e. xtables => matches/targets etc, as described below. And
>> its only about the directory structure of course, how things
>> work together at runtime remains the same.
> 
> I would not want to split matches and targets. Yes I am aware that we
> have these guys wanting to store it on case-ignorant filesystems, but
> we would still have all the include files in a conflicting position.
> 
> net/netfilter/xtables/matches is already over my personal
> preferred limit of 3. If at all, I think they should get
> {pre|post}fixed à la tg_connmark.c (target) and mt_connmark.c (or
> connmark_{tg,mt}.c, whichever makes 'em happy).

Yes, its probably going overboard in case of xtables. For conntrack
helpers and l4 protocols I'm not sure yet. I'll probably just give
it a try without the 4 levels and see how it turns out.

> Also the idea of merging files for the benefit of reduces in-memory
> size was floating. I have revisited the koredux branch since you said
> on-disk size ain't important, so the exact _memory_ statistic is:

Well, not completely unimportant for some people, but its not
really my main concern since in those cases people tend not to
use modules anyway.

> One extension per module:
> 
> Single blob:
> 	Module                  Size  Used by
> 	xtables_ext            94312  0 (on-disk size 160K)
> 
> All blobs that xtables_ext encompasses:
> 	Module                  Size  Used by
>	[...]
> 	total                 165992  (on disk about 1037K)
> 
> That's a 43% reduction in memory.

Its a clear benefit, I don't doubt that. I mainly think this is not
netfilter specific at all and should be done on a kbuild level.

Linking all of them together also has some runtime impact because
of symbol dependencies, using let say tcpudp will probably pull
in (parts of) IPv6 and NAT. Having them grouped by dependencies
would avoid this.

>>> -rw-r--r-- 1 15924 2008-09-09 20:34 nf_conntrack_ftp.c
>>> -rw-r--r-- 1 43369 2008-09-09 20:33 nf_conntrack_proto_tcp.c
>>>
>>> and in everybody's mind, FTP is a protocol too of course, “so why
>>> does not ftp have a _proto part, and why does TCP?” one might ask.
>>> We could just remove the _proto part (also helpful to reduce the
>>> name length), as there is no clash between L3 and L4 trackers
>>> except perhaps for proto_generic and l3proto_generic. Hm?
>> Agreed, this is a good opportunity to fix that as well.
>> The _proto prefix becomes redundant if we use
>>
>> net/netfilter/ct/protocols
>>
>> anyway.
> 
> It does not really solve anything IMHO. Protocols? FTP is one, so hm,
> same confusion. This also got my anti-favorite 4-depth :)

Well, its not *that much* confusing since I believe most people are
aware of that part of the netfilter terminology. The main goal is
to get the directory contents to a more reasonable level and group
related stuff together; using just net/netfilter/ct is still a bit
too coarse in my opinion. Under that aspect, what would your proposed
structure be?

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: net/netfilter reorganization

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sun, 05 Oct 2008 16:48:36 +0200

> The main goal is to get the directory contents to a more reasonable
> level and group related stuff together; using just net/netfilter/ct
> is still a bit too coarse in my opinion. Under that aspect, what
> would your proposed structure be?

One thing I'm looking forward to is the elimination of all of
the xt_* prefixes, and in general better TAB completion
friendliness when I try to open netfilter source files under
emacs or grep things from the command line with tab completed
paths.

Re: RFC: net/netfilter reorganization

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Sun, 05 Oct 2008 16:48:36 +0200
> 
>> The main goal is to get the directory contents to a more reasonable
>> level and group related stuff together; using just net/netfilter/ct
>> is still a bit too coarse in my opinion. Under that aspect, what
>> would your proposed structure be?
> 
> One thing I'm looking forward to is the elimination of all of
> the xt_* prefixes, and in general better TAB completion
> friendliness when I try to open netfilter source files under
> emacs or grep things from the command line with tab completed
> paths.

I'm not sure how emacs handles completion, would using different
prefixes for matches and targets help? Otherwise I guess we can
just get rid of the prefixes entirely.

The main problem with these renames is that it module aliases
don't work for "rmmod", which is a pretty common thing with
these modules. I guess we'd need to fix up module-init-tools
first.

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Sunday 2008-10-05 12:11, Patrick McHardy wrote:
>> 
>> One thing I'm looking forward to is the elimination of all of
>> the xt_* prefixes, and in general better TAB completion
>> friendliness when I try to open netfilter source files under
>> emacs or grep things from the command line with tab completed
>> paths.
>
> I'm not sure how emacs handles completion, would using different
> prefixes for matches and targets help? Otherwise I guess we can
> just get rid of the prefixes entirely.

Allow me to speak generally for humans.. we are lazy to type, so
usually it's just the first three or four characters (or less,
of course) we want to type before hitting TAB the first time.
By eliminating prefixes, you already start typing on the "real"
name, which means your chances that TAB completion returns just
a single file is much higher.

> The main problem with these renames is that it module aliases
> don't work for "rmmod", which is a pretty common thing with
> these modules. I guess we'd need to fix up module-init-tools
> first.

When was the last time you used tab completion on .ko files?
I think this is sufficient:

obj-$(config_foo) += nfct_ftp.o
nfct_ftp-objs := ftp.c

That way, Mr Developer can use ft<TAB> to get ftp.c, but the
final module that we will be using with modprobe/rmmod still
has the prefix (and we should really have one!)

Re: RFC: net/netfilter reorganization

[Patrick McHardy]

Jan Engelhardt wrote:
> On Sunday 2008-10-05 12:11, Patrick McHardy wrote:
>>> One thing I'm looking forward to is the elimination of all of
>>> the xt_* prefixes, and in general better TAB completion
>>> friendliness when I try to open netfilter source files under
>>> emacs or grep things from the command line with tab completed
>>> paths.
>> I'm not sure how emacs handles completion, would using different
>> prefixes for matches and targets help? Otherwise I guess we can
>> just get rid of the prefixes entirely.
> 
> Allow me to speak generally for humans.. we are lazy to type, so
> usually it's just the first three or four characters (or less,
> of course) we want to type before hitting TAB the first time.
> By eliminating prefixes, you already start typing on the "real"
> name, which means your chances that TAB completion returns just
> a single file is much higher.

Thats true. Not using any prefixes requires keeping the
upper letter naming convention for targets though (which
I don't really mind).

>> The main problem with these renames is that it module aliases
>> don't work for "rmmod", which is a pretty common thing with
>> these modules. I guess we'd need to fix up module-init-tools
>> first.
> 
> When was the last time you used tab completion on .ko files?

I don't use tab-completion above file-level at all, its takes
way to long to initialize all the more fancy stuff.

> I think this is sufficient:
> 
> obj-$(config_foo) += nfct_ftp.o
> nfct_ftp-objs := ftp.c
> 
> That way, Mr Developer can use ft<TAB> to get ftp.c, but the
> final module that we will be using with modprobe/rmmod still
> has the prefix (and we should really have one!)

A lot of people are unloading modules in their firewall scripts
and that will break.

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Sunday 2008-10-05 12:21, Patrick McHardy wrote:
>
> Thats true. Not using any prefixes requires keeping the
> upper letter naming convention for targets though (which
> I don't really mind).

Anybody else's voice on this?
I'd like to go for $(always lowercase extension name)_{mt,tg}.c if 
noone objects.
Or, when obvious extensions get combined maybe (like xt_mark and xt_MARK),
just mark.c and the issue is all gone.

Now while we are at it.. should future xtables targets (their actual
name as is used with iptables -j) continue to be uppercase?


>
>> I think this is sufficient:
>> 
>> obj-$(config_foo) += nfct_ftp.o
>> nfct_ftp-objs := ftp.c
>> 
>> That way, Mr Developer can use ft<TAB> to get ftp.c, but the
>> final module that we will be using with modprobe/rmmod still
>> has the prefix (and we should really have one!)
>
> A lot of people are unloading modules in their firewall scripts
> and that will break.
>
Well whether it's nfct_ or nf_conntrack_ is up to you; the point was
that ftp.c exist for the joy of the developer.

Re: RFC: net/netfilter reorganization

[Patrick McHardy]

Jan Engelhardt wrote:
> On Sunday 2008-10-05 12:21, Patrick McHardy wrote:
>> Thats true. Not using any prefixes requires keeping the
>> upper letter naming convention for targets though (which
>> I don't really mind).
> 
> Anybody else's voice on this?
> I'd like to go for $(always lowercase extension name)_{mt,tg}.c if 
> noone objects.
> Or, when obvious extensions get combined maybe (like xt_mark and xt_MARK),
> just mark.c and the issue is all gone.

Thats a good point, once we've go for the directory split,
combining matches and targets will not fit into the scheme
very well. And I think that would be a good change.

So I'm very undecided right now, I think I need to let this
settle in my brain first :)

> Now while we are at it.. should future xtables targets (their actual
> name as is used with iptables -j) continue to be uppercase?

I was afraid I would trigger this kind of suggestions. Lets do
one thing at a time, and I don't think we should change too many
of the visible conventions. We can do this in nftables.

>>> I think this is sufficient:
>>>
>>> obj-$(config_foo) += nfct_ftp.o
>>> nfct_ftp-objs := ftp.c
>>>
>>> That way, Mr Developer can use ft<TAB> to get ftp.c, but the
>>> final module that we will be using with modprobe/rmmod still
>>> has the prefix (and we should really have one!)
>> A lot of people are unloading modules in their firewall scripts
>> and that will break.
>>
> Well whether it's nfct_ or nf_conntrack_ is up to you; the point was
> that ftp.c exist for the joy of the developer.

Right, we don't have to change the module names at all, I didn't
think of that.

I think I'm going to get a bit more rest from the workshop before
trying to use my brain again :)

Re: RFC: net/netfilter reorganization

[Jozsef Kadlecsik]

On Sun, 5 Oct 2008, Jan Engelhardt wrote:

> I'd like to go for $(always lowercase extension name)_{mt,tg}.c if 
> noone objects.
> Or, when obvious extensions get combined maybe (like xt_mark and xt_MARK),
> just mark.c and the issue is all gone.

I think match/target combined in one file is the best way to go. As they 
cover the same realm (match some quantity of something and change it), 
they do fit together.

If there's no match/target counterpart, that's not a big trouble at all 
:-).

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: RFC: net/netfilter reorganization

[David Miller]

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Sun, 5 Oct 2008 21:06:34 +0200 (CEST)

> On Sun, 5 Oct 2008, Jan Engelhardt wrote:
> 
> > I'd like to go for $(always lowercase extension name)_{mt,tg}.c if 
> > noone objects.
> > Or, when obvious extensions get combined maybe (like xt_mark and xt_MARK),
> > just mark.c and the issue is all gone.
> 
> I think match/target combined in one file is the best way to go. As they 
> cover the same realm (match some quantity of something and change it), 
> they do fit together.

This also makes sense to me.

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Sunday 2008-10-05 16:28, David Miller wrote:
>> On Sun, 5 Oct 2008, Jan Engelhardt wrote:
>> 
>> > I'd like to go for $(always lowercase extension name)_{mt,tg}.c if 
>> > noone objects.
>> > Or, when obvious extensions get combined maybe (like xt_mark and xt_MARK),
>> > just mark.c and the issue is all gone.
>> 
>> I think match/target combined in one file is the best way to go. As they 
>> cover the same realm (match some quantity of something and change it), 
>> they do fit together.
>
>This also makes sense to me.

Before I start moving code, would we may want to combine more modules
into one file at this point besides xt_FOO and xt_foo only?

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Sunday 2008-10-05 16:33, Jan Engelhardt wrote:
>On Sunday 2008-10-05 16:28, David Miller wrote:
>>> On Sun, 5 Oct 2008, Jan Engelhardt wrote:
>>> 
>>> > I'd like to go for $(always lowercase extension name)_{mt,tg}.c if 
>>> > noone objects.
>>> > Or, when obvious extensions get combined maybe (like xt_mark and xt_MARK),
>>> > just mark.c and the issue is all gone.
>>> 
>>> I think match/target combined in one file is the best way to go. As they 
>>> cover the same realm (match some quantity of something and change it), 
>>> they do fit together.
>>
>>This also makes sense to me.
>
>Before I start moving code, would we may want to combine more modules
>into one file at this point besides xt_FOO and xt_foo only?
(Probably not.)

Funny thing is, only when you try you see more problems a-coming.
Like, Kconfig option names. Keep/Lose
NETFILTER_XT_{MATCH,TARGET}_CONNMARK, and query users for a new one?
Dunno. Sample patch below.

As for memory saving, the combined module weighs in at 4928 bytes,
whereas previously it took 5632 (2496+3136). Solid win of 12.50000%!


commit 1143a39a1f76331bf47bee6b63fded9fb5227e4b
parent 13992bcb338f0044010a1809f24d2198b5bd91c8
parenturl git://dev.medozas.de/linux master
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Sun Oct 5 16:46:52 2008 -0400

    netfilter: xtables: combine xt_connmark target and match
    
    Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 net/netfilter/Kconfig       |   40 ++-----
 net/netfilter/Makefile      |    3 +-
 net/netfilter/xt_CONNMARK.c |  225 -----------------------------------
 net/netfilter/xt_connmark.c |  206 +++++++++++++++++++++++++++++++-
 4 files changed, 212 insertions(+), 262 deletions(-)
 delete mode 100644 net/netfilter/xt_CONNMARK.c

diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 899e780..bc65b31 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -302,6 +302,18 @@ config NETFILTER_XTABLES
 
 if NETFILTER_XTABLES
 
+# alphabetically ordered list of match/target combos
+
+config NETFILTER_XT_CONNMARK
+	tristate '"CONNMARK" extensions'
+	depends on NF_CONNTRACK
+	depends on NETFILTER_ADVANCED
+	select NF_CONNTRACK_MARK
+	---help---
+	This option adds support for manipulating and matching the
+	connection mark value. It is similar to the packet mark, but
+	is per-connection.
+
 # alphabetically ordered list of targets
 
 config NETFILTER_XT_TARGET_CLASSIFY
@@ -316,21 +328,6 @@ config NETFILTER_XT_TARGET_CLASSIFY
 
 	  To compile it as a module, choose M here.  If unsure, say N.
 
-config NETFILTER_XT_TARGET_CONNMARK
-	tristate  '"CONNMARK" target support'
-	depends on IP_NF_MANGLE || IP6_NF_MANGLE
-	depends on NF_CONNTRACK
-	depends on NETFILTER_ADVANCED
-	select NF_CONNTRACK_MARK
-	help
-	  This option adds a `CONNMARK' target, which allows one to manipulate
-	  the connection mark value.  Similar to the MARK target, but
-	  affects the connection mark value rather than the packet mark value.
-
-	  If you want to compile it as a module, say M here and read
-	  <file:Documentation/kbuild/modules.txt>.  The module will be called
-	  ipt_CONNMARK.ko.  If unsure, say `N'.
-
 config NETFILTER_XT_TARGET_CONNSECMARK
 	tristate '"CONNSECMARK" target support'
 	depends on NF_CONNTRACK && NF_CONNTRACK_SECMARK
@@ -521,19 +518,6 @@ config NETFILTER_XT_MATCH_CONNLIMIT
 	  This match allows you to match against the number of parallel
 	  connections to a server per client IP address (or address block).
 
-config NETFILTER_XT_MATCH_CONNMARK
-	tristate  '"connmark" connection mark match support'
-	depends on NF_CONNTRACK
-	depends on NETFILTER_ADVANCED
-	select NF_CONNTRACK_MARK
-	help
-	  This option adds a `connmark' match, which allows you to match the
-	  connection mark value previously set for the session by `CONNMARK'. 
-
-	  If you want to compile it as a module, say M here and read
-	  <file:Documentation/kbuild/modules.txt>.  The module will be called
-	  ipt_connmark.ko.  If unsure, say `N'.
-
 config NETFILTER_XT_MATCH_CONNTRACK
 	tristate '"conntrack" connection tracking match support'
 	depends on NF_CONNTRACK
diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index 8ce6766..62f48df 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -39,10 +39,10 @@ obj-$(CONFIG_NETFILTER_TPROXY) += nf_tproxy_core.o
 
 # generic X tables 
 obj-$(CONFIG_NETFILTER_XTABLES) += x_tables.o xt_tcpudp.o
+obj-$(CONFIG_NETFILTER_XT_CONNMARK) += xt_connmark.o
 
 # targets
 obj-$(CONFIG_NETFILTER_XT_TARGET_CLASSIFY) += xt_CLASSIFY.o
-obj-$(CONFIG_NETFILTER_XT_TARGET_CONNMARK) += xt_CONNMARK.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_CONNSECMARK) += xt_CONNSECMARK.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_DSCP) += xt_DSCP.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_MARK) += xt_MARK.o
@@ -60,7 +60,6 @@ obj-$(CONFIG_NETFILTER_XT_TARGET_TRACE) += xt_TRACE.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_COMMENT) += xt_comment.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNBYTES) += xt_connbytes.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNLIMIT) += xt_connlimit.o
-obj-$(CONFIG_NETFILTER_XT_MATCH_CONNMARK) += xt_connmark.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNTRACK) += xt_conntrack.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_DCCP) += xt_dccp.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_DSCP) += xt_dscp.o
diff --git a/net/netfilter/xt_CONNMARK.c b/net/netfilter/xt_CONNMARK.c
deleted file mode 100644
index d6e5ab4..0000000
--- a/net/netfilter/xt_CONNMARK.c
+++ /dev/null
@@ -1,225 +0,0 @@
-/*
- *	xt_CONNMARK - Netfilter module to modify the connection mark values
- *
- *	Copyright (C) 2002,2004 MARA Systems AB <http://www.marasystems.com>
- *	by Henrik Nordstrom <hno@marasystems.com>
- *	Copyright © CC Computer Consultants GmbH, 2007 - 2008
- *	Jan Engelhardt <jengelh@computergmbh.de>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- */
-#include <linux/module.h>
-#include <linux/skbuff.h>
-#include <linux/ip.h>
-#include <net/checksum.h>
-
-MODULE_AUTHOR("Henrik Nordstrom <hno@marasystems.com>");
-MODULE_DESCRIPTION("Xtables: connection mark modification");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("ipt_CONNMARK");
-MODULE_ALIAS("ip6t_CONNMARK");
-
-#include <linux/netfilter/x_tables.h>
-#include <linux/netfilter/xt_CONNMARK.h>
-#include <net/netfilter/nf_conntrack_ecache.h>
-
-static unsigned int
-connmark_tg_v0(struct sk_buff *skb, const struct xt_target_param *par)
-{
-	const struct xt_connmark_target_info *markinfo = par->targinfo;
-	struct nf_conn *ct;
-	enum ip_conntrack_info ctinfo;
-	u_int32_t diff;
-	u_int32_t mark;
-	u_int32_t newmark;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	if (ct) {
-		switch(markinfo->mode) {
-		case XT_CONNMARK_SET:
-			newmark = (ct->mark & ~markinfo->mask) | markinfo->mark;
-			if (newmark != ct->mark) {
-				ct->mark = newmark;
-				nf_conntrack_event_cache(IPCT_MARK, ct);
-			}
-			break;
-		case XT_CONNMARK_SAVE:
-			newmark = (ct->mark & ~markinfo->mask) |
-				  (skb->mark & markinfo->mask);
-			if (ct->mark != newmark) {
-				ct->mark = newmark;
-				nf_conntrack_event_cache(IPCT_MARK, ct);
-			}
-			break;
-		case XT_CONNMARK_RESTORE:
-			mark = skb->mark;
-			diff = (ct->mark ^ mark) & markinfo->mask;
-			skb->mark = mark ^ diff;
-			break;
-		}
-	}
-
-	return XT_CONTINUE;
-}
-
-static unsigned int
-connmark_tg(struct sk_buff *skb, const struct xt_target_param *par)
-{
-	const struct xt_connmark_tginfo1 *info = par->targinfo;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-	u_int32_t newmark;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	if (ct == NULL)
-		return XT_CONTINUE;
-
-	switch (info->mode) {
-	case XT_CONNMARK_SET:
-		newmark = (ct->mark & ~info->ctmask) ^ info->ctmark;
-		if (ct->mark != newmark) {
-			ct->mark = newmark;
-			nf_conntrack_event_cache(IPCT_MARK, ct);
-		}
-		break;
-	case XT_CONNMARK_SAVE:
-		newmark = (ct->mark & ~info->ctmask) ^
-		          (skb->mark & info->nfmask);
-		if (ct->mark != newmark) {
-			ct->mark = newmark;
-			nf_conntrack_event_cache(IPCT_MARK, ct);
-		}
-		break;
-	case XT_CONNMARK_RESTORE:
-		newmark = (skb->mark & ~info->nfmask) ^
-		          (ct->mark & info->ctmask);
-		skb->mark = newmark;
-		break;
-	}
-
-	return XT_CONTINUE;
-}
-
-static bool connmark_tg_check_v0(const struct xt_tgchk_param *par)
-{
-	const struct xt_connmark_target_info *matchinfo = par->targinfo;
-
-	if (matchinfo->mode == XT_CONNMARK_RESTORE) {
-		if (strcmp(par->table, "mangle") != 0) {
-			printk(KERN_WARNING "CONNMARK: restore can only be "
-			       "called from \"mangle\" table, not \"%s\"\n",
-			       par->table);
-			return false;
-		}
-	}
-	if (matchinfo->mark > 0xffffffff || matchinfo->mask > 0xffffffff) {
-		printk(KERN_WARNING "CONNMARK: Only supports 32bit mark\n");
-		return false;
-	}
-	if (nf_ct_l3proto_try_module_get(par->family) < 0) {
-		printk(KERN_WARNING "can't load conntrack support for "
-				    "proto=%u\n", par->family);
-		return false;
-	}
-	return true;
-}
-
-static bool connmark_tg_check(const struct xt_tgchk_param *par)
-{
-	if (nf_ct_l3proto_try_module_get(par->family) < 0) {
-		printk(KERN_WARNING "cannot load conntrack support for "
-		       "proto=%u\n", par->family);
-		return false;
-	}
-	return true;
-}
-
-static void connmark_tg_destroy(const struct xt_tgdtor_param *par)
-{
-	nf_ct_l3proto_module_put(par->family);
-}
-
-#ifdef CONFIG_COMPAT
-struct compat_xt_connmark_target_info {
-	compat_ulong_t	mark, mask;
-	u_int8_t	mode;
-	u_int8_t	__pad1;
-	u_int16_t	__pad2;
-};
-
-static void connmark_tg_compat_from_user_v0(void *dst, void *src)
-{
-	const struct compat_xt_connmark_target_info *cm = src;
-	struct xt_connmark_target_info m = {
-		.mark	= cm->mark,
-		.mask	= cm->mask,
-		.mode	= cm->mode,
-	};
-	memcpy(dst, &m, sizeof(m));
-}
-
-static int connmark_tg_compat_to_user_v0(void __user *dst, void *src)
-{
-	const struct xt_connmark_target_info *m = src;
-	struct compat_xt_connmark_target_info cm = {
-		.mark	= m->mark,
-		.mask	= m->mask,
-		.mode	= m->mode,
-	};
-	return copy_to_user(dst, &cm, sizeof(cm)) ? -EFAULT : 0;
-}
-#endif /* CONFIG_COMPAT */
-
-static struct xt_target connmark_tg_reg[] __read_mostly = {
-	{
-		.name		= "CONNMARK",
-		.revision	= 0,
-		.family		= NFPROTO_UNSPEC,
-		.checkentry	= connmark_tg_check_v0,
-		.destroy	= connmark_tg_destroy,
-		.target		= connmark_tg_v0,
-		.targetsize	= sizeof(struct xt_connmark_target_info),
-#ifdef CONFIG_COMPAT
-		.compatsize	= sizeof(struct compat_xt_connmark_target_info),
-		.compat_from_user = connmark_tg_compat_from_user_v0,
-		.compat_to_user	= connmark_tg_compat_to_user_v0,
-#endif
-		.me		= THIS_MODULE
-	},
-	{
-		.name           = "CONNMARK",
-		.revision       = 1,
-		.family         = NFPROTO_UNSPEC,
-		.checkentry     = connmark_tg_check,
-		.target         = connmark_tg,
-		.targetsize     = sizeof(struct xt_connmark_tginfo1),
-		.destroy        = connmark_tg_destroy,
-		.me             = THIS_MODULE,
-	},
-};
-
-static int __init connmark_tg_init(void)
-{
-	return xt_register_targets(connmark_tg_reg,
-	       ARRAY_SIZE(connmark_tg_reg));
-}
-
-static void __exit connmark_tg_exit(void)
-{
-	xt_unregister_targets(connmark_tg_reg, ARRAY_SIZE(connmark_tg_reg));
-}
-
-module_init(connmark_tg_init);
-module_exit(connmark_tg_exit);
diff --git a/net/netfilter/xt_connmark.c b/net/netfilter/xt_connmark.c
index 86cacab..bcfad86 100644
--- a/net/netfilter/xt_connmark.c
+++ b/net/netfilter/xt_connmark.c
@@ -26,12 +26,16 @@
 #include <net/netfilter/nf_conntrack.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_connmark.h>
+#include <linux/netfilter/xt_CONNMARK.h>
+#include <net/netfilter/nf_conntrack_ecache.h>
 
 MODULE_AUTHOR("Henrik Nordstrom <hno@marasystems.com>");
-MODULE_DESCRIPTION("Xtables: connection mark match");
+MODULE_DESCRIPTION("Xtables: connection mark match and modification");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ipt_connmark");
 MODULE_ALIAS("ip6t_connmark");
+MODULE_ALIAS("ipt_CONNMARK");
+MODULE_ALIAS("ip6t_CONNMARK");
 
 static bool
 connmark_mt(const struct sk_buff *skb, const struct xt_match_param *par)
@@ -92,6 +96,122 @@ static void connmark_mt_destroy(const struct xt_mtdtor_param *par)
 	nf_ct_l3proto_module_put(par->family);
 }
 
+static unsigned int
+connmark_tg_v0(struct sk_buff *skb, const struct xt_target_param *par)
+{
+	const struct xt_connmark_target_info *markinfo = par->targinfo;
+	struct nf_conn *ct;
+	enum ip_conntrack_info ctinfo;
+	u_int32_t diff;
+	u_int32_t mark;
+	u_int32_t newmark;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (ct) {
+		switch(markinfo->mode) {
+		case XT_CONNMARK_SET:
+			newmark = (ct->mark & ~markinfo->mask) | markinfo->mark;
+			if (newmark != ct->mark) {
+				ct->mark = newmark;
+				nf_conntrack_event_cache(IPCT_MARK, ct);
+			}
+			break;
+		case XT_CONNMARK_SAVE:
+			newmark = (ct->mark & ~markinfo->mask) |
+				  (skb->mark & markinfo->mask);
+			if (ct->mark != newmark) {
+				ct->mark = newmark;
+				nf_conntrack_event_cache(IPCT_MARK, ct);
+			}
+			break;
+		case XT_CONNMARK_RESTORE:
+			mark = skb->mark;
+			diff = (ct->mark ^ mark) & markinfo->mask;
+			skb->mark = mark ^ diff;
+			break;
+		}
+	}
+
+	return XT_CONTINUE;
+}
+
+static unsigned int
+connmark_tg(struct sk_buff *skb, const struct xt_target_param *par)
+{
+	const struct xt_connmark_tginfo1 *info = par->targinfo;
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+	u_int32_t newmark;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (ct == NULL)
+		return XT_CONTINUE;
+
+	switch (info->mode) {
+	case XT_CONNMARK_SET:
+		newmark = (ct->mark & ~info->ctmask) ^ info->ctmark;
+		if (ct->mark != newmark) {
+			ct->mark = newmark;
+			nf_conntrack_event_cache(IPCT_MARK, ct);
+		}
+		break;
+	case XT_CONNMARK_SAVE:
+		newmark = (ct->mark & ~info->ctmask) ^
+		          (skb->mark & info->nfmask);
+		if (ct->mark != newmark) {
+			ct->mark = newmark;
+			nf_conntrack_event_cache(IPCT_MARK, ct);
+		}
+		break;
+	case XT_CONNMARK_RESTORE:
+		newmark = (skb->mark & ~info->nfmask) ^
+		          (ct->mark & info->ctmask);
+		skb->mark = newmark;
+		break;
+	}
+
+	return XT_CONTINUE;
+}
+
+static bool connmark_tg_check_v0(const struct xt_tgchk_param *par)
+{
+	const struct xt_connmark_target_info *matchinfo = par->targinfo;
+
+	if (matchinfo->mode == XT_CONNMARK_RESTORE) {
+		if (strcmp(par->table, "mangle") != 0) {
+			printk(KERN_WARNING "CONNMARK: restore can only be "
+			       "called from \"mangle\" table, not \"%s\"\n",
+			       par->table);
+			return false;
+		}
+	}
+	if (matchinfo->mark > 0xffffffff || matchinfo->mask > 0xffffffff) {
+		printk(KERN_WARNING "CONNMARK: Only supports 32bit mark\n");
+		return false;
+	}
+	if (nf_ct_l3proto_try_module_get(par->family) < 0) {
+		printk(KERN_WARNING "can't load conntrack support for "
+				    "proto=%u\n", par->family);
+		return false;
+	}
+	return true;
+}
+
+static bool connmark_tg_check(const struct xt_tgchk_param *par)
+{
+	if (nf_ct_l3proto_try_module_get(par->family) < 0) {
+		printk(KERN_WARNING "cannot load conntrack support for "
+		       "proto=%u\n", par->family);
+		return false;
+	}
+	return true;
+}
+
+static void connmark_tg_destroy(const struct xt_tgdtor_param *par)
+{
+	nf_ct_l3proto_module_put(par->family);
+}
+
 #ifdef CONFIG_COMPAT
 struct compat_xt_connmark_info {
 	compat_ulong_t	mark, mask;
@@ -100,6 +220,13 @@ struct compat_xt_connmark_info {
 	u_int16_t	__pad2;
 };
 
+struct compat_xt_connmark_target_info {
+	compat_ulong_t	mark, mask;
+	u_int8_t	mode;
+	u_int8_t	__pad1;
+	u_int16_t	__pad2;
+};
+
 static void connmark_mt_compat_from_user_v0(void *dst, void *src)
 {
 	const struct compat_xt_connmark_info *cm = src;
@@ -121,6 +248,28 @@ static int connmark_mt_compat_to_user_v0(void __user *dst, void *src)
 	};
 	return copy_to_user(dst, &cm, sizeof(cm)) ? -EFAULT : 0;
 }
+
+static void connmark_tg_compat_from_user_v0(void *dst, void *src)
+{
+	const struct compat_xt_connmark_target_info *cm = src;
+	struct xt_connmark_target_info m = {
+		.mark	= cm->mark,
+		.mask	= cm->mask,
+		.mode	= cm->mode,
+	};
+	memcpy(dst, &m, sizeof(m));
+}
+
+static int connmark_tg_compat_to_user_v0(void __user *dst, void *src)
+{
+	const struct xt_connmark_target_info *m = src;
+	struct compat_xt_connmark_target_info cm = {
+		.mark	= m->mark,
+		.mask	= m->mask,
+		.mode	= m->mode,
+	};
+	return copy_to_user(dst, &cm, sizeof(cm)) ? -EFAULT : 0;
+}
 #endif /* CONFIG_COMPAT */
 
 static struct xt_match connmark_mt_reg[] __read_mostly = {
@@ -151,16 +300,59 @@ static struct xt_match connmark_mt_reg[] __read_mostly = {
 	},
 };
 
-static int __init connmark_mt_init(void)
+static struct xt_target connmark_tg_reg[] __read_mostly = {
+	{
+		.name		= "CONNMARK",
+		.revision	= 0,
+		.family		= NFPROTO_UNSPEC,
+		.checkentry	= connmark_tg_check_v0,
+		.destroy	= connmark_tg_destroy,
+		.target		= connmark_tg_v0,
+		.targetsize	= sizeof(struct xt_connmark_target_info),
+#ifdef CONFIG_COMPAT
+		.compatsize	= sizeof(struct compat_xt_connmark_target_info),
+		.compat_from_user = connmark_tg_compat_from_user_v0,
+		.compat_to_user	= connmark_tg_compat_to_user_v0,
+#endif
+		.me		= THIS_MODULE
+	},
+	{
+		.name           = "CONNMARK",
+		.revision       = 1,
+		.family         = NFPROTO_UNSPEC,
+		.checkentry     = connmark_tg_check,
+		.target         = connmark_tg,
+		.targetsize     = sizeof(struct xt_connmark_tginfo1),
+		.destroy        = connmark_tg_destroy,
+		.me             = THIS_MODULE,
+	},
+};
+
+static int __init xt_connmark_init(void)
 {
-	return xt_register_matches(connmark_mt_reg,
-	       ARRAY_SIZE(connmark_mt_reg));
+	int ret;
+
+	ret = xt_register_matches(connmark_mt_reg,
+	      ARRAY_SIZE(connmark_mt_reg));
+	if (ret < 0)
+		return ret;
+
+	ret = xt_register_targets(connmark_tg_reg,
+	      ARRAY_SIZE(connmark_tg_reg));
+	if (ret < 0) {
+		xt_unregister_matches(connmark_mt_reg,
+				      ARRAY_SIZE(connmark_mt_reg));
+		return ret;
+	}
+
+	return 0;
 }
 
-static void __exit connmark_mt_exit(void)
+static void __exit xt_connmark_exit(void)
 {
 	xt_unregister_matches(connmark_mt_reg, ARRAY_SIZE(connmark_mt_reg));
+	xt_unregister_targets(connmark_tg_reg, ARRAY_SIZE(connmark_tg_reg));
 }
 
-module_init(connmark_mt_init);
-module_exit(connmark_mt_exit);
+module_init(xt_connmark_init);
+module_exit(xt_connmark_exit);
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: net/netfilter reorganization

[Jozsef Kadlecsik]

On Sun, 5 Oct 2008, Jan Engelhardt wrote:

> On Sunday 2008-10-05 16:33, Jan Engelhardt wrote:
> >On Sunday 2008-10-05 16:28, David Miller wrote:
> >>> On Sun, 5 Oct 2008, Jan Engelhardt wrote:
> >>> 
> >>> > I'd like to go for $(always lowercase extension name)_{mt,tg}.c if 
> >>> > noone objects.
> >>> > Or, when obvious extensions get combined maybe (like xt_mark and xt_MARK),
> >>> > just mark.c and the issue is all gone.
> >>> 
> >>> I think match/target combined in one file is the best way to go. As they 
> >>> cover the same realm (match some quantity of something and change it), 
> >>> they do fit together.
> >>
> >>This also makes sense to me.
> >
> >Before I start moving code, would we may want to combine more modules
> >into one file at this point besides xt_FOO and xt_foo only?
> (Probably not.)

If restructuring is on the way, then it should cover all possible parts.
Just my quick thoughts, with suggested module names:

addr/packet type matches in one module (addrtype):
	addrtype, pkttype

mark modules, targets in one module (route):
	connmark, mark, realm
	CLASSIFY, CONNMARK, MARK

conntrack related modules in one module (conntrack): 
	conntrack, helper, state

IPv4/IPv6 header matching and modifying in one module (iphdr):
	dscp, length, tos, ttl
	DSCP, TOS, TTL

IPv6 extension headers matching and modifying in one module (exthdr):
	dst, frag, hbh, hl, ipv6hdr, mh, rt
	HL

TCP header matching and modifying in one module (tcphdr):
	ecn, tcpmss
	ECN, TCPMSS, TCPOPTSTRIP

ipsec in one module (ipsec)
	ah, esp, policy

security markings in one module: (secmark):
	CONNSECMARK, SECMARK	

Something similar should be done with the different type of 
limit/statistics modules as well.

> Funny thing is, only when you try you see more problems a-coming.
> Like, Kconfig option names. Keep/Lose
> NETFILTER_XT_{MATCH,TARGET}_CONNMARK, and query users for a new one?

Definitely yes. Kconfig is overloaded with netfilter targets/matches and 
if matches/targets are collapsed into a single file, then Kconfig options 
should be unified, as in your sample patch.

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: RFC: net/netfilter reorganization

[Patrick McHardy]

Jozsef Kadlecsik wrote:
> If restructuring is on the way, then it should cover all possible parts.
> Just my quick thoughts, with suggested module names:
>
> addr/packet type matches in one module (addrtype):
> 	addrtype, pkttype
>
> mark modules, targets in one module (route):
> 	connmark, mark, realm
> 	CLASSIFY, CONNMARK, MARK
>   

CONNMARK and connmark needs to be separated from MARK etc. because
they depend on the conntrack module.

> conntrack related modules in one module (conntrack): 
> 	conntrack, helper, state
>
> IPv4/IPv6 header matching and modifying in one module (iphdr):
> 	dscp, length, tos, ttl
> 	DSCP, TOS, TTL
>
> IPv6 extension headers matching and modifying in one module (exthdr):
> 	dst, frag, hbh, hl, ipv6hdr, mh, rt
> 	HL
>
> TCP header matching and modifying in one module (tcphdr):
> 	ecn, tcpmss
> 	ECN, TCPMSS, TCPOPTSTRIP
>
> ipsec in one module (ipsec)
> 	ah, esp, policy
>
> security markings in one module: (secmark):
> 	CONNSECMARK, SECMARK	
>
> Something similar should be done with the different type of 
> limit/statistics modules as well.
>
>   
>> Funny thing is, only when you try you see more problems a-coming.
>> Like, Kconfig option names. Keep/Lose
>> NETFILTER_XT_{MATCH,TARGET}_CONNMARK, and query users for a new one?
>>     
>
> Definitely yes. Kconfig is overloaded with netfilter targets/matches and 
> if matches/targets are collapsed into a single file, then Kconfig options 
> should be unified, as in your sample patch.

Agreed, but please keep the old options around (doing just a select
on the new ones) for one or two releases.

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

>> > Funny thing is, only when you try you see more problems a-coming.
>> > Like, Kconfig option names. Keep/Lose
>> > NETFILTER_XT_{MATCH,TARGET}_CONNMARK, and query users for a new one?
>>
>> Definitely yes. Kconfig is overloaded with netfilter
>> targets/matches and if matches/targets are collapsed into a single
>> file, then Kconfig options should be unified, as in your sample
>> patch.
>
> Agreed, but please keep the old options around (doing just a select
> on the new ones) for one or two releases.

Hm I just noticed a not-nice kconfig behavior.

If I have a .config with

	TARGET_CTMARK=m
	MATCH_CTMARK=m
	# BOTH_CTMARK not listed at all

and a Kconfig file with

	config TARGET_CTMARK
		tristate
		select BOTH_CTMARK
	config MATCH_CTMARK
		tristate
		select BOTH_CTMARK
	config BOTH_CTMARK
		tristate "shiny new module"

Then oldconfig will still ask me to choose [N/m] for BOTH_CTMARK. Only 
if I use

	config TARGET_CTMARK
		tristate "foo"
		select BOTH_CTMARK
	config MATCH_CTMARK
		tristate "bar"
		select BOTH_CTMARK
	config BOTH_CTMARK
		tristate "shiny new module"

i.e. add strings, it will proceed without asking me anything. Meh that's 
bad. I can think of odd workarounds like

	config TARGET_CTMARK
		tristate "shiny new module"
	config MATCH_CTMARK
		select BOTH_CTMARK
	config BOTH_CTMARK
		select TARGET_CTMARK

but that looks a bit odd. Requesting more comments.

Re: RFC: net/netfilter reorganization

[Patrick McHardy]

Jan Engelhardt wrote:
> Hm I just noticed a not-nice kconfig behavior.
> 
> If I have a .config with
> 
> 	TARGET_CTMARK=m
> 	MATCH_CTMARK=m
> 	# BOTH_CTMARK not listed at all
> 
> and a Kconfig file with
> 
> 	config TARGET_CTMARK
> 		tristate
> 		select BOTH_CTMARK
> 	config MATCH_CTMARK
> 		tristate
> 		select BOTH_CTMARK
> 	config BOTH_CTMARK
> 		tristate "shiny new module"
> 
> Then oldconfig will still ask me to choose [N/m] for BOTH_CTMARK. Only 
> if I use
> 
> 	config TARGET_CTMARK
> 		tristate "foo"
> 		select BOTH_CTMARK
> 	config MATCH_CTMARK
> 		tristate "bar"
> 		select BOTH_CTMARK
> 	config BOTH_CTMARK
> 		tristate "shiny new module"
> 
> i.e. add strings, it will proceed without asking me anything. Meh that's 
> bad. I can think of odd workarounds like
> 
> 	config TARGET_CTMARK
> 		tristate "shiny new module"
> 	config MATCH_CTMARK
> 		select BOTH_CTMARK
> 	config BOTH_CTMARK
> 		select TARGET_CTMARK
> 
> but that looks a bit odd. Requesting more comments.

That should be fine though since the old options already exist
(in most cases)?

BTW, does it make any difference if you add a dependency
on TARGET_CTMARK || MATCH_CTMARK?

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Monday 2008-10-06 06:07, Patrick McHardy wrote:
>>[troubles with Kconfig syntaxe about BOTH_CTMARK]
>
> That should be fine though since the old options already exist
> (in most cases)?
>
> BTW, does it make any difference if you add a dependency
> on TARGET_CTMARK || MATCH_CTMARK?

Unfortunately no.
If I use "BOTH_CTMARK depends on TARGET_CTMARK || MATCH_CTMARK", the 
option does not show itself. Somewhat understandable.
I also tried
"BOTH_CTMARK default m if TARGET_CTMARK=m || MATCH_CTMARK=m", also a 
no-go.

Seems like config options without a one-line text are considered 
nonexistent for kconfig to begin with, so that nothing can depend or 
default on these.

Re: RFC: net/netfilter reorganization

[Roman Zippel]

Hi,

On Mon, 6 Oct 2008, Jan Engelhardt wrote:

> Unfortunately no.
> If I use "BOTH_CTMARK depends on TARGET_CTMARK || MATCH_CTMARK", the 
> option does not show itself. Somewhat understandable.
> I also tried
> "BOTH_CTMARK default m if TARGET_CTMARK=m || MATCH_CTMARK=m", also a 
> no-go.
> 
> Seems like config options without a one-line text are considered 
> nonexistent for kconfig to begin with, so that nothing can depend or 
> default on these.

Not really. If one of these two symbols has a value different than 'n', it 
should should also select BOTH_CTMARK. I did a quick test and I still get 
the expected behaviour, so there is something else...

bye, Roman

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Tuesday 2008-10-07 07:34, Roman Zippel wrote:
>> 
>> Seems like config options without a one-line text are considered 
>> nonexistent for kconfig to begin with, so that nothing can depend or 
>> default on these.
>
>Not really. If one of these two symbols has a value different than 'n', it 
>should should also select BOTH_CTMARK. I did a quick test and I still get 
>the expected behaviour, so there is something else...
>
Nope. A patch as simple as

--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -317,7 +317,7 @@ config NETFILTER_XT_TARGET_CLASSIFY
          To compile it as a module, choose M here.  If unsure, say N.
 
 config NETFILTER_XT_TARGET_CONNMARK
-       tristate  '"CONNMARK" target support'
+       tristate
        depends on NF_CONNTRACK
        depends on NETFILTER_ADVANCED
        select NF_CONNTRACK_MARK

nukes TARGET_CONNMARK from .config - as in, there is not even a
"# TARGET_CONNMARK is unset" line.
As such I can understand why adding a "select ELSE" for TARGET_CONNMARK
won't work.

Re: RFC: net/netfilter reorganization

[Roman Zippel]

Hi,

On Tue, 7 Oct 2008, Jan Engelhardt wrote:

> Nope. A patch as simple as
> 
> --- a/net/netfilter/Kconfig
> +++ b/net/netfilter/Kconfig
> @@ -317,7 +317,7 @@ config NETFILTER_XT_TARGET_CLASSIFY
>           To compile it as a module, choose M here.  If unsure, say N.
>  
>  config NETFILTER_XT_TARGET_CONNMARK
> -       tristate  '"CONNMARK" target support'
> +       tristate
>         depends on NF_CONNTRACK
>         depends on NETFILTER_ADVANCED
>         select NF_CONNTRACK_MARK
> 
> nukes TARGET_CONNMARK from .config - as in, there is not even a
> "# TARGET_CONNMARK is unset" line.

That's correct, as it doesn't get any value (either from a default or a 
select). If it has a value, it will be used to select other symbols.

> As such I can understand why adding a "select ELSE" for TARGET_CONNMARK
> won't work.

I don't quite understand what you're trying to do. :)

bye, Roman

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Tuesday 2008-10-07 13:09, Roman Zippel wrote:

>>  config NETFILTER_XT_TARGET_CONNMARK
>> -       tristate  '"CONNMARK" target support'
>> +       tristate
>>         depends on NF_CONNTRACK
>>         depends on NETFILTER_ADVANCED
>>         select NF_CONNTRACK_MARK
>> 
>> nukes TARGET_CONNMARK from .config - as in, there is not even a
>> "# TARGET_CONNMARK is unset" line.
>
>That's correct, as it doesn't get any value (either from a default or a 
>select). If it has a value, it will be used to select other symbols.
>
>> As such I can understand why adding a "select ELSE" for TARGET_CONNMARK
>> won't work.
>
>I don't quite understand what you're trying to do. :)

We are trying to combine TARGET_CONNMARK and MATCH_CONNMARK into a
new option, and doing so without showing {TARGET,MATCH}, yet
selecting BOTH_CONNMARK when either of {TARGET,MATCH} was previously
selected (e.g by feeding oldconfig something that has these options
active).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: net/netfilter reorganization

[Roman Zippel]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 12111 bytes --]

Hi,

On Tue, 7 Oct 2008, Jan Engelhardt wrote:

> We are trying to combine TARGET_CONNMARK and MATCH_CONNMARK into a
> new option, and doing so without showing {TARGET,MATCH}, yet
> selecting BOTH_CONNMARK when either of {TARGET,MATCH} was previously
> selected (e.g by feeding oldconfig something that has these options
> active).

Try the patch below, you can define an alias (e.g. add "option 
alias=TARGET_CONNMARK" to BOTH_CONNMARK) and kconfig will use the alias 
while reading the .config file (unless there is already a value for it).
Previously .config data was only used as a preset for user prompts, for 
any other derived symbol it's ignored.
If it does what you need, I can get it merged as quickly as possible 
after adding a bit of documentation. I maybe also give it another name, as 
"alias" is a little general and it's not a generally usable alias.

bye, Roman

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index b91cf24..7c9f229 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -151,6 +151,7 @@ int conf_read_simple(const char *name, int def)
 	char line[1024];
 	char *p, *p2;
 	struct symbol *sym;
+	struct expr *e;
 	int i, def_flags;
 
 	if (name) {
@@ -307,6 +308,23 @@ load:
 
 	if (modules_sym)
 		sym_calc_value(modules_sym);
+
+	expr_list_for_each_sym(sym_alias_list, e, sym) {
+		struct symbol *sym2;
+		struct property *prop;
+
+		if (!(sym->flags & def_flags))
+			continue;
+
+		for_all_properties(sym, prop, P_ALIAS) {
+			sym2 = prop_get_symbol(prop);
+			if (!(sym2->flags & def_flags)) {
+				sym2->def[def] = sym->def[def];
+				sym2->flags |= def_flags;
+			}
+		}
+	}
+
 	return 0;
 }
 
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 9d4cba1..224bf4a 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -107,7 +107,7 @@ struct symbol {
 
 enum prop_type {
 	P_UNKNOWN, P_PROMPT, P_COMMENT, P_MENU, P_DEFAULT, P_CHOICE,
-	P_SELECT, P_RANGE, P_ENV
+	P_SELECT, P_RANGE, P_ENV, P_ALIAS
 };
 
 struct property {
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index 4a9af6f..7ea2bc4 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -53,6 +53,7 @@ enum conf_def_mode {
 #define T_OPT_MODULES		1
 #define T_OPT_DEFCONFIG_LIST	2
 #define T_OPT_ENV		3
+#define T_OPT_ALIAS		4
 
 struct kconf_id {
 	int name;
@@ -97,6 +98,7 @@ void menu_add_symbol(enum prop_type type, struct symbol *sym, struct expr *dep);
 void menu_add_option(int token, char *arg);
 void menu_finalize(struct menu *parent);
 void menu_set_type(int type);
+void sym_check_prop(struct symbol *sym);
 
 /* util.c */
 struct file *file_lookup(const char *name);
@@ -115,6 +117,7 @@ const char *str_get(struct gstr *gs);
 
 /* symbol.c */
 extern struct expr *sym_env_list;
+extern struct expr *sym_alias_list;
 
 void sym_init(void);
 void sym_clear_all_valid(void);
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 07ff8d1..e1d8686 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -175,6 +175,11 @@ void menu_add_option(int token, char *arg)
 	case T_OPT_ENV:
 		prop_add_env(arg);
 		break;
+	case T_OPT_ALIAS:
+		prop = prop_alloc(P_ALIAS, sym_lookup(arg, 0));
+		prop->menu = current_entry;
+		prop->expr = expr_alloc_symbol(current_entry->sym);
+		break;
 	}
 }
 
@@ -219,6 +224,23 @@ void sym_check_prop(struct symbol *sym)
 			    !menu_range_valid_sym(sym, prop->expr->right.sym))
 				prop_warn(prop, "range is invalid");
 			break;
+		case P_ALIAS:
+			if (sym->prop->type != P_ALIAS ||
+			    (prop->next && prop->next->type != P_ALIAS)) {
+				prop_warn(prop, "alias is invalid");
+				break;
+			}
+			sym2 = prop_get_symbol(prop);
+			if (sym->type != S_UNKNOWN && sym->type != sym2->type) {
+				prop_warn(prop, "conflicting alias type");
+				break;
+			}
+			sym->type = sym2->type;
+			if (!prop->next) {
+				sym_alias_list = expr_alloc_one(E_LIST, sym_alias_list);
+				sym_alias_list->right.sym = sym;
+			}
+			break;
 		default:
 			;
 		}
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 18f3e5c..f25fb3d 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -34,7 +34,7 @@ struct symbol *sym_defconfig_list;
 struct symbol *modules_sym;
 tristate modules_val;
 
-struct expr *sym_env_list;
+struct expr *sym_env_list, *sym_alias_list;
 
 void sym_add_default(struct symbol *sym, const char *def)
 {
@@ -937,6 +937,8 @@ const char *prop_get_type_name(enum prop_type type)
 		return "select";
 	case P_RANGE:
 		return "range";
+	case P_ALIAS:
+		return "alias";
 	case P_UNKNOWN:
 		break;
 	}
diff --git a/scripts/kconfig/zconf.gperf b/scripts/kconfig/zconf.gperf
index 25ef5d0..2816585 100644
--- a/scripts/kconfig/zconf.gperf
+++ b/scripts/kconfig/zconf.gperf
@@ -41,4 +41,5 @@ on,		T_ON,		TF_PARAM
 modules,	T_OPT_MODULES,	TF_OPTION
 defconfig_list,	T_OPT_DEFCONFIG_LIST,TF_OPTION
 env,		T_OPT_ENV,	TF_OPTION
+alias,		T_OPT_ALIAS,	TF_OPTION
 %%
diff --git a/scripts/kconfig/zconf.hash.c_shipped b/scripts/kconfig/zconf.hash.c_shipped
index 5c73d51..cd2f361 100644
--- a/scripts/kconfig/zconf.hash.c_shipped
+++ b/scripts/kconfig/zconf.hash.c_shipped
@@ -53,10 +53,10 @@ kconf_id_hash (register const char *str, register unsigned int len)
       49, 49, 49, 49, 49, 49, 49, 49, 49, 49,
       49, 49, 49, 49, 49, 49, 49, 49, 49, 49,
       49, 49, 49, 49, 49, 49, 49, 49, 49, 49,
-      49, 49, 49, 49, 49, 49, 49, 49, 11,  5,
+      49, 49, 49, 49, 49, 49, 49,  0, 35,  5,
        0,  0,  5, 49,  5, 20, 49, 49,  5, 20,
        5,  0, 30, 49,  0, 15,  0, 10,  0, 49,
-      25, 49, 49, 49, 49, 49, 49, 49, 49, 49,
+      10, 49, 49, 49, 49, 49, 49, 49, 49, 49,
       49, 49, 49, 49, 49, 49, 49, 49, 49, 49,
       49, 49, 49, 49, 49, 49, 49, 49, 49, 49,
       49, 49, 49, 49, 49, 49, 49, 49, 49, 49,
@@ -100,24 +100,25 @@ struct kconf_id_strings_t
     char kconf_id_strings_str12[sizeof("default")];
     char kconf_id_strings_str13[sizeof("def_bool")];
     char kconf_id_strings_str14[sizeof("help")];
-    char kconf_id_strings_str15[sizeof("bool")];
     char kconf_id_strings_str16[sizeof("config")];
     char kconf_id_strings_str17[sizeof("def_tristate")];
-    char kconf_id_strings_str18[sizeof("boolean")];
+    char kconf_id_strings_str18[sizeof("hex")];
     char kconf_id_strings_str19[sizeof("defconfig_list")];
     char kconf_id_strings_str21[sizeof("string")];
     char kconf_id_strings_str22[sizeof("if")];
     char kconf_id_strings_str23[sizeof("int")];
+    char kconf_id_strings_str25[sizeof("alias")];
     char kconf_id_strings_str26[sizeof("select")];
     char kconf_id_strings_str27[sizeof("modules")];
     char kconf_id_strings_str28[sizeof("tristate")];
     char kconf_id_strings_str29[sizeof("menu")];
     char kconf_id_strings_str31[sizeof("source")];
     char kconf_id_strings_str32[sizeof("comment")];
-    char kconf_id_strings_str33[sizeof("hex")];
     char kconf_id_strings_str35[sizeof("menuconfig")];
     char kconf_id_strings_str36[sizeof("prompt")];
     char kconf_id_strings_str37[sizeof("depends")];
+    char kconf_id_strings_str39[sizeof("bool")];
+    char kconf_id_strings_str42[sizeof("boolean")];
     char kconf_id_strings_str48[sizeof("mainmenu")];
   };
 static struct kconf_id_strings_t kconf_id_strings_contents =
@@ -134,24 +135,25 @@ static struct kconf_id_strings_t kconf_id_strings_contents =
     "default",
     "def_bool",
     "help",
-    "bool",
     "config",
     "def_tristate",
-    "boolean",
+    "hex",
     "defconfig_list",
     "string",
     "if",
     "int",
+    "alias",
     "select",
     "modules",
     "tristate",
     "menu",
     "source",
     "comment",
-    "hex",
     "menuconfig",
     "prompt",
     "depends",
+    "bool",
+    "boolean",
     "mainmenu"
   };
 #define kconf_id_strings ((const char *) &kconf_id_strings_contents)
@@ -166,7 +168,7 @@ kconf_id_lookup (register const char *str, register unsigned int len)
 {
   enum
     {
-      TOTAL_KEYWORDS = 31,
+      TOTAL_KEYWORDS = 32,
       MIN_WORD_LENGTH = 2,
       MAX_WORD_LENGTH = 14,
       MIN_HASH_VALUE = 2,
@@ -189,16 +191,17 @@ kconf_id_lookup (register const char *str, register unsigned int len)
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str12,	T_DEFAULT,	TF_COMMAND, S_UNKNOWN},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str13,	T_DEFAULT,	TF_COMMAND, S_BOOLEAN},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str14,		T_HELP,		TF_COMMAND},
-      {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str15,		T_TYPE,		TF_COMMAND, S_BOOLEAN},
+      {-1},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str16,		T_CONFIG,	TF_COMMAND},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str17,	T_DEFAULT,	TF_COMMAND, S_TRISTATE},
-      {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str18,	T_TYPE,		TF_COMMAND, S_BOOLEAN},
+      {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str18,		T_TYPE,		TF_COMMAND, S_HEX},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str19,	T_OPT_DEFCONFIG_LIST,TF_OPTION},
       {-1},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str21,		T_TYPE,		TF_COMMAND, S_STRING},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str22,		T_IF,		TF_COMMAND|TF_PARAM},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str23,		T_TYPE,		TF_COMMAND, S_INT},
-      {-1}, {-1},
+      {-1},
+      {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str25,		T_OPT_ALIAS,	TF_OPTION},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str26,		T_SELECT,	TF_COMMAND},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str27,	T_OPT_MODULES,	TF_OPTION},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str28,	T_TYPE,		TF_COMMAND, S_TRISTATE},
@@ -206,13 +209,15 @@ kconf_id_lookup (register const char *str, register unsigned int len)
       {-1},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str31,		T_SOURCE,	TF_COMMAND},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str32,	T_COMMENT,	TF_COMMAND},
-      {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str33,		T_TYPE,		TF_COMMAND, S_HEX},
-      {-1},
+      {-1}, {-1},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str35,	T_MENUCONFIG,	TF_COMMAND},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str36,		T_PROMPT,	TF_COMMAND},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str37,	T_DEPENDS,	TF_COMMAND},
-      {-1}, {-1}, {-1}, {-1}, {-1}, {-1}, {-1}, {-1}, {-1},
       {-1},
+      {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str39,		T_TYPE,		TF_COMMAND, S_BOOLEAN},
+      {-1}, {-1},
+      {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str42,	T_TYPE,		TF_COMMAND, S_BOOLEAN},
+      {-1}, {-1}, {-1}, {-1}, {-1},
       {(int)(long)&((struct kconf_id_strings_t *)0)->kconf_id_strings_str48,	T_MAINMENU,	TF_COMMAND}
     };
 
diff --git a/scripts/kconfig/zconf.tab.c_shipped b/scripts/kconfig/zconf.tab.c_shipped
index 95df833..05fbd1d 100644
--- a/scripts/kconfig/zconf.tab.c_shipped
+++ b/scripts/kconfig/zconf.tab.c_shipped
@@ -2278,6 +2278,10 @@ void conf_parse(const char *name)
 	for_all_symbols(i, sym) {
 		if (sym_check_deps(sym))
 			zconfnerrs++;
+		if (!(sym->flags & SYMBOL_WARNED)) {
+			sym_check_prop(sym);
+			sym->flags |= SYMBOL_WARNED;
+		}
         }
 	if (zconfnerrs)
 		exit(1);
diff --git a/scripts/kconfig/zconf.y b/scripts/kconfig/zconf.y
index 9710b82..bb74756 100644
--- a/scripts/kconfig/zconf.y
+++ b/scripts/kconfig/zconf.y
@@ -495,6 +495,10 @@ void conf_parse(const char *name)
 	for_all_symbols(i, sym) {
 		if (sym_check_deps(sym))
 			zconfnerrs++;
+		if (!(sym->flags & SYMBOL_WARNED)) {
+			sym_check_prop(sym);
+			sym->flags |= SYMBOL_WARNED;
+		}
         }
 	if (zconfnerrs)
 		exit(1);

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Monday 2008-10-13 14:52, Roman Zippel wrote:
>On Tue, 7 Oct 2008, Jan Engelhardt wrote:
>
>> We are trying to combine TARGET_CONNMARK and MATCH_CONNMARK into a
>> new option, and doing so without showing {TARGET,MATCH}, yet
>> selecting BOTH_CONNMARK when either of {TARGET,MATCH} was previously
>> selected (e.g by feeding oldconfig something that has these options
>> active).
>
>Try the patch below, you can define an alias (e.g. add "option 
>alias=TARGET_CONNMARK" to BOTH_CONNMARK) and kconfig will use the alias 
>while reading the .config file (unless there is already a value for it).
>Previously .config data was only used as a preset for user prompts, for 
>any other derived symbol it's ignored.
>If it does what you need, I can get it merged as quickly as possible 
>after adding a bit of documentation. I maybe also give it another name, as 
>"alias" is a little general and it's not a generally usable alias.

Nope, does not quite do.. it seems it cannot use more than one:

config BOTH
	option alias=TARGET
	option alias=MATCH

Given a .config with TARGET=n, MATCH=m, I will get BOTH=n when
running oldconfig.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: net/netfilter reorganization

[Jozsef Kadlecsik]

On Mon, 6 Oct 2008, Patrick McHardy wrote:

> Jozsef Kadlecsik wrote:
> > If restructuring is on the way, then it should cover all possible parts.
> > Just my quick thoughts, with suggested module names:
> > mark modules, targets in one module (route):
> > 	connmark, mark, realm
> > 	CLASSIFY, CONNMARK, MARK
> 
> CONNMARK and connmark needs to be separated from MARK etc. because
> they depend on the conntrack module.

They can go into a single file and linked to the "route" module iff 
NF_CONNTRACK was selected.

The same may be applied for other cases as well: it is not needed to force 
everything into a huge file. ;-)

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: RFC: net/netfilter reorganization

[Patrick McHardy]

Jozsef Kadlecsik wrote:
> On Mon, 6 Oct 2008, Patrick McHardy wrote:
> 
>> Jozsef Kadlecsik wrote:
>>> If restructuring is on the way, then it should cover all possible parts.
>>> Just my quick thoughts, with suggested module names:
>>> mark modules, targets in one module (route):
>>> 	connmark, mark, realm
>>> 	CLASSIFY, CONNMARK, MARK
>> CONNMARK and connmark needs to be separated from MARK etc. because
>> they depend on the conntrack module.
> 
> They can go into a single file and linked to the "route" module iff 
> NF_CONNTRACK was selected.
> 
> The same may be applied for other cases as well: it is not needed to force 
> everything into a huge file. ;-)

But then we still have the individual files in the directory.
I don't think we should combine files based on config options
but by actually moving the code into a new file.

Re: RFC: net/netfilter reorganization

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sun, 05 Oct 2008 18:11:34 +0200

> David Miller wrote:
> > From: Patrick McHardy <kaber@trash.net>
> > Date: Sun, 05 Oct 2008 16:48:36 +0200
> > 
> >> The main goal is to get the directory contents to a more reasonable
> >> level and group related stuff together; using just net/netfilter/ct
> >> is still a bit too coarse in my opinion. Under that aspect, what
> >> would your proposed structure be?
> > One thing I'm looking forward to is the elimination of all of
> > the xt_* prefixes, and in general better TAB completion
> > friendliness when I try to open netfilter source files under
> > emacs or grep things from the command line with tab completed
> > paths.
> 
> I'm not sure how emacs handles completion, would using different
> prefixes for matches and targets help? Otherwise I guess we can
> just get rid of the prefixes entirely.

Putting them in different sub-directories would help.

> The main problem with these renames is that it module aliases
> don't work for "rmmod", which is a pretty common thing with
> these modules. I guess we'd need to fix up module-init-tools
> first.

I see.

Re: RFC: net/netfilter reorganization

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Sun, 05 Oct 2008 18:11:34 +0200
> 
>> I'm not sure how emacs handles completion, would using different
>> prefixes for matches and targets help? Otherwise I guess we can
>> just get rid of the prefixes entirely.
> 
> Putting them in different sub-directories would help.

OK, lets do that as a start.

Re: RFC: net/netfilter reorganization

[Jan Engelhardt]

On Sunday 2008-10-05 10:48, Patrick McHardy wrote:
>> 
>> That's a 43% reduction in memory.
>
> Its a clear benefit, I don't doubt that. I mainly think this is not
> netfilter specific at all and should be done on a kbuild level.

This is not easily doable with kbuild at the moment, since you cannot
have two entry functions in the same .ko, which is why that
koredux patch has to call every module's ctors and dtors.

> Linking all of them together also has some runtime impact because
> of symbol dependencies, using let say tcpudp will probably pull
> in (parts of) IPv6 and NAT. Having them grouped by dependencies
> would avoid this.

This has been avoided by providing xtables_ext, xtables_xct (for
conntrack-depending), xtables_xnat (for nat-depending) and
xtables_xv6 (for ipv6-depending). The list above only showed modules
included in _ext which have no dependency whatsoever.

>> It does not really solve anything IMHO. Protocols? FTP is one, so hm,
>> same confusion. This also got my anti-favorite 4-depth :)
>
> Well, its not *that much* confusing since I believe most people are
> aware of that part of the netfilter terminology. The main goal is
> to get the directory contents to a more reasonable level and group
> related stuff together; using just net/netfilter/ct is still a bit
> too coarse in my opinion. Under that aspect, what would your proposed
> structure be?

Taking into account that mixing helpers and trackers in the same 
namespace, I'd go for something like
net/netfilter/conntrack/
	l3_generic.c
	l4_generic.c
	l4_tcp.c
	l5_ftp.c
net/ipv4/netfilter/ / v6/netfilter
	no preference
ebtables/netfilter
	just keep it for great justice, it's nicely clean :)

The directory preference limit of 3 has so much weight for me that 
remaining tweaks need to be done on the filename.
There are only 28 nfct files, and if we - at the minimum - get rid of 
the overly long nf_conntrack_ prefix, a wide display (ls -x) won't 
have too much blanks (the criterion for splitting things up in the first 
place, imo ;-).
I .. hope that makes sense.

Re: RFC: net/netfilter reorganization

[Jozsef Kadlecsik]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1214 bytes --]

On Sun, 5 Oct 2008, Jan Engelhardt wrote:

> What I really certainly would like to see is that we somehow get rid
> of the oddness of naming L3 trackers and L4 trackers. Right now we
> have (two examples)
> 
> -rw-r--r-- 1 15924 2008-09-09 20:34 nf_conntrack_ftp.c
> -rw-r--r-- 1 43369 2008-09-09 20:33 nf_conntrack_proto_tcp.c
> 
> and in everybody's mind, FTP is a protocol too of course, ˙˙so why
> does not ftp have a _proto part, and why does TCP?˙˙ one might ask.
> We could just remove the _proto part (also helpful to reduce the
> name length), as there is no clash between L3 and L4 trackers
> except perhaps for proto_generic and l3proto_generic. Hm?

I'd regard it more odd if the _proto part would just be stripped off. 
ICMP(v6)/TCP/UDP/STCP/etc. are fundamentally different from FTP/IRC/etc:
the latter are "just" helpers while the former ones build up the heart of 
conntrack. Do not just mix the two things in the naming.

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