linux-kernel - Re: [RFC][PATCH 1/3] trace_seq: Move the trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 20 Jun 2014 10:12:44 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>, Jiri Kosina <jkosina@...e.cz>,
	Michal Hocko <mhocko@...e.cz>, Jan Kara <jack@...e.cz>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Dave Anderson <anderson@...hat.com>,
	Petr Mladek <pmladek@...e.cz>,
	Johannes Berg <johannes@...solutions.net>
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@...dmis.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/