Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 11:21:22 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Petr Mladek <pmladek@suse.com>
Cc:     John Ogness <john.ogness@linutronix.de>,
        Jim Cromie <jim.cromie@gmail.com>,
        linux-kernel@vger.kernel.org,
        Vincent Whitchurch <vincent.whitchurch@axis.com>,
        daniel@ffwll.ch, sergey.senozhatsky@gmail.com,
        Simon Ser <contact@emersion.fr>,
        Sean Paul <seanpaul@chromium.org>
Subject: Re: [RFC PATCH 1/1] tracefs: add TP_printk_no_nl - RFC
Message-ID: <20230731112122.41a76154@gandalf.local.home>
In-Reply-To: <ZMeLKTzSWh3kbw8c@alley>
References: <20230730222134.54547-1-jim.cromie@gmail.com>
        <87y1iwlc80.fsf@jogness.linutronix.de>
        <ZMeLKTzSWh3kbw8c@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 31 Jul 2023 12:21:29 +0200
Petr Mladek <pmladek@suse.com> wrote:

> > Grepping through the kernel, I am surprised how many messages are
> > missing the trailing newline when it is obvious they are not intended to
> > be extended. I consider these bugs.  
> 
> I consider this bug as well.
> 
> My understanding is that this patch affects only messages printed to
> the ftrace buffer so that it does not affect printk() directly.
> 
> But still. It creates a habit that is bad for printk(). Also relies
> on the fact that people are aware of this macro and use it. IMHO,
> it will not work in practice. Or do I miss something?

I believe that the problem Jim is addressing is that there's printk()
messages that also go to a tracepoint() function. The tracepoint function
will add its own '\n' to the message, and now we have two '\n's and this
causes extra spaces in the output of the ftrace ring buffer.

Perhaps what we should do is to make sure that the output doesn't add more
newlines than just one.

Something like this: (totally untested)

diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index db575094c498..a1b73ffa1552 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -333,6 +333,8 @@ int trace_output_raw(struct trace_iterator *iter, char *name,
 
 	trace_seq_printf(s, "%s: ", name);
 	trace_seq_vprintf(s, trace_event_format(iter, fmt), ap);
+	trace_seq_trim_newlines(s);
+	trace_seq_putc(s, '\n');
 
 	return trace_handle_return(s);
 }
diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
index bac06ee3b98b..d4e2049809e3 100644
--- a/kernel/trace/trace_seq.c
+++ b/kernel/trace/trace_seq.c
@@ -221,6 +221,22 @@ void trace_seq_puts(struct trace_seq *s, const char *str)
 }
 EXPORT_SYMBOL_GPL(trace_seq_puts);
 
+/**
+ * trace_seq_trim_newlines - remove ending newlines
+ * @s: trace sequence descriptor
+ *
+ * Remove ending newlines from the buffer.
+ */
+void trace_seq_trim_newlines(struct trace_seq *s)
+{
+	if (unlikely(!s->seq.size || !s->seq.len))
+		return;
+
+	while (s->seq.len && s->buffer[s->seq.len - 1] == '\n') {
+		s->seq.len--;
+	}
+}
+
 /**
  * trace_seq_putc - trace sequence printing of simple character
  * @s: trace sequence descriptor


A similar thing would need to go into libtraceevent.

-- Steve