| Message ID | 20201115224509.2020651-1-flokli@flokli.de |
|---|---|
| State | New |
| Headers | show |
| Series | ipv4: use IS_ENABLED instead of ifdef | expand |
On Sun, 15 Nov 2020 23:45:09 +0100 Florian Klink wrote: > Checking for ifdef CONFIG_x fails if CONFIG_x=m. > > Use IS_ENABLED instead, which is true for both built-ins and modules. > > Otherwise, a > > ip -4 route add 1.2.3.4/32 via inet6 fe80::2 dev eth1 > fails with the message "Error: IPv6 support not enabled in kernel." if > CONFIG_IPV6 is `m`. > > In the spirit of b8127113d01e53adba15b41aefd37b90ed83d631. > > Cc: Kim Phillips <kim.phillips@arm.com> > Signed-off-by: Florian Klink <flokli@flokli.de> LGTM, this is the fixes tag right? Fixes: d15662682db2 ("ipv4: Allow ipv6 gateway with ipv4 routes") CCing David to give him a chance to ack. > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c > index 86a23e4a6a50..b87140a1fa28 100644 > --- a/net/ipv4/fib_frontend.c > +++ b/net/ipv4/fib_frontend.c > @@ -696,7 +696,7 @@ int fib_gw_from_via(struct fib_config *cfg, struct nlattr *nla, > cfg->fc_gw4 = *((__be32 *)via->rtvia_addr); > break; > case AF_INET6: > -#ifdef CONFIG_IPV6 > +#if IS_ENABLED(CONFIG_IPV6) > if (alen != sizeof(struct in6_addr)) { > NL_SET_ERR_MSG(extack, "Invalid IPv6 address in RTA_VIA"); > return -EINVAL;
On 11/17/20 5:01 PM, Jakub Kicinski wrote: > On Sun, 15 Nov 2020 23:45:09 +0100 Florian Klink wrote: >> Checking for ifdef CONFIG_x fails if CONFIG_x=m. >> >> Use IS_ENABLED instead, which is true for both built-ins and modules. >> >> Otherwise, a >>> ip -4 route add 1.2.3.4/32 via inet6 fe80::2 dev eth1 >> fails with the message "Error: IPv6 support not enabled in kernel." if >> CONFIG_IPV6 is `m`. >> >> In the spirit of b8127113d01e53adba15b41aefd37b90ed83d631. >> >> Cc: Kim Phillips <kim.phillips@arm.com> >> Signed-off-by: Florian Klink <flokli@flokli.de> > > LGTM, this is the fixes tag right? > > Fixes: d15662682db2 ("ipv4: Allow ipv6 gateway with ipv4 routes") yep. > > CCing David to give him a chance to ack. Reviewed-by: David Ahern <dsahern@kernel.org> I looked at this yesterday and got distracted diving into the generated file to see the difference: #define CONFIG_IPV6 1 vs #define CONFIG_IPV6_MODULE 1
On Tue, 17 Nov 2020 17:55:54 -0700 David Ahern wrote: > On 11/17/20 5:01 PM, Jakub Kicinski wrote: > > On Sun, 15 Nov 2020 23:45:09 +0100 Florian Klink wrote: > >> Checking for ifdef CONFIG_x fails if CONFIG_x=m. > >> > >> Use IS_ENABLED instead, which is true for both built-ins and modules. > >> > >> Otherwise, a > >>> ip -4 route add 1.2.3.4/32 via inet6 fe80::2 dev eth1 > >> fails with the message "Error: IPv6 support not enabled in kernel." if > >> CONFIG_IPV6 is `m`. > >> > >> In the spirit of b8127113d01e53adba15b41aefd37b90ed83d631. > >> > >> Cc: Kim Phillips <kim.phillips@arm.com> > >> Signed-off-by: Florian Klink <flokli@flokli.de> > > > > LGTM, this is the fixes tag right? > > > > Fixes: d15662682db2 ("ipv4: Allow ipv6 gateway with ipv4 routes") > > yep. > > > > > CCing David to give him a chance to ack. > > Reviewed-by: David Ahern <dsahern@kernel.org> Great, applied, thanks! > I looked at this yesterday and got distracted diving into the generated > file to see the difference: > > #define CONFIG_IPV6 1 > > vs > > #define CONFIG_IPV6_MODULE 1 Interesting. drivers/net/ethernet/netronome/nfp/flower/action.c:#ifdef CONFIG_IPV6 Oops.
On 11/17/20 5:07 PM, Jakub Kicinski wrote: > On Tue, 17 Nov 2020 17:55:54 -0700 David Ahern wrote: >> On 11/17/20 5:01 PM, Jakub Kicinski wrote: >>> On Sun, 15 Nov 2020 23:45:09 +0100 Florian Klink wrote: >>>> Checking for ifdef CONFIG_x fails if CONFIG_x=m. >>>> >>>> Use IS_ENABLED instead, which is true for both built-ins and modules. >>>> >>>> Otherwise, a >>>>> ip -4 route add 1.2.3.4/32 via inet6 fe80::2 dev eth1 >>>> fails with the message "Error: IPv6 support not enabled in kernel." if >>>> CONFIG_IPV6 is `m`. >>>> >>>> In the spirit of b8127113d01e53adba15b41aefd37b90ed83d631. >>>> >>>> Cc: Kim Phillips <kim.phillips@arm.com> >>>> Signed-off-by: Florian Klink <flokli@flokli.de> >>> >>> LGTM, this is the fixes tag right? >>> >>> Fixes: d15662682db2 ("ipv4: Allow ipv6 gateway with ipv4 routes") >> >> yep. >> >>> >>> CCing David to give him a chance to ack. >> >> Reviewed-by: David Ahern <dsahern@kernel.org> > > Great, applied, thanks! > >> I looked at this yesterday and got distracted diving into the generated >> file to see the difference: >> >> #define CONFIG_IPV6 1 >> >> vs >> >> #define CONFIG_IPV6_MODULE 1 Digging up ancient history. ;) > Interesting. > > drivers/net/ethernet/netronome/nfp/flower/action.c:#ifdef CONFIG_IPV6 > > Oops. Notify the maintainer! -- ~Randy
>>> I looked at this yesterday and got distracted diving into the generated >>> file to see the difference: >>> >>> #define CONFIG_IPV6 1 >>> >>> vs >>> >>> #define CONFIG_IPV6_MODULE 1 > >Digging up ancient history. ;) > >> Interesting. >> >> drivers/net/ethernet/netronome/nfp/flower/action.c:#ifdef CONFIG_IPV6 >> >> Oops. > >Notify the maintainer! Yeah, this is super scary stuff - allmodyes-like configs are quite common in generic distros, and I assume similar mistakes could have happened in many other places in the kernel as well. I wonder if the "ifdef CONFIG_…" pattern should be discouraged, and "IS_ENABLED(CONFIG_…)" used instead, at least for all tristate config options. Florian
On Tue, 17 Nov 2020 19:09:16 -0800 Randy Dunlap wrote: > On 11/17/20 5:07 PM, Jakub Kicinski wrote: > > On Tue, 17 Nov 2020 17:55:54 -0700 David Ahern wrote: > >> On 11/17/20 5:01 PM, Jakub Kicinski wrote: > >>> On Sun, 15 Nov 2020 23:45:09 +0100 Florian Klink wrote: > >>>> Checking for ifdef CONFIG_x fails if CONFIG_x=m. > >>>> > >>>> Use IS_ENABLED instead, which is true for both built-ins and modules. > >>>> > >>>> Otherwise, a > >>>>> ip -4 route add 1.2.3.4/32 via inet6 fe80::2 dev eth1 > >>>> fails with the message "Error: IPv6 support not enabled in kernel." if > >>>> CONFIG_IPV6 is `m`. > >>>> > >>>> In the spirit of b8127113d01e53adba15b41aefd37b90ed83d631. > >>>> > >>>> Cc: Kim Phillips <kim.phillips@arm.com> > >>>> Signed-off-by: Florian Klink <flokli@flokli.de> > >>> > >>> LGTM, this is the fixes tag right? > >>> > >>> Fixes: d15662682db2 ("ipv4: Allow ipv6 gateway with ipv4 routes") > >> > >> yep. > >> > >>> > >>> CCing David to give him a chance to ack. > >> > >> Reviewed-by: David Ahern <dsahern@kernel.org> > > > > Great, applied, thanks! > > > >> I looked at this yesterday and got distracted diving into the generated > >> file to see the difference: > >> > >> #define CONFIG_IPV6 1 > >> > >> vs > >> > >> #define CONFIG_IPV6_MODULE 1 > > Digging up ancient history. ;) > > > Interesting. > > > > drivers/net/ethernet/netronome/nfp/flower/action.c:#ifdef CONFIG_IPV6 > > > > Oops. > > Notify the maintainer! The joke was that I was the maintainer when it was added ;) CCing Simon
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c index 86a23e4a6a50..b87140a1fa28 100644 --- a/net/ipv4/fib_frontend.c +++ b/net/ipv4/fib_frontend.c @@ -696,7 +696,7 @@ int fib_gw_from_via(struct fib_config *cfg, struct nlattr *nla, cfg->fc_gw4 = *((__be32 *)via->rtvia_addr); break; case AF_INET6: -#ifdef CONFIG_IPV6 +#if IS_ENABLED(CONFIG_IPV6) if (alen != sizeof(struct in6_addr)) { NL_SET_ERR_MSG(extack, "Invalid IPv6 address in RTA_VIA"); return -EINVAL;