lkml.org 
[lkml]   [2013]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/4] uprobes/tracing: generalize struct uprobe_trace_entry_head
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.



\
 
 \ /
  Last update: 2013-04-09 17:21    [W:0.081 / U:0.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site