Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Steven Rostedt]

On Wed, 2009-09-09 at 07:12 -0700, Daniel Walker wrote:
> On Wed, 2009-09-09 at 07:46 -0400, Steven Rostedt wrote:
> > On Tue, 2009-09-08 at 21:59 -0700, Daniel Walker wrote:
> > > On Wed, 2009-09-09 at 09:26 +0800, Li Zefan wrote:
> > > > Daniel Walker wrote:
> > > > > On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> > > > >> +       TP_STRUCT__entry(
> > > > >> +               __string(       msg,    msg     )
> > > > >> +       ),
> > > > > 
> > > > > Why the funny spacing here?
> > > > > 
> > > > 
> > > > To make the code better-looking.
> > > > 
> > > > This is the coding-style we use for the code in
> > > > include/trace/events/*.
> > > 
> > > It's part of Linux right? We already have a coding style ..
> > 
> > It's a special macro. What are you now, part of the style police?
> 
> I'm just like everyone else, someone who asks questions and makes
> comments .. By using a different style than what the rest of Linux uses
> your putting yourself at a disadvantage since you can't easily use
> checkpatch on that code (even the stuff that is compliant) ..

I'm fine with questions, but yours sounded a bit more cynical
 
> 
> Not to mention people might start sending you clean ups that you maybe
> won't accept. You could always modify checkpatch to recognize your
> style.

Not sure this is worthy of modifying checkpatch. I haven't looked at
what that would take.

Here's an example of what the code currently looks like:

TRACE_EVENT(sched_wait_task,

	TP_PROTO(struct rq *rq, struct task_struct *p),

	TP_ARGS(rq, p),

	TP_STRUCT__entry(
		__array(	char,	comm,	TASK_COMM_LEN	)
		__field(	pid_t,	pid			)
		__field(	int,	prio			)
	),

	TP_fast_assign(
		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
		__entry->pid	= p->pid;
		__entry->prio	= p->prio;
	),

	TP_printk("task %s:%d [%d]",
		  __entry->comm, __entry->pid, __entry->prio)
);

And here's that same code with normal Linux style:

TRACE_EVENT(sched_wait_task,
	TP_PROTO(struct rq *rq, struct task_struct *p),
	TP_ARGS(rq, p),
	TP_STRUCT__entry(__array(char, comm, TASK_COMM_LEN)
		__field(pid_t, pid)
		__field(int, prio)),
	TP_fast_assign(memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
		__entry->pid	= p->pid;
		__entry->prio	= p->prio;
	),
	TP_printk("task %s:%d [%d]",
		  __entry->comm, __entry->pid, __entry->prio));

Not as nice to work with. The include/trace/events/ directory is quite
different than the rest of the kernel code. If checkpatch fails on it,
that's fine. Any changes here really need to be examined by eye anyway.
Having the extra spaces actually helps to see what is there.

Note, most changes in include/trace/ftrace.h failed checkpatch
horribly. ;-)  checkpatch does not handle crazy macros well.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/