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

Arnaldo Carvalho de Melo
2020-03-10T17:30:21Z

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@linux.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