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;
MIME-Version: 1.0
References: <20230730222134.54547-1-jim.cromie@gmail.com> <87y1iwlc80.fsf@jogness.linutronix.de>
 <ZMeLKTzSWh3kbw8c@alley> <20230731112122.41a76154@gandalf.local.home>
In-Reply-To: <20230731112122.41a76154@gandalf.local.home>
From:   jim.cromie@gmail.com
Date:   Mon, 31 Jul 2023 15:01:28 -0600
Message-ID: <CAJfuBxyLMO2v4CTUWP8Lhy9EmzjZ3BVxhP1w1S4Cb4siEpVWEQ@mail.gmail.com>
Subject: Re: [RFC PATCH 1/1] tracefs: add TP_printk_no_nl - RFC
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Petr Mladek <pmladek@suse.com>,
        John Ogness <john.ogness@linutronix.de>,
        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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 31, 2023 at 9:21=E2=80=AFAM Steven Rostedt <rostedt@goodmis.org=
> wrote:
>
> 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.
> >

agreed.
I blame pr_cont()  at least in part.

Note - I have submitted
drm_dbg: add trailing newlines where missing
https://patchwork.freedesktop.org/series/121583/

it appears to have failed - on what look like unrelated failures - pipe etc


> > 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?

something like (opposite of) this, used inside pr_debug, would force
all callers to add \n.
#include <assert.h>
#define FAIL_IF_TRAILING_NEWLINE(str) \
 _Static_assert((str[sizeof(str) - 2] !=3D '\n'), "String literal has a
trailing newline")

obviously, Inverted, gentler versions are perhaps better.

>
> 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.

Yes. only quibble - there arent yet, there could be.
Vincents patch didnt go in, Im trying to get it ready for another submit.

I uncovered this bit on the way,
figured I could turn it into a question, and maybe a little conceptual buy-=
in.

This is a 0 or 1 "\n" problem,
quietly accepting 25 in a row could paper over another problem.
maybe complain-once after 2-3 ?

TP_printk_no_nl   doesnt solve all problems -
it could work for adding DRM.debug trace-events,
cuz the callsites have few of the bad-habits noted above,
*very* few drm_dbg()s without trailing \n. - presuming theyre fixable.

fixing the larger body of pr_debug()s is quite a nuisance,
and if dyndbg -T flag sends them, you'd get the larger varying
population of \n's
So some real-time stripping /adding is needed for some populations of pr_de=
bugs

the no_nl macro would at least allow event-definers
to assert that theyre sending their own.


>
> Perhaps what we should do is to make sure that the output doesn't add mor=
e
> 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, cha=
r *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] =3D=3D '\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