Messages in this thread | | | Date | Mon, 23 Sep 2019 10:15:26 -0700 | From | Masami Hiramatsu <> | Subject | Re: [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event |
| |
On Mon, 23 Sep 2019 16:12:53 +0530 Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:
> Hey Masami, Steven > > > > > +static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig, > > + struct trace_kprobe *comp) > > +{ > > + struct trace_probe_event *tpe = orig->tp.event; > > + struct trace_probe *pos; > > + int i; > > + > > + list_for_each_entry(pos, &tpe->probes, list) { > > + orig = container_of(pos, struct trace_kprobe, tp); > > + if (strcmp(trace_kprobe_symbol(orig), > > + trace_kprobe_symbol(comp)) || > > + trace_kprobe_offset(orig) != trace_kprobe_offset(comp)) > > + continue; > > + > > + /* > > + * trace_probe_compare_arg_type() ensured that nr_args and > > + * each argument name and type are same. Let's compare comm. > > + */ > > + for (i = 0; i < orig->tp.nr_args; i++) { > > + if (strcmp(orig->tp.args[i].comm, > > + comp->tp.args[i].comm)) > > + continue; > > In a nested loop, *continue* is going to continue iterating through the > inner loop. In which case, continue is doing nothing here. I thought we > should have used a goto instead. No? To me, continue as a last statement of > a for loop always looks weird.
Oops, thanks for pointing it out!
> > > + } > > + > > + return true; > > + } > > I think we need something like this: > > list_for_each_entry(pos, &tpe->probes, list) { > orig = container_of(pos, struct trace_kprobe, tp); > if (strcmp(trace_kprobe_symbol(orig), > trace_kprobe_symbol(comp)) || > trace_kprobe_offset(orig) != trace_kprobe_offset(comp)) > continue; > > /* > * trace_probe_compare_arg_type() ensured that nr_args and > * each argument name and type are same. Let's compare comm. > */ > for (i = 0; i < orig->tp.nr_args; i++) { > if (strcmp(orig->tp.args[i].comm, > comp->tp.args[i].comm)) > goto outer_loop; > > } > > return true; > outer_loop: > }
Correct, that's what I intended. Could you make a fix patch on top of it? (or do I?)
Thank you,
> > > > + > > + return false; > > +} > > + > > > > ...... > > > +static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig, > > + struct trace_uprobe *comp) > > +{ > > + struct trace_probe_event *tpe = orig->tp.event; > > + struct trace_probe *pos; > > + struct inode *comp_inode = d_real_inode(comp->path.dentry); > > + int i; > > + > > + list_for_each_entry(pos, &tpe->probes, list) { > > + orig = container_of(pos, struct trace_uprobe, tp); > > + if (comp_inode != d_real_inode(orig->path.dentry) || > > + comp->offset != orig->offset) > > + continue; > > + > > + /* > > + * trace_probe_compare_arg_type() ensured that nr_args and > > + * each argument name and type are same. Let's compare comm. > > + */ > > + for (i = 0; i < orig->tp.nr_args; i++) { > > + if (strcmp(orig->tp.args[i].comm, > > + comp->tp.args[i].comm)) > > + continue; > > Same as above. > > > + } > > + > > + return true; > > + } > > + > > + return false; > > +} > > + > > -- > Thanks and Regards > Srikar Dronamraju >
-- Masami Hiramatsu <mhiramat@kernel.org>
| |