Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 01 Sep 2021 22:33:38 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To:     "Jin, Yao" <yao.jin@linux.intel.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>
CC:     jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com,
        alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org, ak@linux.intel.com,
        kan.liang@intel.com, yao.jin@intel.com, rickyman7@gmail.com,
        john.garry@huawei.com, Kan Liang <kan.liang@linux.intel.com>
Subject: Re: [PATCH v6 1/2] perf pmu: Add PMU alias support
User-Agent: K-9 Mail for Android
In-Reply-To: <298a4efe-72f3-f99a-3923-939eab44885a@linux.intel.com>
References: <20210901054602.17010-1-yao.jin@linux.intel.com> <20210901054602.17010-2-yao.jin@linux.intel.com> <YS+G5s41RWX0GIqG@kernel.org> <298a4efe-72f3-f99a-3923-939eab44885a@linux.intel.com>
Message-ID: <59B604C2-8E26-4B11-A9E7-F0885058D990@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On September 1, 2021 9:58:16 PM GMT-03:00, "Jin, Yao" <yao=2Ejin@linux=2Ei=
ntel=2Ecom> 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>
>>=20
>>> +++ b/tools/perf/arch/x86/util/pmu=2Ec
>>=20
>> <SNIP>
>>=20
>>> +static int setup_pmu_alias_list(void)
>>> +{
>>> +	char path[PATH_MAX];
>>> +	DIR *dir;
>>> +	struct dirent *dent;
>>> +	const char *sysfs =3D sysfs__mountpoint();
>>> +	struct perf_pmu_alias_name *pmu;
>>> +	char buf[MAX_PMU_NAME_LEN];
>>> +	FILE *file;
>>> +	int ret =3D 0;
>>> +
>>> +	if (!sysfs)
>>> +		return -1;
>>> +
>>> +	snprintf(path, PATH_MAX,
>>> +		 "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
>>> +
>>> +	dir =3D opendir(path);
>>> +	if (!dir)
>>> +		return -1;
>>> +
>>> +	while ((dent =3D readdir(dir))) {
>>> +		if (!strcmp(dent->d_name, "=2E") ||
>>> +		    !strcmp(dent->d_name, "=2E=2E"))
>>> +			continue;
>>> +
>>> +		snprintf(path, PATH_MAX,
>>> +			 TEMPLATE_ALIAS, sysfs, dent->d_name);
>>> +
>>> +		if (!file_available(path))
>>> +			continue;
>>> +
>>> +		file =3D fopen(path, "r");
>>> +		if (!file)
>>> +			continue;
>>> +
>>> +		if (!fgets(buf, sizeof(buf), file)) {
>>> +			fclose(file);
>>> +			continue;
>>> +		}
>>> +
>>> +		fclose(file);
>>> +
>>> +		pmu =3D zalloc(sizeof(*pmu));
>>> +		if (!pmu) {
>>> +			ret =3D -ENOMEM;
>>> +			break;
>>> +		}
>>> +
>>> +		/* Remove the last '\n' */
>>> +		buf[strlen(buf) - 1] =3D 0;
>>> +
>>> +		pmu->alias =3D strdup(buf);
>>> +		if (!pmu->alias)
>>> +			goto mem_err;
>>=20
>> This isn't returning -ENOMEM like when zalloc() fails above=2E Also you=
're
>> mixing 'return -1' with 'return -ENOMEM', please be consistent=2E Pleas=
e
>> find some -E errno for the !sysfs case, perhaps -ENODEV?
>>=20
>
>For opendir() error, can we just return -errno?
>
>dir =3D opendir(path);
>if (!dir)
>	return -errno;

Yeah

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


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

- Arnaldo