Re: [RFC][PATCH 03/12 v3] tracing: Create seq_buf layer in trace_seq

Petr Mladek
2014-11-10T19:02:56Z

On Mon 2014-11-10 12:37:47, Steven Rostedt wrote:
> On Mon, 10 Nov 2014 14:53:30 +0100
> Petr Mladek <pmladek@suse.cz> wrote:
>
> > > +/*
> > > + * How much buffer is left on the seq_buf?
> > > + */
> > > +static inline unsigned int
> > > +seq_buf_buffer_left(struct seq_buf *s)
> > > +{
> > > + return (s->size - 1) - s->len;
> >
> > This should be
> >
> > if (seq_buf_has_overflowed(s)
> > return 0;
> > return (s->size - 1) - s->len;
> >
> > otherwise, it would return UNIT_MAX for the overflown state. If I am
> > not mistaken.
>
> I guess I could add that. Probably the safer bet. Or document it that
> this is undefined if buffer has overflowed. I have to check how my use
> cases worked.
>
> Probably best to add the overflow check anyway.

I vote for it :-)

> > [...]
> >
> > > diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
> > > new file mode 100644
> > > index 000000000000..88738b200bf3
> > > --- /dev/null
> > > +++ b/kernel/trace/seq_buf.c
> >
> > [...]
> >
> > > +
> > > +/**
> > > + * seq_buf_bitmask - write a bitmask array in its ASCII representation
> > > + * @s: seq_buf descriptor
> > > + * @maskp: points to an array of unsigned longs that represent a bitmask
> > > + * @nmaskbits: The number of bits that are valid in @maskp
> > > + *
> > > + * Writes a ASCII representation of a bitmask string into @s.
> > > + *
> > > + * Returns zero on success, -1 on overflow.
> > > + */
> > > +int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
> > > + int nmaskbits)
> > > +{
> > > + unsigned int len = seq_buf_buffer_left(s);
> > > + int ret;
> > > +
> > > + WARN_ON(s->size == 0);
> > > +
> > > + /*
> > > + * The last byte of the buffer is used to determine if we
> > > + * overflowed or not.
> > > + */
> > > + if (len > 1) {
> > > + ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
> >
> > It should be:
> >
> > ret = bitmap_scnprintf(s->buffer + s->len, len,
> > maskp, nmaskbits);
> >
> > otherwise, we would write to the beginning to the buffer.
>
> You are correct. But I'll make that a separate patch. This is just
> keeping the bug that was in the original code.

Fair enough.

> >
> > > + if (ret < len) {
> > > + s->len += ret;
> > > + return 0;
> > > + }
> > > + }
> > > + seq_buf_set_overflow(s);
> > > + return -1;
> > > +}
> > > +
> >
> > [...]
> >
> > > diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
> > > index 1f24ed99dca2..3ad8738aea19 100644
> > > --- a/kernel/trace/trace_seq.c
> > > +++ b/kernel/trace/trace_seq.c
> >
> > [...]
> >
> > > @@ -144,23 +160,24 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask);
> > > */
> > > int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
> > > {
> > > - unsigned int len = TRACE_SEQ_BUF_LEFT(s);
> > > + unsigned int save_len = s->seq.len;
> > > int ret;
> > >
> > > - if (s->full || !len)
> > > + if (s->full)
> > > return 0;
> > >
> > > - ret = vsnprintf(s->buffer + s->len, len, fmt, args);
> > > + __trace_seq_init(s);
> > > +
> > > + ret = seq_buf_vprintf(&s->seq, fmt, args);
> >
> > Note that this returns 0 on success => we do not need to store it
> >
> > > /* If we can't write it all, don't bother writing anything */
> > > - if (ret >= len) {
> > > + if (unlikely(seq_buf_has_overflowed(&s->seq))) {
> > > + s->seq.len = save_len;
> > > s->full = 1;
> > > return 0;
> > > }
> > >
> > > - s->len += ret;
> > > -
> > > - return len;
> > > + return ret;
> >
> > Instead, we have to do something like:
> >
> > return s->seq.len - save_len;
>
> Actually, I need to make the trace_seq_*() functions return the same as
> the seq_buf_*() functions.
>
> I'll update this for now, but it's gotta change later. Probably why I
> wasn't so careful about it.
>
> Hmm, I may make the trace_seq_*() functions not return length written
> first, before pulling out the seq_buf_*() code. That is, make the
> trace_seq_*() behave more like what the seq_buf_*() code does first,
> before pulling out the seq_buf_*() code.

Sounds like the best solution if it does not cause too many changes in
the trace_seq() callers.

Best Regards,
Petr