This series makes some minor changes to kernel/printk/printk.c.
I've been poking around that file and will have a few more
substantive changes to propose soon, but I've been carrying
around these little things for a bit and thought I should
just get them out there.
-Alex
This series, based on v3.16-rc4, is available here:
http://git.linaro.org/landing-teams/working/broadcom/kernel.git
Branch review/minor-printk-fixes
Alex Elder (4):
printk: rename DEFAULT_MESSAGE_LOGLEVEL
printk: fix some comments
printk: use a clever macro
printk: miscellaneous cleanups
include/linux/printk.h | 2 +-
kernel/printk/printk.c | 63 ++++++++++++++++++++++----------------------------
2 files changed, 29 insertions(+), 36 deletions(-)
--
1.9.1
This patch contains some small cleanups to kernel/printk/printk.c.
None of them should cause any change in behavior.
- When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
- When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
definition; delete it.
- Pull an assignment out of a conditional expression in console_setup().
- Use isdigit() in console_setup() rather than open coding it.
- In update_console_cmdline(), drop a NUL-termination assignment;
the strlcpy() call that precedes it guarantees it's not needed.
- Simplify some logic in printk_timed_ratelimit().
Signed-off-by: Alex Elder <[email protected]>
---
kernel/printk/printk.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 6f75e8a..909029e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -45,6 +45,7 @@
#include <linux/poll.h>
#include <linux/irq_work.h>
#include <linux/utsname.h>
+#include <linux/ctype.h>
#include <asm/uaccess.h>
@@ -257,7 +258,7 @@ static u64 clear_seq;
static u32 clear_idx;
#define PREFIX_MAX 32
-#define LOG_LINE_MAX 1024 - PREFIX_MAX
+#define LOG_LINE_MAX (1024 - PREFIX_MAX)
/* record buffer */
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
@@ -1794,7 +1795,7 @@ EXPORT_SYMBOL(printk);
#define LOG_LINE_MAX 0
#define PREFIX_MAX 0
-#define LOG_LINE_MAX 0
+
static u64 syslog_seq;
static u32 syslog_idx;
static u64 console_seq;
@@ -1895,7 +1896,8 @@ static int __init console_setup(char *str)
strncpy(buf, str, sizeof(buf) - 1);
}
buf[sizeof(buf) - 1] = 0;
- if ((options = strchr(str, ',')) != NULL)
+ options = strchr(str, ',');
+ if (options)
*(options++) = 0;
#ifdef __sparc__
if (!strcmp(str, "ttya"))
@@ -1904,7 +1906,7 @@ static int __init console_setup(char *str)
strcpy(buf, "ttyS1");
#endif
for (s = buf; *s; s++)
- if ((*s >= '0' && *s <= '9') || *s == ',')
+ if (isdigit(*s) || *s == ',')
break;
idx = simple_strtoul(s, NULL, 10);
*s = 0;
@@ -1943,7 +1945,6 @@ int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, cha
i++, c++)
if (strcmp(c->name, name) == 0 && c->index == idx) {
strlcpy(c->name, name_new, sizeof(c->name));
- c->name[sizeof(c->name) - 1] = 0;
c->options = options;
c->index = idx_new;
return i;
@@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
bool printk_timed_ratelimit(unsigned long *caller_jiffies,
unsigned int interval_msecs)
{
- if (*caller_jiffies == 0
- || !time_in_range(jiffies, *caller_jiffies,
- *caller_jiffies
- + msecs_to_jiffies(interval_msecs))) {
- *caller_jiffies = jiffies;
- return true;
- }
- return false;
+ unsigned long elapsed = jiffies - *caller_jiffies;
+
+ if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
+ return false;
+
+ *caller_jiffies = jiffies;
+ return true;
}
EXPORT_SYMBOL(printk_timed_ratelimit);
--
1.9.1
Use the IS_ENABLED() macro rather than #ifdef blocks to set certain
global values.
Signed-off-by: Alex Elder <[email protected]>
---
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);
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
This patch fixes a few comments that don't accurately describe their
corresponding code. It also fixes some minor typographical errors.
Signed-off-by: Alex Elder <[email protected]>
---
kernel/printk/printk.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ac2b64e..646146c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -113,9 +113,9 @@ static int __down_trylock_console_sem(unsigned long ip)
* This is used for debugging the mess that is the VT code by
* keeping track if we have the console semaphore held. It's
* definitely not the perfect debug tool (we don't know if _WE_
- * hold it are racing, but it helps tracking those weird code
- * path in the console code where we end up in places I want
- * locked without the console sempahore held
+ * hold it and are racing, but it helps tracking those weird code
+ * paths in the console code where we end up in places I want
+ * locked without the console sempahore held).
*/
static int console_locked, console_suspended;
@@ -146,8 +146,8 @@ static int console_may_schedule;
* the overall length of the record.
*
* The heads to the first and last entry in the buffer, as well as the
- * sequence numbers of these both entries are maintained when messages
- * are stored..
+ * sequence numbers of these entries are maintained when messages are
+ * stored.
*
* If the heads indicate available messages, the length in the header
* tells the start next message. A length == 0 for the next message
@@ -344,7 +344,7 @@ static int log_make_free_space(u32 msg_size)
while (log_first_seq < log_next_seq) {
if (logbuf_has_space(msg_size, false))
return 0;
- /* drop old messages until we have enough continuous space */
+ /* drop old messages until we have enough contiguous space */
log_first_idx = log_next(log_first_idx);
log_first_seq++;
}
@@ -1476,7 +1476,7 @@ static struct cont {
struct task_struct *owner; /* task of first print*/
u64 ts_nsec; /* time of first print */
u8 level; /* log level of first message */
- u8 facility; /* log level of first message */
+ u8 facility; /* log facility of first message */
enum log_flags flags; /* prefix, newline flags */
bool flushed:1; /* buffer sealed and committed */
} cont;
@@ -1881,11 +1881,12 @@ static int __add_preferred_console(char *name, int idx, char *options,
return 0;
}
/*
- * Set up a list of consoles. Called from init/main.c
+ * Set up a console. Called via do_early_param() in init/main.c
+ * for each "console=" parameter in the boot command line.
*/
static int __init console_setup(char *str)
{
- char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for index */
+ char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
char *s, *options, *brl_options = NULL;
int idx;
@@ -2045,8 +2046,8 @@ EXPORT_SYMBOL(console_lock);
/**
* console_trylock - try to lock the console system for exclusive use.
*
- * Tried to acquire a lock which guarantees that the caller has
- * exclusive access to the console system and the console_drivers list.
+ * Try to acquire a lock which guarantees that the caller has exclusive
+ * access to the console system and the console_drivers list.
*
* returns 1 on success, and 0 on failure to acquire the lock.
*/
--
1.9.1
This commit:
a8fe19eb kernel/printk: use symbolic defines for console loglevels
makes consistent use of symbolic values for printk() log levels.
The naming scheme used is different from the one used for
DEFAULT_MESSAGE_LOGLEVEL though. Change that symbol name to be
MESSAGE_LOGLEVEL_DEFAULT for consistency.
Note that we don't rename CONFIG_DEFAULT_MESSAGE_LOGLEVEL (to avoid
breaking existing config files that might reference it).
Signed-off-by: Alex Elder <[email protected]>
---
include/linux/printk.h | 2 +-
kernel/printk/printk.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 319ff7e..3d1ccad 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -31,7 +31,7 @@ static inline const char *printk_skip_level(const char *buffer)
}
/* printk's without a loglevel use this.. */
-#define DEFAULT_MESSAGE_LOGLEVEL CONFIG_DEFAULT_MESSAGE_LOGLEVEL
+#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_DEFAULT_MESSAGE_LOGLEVEL
/* We show everything that is MORE important than this.. */
#define CONSOLE_LOGLEVEL_SILENT 0 /* Mum's the word */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 13e839d..ac2b64e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -56,7 +56,7 @@
int console_printk[4] = {
CONSOLE_LOGLEVEL_DEFAULT, /* console_loglevel */
- DEFAULT_MESSAGE_LOGLEVEL, /* default_message_loglevel */
+ MESSAGE_LOGLEVEL_DEFAULT, /* default_message_loglevel */
CONSOLE_LOGLEVEL_MIN, /* minimum_console_loglevel */
CONSOLE_LOGLEVEL_DEFAULT, /* default_console_loglevel */
};
--
1.9.1
On Wed, Jul 09, 2014 at 08:04:13AM -0500, Alex Elder wrote:
> This commit:
> a8fe19eb kernel/printk: use symbolic defines for console loglevels
> makes consistent use of symbolic values for printk() log levels.
>
> The naming scheme used is different from the one used for
> DEFAULT_MESSAGE_LOGLEVEL though. Change that symbol name to be
> MESSAGE_LOGLEVEL_DEFAULT for consistency.
>
> Note that we don't rename CONFIG_DEFAULT_MESSAGE_LOGLEVEL (to avoid
> breaking existing config files that might reference it).
>
> Signed-off-by: Alex Elder <[email protected]>
> ---
> include/linux/printk.h | 2 +-
> kernel/printk/printk.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 319ff7e..3d1ccad 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -31,7 +31,7 @@ static inline const char *printk_skip_level(const char *buffer)
> }
>
> /* printk's without a loglevel use this.. */
> -#define DEFAULT_MESSAGE_LOGLEVEL CONFIG_DEFAULT_MESSAGE_LOGLEVEL
> +#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_DEFAULT_MESSAGE_LOGLEVEL
Well, I can't say I like it - we have the config item
CONFIG_DEFAULT_MESSAGE_LOGLEVEL and DEFAULT_MESSAGE_LOGLEVEL resembles
it for a reason - it is the corresponding define coming from .config.
With this change you have:
CONFIG_DEFAULT_MESSAGE_LOGLEVEL
MESSAGE_LOGLEVEL_DEFAULT
which is more confusing. To me at least. I can't see the resemblance at
a quick glance anymore.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
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 <[email protected]>
That's good - every patch which removes ifdeffery is a good patch. :)
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 07/09/2014 10:00 AM, Borislav Petkov wrote:
> On Wed, Jul 09, 2014 at 08:04:13AM -0500, Alex Elder wrote:
>> This commit:
>> a8fe19eb kernel/printk: use symbolic defines for console loglevels
>> makes consistent use of symbolic values for printk() log levels.
>>
>> The naming scheme used is different from the one used for
>> DEFAULT_MESSAGE_LOGLEVEL though. Change that symbol name to be
>> MESSAGE_LOGLEVEL_DEFAULT for consistency.
>>
>> Note that we don't rename CONFIG_DEFAULT_MESSAGE_LOGLEVEL (to avoid
>> breaking existing config files that might reference it).
>>
>> Signed-off-by: Alex Elder <[email protected]>
>> ---
>> include/linux/printk.h | 2 +-
>> kernel/printk/printk.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 319ff7e..3d1ccad 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -31,7 +31,7 @@ static inline const char *printk_skip_level(const char *buffer)
>> }
>>
>> /* printk's without a loglevel use this.. */
>> -#define DEFAULT_MESSAGE_LOGLEVEL CONFIG_DEFAULT_MESSAGE_LOGLEVEL
>> +#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_DEFAULT_MESSAGE_LOGLEVEL
>
> Well, I can't say I like it - we have the config item
> CONFIG_DEFAULT_MESSAGE_LOGLEVEL and DEFAULT_MESSAGE_LOGLEVEL resembles
> it for a reason - it is the corresponding define coming from .config.
>
> With this change you have:
>
> CONFIG_DEFAULT_MESSAGE_LOGLEVEL
> MESSAGE_LOGLEVEL_DEFAULT
>
> which is more confusing. To me at least. I can't see the resemblance at
> a quick glance anymore.
Yes I realized this just sort of moved that sort of problem
to a different place. The change was responding to the
inconsistency in naming in "printk.c". I can control the
effects of that, but I can't predict who might be using
various config options, so I avoided doing that rename.
Was I being overly cautious on the config option name?
I could fix that too and have consistency everywhere.
-Alex
On Wed, Jul 09, 2014 at 10:10:28AM -0500, Alex Elder wrote:
> Yes I realized this just sort of moved that sort of problem
> to a different place. The change was responding to the
> inconsistency in naming in "printk.c". I can control the
> effects of that, but I can't predict who might be using
> various config options, so I avoided doing that rename.
>
> Was I being overly cautious on the config option name?
> I could fix that too and have consistency everywhere.
You mean turn it into CONFIG_MESSAGE_LOGLEVEL_DEFAULT?
I think you're free to do so because .config defines are not API anyway
and anyone who thinks so should get off the bad sh*t he's smoking.
:-)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 07/09/2014 10:13 AM, Borislav Petkov wrote:
> On Wed, Jul 09, 2014 at 10:10:28AM -0500, Alex Elder wrote:
>> Yes I realized this just sort of moved that sort of problem
>> to a different place. The change was responding to the
>> inconsistency in naming in "printk.c". I can control the
>> effects of that, but I can't predict who might be using
>> various config options, so I avoided doing that rename.
>>
>> Was I being overly cautious on the config option name?
>> I could fix that too and have consistency everywhere.
>
> You mean turn it into CONFIG_MESSAGE_LOGLEVEL_DEFAULT?
OK, I'll repost a little later with that change included.
Thanks.
-Alex
On Wed, Jul 09, 2014 at 10:14:54AM -0500, Alex Elder wrote:
> OK, I'll repost a little later with that change included.
I'd wait a couple of days for the others to have a look too, if I were
you. Flooding people can be counterproductive. :-)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed 2014-07-09 08:04:14, Alex Elder wrote:
> This patch fixes a few comments that don't accurately describe their
> corresponding code. It also fixes some minor typographical errors.
>
> Signed-off-by: Alex Elder <[email protected]>
Nice clean up. All changes make sense to me.
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
> ---
> kernel/printk/printk.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ac2b64e..646146c 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -113,9 +113,9 @@ static int __down_trylock_console_sem(unsigned long ip)
> * This is used for debugging the mess that is the VT code by
> * keeping track if we have the console semaphore held. It's
> * definitely not the perfect debug tool (we don't know if _WE_
> - * hold it are racing, but it helps tracking those weird code
> - * path in the console code where we end up in places I want
> - * locked without the console sempahore held
> + * hold it and are racing, but it helps tracking those weird code
> + * paths in the console code where we end up in places I want
> + * locked without the console sempahore held).
> */
> static int console_locked, console_suspended;
>
> @@ -146,8 +146,8 @@ static int console_may_schedule;
> * the overall length of the record.
> *
> * The heads to the first and last entry in the buffer, as well as the
> - * sequence numbers of these both entries are maintained when messages
> - * are stored..
> + * sequence numbers of these entries are maintained when messages are
> + * stored.
> *
> * If the heads indicate available messages, the length in the header
> * tells the start next message. A length == 0 for the next message
> @@ -344,7 +344,7 @@ static int log_make_free_space(u32 msg_size)
> while (log_first_seq < log_next_seq) {
> if (logbuf_has_space(msg_size, false))
> return 0;
> - /* drop old messages until we have enough continuous space */
> + /* drop old messages until we have enough contiguous space */
> log_first_idx = log_next(log_first_idx);
> log_first_seq++;
> }
> @@ -1476,7 +1476,7 @@ static struct cont {
> struct task_struct *owner; /* task of first print*/
> u64 ts_nsec; /* time of first print */
> u8 level; /* log level of first message */
> - u8 facility; /* log level of first message */
> + u8 facility; /* log facility of first message */
> enum log_flags flags; /* prefix, newline flags */
> bool flushed:1; /* buffer sealed and committed */
> } cont;
> @@ -1881,11 +1881,12 @@ static int __add_preferred_console(char *name, int idx, char *options,
> return 0;
> }
> /*
> - * Set up a list of consoles. Called from init/main.c
> + * Set up a console. Called via do_early_param() in init/main.c
> + * for each "console=" parameter in the boot command line.
> */
> static int __init console_setup(char *str)
> {
> - char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for index */
> + char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
> char *s, *options, *brl_options = NULL;
> int idx;
>
> @@ -2045,8 +2046,8 @@ EXPORT_SYMBOL(console_lock);
> /**
> * console_trylock - try to lock the console system for exclusive use.
> *
> - * Tried to acquire a lock which guarantees that the caller has
> - * exclusive access to the console system and the console_drivers list.
> + * Try to acquire a lock which guarantees that the caller has exclusive
> + * access to the console system and the console_drivers list.
> *
> * returns 1 on success, and 0 on failure to acquire the lock.
> */
> --
> 1.9.1
>
This patch contains some small cleanups to kernel/printk/printk.c.
None of them should cause any change in behavior.
- When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
- When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
definition; delete it.
- Pull an assignment out of a conditional expression in console_setup().
- Use isdigit() in console_setup() rather than open coding it.
- In update_console_cmdline(), drop a NUL-termination assignment;
the strlcpy() call that precedes it guarantees it's not needed.
- Simplify some logic in printk_timed_ratelimit().
Signed-off-by: Alex Elder <[email protected]>
---
kernel/printk/printk.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 6f75e8a..909029e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -45,6 +45,7 @@
#include <linux/poll.h>
#include <linux/irq_work.h>
#include <linux/utsname.h>
+#include <linux/ctype.h>
#include <asm/uaccess.h>
@@ -257,7 +258,7 @@ static u64 clear_seq;
static u32 clear_idx;
#define PREFIX_MAX 32
-#define LOG_LINE_MAX 1024 - PREFIX_MAX
+#define LOG_LINE_MAX (1024 - PREFIX_MAX)
/* record buffer */
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
@@ -1794,7 +1795,7 @@ EXPORT_SYMBOL(printk);
#define LOG_LINE_MAX 0
#define PREFIX_MAX 0
-#define LOG_LINE_MAX 0
+
static u64 syslog_seq;
static u32 syslog_idx;
static u64 console_seq;
@@ -1895,7 +1896,8 @@ static int __init console_setup(char *str)
strncpy(buf, str, sizeof(buf) - 1);
}
buf[sizeof(buf) - 1] = 0;
- if ((options = strchr(str, ',')) != NULL)
+ options = strchr(str, ',');
+ if (options)
*(options++) = 0;
#ifdef __sparc__
if (!strcmp(str, "ttya"))
@@ -1904,7 +1906,7 @@ static int __init console_setup(char *str)
strcpy(buf, "ttyS1");
#endif
for (s = buf; *s; s++)
- if ((*s >= '0' && *s <= '9') || *s == ',')
+ if (isdigit(*s) || *s == ',')
break;
idx = simple_strtoul(s, NULL, 10);
*s = 0;
@@ -1943,7 +1945,6 @@ int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, cha
i++, c++)
if (strcmp(c->name, name) == 0 && c->index == idx) {
strlcpy(c->name, name_new, sizeof(c->name));
- c->name[sizeof(c->name) - 1] = 0;
c->options = options;
c->index = idx_new;
return i;
@@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
bool printk_timed_ratelimit(unsigned long *caller_jiffies,
unsigned int interval_msecs)
{
- if (*caller_jiffies == 0
- || !time_in_range(jiffies, *caller_jiffies,
- *caller_jiffies
- + msecs_to_jiffies(interval_msecs))) {
- *caller_jiffies = jiffies;
- return true;
- }
- return false;
+ unsigned long elapsed = jiffies - *caller_jiffies;
I wondered if the deduction might be negative. Well, it should not be
if none manipulates *caller_jiffies a strange way. If it happens,
the following condition will most likely fail and *caller_jiffies will
get reset to jiffies. It looks reasonable to me.
Reviewed-by: Petr Mladek <[email protected]>
+
+ if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
+ return false;
+
+ *caller_jiffies = jiffies;
+ return true;
}
EXPORT_SYMBOL(printk_timed_ratelimit);
Best Regards,
Petr
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 <[email protected]>
> ---
> 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 <[email protected]>
Best Regards,
Petr
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
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
Sending once again as a correct reply. I am sorry for the
confusion. I think that it is high time for me to go home and sleep :-)
On Wed 2014-07-09 08:04:16, Alex Elder wrote:
> This patch contains some small cleanups to kernel/printk/printk.c.
> None of them should cause any change in behavior.
> - When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
> - When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
> definition; delete it.
> - Pull an assignment out of a conditional expression in console_setup().
> - Use isdigit() in console_setup() rather than open coding it.
> - In update_console_cmdline(), drop a NUL-termination assignment;
> the strlcpy() call that precedes it guarantees it's not needed.
> - Simplify some logic in printk_timed_ratelimit().
>
> Signed-off-by: Alex Elder <[email protected]>
> ---
> kernel/printk/printk.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 6f75e8a..909029e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -45,6 +45,7 @@
> #include <linux/poll.h>
> #include <linux/irq_work.h>
> #include <linux/utsname.h>
> +#include <linux/ctype.h>
>
> #include <asm/uaccess.h>
>
> @@ -257,7 +258,7 @@ static u64 clear_seq;
> static u32 clear_idx;
>
> #define PREFIX_MAX 32
> -#define LOG_LINE_MAX 1024 - PREFIX_MAX
> +#define LOG_LINE_MAX (1024 - PREFIX_MAX)
>
> /* record buffer */
> #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> @@ -1794,7 +1795,7 @@ EXPORT_SYMBOL(printk);
>
> #define LOG_LINE_MAX 0
> #define PREFIX_MAX 0
> -#define LOG_LINE_MAX 0
> +
> static u64 syslog_seq;
> static u32 syslog_idx;
> static u64 console_seq;
> @@ -1895,7 +1896,8 @@ static int __init console_setup(char *str)
> strncpy(buf, str, sizeof(buf) - 1);
> }
> buf[sizeof(buf) - 1] = 0;
> - if ((options = strchr(str, ',')) != NULL)
> + options = strchr(str, ',');
> + if (options)
> *(options++) = 0;
> #ifdef __sparc__
> if (!strcmp(str, "ttya"))
> @@ -1904,7 +1906,7 @@ static int __init console_setup(char *str)
> strcpy(buf, "ttyS1");
> #endif
> for (s = buf; *s; s++)
> - if ((*s >= '0' && *s <= '9') || *s == ',')
> + if (isdigit(*s) || *s == ',')
> break;
> idx = simple_strtoul(s, NULL, 10);
> *s = 0;
> @@ -1943,7 +1945,6 @@ int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, cha
> i++, c++)
> if (strcmp(c->name, name) == 0 && c->index == idx) {
> strlcpy(c->name, name_new, sizeof(c->name));
> - c->name[sizeof(c->name) - 1] = 0;
> c->options = options;
> c->index = idx_new;
> return i;
> @@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
> bool printk_timed_ratelimit(unsigned long *caller_jiffies,
> unsigned int interval_msecs)
> {
> - if (*caller_jiffies == 0
> - || !time_in_range(jiffies, *caller_jiffies,
> - *caller_jiffies
> - + msecs_to_jiffies(interval_msecs))) {
> - *caller_jiffies = jiffies;
> - return true;
> - }
> - return false;
> + unsigned long elapsed = jiffies - *caller_jiffies;
I wondered if the deduction might be negative. Well, it should not be
if none manipulates *caller_jiffies in a strange way. If it happens,
the following condition will most likely fail and *caller_jiffies will
get reset to jiffies. It looks reasonable to me.
Reviewed-by: Petr Mladek <[email protected]>
> + if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
> + return false;
> +
> + *caller_jiffies = jiffies;
> + return true;
> }
> EXPORT_SYMBOL(printk_timed_ratelimit);
Best Regards,
Petr
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
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.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 07/09/2014 11:29 AM, Petr Ml?dek wrote:
> Sending once again as a correct reply. I am sorry for the
> confusion. I think that it is high time for me to go home and sleep :-)
>
> On Wed 2014-07-09 08:04:16, Alex Elder wrote:
>> This patch contains some small cleanups to kernel/printk/printk.c.
>> None of them should cause any change in behavior.
>> - When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
>> - When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
>> definition; delete it.
>> - Pull an assignment out of a conditional expression in console_setup().
>> - Use isdigit() in console_setup() rather than open coding it.
>> - In update_console_cmdline(), drop a NUL-termination assignment;
>> the strlcpy() call that precedes it guarantees it's not needed.
>> - Simplify some logic in printk_timed_ratelimit().
>>
>> Signed-off-by: Alex Elder <[email protected]>
>> ---
>> kernel/printk/printk.c | 26 +++++++++++++-------------
>> 1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 6f75e8a..909029e 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
. . .
>> @@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
>> bool printk_timed_ratelimit(unsigned long *caller_jiffies,
>> unsigned int interval_msecs)
>> {
>> - if (*caller_jiffies == 0
>> - || !time_in_range(jiffies, *caller_jiffies,
>> - *caller_jiffies
>> - + msecs_to_jiffies(interval_msecs))) {
>> - *caller_jiffies = jiffies;
>> - return true;
>> - }
>> - return false;
>> + unsigned long elapsed = jiffies - *caller_jiffies;
>
Currently, all callers pass a 0 value in *caller_jiffies
initially (using a static/BSS variable), and a value updated
by a previous call to __printk_ratelimit() thereafter.
If a caller passed something different, yes, it's possible the
result would wrap to a high unsigned value (i.e., go negative).
However the logic used here involves the same subtraction
operation as was used before--though previously that was
buried inside the time_after() macro called by time_in_range().
-Alex
> I wondered if the deduction might be negative. Well, it should not be
> if none manipulates *caller_jiffies in a strange way. If it happens,
> the following condition will most likely fail and *caller_jiffies will
> get reset to jiffies. It looks reasonable to me.
>
> Reviewed-by: Petr Mladek <[email protected]>
>
>> + if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
>> + return false;
>> +
>> + *caller_jiffies = jiffies;
>> + return true;
>> }
>> EXPORT_SYMBOL(printk_timed_ratelimit);
>
> Best Regards,
> Petr
>
Hi Alex,
On Wed, Jul 9, 2014 at 3:04 PM, Alex Elder <[email protected]> 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 -- [email protected]
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
On 07/09/2014 12:58 PM, Geert Uytterhoeven wrote:
> Hi Alex,
>
> On Wed, Jul 9, 2014 at 3:04 PM, Alex Elder <[email protected]> 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 -- [email protected]
>
> 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
>