Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear

[Petr Mladek]

On Tue 2018-06-26 10:39:59, Steven Rostedt wrote:
> On Tue, 26 Jun 2018 14:43:32 +0200
> Petr Mladek <pmladek@xxxxxxxx> wrote:
> 
> > On Mon 2018-06-25 23:44:07, Sergey Senozhatsky wrote:
> > > On (06/25/18 10:37), Steven Rostedt wrote:  
> > > > 
> > > > Is IMHO rather ugly.  
> > > 
> > > Either way works for me. So I'll leave it to you and Petr to decide :)
> > >   
> > > > And the original patch has one more advantage. If buf and clear are
> > > > both NULL/zero, we don't take any locks.  
> > > 
> > > But we never use syslog_print_all(buf = NULL, clear = false). It's either
> > > NULL/true [move forward clear idx, do not copy to user], or !NULL/use defined
> > > value [copy to user, move or don't move clear idx forward]  
> > 
> > Yup, I suggest the following version as a compromise. It has the code
> > duplication but I agree that it is negligible. Otherwise, it looks
> > cleaner.
> > 
> 
> This looks fine. For proper history though, what I do in this case,
> would be to make this into two patches. One with Namit's original
> patch, and then a second that removes the code duplication (your patch).

Makes sense. I have just pushed the original patch (with Sergey's
formatting fixes) into printk.git, for-4.19 branch. And I propose
the clean up patch, see below.

I am sorry for the mess. It seems that nothing can be considered
a simple obvious change. And my attempt to safe people time had
the opposite effect.


Proposed patch:

From: Petr Mladek <pmladek@xxxxxxxx>
Date: Wed, 27 Jun 2018 16:37:42 +0200
Subject: [PATCH] printk: Clean up syslog_print_all()

syslog_print_all() is called twice. Once with a valid buffer
and once just to set the indexes.

Both variants are already handled separately. This patch just
makes it more obvious. It does not change the existing behavior.

Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
---
 kernel/printk/printk.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 16b02cc51a14..fcc1992c040a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1356,16 +1356,6 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
 	u64 seq;
 	u32 idx;
 
-	if (!buf) {
-		if (clear) {
-			logbuf_lock_irq();
-			clear_seq = log_next_seq;
-			clear_idx = log_next_idx;
-			logbuf_unlock_irq();
-		}
-		return 0;
-	}
-
 	text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
 	if (!text)
 		return -ENOMEM;
@@ -1437,6 +1427,14 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
 	return len;
 }
 
+static void syslog_clear(void)
+{
+	logbuf_lock_irq();
+	clear_seq = log_next_seq;
+	clear_idx = log_next_idx;
+	logbuf_unlock_irq();
+}
+
 int do_syslog(int type, char __user *buf, int len, int source)
 {
 	bool clear = false;
@@ -1481,7 +1479,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
 		break;
 	/* Clear ring buffer */
 	case SYSLOG_ACTION_CLEAR:
-		syslog_print_all(NULL, 0, true);
+		syslog_clear();
 		break;
 	/* Disable logging to console */
 	case SYSLOG_ACTION_CONSOLE_OFF:
-- 
2.13.7