[patch] iptables version defines

[patch] iptables version defines

[Thomas Jarosch]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

Hi Patrick,

[resent as I somehow sent an HTML email at first]

> >>>> #ifdef _XTABLES_H
> >>>> init(struct xt_entry_target *t)
> >>>> #else
> >>>> init(struct ipt_entry_target *t, unsigned int *nfcache)
> >>>> #endif
> >
> > Woah this is ridiculously ugly. (Remember, such constructs were
> > just eliminated from the kernel in the past years.)
>
> I don't care about uglyness as long as it stays in external
> code. So if someone sends me a patch to add this version
> define, I'll add it.

External code has to be "ugly" if you want to keep the user experience high.
I don't feel like breaking ipt_ACCOUNT for older iptables versions without
any real gain, it should work out of the box with iptables 1.4.0 and 1.4.1.

Attached is a patch to add the new defines. The macro XTABLES_VERSION
is already in use, so I named it XTABLES_VERSION_CHECK. I've also tested
that an empty XTABLES_VERSION_EXTRA in configure.ac works.

Now we can write code like this:

#if XTABLES_VERSION_CODE < XTABLES_VERSION_CHECK(1,5,0)
    #warning You are obselete and will be assimilated.
#endif

Cheers,
Thomas

[-- Attachment #2: iptables-add-version.patch --]
[-- Type: text/x-diff, Size: 1795 bytes --]

Add xtables version defines.

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>

--- iptables-1.4.1-rc2/configure.ac	Mon May 26 14:23:58 2008
+++ iptables.version/configure.ac	Mon Jun  2 15:05:57 2008
@@ -1,5 +1,11 @@
+define([_XTABLES_VERSION_MAJOR], 1)
+define([_XTABLES_VERSION_MINOR], 4)
+define([_XTABLES_VERSION_PATCH], 1)
+define([_XTABLES_VERSION_EXTRA], -rc2)
 
-AC_INIT([iptables], [1.4.1-rc2])
+define([_XTABLES_VERSION],_XTABLES_VERSION_MAJOR._XTABLES_VERSION_MINOR._XTABLES_VERSION_PATCH[]_XTABLES_VERSION_EXTRA)
+
+AC_INIT([iptables], _XTABLES_VERSION)
 AC_CONFIG_HEADERS([config.h])
 AC_PROG_INSTALL
 AM_INIT_AUTOMAKE
@@ -56,4 +62,14 @@
 AC_SUBST([kbuilddir])
 AC_SUBST([ksourcedir])
 AC_SUBST([xtlibdir])
+
+XTABLES_VERSION_MAJOR=_XTABLES_VERSION_MAJOR
+XTABLES_VERSION_MINOR=_XTABLES_VERSION_MINOR
+XTABLES_VERSION_PATCH=_XTABLES_VERSION_PATCH
+XTABLES_VERSION_EXTRA=_XTABLES_VERSION_EXTRA
+AC_SUBST([XTABLES_VERSION_MAJOR])
+AC_SUBST([XTABLES_VERSION_MINOR])
+AC_SUBST([XTABLES_VERSION_PATCH])
+AC_SUBST([XTABLES_VERSION_EXTRA])
+
 AC_OUTPUT([Makefile extensions/GNUmakefile libipq/Makefile include/xtables.h])
--- iptables-1.4.1-rc2/include/xtables.h.in	Mon May 26 14:15:40 2008
+++ iptables.version/include/xtables.h.in	Mon Jun  2 15:03:46 2008
@@ -18,6 +18,12 @@
 #endif
 
 #define XTABLES_VERSION "@PACKAGE_VERSION@"
+#define XTABLES_VERSION_CODE (0x10000 * @XTABLES_VERSION_MAJOR@ + 0x100 * @XTABLES_VERSION_MINOR@ + @XTABLES_VERSION_PATCH@)
+
+#define XTABLES_VERSION_CHECK(x,y,z)    (0x10000*(x) + 0x100*(y) + z)
+#define XTABLES_VERSION_MAJOR(x)  (((x)>>16) & 0xFF)
+#define XTABLES_VERSION_MINOR(x)  (((x)>> 8) & 0xFF)
+#define XTABLES_VERSION_PATCH(x)  ( (x)      & 0xFF)
 
 /* Include file for additions: new matches and targets. */
 struct xtables_match

Re: [patch] iptables version defines

[Patrick McHardy]

Thomas Jarosch wrote:
> Hi Patrick,
> 
>>>>>> #ifdef _XTABLES_H
>>>>>> init(struct xt_entry_target *t)
>>>>>> #else
>>>>>> init(struct ipt_entry_target *t, unsigned int *nfcache)
>>>>>> #endif
>>> Woah this is ridiculously ugly. (Remember, such constructs were
>>> just eliminated from the kernel in the past years.)
>> I don't care about uglyness as long as it stays in external
>> code. So if someone sends me a patch to add this version
>> define, I'll add it.
> 
> External code has to be "ugly" if you want to keep the user experience high.
> I don't feel like breaking ipt_ACCOUNT for older iptables versions without
> any real gain, it should work out of the box with iptables 1.4.0 and 1.4.1.
> 
> Attached is a patch to add the new defines. The macro XTABLES_VERSION is already in use, so I named it XTABLES_VERSION_CHECK. I've also tested
> that an empty XTABLES_VERSION_EXTRA in configure.ac works.
> 
> Now we can write code like this:
> 
> #if XTABLES_VERSION_CODE < XTABLES_VERSION_CHECK(1,5,0)
>     #warning You are obselete and will be assimilated.
> #endif


Looks good to me - I'll let it sit on netfilter-devel until tonight
though since my auto* knowlegde is close to zero :)

