| Message ID | 1468512050-26272-1-git-send-email-javier@osg.samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hello Casey, On 07/14/2016 12:17 PM, Casey Schaufler wrote: > On 7/14/2016 9:00 AM, Javier Martinez Canillas wrote: >> The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either >> built-in or as a module, use that macro instead of open coding the same. > > Why? > Why not? We have a macro for this so why is better to open coding it? Best regards,
On 7/14/2016 9:20 AM, Javier Martinez Canillas wrote: > Hello Casey, > > On 07/14/2016 12:17 PM, Casey Schaufler wrote: >> On 7/14/2016 9:00 AM, Javier Martinez Canillas wrote: >>> The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either >>> built-in or as a module, use that macro instead of open coding the same. >> Why? >> > Why not? We have a macro for this so why is better to open coding it? Unless there is a real advantage to IS_ENABLED() over ifdef there is no value in making the change. Any change can introduce a problem, so we don't make changes based on "why not". It's called code churn. > > Best regards,
On Thu, Jul 14, 2016 at 12:30 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 7/14/2016 9:20 AM, Javier Martinez Canillas wrote: >> Hello Casey, >> >> On 07/14/2016 12:17 PM, Casey Schaufler wrote: >>> On 7/14/2016 9:00 AM, Javier Martinez Canillas wrote: >>>> The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either >>>> built-in or as a module, use that macro instead of open coding the same. >>> Why? >> >> Why not? We have a macro for this so why is better to open coding it? > > Unless there is a real advantage to IS_ENABLED() over ifdef there > is no value in making the change. Any change can introduce a problem, > so we don't make changes based on "why not". It's called code churn. I think the IS_ENABLED() macro makes the code more readable by helping abstract away some of the Kconfig/module details; not to mention it provides some insulation from Kconfig changes (although I suppose it is doubtful this will be a real issue anytime soon). Javier, if you want to respin this patch without the Smack changes I'll merge it into the SELinux tree (not for the v4.8 merge window, but for the next merge window). However, if Casey changes his mind and ACKs this patch, I'll go ahead and merge the original patch.
On 7/14/2016 12:57 PM, Paul Moore wrote: > On Thu, Jul 14, 2016 at 12:30 PM, Casey Schaufler > <casey@schaufler-ca.com> wrote: >> On 7/14/2016 9:20 AM, Javier Martinez Canillas wrote: >>> Hello Casey, >>> >>> On 07/14/2016 12:17 PM, Casey Schaufler wrote: >>>> On 7/14/2016 9:00 AM, Javier Martinez Canillas wrote: >>>>> The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either >>>>> built-in or as a module, use that macro instead of open coding the same. >>>> Why? >>> Why not? We have a macro for this so why is better to open coding it? >> Unless there is a real advantage to IS_ENABLED() over ifdef there >> is no value in making the change. Any change can introduce a problem, >> so we don't make changes based on "why not". It's called code churn. > I think the IS_ENABLED() macro makes the code more readable by helping > abstract away some of the Kconfig/module details; not to mention it > provides some insulation from Kconfig changes (although I suppose it > is doubtful this will be a real issue anytime soon). > > Javier, if you want to respin this patch without the Smack changes > I'll merge it into the SELinux tree (not for the v4.8 merge window, > but for the next merge window). However, if Casey changes his mind > and ACKs this patch, I'll go ahead and merge the original patch. Don't let me stand in the way. If you think it's worth doing go ahead and add my ACK.
On Thu, Jul 14, 2016 at 4:54 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 7/14/2016 12:57 PM, Paul Moore wrote: >> On Thu, Jul 14, 2016 at 12:30 PM, Casey Schaufler >> <casey@schaufler-ca.com> wrote: >>> On 7/14/2016 9:20 AM, Javier Martinez Canillas wrote: >>>> Hello Casey, >>>> >>>> On 07/14/2016 12:17 PM, Casey Schaufler wrote: >>>>> On 7/14/2016 9:00 AM, Javier Martinez Canillas wrote: >>>>>> The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either >>>>>> built-in or as a module, use that macro instead of open coding the same. >>>>> Why? >>>> >>>> Why not? We have a macro for this so why is better to open coding it? >>> >>> Unless there is a real advantage to IS_ENABLED() over ifdef there >>> is no value in making the change. Any change can introduce a problem, >>> so we don't make changes based on "why not". It's called code churn. >> >> I think the IS_ENABLED() macro makes the code more readable by helping >> abstract away some of the Kconfig/module details; not to mention it >> provides some insulation from Kconfig changes (although I suppose it >> is doubtful this will be a real issue anytime soon). >> >> Javier, if you want to respin this patch without the Smack changes >> I'll merge it into the SELinux tree (not for the v4.8 merge window, >> but for the next merge window). However, if Casey changes his mind >> and ACKs this patch, I'll go ahead and merge the original patch. > > Don't let me stand in the way. If you think it's worth > doing go ahead and add my ACK. I think it's a reasonable patch and I'm at that point in the day where I'm looking for distractions so I just added it to the selinux#next queue; once the merge window closes I'll rotate this into my next branch.
Hello Paul, On 07/14/2016 06:06 PM, Paul Moore wrote: [snip] > > I think it's a reasonable patch and I'm at that point in the day where > I'm looking for distractions so I just added it to the selinux#next > queue; once the merge window closes I'll rotate this into my next > branch. > Great, thanks a lot for your help. Best regards,
diff --git a/security/lsm_audit.c b/security/lsm_audit.c index cccbf3068cdc..5369036cf905 100644 --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -99,7 +99,7 @@ int ipv4_skb_to_auditdata(struct sk_buff *skb, } return ret; } -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) /** * ipv6_skb_to_auditdata : fill auditdata from skb * @skb : the skb diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index ec30880c4b98..c20ea9fe9274 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3984,7 +3984,7 @@ out: return ret; } -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) /* Returns error only if unable to parse addresses */ static int selinux_parse_skb_ipv6(struct sk_buff *skb, @@ -4075,7 +4075,7 @@ static int selinux_parse_skb(struct sk_buff *skb, struct common_audit_data *ad, &ad->u.net->v4info.daddr); goto okay; -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) case PF_INET6: ret = selinux_parse_skb_ipv6(skb, ad, proto); if (ret) @@ -5029,7 +5029,7 @@ static unsigned int selinux_ipv4_forward(void *priv, return selinux_ip_forward(skb, state->in, PF_INET); } -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) static unsigned int selinux_ipv6_forward(void *priv, struct sk_buff *skb, const struct nf_hook_state *state) @@ -5087,7 +5087,7 @@ static unsigned int selinux_ipv4_output(void *priv, return selinux_ip_output(skb, PF_INET); } -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) static unsigned int selinux_ipv6_output(void *priv, struct sk_buff *skb, const struct nf_hook_state *state) @@ -5273,7 +5273,7 @@ static unsigned int selinux_ipv4_postroute(void *priv, return selinux_ip_postroute(skb, state->out, PF_INET); } -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) static unsigned int selinux_ipv6_postroute(void *priv, struct sk_buff *skb, const struct nf_hook_state *state) @@ -6317,7 +6317,7 @@ static struct nf_hook_ops selinux_nf_ops[] = { .hooknum = NF_INET_LOCAL_OUT, .priority = NF_IP_PRI_SELINUX_FIRST, }, -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) { .hook = selinux_ipv6_postroute, .pf = NFPROTO_IPV6, diff --git a/security/smack/smack_netfilter.c b/security/smack/smack_netfilter.c index aa6bf1b22ec5..205b785fb400 100644 --- a/security/smack/smack_netfilter.c +++ b/security/smack/smack_netfilter.c @@ -20,7 +20,7 @@ #include <net/inet_sock.h> #include "smack.h" -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) static unsigned int smack_ipv6_output(void *priv, struct sk_buff *skb, @@ -64,7 +64,7 @@ static struct nf_hook_ops smack_nf_ops[] = { .hooknum = NF_INET_LOCAL_OUT, .priority = NF_IP_PRI_SELINUX_FIRST, }, -#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +#if IS_ENABLED(CONFIG_IPV6) { .hook = smack_ipv6_output, .pf = NFPROTO_IPV6,
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> --- security/lsm_audit.c | 2 +- security/selinux/hooks.c | 12 ++++++------ security/smack/smack_netfilter.c | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-)