diff mbox

[IPROUTE,v2] iproute2: act_ipt fix xtables breakage on older versions.

Message ID 20130429154644.52016.60284.stgit@ahduyck-cp1.jf.intel.com
State Accepted, archived
Delegated to: stephen hemminger
Headers show

Commit Message

Duyck, Alexander H April 29, 2013, 3:50 p.m. UTC 
In trying to build on a RHEL6.3 I ran into several build issues that are
addressed in this patch.

The first is that xtables_merge_options only has 3 parameters.  It appears
this is how this code was originally.  As such for the case where the version
is less than 6 I am assuming it would be correct to maintain the original
setup that only had 3 parameters being passed instead of 4.

I also ran into an issue with the define for __ALIGN_KERNEL not being present.
I believe this may be due to the fact that __ALIGN_KERNEL was moved into a
separate header from ALIGN after the UAPI changes.  In order to just cover all
of the bases I have moved the main definition for the macros into
__ALIGN_KERNEL_MASK and __ALIGN_KERNEL and if ALIGN is also needed then it is
just a direct redefine to __ALIGN_KERNEL.

Cc: Hasan Chowdhury <shemonc@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

v2: Correct check-in comment that referred to xtables_options_xfrm when it
    was actually xtables_merge_options that I was fixing the references to.
    Fixed capitalization of UAPI in check-in comment.

 tc/m_xt.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)


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

Comments

Jamal Hadi Salim April 30, 2013, 12:59 p.m. UTC | #1 
Hi Alex,
Theres still some confusion on my part.

On 13-04-29 11:50 AM, Alexander Duyck wrote:

> The first is that xtables_merge_options only has 3 parameters.  It appears
> this is how this code was originally.

what you are describing above is still not making sense:

  As such for the case where the version
> is less than 6 I am assuming it would be correct to maintain the original
> setup that only had 3 parameters being passed instead of 4.
>

More below, lets just pick one of those:

