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

[Arnaldo Carvalho de Melo]

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@xxxxxxxxxxxxxxx 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