| Message ID | 1447172300-10831-1-git-send-email-pshelar@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 10 November 2015 at 08:18, Pravin B Shelar <pshelar@nicira.com> wrote: > Remove unnecessary check and definition of inet_get_local_port_range. > It is already there in ip.h > > Reported-by: Joe Stringer <joestringer@nicira.com> > Signed-off-by: Pravin B Shelar <pshelar@nicira.com> Did you try this on RHEL7? Our net/ip.h defines a 3-argument version of the function. But RHEL7 net/udp.h uses the 2-argument version. So, we need to make sure that we don't redefine it while including net/udp.h. AFAICT, currently any files that include net/udp.h also include net/ip.h first so this logic works correctly. I suspect that the lwtunnel backport introduced something which did not include net/ip.h first, so the #undef logic is a no-op and the net/udp.h header would fail. The patch that I sent would always include net/ip.h first, so this logic would trigger and undefine our backport for the include of net/udp.h, then redefine it once that was done.
On Wed, Nov 11, 2015 at 12:37 AM, Joe Stringer <joestringer@nicira.com> wrote: > On 10 November 2015 at 08:18, Pravin B Shelar <pshelar@nicira.com> wrote: >> Remove unnecessary check and definition of inet_get_local_port_range. >> It is already there in ip.h >> >> Reported-by: Joe Stringer <joestringer@nicira.com> >> Signed-off-by: Pravin B Shelar <pshelar@nicira.com> > > Did you try this on RHEL7? > Yes, I tried it on multiple kernels - 3.10.0-229.20.1.el7, 3.10.0-229.14.1.el7 and 3.10.0-123.el7. > Our net/ip.h defines a 3-argument version of the function. But RHEL7 > net/udp.h uses the 2-argument version. So, we need to make sure that > we don't redefine it while including net/udp.h. > > AFAICT, currently any files that include net/udp.h also include > net/ip.h first so this logic works correctly. I suspect that the > lwtunnel backport introduced something which did not include net/ip.h > first, so the #undef logic is a no-op and the net/udp.h header would > fail. The patch that I sent would always include net/ip.h first, so > this logic would trigger and undefine our backport for the include of > net/udp.h, then redefine it once that was done. I do not see it with kernel that I used. This issue exist only on few RHEL7 releases. But it is simple fix to keep support for the kernel so I am fine keeping in the code. Can you send the patch you have posted on github?
On 11 November 2015 at 08:00, Pravin Shelar <pshelar@nicira.com> wrote: > On Wed, Nov 11, 2015 at 12:37 AM, Joe Stringer <joestringer@nicira.com> wrote: >> On 10 November 2015 at 08:18, Pravin B Shelar <pshelar@nicira.com> wrote: >>> Remove unnecessary check and definition of inet_get_local_port_range. >>> It is already there in ip.h >>> >>> Reported-by: Joe Stringer <joestringer@nicira.com> >>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com> >> >> Did you try this on RHEL7? >> > Yes, I tried it on multiple kernels - 3.10.0-229.20.1.el7, > 3.10.0-229.14.1.el7 and 3.10.0-123.el7. Okay, thanks for the details. I was trying on kernel 3.10.0-229.7.2.el7.x86_64. I guess that a later minor point release changed/removed this function in the udp header. >> Our net/ip.h defines a 3-argument version of the function. But RHEL7 >> net/udp.h uses the 2-argument version. So, we need to make sure that >> we don't redefine it while including net/udp.h. >> >> AFAICT, currently any files that include net/udp.h also include >> net/ip.h first so this logic works correctly. I suspect that the >> lwtunnel backport introduced something which did not include net/ip.h >> first, so the #undef logic is a no-op and the net/udp.h header would >> fail. The patch that I sent would always include net/ip.h first, so >> this logic would trigger and undefine our backport for the include of >> net/udp.h, then redefine it once that was done. > > I do not see it with kernel that I used. This issue exist only on few > RHEL7 releases. But it is simple fix to keep support for the kernel so > I am fine keeping in the code. Can you send the patch you have posted > on github? Sure, I'll send the patch that I previously posted, and probably also amend the comment to give a more specific detail on which RHEL version depends on that logic.
diff --git a/datapath/linux/compat/include/net/udp.h b/datapath/linux/compat/include/net/udp.h index fcb8f6a..d078b79 100644 --- a/datapath/linux/compat/include/net/udp.h +++ b/datapath/linux/compat/include/net/udp.h @@ -1,17 +1,7 @@ #ifndef __NET_UDP_WRAPPER_H #define __NET_UDP_WRAPPER_H 1 -#include <linux/version.h> - -#ifdef inet_get_local_port_range -/* RHEL7 backports udp_flow_src_port() using an older version of - * inet_get_local_port_range(). */ -#undef inet_get_local_port_range -#include_next <net/udp.h> -#define inet_get_local_port_range rpl_inet_get_local_port_range -#else #include_next <net/udp.h> -#endif #ifndef HAVE_UDP_FLOW_SRC_PORT static inline __be16 rpl_udp_flow_src_port(struct net *net, struct sk_buff *skb,
Remove unnecessary check and definition of inet_get_local_port_range. It is already there in ip.h Reported-by: Joe Stringer <joestringer@nicira.com> Signed-off-by: Pravin B Shelar <pshelar@nicira.com> --- datapath/linux/compat/include/net/udp.h | 10 ---------- 1 files changed, 0 insertions(+), 10 deletions(-)