Re: [PATCH 1/4] perf evsel: Add evsel__clone() function

[Namhyung Kim]

Hi Jiri,

On Fri, Sep 18, 2020 at 10:31 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> On Wed, Sep 16, 2020 at 03:31:26PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +struct evsel *evsel__clone(struct evsel *orig)
> > +{
> > +     struct evsel *evsel;
> > +     struct evsel_config_term *pos, *tmp;
> > +
> > +     BUG_ON(orig->core.fd);
> > +     BUG_ON(orig->counts);
> > +     BUG_ON(orig->priv);
> > +     BUG_ON(orig->per_pkg_mask);
> > +
> > +     /* cannot handle BPF objects for now */
> > +     if (orig->bpf_obj)
> > +             return NULL;
> > +
> > +     evsel = evsel__new(&orig->core.attr);
> > +     if (evsel == NULL)
> > +             return NULL;
> > +
> > +     evsel->core.cpus = perf_cpu_map__get(orig->core.cpus);
> > +     evsel->core.own_cpus = perf_cpu_map__get(orig->core.own_cpus);
> > +     evsel->core.threads = perf_thread_map__get(orig->core.threads);
> > +     evsel->core.nr_members = orig->core.nr_members;
> > +     evsel->core.system_wide = orig->core.system_wide;
> > +
> > +     if (orig->name)
> > +             evsel->name = strdup(orig->name);
> > +     if (orig->group_name)
> > +             evsel->group_name = strdup(orig->group_name);
> > +     if (orig->pmu_name)
> > +             evsel->pmu_name = strdup(orig->pmu_name);
> > +     if (orig->filter)
> > +             evsel->filter = strdup(orig->filter);
>
> we should check those strdup results

ok.

>
> > +     evsel->cgrp = cgroup__get(orig->cgrp);
> > +     evsel->tp_format = orig->tp_format;
> > +     evsel->handler = orig->handler;
> > +     evsel->leader = orig->leader;
> > +
> > +     evsel->max_events = orig->max_events;
> > +     evsel->tool_event = orig->tool_event;
> > +     evsel->unit = orig->unit;
> > +     evsel->scale = orig->scale;
> > +     evsel->snapshot = orig->snapshot;
> > +     evsel->per_pkg = orig->per_pkg;
> > +     evsel->percore = orig->percore;
> > +     evsel->precise_max = orig->precise_max;
> > +     evsel->use_uncore_alias = orig->use_uncore_alias;
> > +     evsel->is_libpfm_event = orig->is_libpfm_event;
> > +
> > +     evsel->exclude_GH = orig->exclude_GH;
> > +     evsel->sample_read = orig->sample_read;
> > +     evsel->auto_merge_stats = orig->auto_merge_stats;
> > +     evsel->collect_stat = orig->collect_stat;
> > +     evsel->weak_group = orig->weak_group;
>
> so all those evsel's members are possibly defined in parse time right?
> perhaps we should separate them in the struct? and make some note about
> evsel__clone function that new members should be considered for copy
> in evsel__close.. or something like that

Sounds good.

>
> > +
> > +     list_for_each_entry(pos, &orig->config_terms, list) {
> > +             tmp = malloc(sizeof(*tmp));
> > +             if (tmp == NULL) {
> > +                     evsel__delete(evsel);
> > +                     evsel = NULL;
> > +                     break;
> > +             }
> > +
> > +             *tmp = *pos;
> > +             if (tmp->free_str) {
> > +                     tmp->val.str = strdup(pos->val.str);
> > +                     if (tmp->val.str == NULL) {
> > +                             evsel__delete(evsel);
> > +                             evsel = NULL;
> > +                             free(tmp);
> > +                             break;
> > +                     }
> > +             }
> > +             list_add_tail(&tmp->list, &evsel->config_terms);
> > +     }
>
> could this go in separate function? copy_terms

Will do.

Thanks
Namhyung

>
> > +
> > +     return evsel;
> > +}
> > +
> >  /*
> >   * Returns pointer with encoded error via <linux/err.h> interface.
> >   */
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 35e3f6d66085..507c31d6a389 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -169,6 +169,7 @@ static inline struct evsel *evsel__new(struct perf_event_attr *attr)
> >       return evsel__new_idx(attr, 0);
> >  }
> >
> > +struct evsel *evsel__clone(struct evsel *orig);
> >  struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
> >
> >  /*
> > --
> > 2.28.0.618.gf4bc123cb7-goog
> >
>