Re: [patch] iptables version defines

[Jan Engelhardt]

On Monday 2008-06-02 15:49, Thomas Jarosch wrote:
>> I don't care about uglyness as long as it stays in external
>> code. So if someone sends me a patch to add this version
>> define, I'll add it.
>
>External code has to be "ugly" if you want to keep the user
>experience high. I don't feel like breaking ipt_ACCOUNT for older
>iptables versions without any real gain, it should work out of the
>box with iptables 1.4.0 and 1.4.1.

External code does not _necessarily_ have to be ugly; at least
one can make it so as to reduce the lot of redundant #if hackery;
xt-a does this for the kernel interface already, it should not be
hard to do the same for the iptables API once it becomes necessary.
For that to work, we need a XTABLES_VERSION_CODE, and since your
patch does just that, you get my approval. Comments follow...

>Add xtables version defines.
>
>Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
>
>--- iptables-1.4.1-rc2/configure.ac	Mon May 26 14:23:58 2008
>+++ iptables.version/configure.ac	Mon Jun  2 15:05:57 2008
>@@ -1,5 +1,11 @@
>+define([_XTABLES_VERSION_MAJOR], 1)
>+define([_XTABLES_VERSION_MINOR], 4)
>+define([_XTABLES_VERSION_PATCH], 1)
>+define([_XTABLES_VERSION_EXTRA], -rc2)
> 
>-AC_INIT([iptables], [1.4.1-rc2])
>+define([_XTABLES_VERSION],_XTABLES_VERSION_MAJOR._XTABLES_VERSION_MINOR._XTABLES_VERSION_PATCH[]_XTABLES_VERSION_EXTRA)
>+
>+AC_INIT([iptables], _XTABLES_VERSION)
> AC_CONFIG_HEADERS([config.h])
> AC_PROG_INSTALL
> AM_INIT_AUTOMAKE
>@@ -56,4 +62,14 @@
> AC_SUBST([kbuilddir])
> AC_SUBST([ksourcedir])
> AC_SUBST([xtlibdir])
>+
>+XTABLES_VERSION_MAJOR=_XTABLES_VERSION_MAJOR
>+XTABLES_VERSION_MINOR=_XTABLES_VERSION_MINOR
>+XTABLES_VERSION_PATCH=_XTABLES_VERSION_PATCH
>+XTABLES_VERSION_EXTRA=_XTABLES_VERSION_EXTRA
>+AC_SUBST([XTABLES_VERSION_MAJOR])
>+AC_SUBST([XTABLES_VERSION_MINOR])
>+AC_SUBST([XTABLES_VERSION_PATCH])
>+AC_SUBST([XTABLES_VERSION_EXTRA])
>+
> AC_OUTPUT([Makefile extensions/GNUmakefile libipq/Makefile include/xtables.h])
>--- iptables-1.4.1-rc2/include/xtables.h.in	Mon May 26 14:15:40 2008
>+++ iptables.version/include/xtables.h.in	Mon Jun  2 15:03:46 2008
>@@ -18,6 +18,12 @@
> #endif
> 
> #define XTABLES_VERSION "@PACKAGE_VERSION@"
>+#define XTABLES_VERSION_CODE (0x10000 * @XTABLES_VERSION_MAJOR@ + 0x100 * @XTABLES_VERSION_MINOR@ + @XTABLES_VERSION_PATCH@)
>+
>+#define XTABLES_VERSION_CHECK(x,y,z)    (0x10000*(x) + 0x100*(y) + z)