> @@ -335,8 +338,7 @@ print_ipt(struct action_util *au,FILE * f, struct rtattr *arg)
>   					    m->x6_options,
>   					    &m->option_offset);
>   #else
> -		opts = xtables_merge_options(tcipt_globals.orig_opts,
> -					     tcipt_globals.opts,
> +		opts = xtables_merge_options(tcipt_globals.opts,
>   					     m->extra_opts,
>   					     &m->option_offset);
>   #endif
>


Here's the original equivalent change that you are updating:
-------
-                       tcipt_globals.opts =
-                           xtables_merge_options(
  #if (XTABLES_VERSION_CODE >= 6)
-                                                 tcipt_globals.orig_opts,
+               opts = xtables_options_xfrm(tcipt_globals.orig_opts,
+                                           tcipt_globals.opts,
+                                           m->x6_options,
+                                           &m->option_offset);
+#else
+               opts = xtables_merge_options(tcipt_globals.orig_opts,
+                                            tcipt_globals.opts,
+                                            m->extra_opts,
+                                            &m->option_offset);
  #endif
-                                                 tcipt_globals.opts,
-                                                 m->extra_opts,
-                                                 &m->option_offset);
------------------

I see, originally these parameters for xtables_merge_options():
tcipt_globals.orig_opts  <---- This is what you are trying to kill
tcipt_globals.opts
m->extra_opts
&m->option_offset

Which says there were 4 parameters to begin with....
Unless you are saying we had it wrong the first time. Pablo?

cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duyck, Alexander H April 30, 2013, 4:07 p.m. UTC | #2 
On 04/30/2013 05:59 AM, Jamal Hadi Salim wrote:
>
> Hi Alex,
> Theres still some confusion on my part.
>
> On 13-04-29 11:50 AM, Alexander Duyck wrote:
>
>> The first is that xtables_merge_options only has 3 parameters.  It
>> appears
>> this is how this code was originally.
>
> what you are describing above is still not making sense:
>
>  As such for the case where the version
>> is less than 6 I am assuming it would be correct to maintain the
>> original
>> setup that only had 3 parameters being passed instead of 4.
>>
>
> More below, lets just pick one of those:
>
>> @@ -335,8 +338,7 @@ print_ipt(struct action_util *au,FILE * f, struct
>> rtattr *arg)
>>                           m->x6_options,
>>                           &m->option_offset);
>>   #else
>> -        opts = xtables_merge_options(tcipt_globals.orig_opts,
>> -                         tcipt_globals.opts,
>> +        opts = xtables_merge_options(tcipt_globals.opts,
>>                            m->extra_opts,
>>                            &m->option_offset);
>>   #endif
>>
>
>
> Here's the original equivalent change that you are updating:
> -------
> -                       tcipt_globals.opts =
> -                           xtables_merge_options(
>  #if (XTABLES_VERSION_CODE >= 6)
> -                                                
> tcipt_globals.orig_opts,
> +               opts = xtables_options_xfrm(tcipt_globals.orig_opts,
> +                                           tcipt_globals.opts,
> +                                           m->x6_options,
> +                                           &m->option_offset);
> +#else
> +               opts = xtables_merge_options(tcipt_globals.orig_opts,
> +                                            tcipt_globals.opts,
> +                                            m->extra_opts,
> +                                            &m->option_offset);
>  #endif
> -                                                 tcipt_globals.opts,
> -                                                 m->extra_opts,
> -                                                 &m->option_offset);
> ------------------
>
> I see, originally these parameters for xtables_merge_options():
> tcipt_globals.orig_opts  <---- This is what you are trying to kill
> tcipt_globals.opts
> m->extra_opts
> &m->option_offset
>
> Which says there were 4 parameters to begin with....
> Unless you are saying we had it wrong the first time. Pablo?
>
> cheers,
> jamal

Jamal,

It was 4 parameters for (XTABLES_VERSION_CODE >= 6), for versions prior
to that it was only 3 parameters.  Notice the line that is removing
tcipt_globals.orit_opts was wrapped inside the ifdef while the rest of
the function was originally past the endif.  What the original patch did
is make it so that on 1.4.10 and prior we cannot build.

Just take a look at:
https://git.netfilter.org/iptables/tree/include/xtables.h.in?id=v1.4.10#n222

In the git tree it is obvious that xtables_merge_options only takes 3
parameters.  It is not until v1.4.11 which is XTABLES_VERSION_CODE == 6
that we see xtables_merge_options take 4 parameters.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jamal Hadi Salim May 1, 2013, 1:47 p.m. UTC | #3 
Hi Alex,

On 13-04-30 12:07 PM, Alexander Duyck wrote:
> On 04/30/2013 05:59 AM, Jamal Hadi Salim wrote:


>
> In the git tree it is obvious that xtables_merge_options only takes 3
> parameters.  It is not until v1.4.11 which is XTABLES_VERSION_CODE == 6
> that we see xtables_merge_options take 4 parameters.
>

I apologize, you are correct. So whats incorporated now is wrong
but compiles fine on Debian.
So now i am wondering how the FSCK this thing even compiled.
I am going to test your patch on Debian squeeze, with iptables 1.4.8
(where the current code compiles just fine) and will ack it if all is
good

cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jamal Hadi Salim May 1, 2013, 2:21 p.m. UTC | #4 
On 13-05-01 09:47 AM, Jamal Hadi Salim wrote:
> Hi Alex,
>

> So now i am wondering how the FSCK this thing even compiled.

Mystery solved. Man, this idea of keeping up with the distros and
iptables variants is a little insane.
Debian's setup ends up using m_ipt.c for anything less than iptable
1.4.11. Kudos to the debian iproute maintainer.
Fedora on the other hand seems to going to m_xt.c
(This is why i asked for your Config earlier).
In any case, your patch is correct. So i will go sign off on it;
its just nobody else had to deal with that (or reported it).

Thanks for your patience Alex.

cheers,
jamal

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jamal Hadi Salim May 1, 2013, 2:22 p.m. UTC | #5 
On 13-04-29 11:50 AM, Alexander Duyck wrote:
> In trying to build on a RHEL6.3 I ran into several build issues that are
> addressed in this patch.
>
> The first is that xtables_merge_options only has 3 parameters.  It appears
> this is how this code was originally.  As such for the case where the version
> is less than 6 I am assuming it would be correct to maintain the original
> setup that only had 3 parameters being passed instead of 4.
>
> I also ran into an issue with the define for __ALIGN_KERNEL not being present.
> I believe this may be due to the fact that __ALIGN_KERNEL was moved into a
> separate header from ALIGN after the UAPI changes.  In order to just cover all
> of the bases I have moved the main definition for the macros into
> __ALIGN_KERNEL_MASK and __ALIGN_KERNEL and if ALIGN is also needed then it is
> just a direct redefine to __ALIGN_KERNEL.
>
> Cc: Hasan Chowdhury <shemonc@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>



Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>


cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duyck, Alexander H May 1, 2013, 5:18 p.m. UTC | #6 
On 05/01/2013 07:21 AM, Jamal Hadi Salim wrote:
> On 13-05-01 09:47 AM, Jamal Hadi Salim wrote:
>> Hi Alex,
>>
>
>> So now i am wondering how the FSCK this thing even compiled.
>
> Mystery solved. Man, this idea of keeping up with the distros and
> iptables variants is a little insane.
> Debian's setup ends up using m_ipt.c for anything less than iptable
> 1.4.11. Kudos to the debian iproute maintainer.
> Fedora on the other hand seems to going to m_xt.c
> (This is why i asked for your Config earlier).
> In any case, your patch is correct. So i will go sign off on it;
> its just nobody else had to deal with that (or reported it).
>
> Thanks for your patience Alex.
>
> cheers,
> jamal
>

No problem.  Just glad to have that patch off of my backlog of things to
get upstream.

Thanks,

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

Patch

diff --git a/tc/m_xt.c b/tc/m_xt.c
index 3edf520..e918670 100644
--- a/tc/m_xt.c
+++ b/tc/m_xt.c
@@ -38,9 +38,13 @@ 
 #       define XT_LIB_DIR "/lib/xtables"
 #endif
 
+#ifndef __ALIGN_KERNEL
+#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
+#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+#endif
+
 #ifndef ALIGN
-#define ALIGN(x,a)		__ALIGN_MASK(x,(typeof(x))(a)-1)
-#define __ALIGN_MASK(x,mask)	(((x)+(mask))&~(mask))
+#define ALIGN(x,a)	__ALIGN_KERNEL((x), (a))
 #endif
 
 static const char *tname = "mangle";
@@ -166,8 +170,7 @@  static int parse_ipt(struct action_util *a,int *argc_p,
 						    m->x6_options,
 						    &m->option_offset);
 #else
-			opts = xtables_merge_options(tcipt_globals.orig_opts,
-						     tcipt_globals.opts,
+			opts = xtables_merge_options(tcipt_globals.opts,
 						     m->extra_opts,
 						     &m->option_offset);
 #endif
@@ -335,8 +338,7 @@  print_ipt(struct action_util *au,FILE * f, struct rtattr *arg)
 					    m->x6_options,
 					    &m->option_offset);
 #else
-		opts = xtables_merge_options(tcipt_globals.orig_opts,
-					     tcipt_globals.opts,
+		opts = xtables_merge_options(tcipt_globals.opts,
 					     m->extra_opts,
 					     &m->option_offset);
 #endif