2016-03-02 17:39:35

by Steven Rostedt

Subject: Re: [PATCH v15 10/23] tracing: Add hist trigger 'execname' modifier

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_*

?

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

2016-03-02 19:52:03

by Tom Zanussi

Subject: Re: [PATCH v15 10/23] tracing: Add hist trigger 'execname' modifier

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;