On September 1, 2021 9:58:16 PM GMT-03:00, "Jin, Yao" [off-list ref] wrote:

Hi Arnaldo,

On 9/1/2021 9:57 PM, Arnaldo Carvalho de Melo wrote:

quoted

Em Wed, Sep 01, 2021 at 01:46:01PM +0800, Jin Yao escreveu:
<SNIP>

quoted

+++ b/tools/perf/arch/x86/util/pmu.c

<SNIP>

quoted

+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

quoted

quoted

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