linux-kernel - Re: [Patch -tip 1/3] Tracing/ftrace: Relay unhandled entry output

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]

Message-ID: <20080925180916.75d3c9ec@daedalus.pq.iki.fi>
Date:	Thu, 25 Sep 2008 18:09:16 +0300
From:	Pekka Paalanen <pq@....fi>
To:	Frédéric Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...e.hu>
Cc:	linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [Patch -tip 1/3] Tracing/ftrace: Relay unhandled entry output

On Thu, 25 Sep 2008 15:56:03 +0100
Frédéric Weisbecker <fweisbec@...il.com> wrote:

> Correct the coding style in the commit 
> 803294369973b740f4c346553850b56e4b0c3c8a
> And remove the ret variable since print functions
> only return 0 or 1.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
> ---

Frederic, in the future, could you just copy the patch
into the email body (watch out for line wraps and other damage),
attachments are not usually included in "reply with quote",
so commenting on them is a tiny bit harder. Thanks.

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6ada059..50ac334 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1904,20 +1904,27 @@ static int trace_empty(struct trace_iterator *iter)
>  
>  static int print_trace_line(struct trace_iterator *iter)
>  {
> +	int ret;
> +
>  	if (iter->trace && iter->trace->print_line)
> -		return iter->trace->print_line(iter);
> +		if ((ret = iter->trace->print_line(iter)))
> +			return ret;

NACK.

IMHO this breaks the trace_seq handling. trace_seq may contain
the output of several entries, as far as they fit in it as a whole.
E.g. trace_seq_printf() does not print partial lines but returns 0,
so that the entry is not consumed right now. The user space reader
must consume trace_seq content, before trace_seq_printf()
is attempted again, hopefully with enough space in trace_seq to
succeed. See tracing_read_pipe().

print_line() callback works the same way. Returning 0 means "could
not print it this time, call me back later". You can't use that to
say "use the default output function instead". Note, that possibly
the default output function will fail, too, so it could actually
try many of the default output functions and still fail, eventually
leading by luck to the correct behaviour in most cases.

Note, that mmiotrace follows this convention: it deliberatly
returns 1 without processing when it wants the entry discarded, and
it returns 0 when there was not enough space to process the entry.
This is explained in my other email.

What is the supposed return value of print_trace_line(), I do not
know. Looks like it is used as boolean, so maybe the type should be
changed to bool. Unless we want three options:
- 0: could not print now, try again
- 1: success, entry processed, it can be thrown away
- 2: fall back to the default output formatting
Cases 0 and 1 must exist like that, I do not know how useful 2 is,
but it must be distinct from the first two.

Thanks.

-- 
Pekka Paalanen
http://www.iki.fi/pq/
--
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/