Re: [RFC][PATCH 1/3] trace_seq: Move the trace_seq code to lib/

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 20 Jun 2014 12:58:23 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:

>
> ...
>
> > 
> > > + * Writes a ASCII representation of a bitmask string into @s.
> > > + */
> > > +int
> > > +trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
> > > +		  int nmaskbits)
> > > +{
> > > +	int len = (PAGE_SIZE - 1) - s->len;
> > > +	int ret;
> > > +
> > > +	if (s->full || !len)
> > > +		return 0;
> > > +
> > > +	ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
> > > +	s->len += ret;
> > > +
> > > +	return 1;
> > > +}
> > > +EXPORT_SYMBOL_GPL(trace_seq_bitmask);
> > 
> > More dittos.
> 
> Confused. What dittos is this dittoing?

Unneeded newline, poorly considered choice of types.

>
> ...
>
> > > + * buffer (@s). Then the output may be either used by
> > > + * the sequencer or pulled into another buffer.
> > > + */
> > > +int
> > > +trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
> > > +{
> > > +	int len = (PAGE_SIZE - 1) - s->len;
> > > +	int ret;
> > > +
> > > +	if (s->full || !len)
> > > +		return 0;
> > > +
> > > +	ret = vsnprintf(s->buffer + s->len, len, fmt, args);
> > > +
> > > +	/* If we can't write it all, don't bother writing anything */
> > > +	if (ret >= len) {
> > > +		s->full = 1;
> > > +		return 0;
> > > +	}
> > > +
> > > +	s->len += ret;
> > > +
> > > +	return len;
> > > +}
> > > +EXPORT_SYMBOL_GPL(trace_seq_vprintf);
> > 
> > Several dittos.
> 
> Oh, just on the function. You're not dittoing a comment about the
> EXPORT_SYMBOL_GPL() that you forgot to add, are you?

yup.  Unneded newline, types, EXPORT_ confusion.

>
> ...
>
> > 
> > > +#define HEX_CHARS		(MAX_MEMHEX_BYTES*2 + 1)
> > > +
> > > +int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, size_t len)
> > > +{
> > > +	unsigned char hex[HEX_CHARS];
> > > +	const unsigned char *data = mem;
> > > +	int i, j;
> > > +
> > > +	if (s->full)
> > > +		return 0;
> > 
> > What's this ->full thing all about anyway?  Some central comment which
> > explains the design is needed.
> 
> Comment? What? Git blame isn't good enough for ya? ;-)

There's always that.  There's also googling for the original list
dicsussion.  But it's a bit user-unfriendly, particularly when then
code has aged was subsequently altered many times.

>
> ...
>
> 
> Hey! Thanks for the review. Much appreciated. And maybe you should read
> those messages in your /dev/null folder that I cc you with. :-)

I sometimes do.