Mh.... too much arithmetic for my taste, just use bitops like
linux/version.h. Also, regarding the _CHECK name, I'd propose
"XTABLES_API_VERSION" instead, which should look nicer on
	#if XTABLES_VERSION_CODE < XTABLES_API_VERSION(1,4,0)
	#error Upgrade, yo!
	#endif


#define XTABLES_API_VERSION(a,b,c) (((a) << 16) | ((b) << 8) | (c))


>+#define XTABLES_VERSION_MAJOR(x)  (((x)>>16) & 0xFF)
>+#define XTABLES_VERSION_MINOR(x)  (((x)>> 8) & 0xFF)
>+#define XTABLES_VERSION_PATCH(x)  ( (x)      & 0xFF)

I do not see a need for these three. The linux kernel does not
have such either, so please don't overdesign :)

Re: [patch] iptables version defines

[Thomas Jarosch]

[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]

Jan,

On Monday, 2. June 2008 16:54:57 you wrote:
> > #define XTABLES_VERSION "@PACKAGE_VERSION@"
> >+#define XTABLES_VERSION_CODE (0x10000 * @XTABLES_VERSION_MAJOR@ + 0x100 *
> > @XTABLES_VERSION_MINOR@ + @XTABLES_VERSION_PATCH@) +
> >+#define XTABLES_VERSION_CHECK(x,y,z)    (0x10000*(x) + 0x100*(y) + z)
>
> Mh.... too much arithmetic for my taste, just use bitops like
> linux/version.h. Also, regarding the _CHECK name, I'd propose
> "XTABLES_API_VERSION" instead, which should look nicer on
> 	#if XTABLES_VERSION_CODE < XTABLES_API_VERSION(1,4,0)
> 	#error Upgrade, yo!
> 	#endif
>
>
> #define XTABLES_API_VERSION(a,b,c) (((a) << 16) | ((b) << 8) | (c))

As you already pointed out, it's a matter of taste and neither
of both versions will hurt as it will be expanded at compile time.

Infact, imagine we would add another version level like "(x) << 32",
on x86 it is only valid to do a left shift operation for 0-31 bits
and so it could fail...

> >+#define XTABLES_VERSION_MAJOR(x)  (((x)>>16) & 0xFF)
> >+#define XTABLES_VERSION_MINOR(x)  (((x)>> 8) & 0xFF)
> >+#define XTABLES_VERSION_PATCH(x)  ( (x)      & 0xFF)
>
> I do not see a need for these three. The linux kernel does not
> have such either, so please don't overdesign :)

Well, I've created the same macros as there are already in include/iptables.h.

I'm alright with your proposed change to XTABLES_API_VERSION
and the drop of _MAJOR, _MINOR and _PATCH macros as
one can easily check for same thing via "xyz < XTABLES_API_VERSION(2,6,0)"

Attached is an updated patch.

Thomas

