Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/3] perf alias: Rebuild alias expression string to make
 it comparable
To:     Paul Clarke <pc@us.ibm.com>, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org, acme@kernel.org, jolsa@redhat.com
Cc:     brueckner@linux.vnet.ibm.com, schwidefsky@de.ibm.com,
        heiko.carstens@de.ibm.com
References: <20180614114845.41221-1-tmricht@linux.ibm.com>
 <20180614114845.41221-2-tmricht@linux.ibm.com>
 <809ff562-511d-26e9-bac3-e8f7b84666e5@us.ibm.com>
From:   Thomas-Mich Richter <tmricht@linux.ibm.com>
Organization: IBM LTC
Date:   Thu, 14 Jun 2018 16:16:09 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <809ff562-511d-26e9-bac3-e8f7b84666e5@us.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Message-Id: <763acee6-ab63-9f76-07a5-528ed7c9906e@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 06/14/2018 03:53 PM, Paul Clarke wrote:
> On 06/14/2018 06:48 AM, Thomas Richter wrote:
>> PMU alias definitions in sysfs files may have spaces, newlines
>> and number with leading zeroes. Same alias definitions may
>> also appear in JSON files without spaces, etc.
>>
>> Scan alias definitions and remove leading zeroes, spaces,
>> newlines, etc and rebuild string to make alias->str member
>> comparable.
>>
>> s390 for example  has terms specified as
>> event=0x0091 (read from files ../<PMU>/events/<FILE>
>> and terms specified as event=0x91 (read from JSON files).
>>
>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>> ---
>>  tools/perf/util/pmu.c | 25 ++++++++++++++++++++++++-
>>  1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 26c79a9c4142..da8f243743d3 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -241,9 +241,11 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
>>  				 char *metric_expr,
>>  				 char *metric_name)
>>  {
>> +	struct parse_events_term *term;
>>  	struct perf_pmu_alias *alias;
>>  	int ret;
>>  	int num;
>> +	char newval[256];
> 
> How was 256 chosen?

I copied this from function perf_pmu__new_alias() which also uses:

char buf[256];

Looking a the sysfs file contents this seems to be sufficient.
This number comes from commit long time ago.

> 
>>
>>  	alias = malloc(sizeof(*alias));
>>  	if (!alias)
>> @@ -262,6 +264,27 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
>>  		return ret;
>>  	}
>>
>> +	/* Scan event and remove leading zeroes, spaces, newlines, some
>> +	 * platforms have terms specified as
>> +	 * event=0x0091 (read from files ../<PMU>/events/<FILE>
>> +	 * and terms specified as event=0x91 (read from JSON files).
>> +	 *
>> +	 * Rebuild string to make alias->str member comparable.
>> +	 */
>> +	memset(newval, 0, sizeof(newval));
>> +	ret = 0;
>> +	list_for_each_entry(term, &alias->terms, list) {
>> +		if (ret)
>> +			ret += scnprintf(newval + ret, sizeof(newval) - ret,
>> +					 ",");
>> +		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
>> +			ret += scnprintf(newval + ret, sizeof(newval) - ret,
>> +					 "%s=%#x", term->config, term->val.num);
>> +		else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
>> +			ret += scnprintf(newval + ret, sizeof(newval) - ret,
>> +					 "%s=%s", term->config, term->val.str);
> 
> If we exceed 256, we just suddenly terminate the rebuilding without reporting any issues.

Correct the string would be truncated, but see above, it would not have been read correctly
anyway.

> 
>> +	}
>> +
>>  	alias->name = strdup(name);
>>  	if (dir) {
>>  		/*
>> @@ -285,7 +308,7 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
>>  		snprintf(alias->unit, sizeof(alias->unit), "%s", unit);
>>  	}
>>  	alias->per_pkg = perpkg && sscanf(perpkg, "%d", &num) == 1 && num == 1;
>> -	alias->str = strdup(val);
>> +	alias->str = strdup(newval);
>>
>>  	list_add_tail(&alias->list, list);
>>
> 
> PC
> 

-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294