Re: A style question: repeated return value check

Previous thread: [PATCH] PIIX3: Warn softer about enabling passive release. by Adam Jackson on Monday, September 29, 2008 - 11:10 am. (1 message)

Next thread: e1000e NVM corruption issue status (updated 29-Sept-2008) by Brandeburg, Jesse on Monday, September 29, 2008 - 11:22 am. (1 message)
From: Frederic Weisbecker
Date: Monday, September 29, 2008 - 11:18 am

We need a kind of disambiguation when a print_line callback
returns 0.

_There is not enough space to print all the entry. 
 Please flush the seq and retry.
_I can't handle this type of entry

This patch changes the type of this callback for better information.

Also some changes have been made in this V2.

_ Only relay to default functions after the print_line callback fails.
_ This patch doesn't fix the issue with the broken pipe (see patch 2/4 for that)

Some things are still in discussion:

_ Find better names for the enum print_line_t values
_ Change the type of print_trace_line into boolean.

Patches to change that can be sent later.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace.c |   77 ++++++++++++++++++++++++++-----------------------
 kernel/trace/trace.h |   10 ++++++-
 2 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6ada059..61f33da 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1510,7 +1510,7 @@ void trace_seq_print_cont(struct trace_seq *s, struct trace_iterator *iter)
 		trace_seq_putc(s, '\n');
 }
 
-static int
+static enum print_line_t
 print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu)
 {
 	struct trace_seq *s = &iter->seq;
@@ -1530,7 +1530,7 @@ print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu)
 		next_entry = entry;
 
 	if (entry->type == TRACE_CONT)
-		return 1;
+		return TRACE_TYPE_HANDLED;
 
 	rel_usecs = ns2usecs(next_entry->field.t - entry->field.t);
 	abs_usecs = ns2usecs(entry->field.t - iter->tr->time_start);
@@ -1601,10 +1601,10 @@ print_lat_fmt(struct trace_iterator *iter, unsigned int trace_idx, int cpu)
 	default:
 		trace_seq_printf(s, "Unknown type %d\n", entry->type);
 	}
-	return 1;
+	return TRACE_TYPE_HANDLED;
 }
 
-static int print_trace_fmt(struct trace_iterator *iter)
+static enum print_line_t print_trace_fmt(struct trace_iterator *iter)
 {
 ...
From: Pekka Paalanen
Date: Monday, September 29, 2008 - 12:14 pm

On Mon, 29 Sep 2008 20:18:34 +0200

Off-thread style question: Would it be better or worse to write the
above as

	ret = trace_seq_printf(s, "%16s-%-5d ", comm, field->pid);
	ret = ret && trace_seq_printf(s, "[%03d] ", iter->cpu);
	ret = ret && trace_seq_printf(s, "%5lu.%06lu: ", secs, usec_rem);
	if (!ret)
		return TRACE_TYPE_PARTIAL_LINE;

which would do exactly the same, but is more compact.
Good or bad style?

Well, it could be rolled into a single trace_seq_printf() call, too.

-- 
Pekka Paalanen
http://www.iki.fi/pq/
--

From: Ingo Molnar
Date: Tuesday, September 30, 2008 - 1:33 am

in this particular case it's marginally worse style i think, even 
considering that it makes the code more compact. The reason is that it 
makes the code a tiny bit less obvious: the flow looks a bit unusual and 
when skimming it i'd have to look once more to understand its purpose. 
With the returns its more verbose but also plain obvious. YMMV.

	Ingo
--

From: Frédéric Weisbecker
Date: Tuesday, September 30, 2008 - 1:48 am

I think the same. The code flow seems to me more natural as is even if
it looks more
noisy.
IMHO, when one is reading the code, such a compact path forces a break
to figure out
what is going on in these tests.

But I agree with Pekka for the fact that it could be unified in a
single call to trace_seq_printf.
That will produce a small "3 format" easy to understand. Seems good.
--

From: Frédéric Weisbecker
Date: Tuesday, September 30, 2008 - 1:50 am

Actually it's a "5 format". Why not unify...
--

Previous thread: [PATCH] PIIX3: Warn softer about enabling passive release. by Adam Jackson on Monday, September 29, 2008 - 11:10 am. (1 message)

Next thread: e1000e NVM corruption issue status (updated 29-Sept-2008) by Brandeburg, Jesse on Monday, September 29, 2008 - 11:22 am. (1 message)