[-- Attachment #2: iptables-add-version.patch --]
[-- Type: text/x-diff, Size: 1792 bytes --]

Add xtables version defines.

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>

diff -u -r iptables-1.4.1-rc2/configure.ac iptables.version/configure.ac
--- iptables-1.4.1-rc2/configure.ac	Mon May 26 14:23:58 2008
+++ iptables.version/configure.ac	Mon Jun  2 17:19:42 2008
@@ -1,5 +1,11 @@
+define([_XTABLES_VERSION_MAJOR], 1)
+define([_XTABLES_VERSION_MINOR], 4)
+define([_XTABLES_VERSION_PATCH], 1)
+define([_XTABLES_VERSION_EXTRA], -rc2)
 
-AC_INIT([iptables], [1.4.1-rc2])
+define([_XTABLES_VERSION],_XTABLES_VERSION_MAJOR._XTABLES_VERSION_MINOR._XTABLES_VERSION_PATCH[]_XTABLES_VERSION_EXTRA)
+
+AC_INIT([iptables], _XTABLES_VERSION)
 AC_CONFIG_HEADERS([config.h])
 AC_PROG_INSTALL
 AM_INIT_AUTOMAKE
@@ -56,4 +62,14 @@
 AC_SUBST([kbuilddir])
 AC_SUBST([ksourcedir])
 AC_SUBST([xtlibdir])
+
+XTABLES_VERSION_MAJOR=_XTABLES_VERSION_MAJOR
+XTABLES_VERSION_MINOR=_XTABLES_VERSION_MINOR
+XTABLES_VERSION_PATCH=_XTABLES_VERSION_PATCH
+XTABLES_VERSION_EXTRA=_XTABLES_VERSION_EXTRA
+AC_SUBST([XTABLES_VERSION_MAJOR])
+AC_SUBST([XTABLES_VERSION_MINOR])
+AC_SUBST([XTABLES_VERSION_PATCH])
+AC_SUBST([XTABLES_VERSION_EXTRA])
+
 AC_OUTPUT([Makefile extensions/GNUmakefile libipq/Makefile include/xtables.h])
diff -u -r iptables-1.4.1-rc2/include/xtables.h.in iptables.version/include/xtables.h.in
--- iptables-1.4.1-rc2/include/xtables.h.in	Mon May 26 14:15:40 2008
+++ iptables.version/include/xtables.h.in	Mon Jun  2 17:20:55 2008
@@ -18,6 +18,9 @@
 #endif
 
 #define XTABLES_VERSION "@PACKAGE_VERSION@"
+#define XTABLES_VERSION_CODE (0x10000 * @XTABLES_VERSION_MAJOR@ + 0x100 * @XTABLES_VERSION_MINOR@ + @XTABLES_VERSION_PATCH@)
+
+#define XTABLES_API_VERSION(x,y,z)    (0x10000*(x) + 0x100*(y) + z)
 
 /* Include file for additions: new matches and targets. */
 struct xtables_match

Re: [patch] iptables version defines

[Jan Engelhardt]

On Monday 2008-06-02 17:32, Thomas Jarosch wrote:
>Jan,
>>
>> #define XTABLES_API_VERSION(a,b,c) (((a) << 16) | ((b) << 8) | (c))
>
>As you already pointed out, it's a matter of taste and neither
>of both versions will hurt as it will be expanded at compile time.
>
>Infact, imagine we would add another version level like "(x) << 32",
>on x86 it is only valid to do a left shift operation for 0-31 bits
>and so it could fail...

First, you would use << 24 not << 32 :-)
Even so, << 32 is perfectly valid if you correctly declare it as a
unsigned long long.

>> >+#define XTABLES_VERSION_MAJOR(x)  (((x)>>16) & 0xFF)
>> >+#define XTABLES_VERSION_MINOR(x)  (((x)>> 8) & 0xFF)
>> >+#define XTABLES_VERSION_PATCH(x)  ( (x)      & 0xFF)
>>
>> I do not see a need for these three. The linux kernel does not
>> have such either, so please don't overdesign :)
>
>Well, I've created the same macros as there are already in include/iptables.h.

That's just leftover, they are not used either.

>I'm alright with your proposed change to XTABLES_API_VERSION
>and the drop of _MAJOR, _MINOR and _PATCH macros as
>one can easily check for same thing via "xyz < XTABLES_API_VERSION(2,6,0)"
>
>Attached is an updated patch.

I approve.

Re: [patch] iptables version defines

[Patrick McHardy]

Thomas Jarosch wrote:
> I'm alright with your proposed change to XTABLES_API_VERSION
> and the drop of _MAJOR, _MINOR and _PATCH macros as
> one can easily check for same thing via "xyz < XTABLES_API_VERSION(2,6,0)"
> 
> Attached is an updated patch.

Applied and pushed out, thanks.