On Mon, 22 Jan 2024 19:45:41 -0300
"Ricardo B. Marliere" <rica...@marliere.net> wrote:
> > Perhaps we need a:
> >
> > if (WARN_ON_ONCE(!s->seq.size))
> > seq_buf_init(&s->seq, s->buffer, TRACE_SEQ_BUFFER_SIZE);
> > else
> > seq_buf_clear(&s->seq);
> >
> >
> > in the trace_seq_reset() to catch any place that doesn't have it
> > initialized yet.
>
> But that would be temporary, right? Kind of a "trace_seq_init_or_reset".
> If that's the case it would be best to simply work out all the places
> instead. Would the current tests [1] cover everything or should
> something else be made to make sure no place was missing from the patch?
No it would be permanent. And no, it is *not* a "trace_seq_init_or_reset".
That WARN_ON_ONCE() means this should never happen, but if it does, the
if () statement keeps it from crashing.
If we later make it dynamic, where there is no s->buffer, that would then
change to:
if (WARN_ON_ONCE(!s->seq.size))
seq_buf_init(&s->seq, NULL, 0);
else
seq_buf_clear(&s->seq);
Where the trace_seq is basically disabled from that point on.
I would try to fix everything, but this will find those places you missed. ;-)
The kernel is like this, because it's not like any other application. If an
application crashes, you may get annoyed, and just restart it. If the
kernel crashes, you can easily lose data and you have to reboot your
machine. Thus, we treat things that could cause harm much more carefully.
We do not want to crash the kernel just because we missed a location that
did not initialize trace_seq. That's why this would be a permanent change.
-- Steve
