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

Andrew Morton
2014-06-20T17:12:47Z

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.