Re: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string

[Liang, Kan]

On 2023-04-24 1:27 p.m., Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 24, 2023 at 02:25:27PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Thu, Apr 20, 2023 at 05:16:18PM -0400, Liang, Kan escreveu:
>>>
>>>
>>> On 2023-04-20 3:28 p.m., Arnaldo Carvalho de Melo wrote:
>>>> Em Thu, Apr 20, 2023 at 04:10:30PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>> Em Thu, Apr 20, 2023 at 03:57:55PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>> This makes the logic a bit clear by avoiding the !strcmp() pattern and
>>>>>> also a way to intercept the pointer if we need to do extra validation on
>>>>>> it or to do lazy setting of evsel->name via evsel__name(evsel).
>>>>>
>>>>> + this, looking if there are others...
>>>>
>>>> Somehow the first message didn't go thru, so below is the combined
>>>> patch, this is an effort to avoid accessing evsel->name directly as the
>>>> preferred way to get an evsel name is evsel__name(), so looking for
>>>> direct access and providing accessors that avoid that.
>>>
>>> One more
>>>
>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>> index 2260e27adf44..3a960a3f6962 100644
>>> --- a/tools/perf/util/evlist.c
>>> +++ b/tools/perf/util/evlist.c
>>> @@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char
>>> *evsel_name)
>>>  		return 0;
>>>  	if (evsel__is_dummy_event(pos))
>>>  		return 1;
>>> -	return strcmp(pos->name, evsel_name);
>>> +	return !evsel__name_is(pos, evsel_name);
>>>  }
>>>
>>>  static int evlist__is_enabled(struct evlist *evlist)
>>
>> Added
>>  
>>>>
>>>> From e60455d6a4e35ba0c376966443294586a1adc3ec Mon Sep 17 00:00:00 2001
>>>> From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>>>> Date: Thu, 20 Apr 2023 15:54:11 -0300
>>>> Subject: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if
>>>>  the evsel name is equal to a given string
>>>>
>>>> This makes the logic a bit clear by avoiding the !strcmp() pattern and
>>>> also a way to intercept the pointer if we need to do extra validation on
>>>> it or to do lazy setting of evsel->name via evsel__name(evsel).
>>>>
>>>> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>>>> Cc: Ian Rogers <irogers@xxxxxxxxxx>
>>>> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>>>> Cc: "Liang, Kan" <kan.liang@xxxxxxxxxxxxxxx>
>>>> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
>>>> Link: https://lore.kernel.org/lkml/ZEGLM8VehJbS0gP2@xxxxxxxxxx
>>>> Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>>>
>>> With the above one,
>>>
>>> Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>
>> Added these extra ones and actually made evsel__name_is() use
>> evsel__name().
>>
>> Does your reviewed-by stands after these extra changes?
>

Yes, the below extra changes look good to me as well.

Thanks,
Kan

> 
> oops, fix that pos->name leftover:
> 
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 2260e27adf44c579..a0504316b06fbcba 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char *evsel_name)
>  		return 0;
>  	if (evsel__is_dummy_event(pos))
>  		return 1;
> -	return strcmp(pos->name, evsel_name);
> +	return !evsel__name_is(pos, evsel_name);
>  }
>  
>  static int evlist__is_enabled(struct evlist *evlist)
> @@ -1706,7 +1706,7 @@ struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
>  	evlist__for_each_entry(evlist, evsel) {
>  		if (!evsel->name)
>  			continue;
> -		if (strcmp(str, evsel->name) == 0)
> +		if (evsel__name_is(evsel, str))
>  			return evsel;
>  	}
>  
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 81b854650160c2b0..356c07f03be6bfce 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -823,7 +823,7 @@ const char *evsel__name(struct evsel *evsel)
>  
>  bool evsel__name_is(struct evsel *evsel, const char *name)
>  {
> -	return !strcmp(evsel->name, name);
> +	return !strcmp(evsel__name(evsel), name);
>  }
>  
>  const char *evsel__group_pmu_name(const struct evsel *evsel)
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 31b1cd0935e277ba..650cd8df40412a38 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -2893,7 +2893,7 @@ static struct evsel *find_evsel(struct evlist *evlist, char *event_name)
>  	full_name = !!strchr(event_name, ':');
>  	evlist__for_each_entry(evlist, pos) {
>  		/* case 2 */
> -		if (full_name && !strcmp(pos->name, event_name))
> +		if (full_name && evsel__name_is(pos, event_name))
>  			return pos;
>  		/* case 3 */
>  		if (!full_name && strstr(pos->name, event_name)) {