linux-kernel - Re: [PATCH V2 1/9] perf pmu: Add support for PMU capabilities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 10 Mar 2020 14:30:09 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>
Cc:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
        jolsa@...hat.com, peterz@...radead.org, mingo@...hat.com,
        linux-kernel@...r.kernel.org, namhyung@...nel.org,
        adrian.hunter@...el.com, mathieu.poirier@...aro.org,
        ravi.bangoria@...ux.ibm.com, alexey.budankov@...ux.intel.com,
        vitaly.slobodskoy@...el.com, pavel.gerasimov@...el.com,
        mpe@...erman.id.au, eranian@...gle.com, ak@...ux.intel.com
Subject: Re: [PATCH V2 1/9] perf pmu: Add support for PMU capabilities

Em Tue, Mar 10, 2020 at 12:54:05PM -0400, Liang, Kan escreveu:
> 
> 
> On 3/10/2020 10:04 AM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Mar 10, 2020 at 09:53:24AM -0400, Liang, Kan escreveu:
> > > On 3/10/2020 9:06 AM, Arnaldo Carvalho de Melo wrote:
> > > > Em Mon, Mar 09, 2020 at 10:46:31AM -0700, kan.liang@...ux.intel.com escreveu:
> > > > > +static int perf_pmu__new_caps(struct list_head *list, char *name, char *value)
> > > > > +{
> > > > > +	struct perf_pmu_caps *caps;
> > > > > +
> > > > > +	caps = zalloc(sizeof(*caps));
> > > > > +	if (!caps)
> > > > > +		return -ENOMEM;
> > 
> > > > So here you check if zalloc fails and returns a proper error
> > 
> > > > > +	caps->name = strdup(name);
> > > > > +	caps->value = strndup(value, strlen(value) - 1);
> > 
> > > > But then you don't check strdup()?
> > > Right, I should check strdup(), otherwise the capability information may be
> > > incomplete. I will fix it in V3.
> > 
> > Thanks, overall just consider making the patches smaller if possible,
> > with prep patches paving the way for more complex changes so that
> > reviewing becomes easier, for instance:
> > 
> >    perf machine: Refine the function for LBR call stack reconstruction
> > 
> > Seems to do too many things at once. It was unfortunate, for instance,
> > that the pre-existing code had that
> > 
> > resolve_lbr_callchain_sample()
> > {
> > 	/* LBR only affects the user callchain */
> > 	if (i != chain_nr) {
> > 		body of the function, long
> > 		....
> > 		return err;
> > 	}
> > 
> > 	return 0;
> > }
> > 
> > One of the things you did in this patch was to the more sensible:
> > 
> > 	/* LBR only affects the user callchain */
> > 	if (i == chain_nr)
> > 		return 0;
> > 
> > 	body of the function
> > 	...
> > 	return err;
> > 
> > So if you had a prep patch at this point just removing that silly
> > indent, then we would see that that is just removing the indent, the
> > next patch wouldn't have that check for user callchains, would be
> > smaller, I think that would help reduce the patch sizes.

> > Then if you just moved to a separate function the (callchain_param.order
> > == ORDER_CALLEE) part, the patch would again be smaller, etc.
 
> > This helps reviewing and usually helps us later, with bisection, when
> > some bug is introduced,
 
> Sure, I will go through all patches and see what I can do to reduce the size
> of patches in V3.

Thanks a lot for considering my suggestions!

- Arnaldo