Re: [PATCH 1/4] printk: LOG_CONT and LOG_NEWLINE are separate

[Petr Mládek]

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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/