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

[Petr Mladek <pmladek@suse.cz>]

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