Messages in this thread | | | Date | Fri, 20 Jun 2014 10:12:44 -0700 | From | Andrew Morton <> | Subject | Re: [RFC][PATCH 1/3] trace_seq: Move the trace_seq code to lib/ |
| |
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.
| |