linux-kernel - Re: [PATCH 1/4] printk: LOG_CONT and LOG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140717083918.GP6774@pathway.suse.cz>
Date:	Thu, 17 Jul 2014 10:39:18 +0200
From:	Petr Mládek <pmladek@...e.cz>
To:	Alex Elder <elder@...aro.org>
Cc:	akpm@...ux-foundation.org, bp@...e.de, john.stultz@...aro.org,
	jack@...e.cz, linux-kernel@...r.kernel.org, kay.sievers@...y.org
Subject: Re: [PATCH 1/4] printk: LOG_CONT and LOG_NEWLINE are separate

On Wed 2014-07-16 12:26:57, Alex Elder wrote:
> Two log record flags--LOG_CONT and LOG_NEWLINE--are never both set
> at the same time in a log record flags field.  What follows is a
> great deal of explanation that aims to prove this assertion.

It makes perfect sense. If you found a situation where both flags were
set together, it would mean a bug. If a record ends with new line, it
is not continuous and vice versa.

[...]

> Signed-off-by: Alex Elder <elder@...aro.org>
> ---
>  kernel/printk/printk.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 13e839d..301ade3 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1006,11 +1006,9 @@ static size_t msg_print_text(const struct printk_log *msg, enum log_flags prev,
>  		prefix = false;
>  
>  	if (msg->flags & LOG_CONT) {
> -		if ((prev & LOG_CONT) && !(prev & LOG_NEWLINE))
> +		if (prev & LOG_CONT)
>  			prefix = false;
> -
> -		if (!(msg->flags & LOG_NEWLINE))
> -			newline = false;
> +		newline = false;
>  	}

Makes sense. I like it.

>  	do {
> @@ -1642,7 +1640,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	/* mark and strip a trailing newline */
>  	if (text_len && text[text_len-1] == '\n') {
>  		text_len--;
> -		lflags |= LOG_NEWLINE;
> +		lflags = LOG_NEWLINE;
>  	}
>  
>  	/* strip kernel syslog prefix and extract log level or control flags */
> @@ -1672,7 +1670,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		level = default_message_loglevel;
>  
>  	if (dict)
> -		lflags |= LOG_PREFIX|LOG_NEWLINE;
> +		lflags = LOG_PREFIX|LOG_NEWLINE;
>  
>  	if (!(lflags & LOG_NEWLINE)) {
>  		/*
> @@ -1688,7 +1686,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		else
>  			printed_len += log_store(facility, level,
>  						 lflags | LOG_CONT, 0,
> -						 dict, dictlen, text, text_len);
> +						 NULL, 0, text, text_len);
>  	} else {
>  		bool stored = false;
>  

I am not sure that I like the last three changes. The logic is
correct. But I think that these micro-optimizations makes the code less
readable and prone to errors with reordering and other changes.

The original code does not harm. The new code is less obvious and will
force many people to think why it is correct. Even you might be in
doubts if you see it after few months :-)

Well, I do not have strong opinion here. Other people might see it
different. Forcing people to think is not a bad idea after all :-)

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