Hi Steve,
On Wed, 2016-03-02 at 12:39 -0500, Steven Rostedt wrote:
> On Fri, 26 Feb 2016 10:01:13 -0600
> Tom Zanussi <[email protected]> wrote:
>
>
> > +static void hist_trigger_elt_copy(struct tracing_map_elt *to,
> > + struct tracing_map_elt *from)
> > +{
> > + char *comm_from = from->private_data;
> > + char *comm_to = to->private_data;
> > +
> > + if (comm_from)
> > + memcpy(comm_to, comm_from, TASK_COMM_LEN + 1);
> > +}
> > +
> > +static void hist_trigger_elt_init(struct tracing_map_elt *elt)
> > +{
> > + char *comm = elt->private_data;
> > +
> > + if (comm)
> > + save_comm(comm, current);
> > +}
> > +
> > +static const struct tracing_map_ops hist_trigger_ops = {
> > + .elt_alloc = hist_trigger_elt_alloc,
> > + .elt_copy = hist_trigger_elt_copy,
> > + .elt_free = hist_trigger_elt_free,
> > + .elt_init = hist_trigger_elt_init,
>
> These are only used for saving or displaying comm. Wouldn't adding that
> in the name be better. Otherwise it looks like they are more generic. I
> find that dangerous, especially since they just assume that the
> private_data is a string.
>
> What about hist_trigger_elt_comm_*
>
> ?
Yeah, I think that makes it clearer - I'll rename those to be more
explicit.
Thanks,
Tom
>
> -- Steve
>
> > +};
> > +
> > static void destroy_hist_field(struct hist_field *hist_field)
> > {
> > kfree(hist_field);
> > @@ -399,6 +467,9 @@ static int create_key_field(struct hist_trigger_data
> > *hist_data,
> > flags |= HIST_FIELD_FL_SYM;
> > else if (strcmp(field_str, "sym-offset") == 0)
> > flags |= HIST_FIELD_FL_SYM_OFFSET;
> > + else if ((strcmp(field_str, "execname") == 0) &&
> > + (strcmp(field_name, "common_pid") == 0))
> > + flags |= HIST_FIELD_FL_EXECNAME;
> > else {
> > ret = -EINVAL;
> > goto out;
