Re: [PATCH v2 6/6] libxl: add support for parsing MSR features

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Jul 17, 2023 at 04:46:25PM +0200, Roger Pau Monné wrote:
> On Thu, Jul 13, 2023 at 11:39:53AM +0100, Anthony PERARD wrote:
> > On Tue, Jul 11, 2023 at 11:22:30AM +0200, Roger Pau Monne wrote:
> > > diff --git a/tools/libs/light/libxl_cpuid.c 
> > > b/tools/libs/light/libxl_cpuid.c
> > > index b1c4f8f2f45b..86a08f29a19c 100644
> > > --- a/tools/libs/light/libxl_cpuid.c
> > > +++ b/tools/libs/light/libxl_cpuid.c
> > > @@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
> > >      return 0;
> > >  }
> > >  
> > > +static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, 
> > > uint32_t index)
> > > +{
> > > +    unsigned int i = 0;
> > > +    libxl_cpuid_policy_list policy = *pl;
> > > +
> > > +    if (policy == NULL)
> > > +        policy = *pl = calloc(1, sizeof(*policy));
> > > +
> > > +    if (policy->msr != NULL)
> > > +        for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
> > 
> > Could you add { } for this two blocks? One line after a if() without { }
> > is ok, but not more.
> 
> Sure.
> 
> > > +            if (policy->msr[i].index == index)
> > > +                return &policy->msr[i];
> > > +
> > > +    policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
> > > +    policy->msr[i].index = index;
> > > +    memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) 
> > > - 1);
> > 
> > Is this "array_size() - 1" correct? The -1 need to go, right?
> > 
> > > +    policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
> > 
> > Is it for convenience? Maybe for easier debugging (printf)? Also, I
> > guess having a NUL at the end mean the -1 on the previous statement kind
> > of useful.
> 
> Yes, it's also to match the format of the policy string used by
> xc_xend_cpuid, which also has a terminating zero.
> 
> Are you OK with this?

Yes.

With the other style change done:
Acked-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD