[3/4] printk: use a clever macro

Message ID 1404911056-29064-4-git-send-email-elder@linaro.org
State New
Headers show

Commit Message

Alex Elder July 9, 2014, 1:04 p.m.
Use the IS_ENABLED() macro rather than #ifdef blocks to set certain
global values.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 kernel/printk/printk.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

Comments

Borislav Petkov July 9, 2014, 3:05 p.m. | #1
On Wed, Jul 09, 2014 at 08:04:15AM -0500, Alex Elder wrote:
> Use the IS_ENABLED() macro rather than #ifdef blocks to set certain
> global values.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

That's good - every patch which removes ifdeffery is a good patch. :)

Acked-by: Borislav Petkov <bp@suse.de>
Petr Mladek July 9, 2014, 4:19 p.m. | #2
On Wed 2014-07-09 08:04:15, Alex Elder wrote:
> Use the IS_ENABLED() macro rather than #ifdef blocks to set certain
> global values.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  kernel/printk/printk.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 646146c..6f75e8a 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -453,11 +453,7 @@ static int log_store(int facility, int level,
>  	return msg->text_len;
>  }
>  
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> +int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);

I wondered if IS_ENABLED() is guaranteed to set 1 when enabled. Well, it
does not matter because the variable is used as a boolean.

>  static int syslog_action_restricted(int type)
>  {
> @@ -947,11 +943,7 @@ static inline void boot_delay_msec(int level)
>  }
>  #endif
>  
> -#if defined(CONFIG_PRINTK_TIME)
> -static bool printk_time = 1;
> -#else
> -static bool printk_time;
> -#endif
> +static bool printk_time = IS_ENABLED(CONFIG_PRINTK_TIME);
>  module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
>  
>  static size_t print_time(u64 ts, char *buf)
> -- 
> 1.9.1

Nice clean up.

Reviewed-by: Petr Mladek <pmladek@suse.cz>

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Borislav Petkov July 9, 2014, 4:24 p.m. | #3
On Wed, Jul 09, 2014 at 06:19:46PM +0200, Petr Mládek wrote:
> I wondered if IS_ENABLED() is guaranteed to set 1 when enabled.

See include/linux/kconfig.h
Petr Mladek July 9, 2014, 4:32 p.m. | #4
On Wed 2014-07-09 18:24:04, Borislav Petkov wrote:
> On Wed, Jul 09, 2014 at 06:19:46PM +0200, Petr Mládek wrote:
> > I wondered if IS_ENABLED() is guaranteed to set 1 when enabled.
> 
> See include/linux/kconfig.h

I know that it sets 1 now. I wondered about possible future
changes. Well, it is probably too paranoid. Anyway, any non-zero value
is fine.

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Borislav Petkov July 9, 2014, 4:40 p.m. | #5
On Wed, Jul 09, 2014 at 06:32:05PM +0200, Petr Mládek wrote:
> I know that it sets 1 now. I wondered about possible future changes.
> Well, it is probably too paranoid. Anyway, any non-zero value is fine.

Yes, because they both are used only in a boolean context.
Geert Uytterhoeven July 9, 2014, 5:58 p.m. | #6
Hi Alex,

On Wed, Jul 9, 2014 at 3:04 PM, Alex Elder <elder@linaro.org> wrote:
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -453,11 +453,7 @@ static int log_store(int facility, int level,
>         return msg->text_len;
>  }
>
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> +int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);

Doesn't this move dmesg_restrict from the bss to the data section
in case CONFIG_SECURITY_DMESG_RESTRICT is not enabled, due
to the explicit initialization to zero?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alex Elder July 9, 2014, 6:15 p.m. | #7
On 07/09/2014 12:58 PM, Geert Uytterhoeven wrote:
> Hi Alex,
> 
> On Wed, Jul 9, 2014 at 3:04 PM, Alex Elder <elder@linaro.org> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -453,11 +453,7 @@ static int log_store(int facility, int level,
>>         return msg->text_len;
>>  }
>>
>> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
>> -int dmesg_restrict = 1;
>> -#else
>> -int dmesg_restrict;
>> -#endif
>> +int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);
> 
> Doesn't this move dmesg_restrict from the bss to the data section
> in case CONFIG_SECURITY_DMESG_RESTRICT is not enabled, due
> to the explicit initialization to zero?

I honestly don't know.  Is that even a well-defined behavior?
Couldn't the compiler, noting an explicit 0 initialization,
put it into BSS anyway?

In any case, does this distinction matter?

					-Alex

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch hide | download patch | download mbox

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 646146c..6f75e8a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -453,11 +453,7 @@  static int log_store(int facility, int level,
 	return msg->text_len;
 }
 
-#ifdef CONFIG_SECURITY_DMESG_RESTRICT
-int dmesg_restrict = 1;
-#else
-int dmesg_restrict;
-#endif
+int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);
 
 static int syslog_action_restricted(int type)
 {
@@ -947,11 +943,7 @@  static inline void boot_delay_msec(int level)
 }
 #endif
 
-#if defined(CONFIG_PRINTK_TIME)
-static bool printk_time = 1;
-#else
-static bool printk_time;
-#endif
+static bool printk_time = IS_ENABLED(CONFIG_PRINTK_TIME);
 module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
 
 static size_t print_time(u64 ts, char *buf)