Re: [PATCH 4/4] uprobes/tracing: generalize structuprobe_trace_entry_head

[Oleg Nesterov]

On 04/08, Steven Rostedt wrote:
>
> On Fri, 2013-03-29 at 19:15 +0100, Oleg Nesterov wrote:
> >
> > @@ -491,20 +502,19 @@ static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
> >  	struct uprobe_trace_entry_head *entry;
> >  	struct ring_buffer_event *event;
> >  	struct ring_buffer *buffer;
> > -	u8 *data;
> > +	void *data;
> >  	int size, i;
> >  	struct ftrace_event_call *call = &tu->call;
> >
> > -	size = sizeof(*entry) + tu->size;
> > -
> > +	size = SIZEOF_TRACE_ENTRY(1) + tu->size;
>
> That '1' is confusing. When I first looked at this code, I thought it
> was a bug as it should have been '0' (thinking of arrays). And then I
> realized that you want the entry *after* the element.
>
> Instead of '1' and I assume '2' for ret probes, how about defining an
> enum:
>
> enum {
> 	UPROBE_NORM	= 1,
> 	UPROBE_RET	= 2,
> };
>
> and then you can have;
>
> 	size = SIZEOF_TRACE_ENTRY(UPROBE_NORM);
>
> and later:
>
> 	size = SIZEOF_TRACE_ENTRY(UPROBE_RET);
>
> Same goes for DATAOF_TRACE_ENTRY().
>
> This would make it a lot easier to understand and review, and much less
> bug prone when we have to deal with two different types of
> uprobe_trace_entry_head's.

OK, will do.

Or. Instead of enum we can use "bool is_return". So, instead of

	if (is_ret_probe(tu))
		size = SIZEOF_TRACE_ENTRY(UPROBE_ENTRY_RETPROBE);
	else
		size = SIZEOF_TRACE_ENTRY(UPROBE_ENTRY_NORMAL);

we can do

	size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));

What do you like more?

Oleg.

--
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/