Re: [PATCH v6 1/2] perf pmu: Add PMU alias support

[Arnaldo Carvalho de Melo]

On September 1, 2021 9:58:16 PM GMT-03:00, "Jin, Yao" <yao.jin@xxxxxxxxxxxxxxx> wrote:
>Hi Arnaldo,
>
>On 9/1/2021 9:57 PM, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Sep 01, 2021 at 01:46:01PM +0800, Jin Yao escreveu:
>> <SNIP>
>> 
>>> +++ b/tools/perf/arch/x86/util/pmu.c
>> 
>> <SNIP>
>> 
>>> +static int setup_pmu_alias_list(void)
>>> +{
>>> +	char path[PATH_MAX];
>>> +	DIR *dir;
>>> +	struct dirent *dent;
>>> +	const char *sysfs = sysfs__mountpoint();
>>> +	struct perf_pmu_alias_name *pmu;
>>> +	char buf[MAX_PMU_NAME_LEN];
>>> +	FILE *file;
>>> +	int ret = 0;
>>> +
>>> +	if (!sysfs)
>>> +		return -1;
>>> +
>>> +	snprintf(path, PATH_MAX,
>>> +		 "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
>>> +
>>> +	dir = opendir(path);
>>> +	if (!dir)
>>> +		return -1;
>>> +
>>> +	while ((dent = readdir(dir))) {
>>> +		if (!strcmp(dent->d_name, ".") ||
>>> +		    !strcmp(dent->d_name, ".."))
>>> +			continue;
>>> +
>>> +		snprintf(path, PATH_MAX,
>>> +			 TEMPLATE_ALIAS, sysfs, dent->d_name);
>>> +
>>> +		if (!file_available(path))
>>> +			continue;
>>> +
>>> +		file = fopen(path, "r");
>>> +		if (!file)
>>> +			continue;
>>> +
>>> +		if (!fgets(buf, sizeof(buf), file)) {
>>> +			fclose(file);
>>> +			continue;
>>> +		}
>>> +
>>> +		fclose(file);
>>> +
>>> +		pmu = zalloc(sizeof(*pmu));
>>> +		if (!pmu) {
>>> +			ret = -ENOMEM;
>>> +			break;
>>> +		}
>>> +
>>> +		/* Remove the last '\n' */
>>> +		buf[strlen(buf) - 1] = 0;
>>> +
>>> +		pmu->alias = strdup(buf);
>>> +		if (!pmu->alias)
>>> +			goto mem_err;
>> 
>> This isn't returning -ENOMEM like when zalloc() fails above. Also you're
>> mixing 'return -1' with 'return -ENOMEM', please be consistent. Please
>> find some -E errno for the !sysfs case, perhaps -ENODEV?
>> 
>
>For opendir() error, can we just return -errno?
>
>dir = opendir(path);
>if (!dir)
>	return -errno;

Yeah

>
>>> +
>>> +		pmu->name = strdup(dent->d_name);
>>> +		if (!pmu->name)
>>> +			goto mem_err;
>>> +
>>> +		list_add_tail(&pmu->list, &pmu_alias_name_list);
>>> +		continue;
>> 
>> 
>> Don't we have a 'struct pmu' constructor/destructor pair? I.e. instead
>> of doing all this in an open coded way as above, why not have:
>> 
>> void pmu__delete(struct pmu *pmu)
>> {
>> 	if (!pmu)
>> 		return;
>> 
>> 	zfree(&pmu->name);
>> 	zfree(&pmu->alias);
>> 	free(pmu);
>> }
>> 
>> struct pmu *pmu__new(const char *name, const char *alias)
>> {
>> 	struct pmu *pmu = zalloc(sizeof(*pmu));
>> 
>> 	if (pmu) {
>> 		pmu->name = strdup(name);
>> 		if (!pmu->name)
>> 			goto out_delete;
>> 
>> 		pmu->alias = strdup(alias);
>> 		if (!pmu->alias)
>> 			goto out_delete;
>> 	}
>> 
>> 	return pmu;
>> out_err:
>> 	pmu__delete(pmu);
>> 	return NULL;
>> }
>> 
>> 	And then just:
>> 
>> 	pmu = pmu__new(dent->d_name, buf);
>> 	if (!pmu)
>> 		goto out_closedir;
>> 
>> 	list_add_tail(&pmu->list, &pmu_alias_name_list);
>> 
>> And then you don't need the 'continue', as this is the end of the loop
>> block.
>> 		
>> That 'ret' probably should start with -ENOMEM and you end the function
>> with:
>> 
>> 	ret = 0;
>> out_closedir:
>> 	closedir(dir);
>> 	return ret;
>> }
>
>Yes, using 'struct pmu' constructor/destructor is absolutely a good design.
>
>I will follow this approach.


I've read the other message you sent, so do the constructor/destructor for the right struct, that long named one.

- Arnaldo