Re: [PATCH v2 2/6] libxl: change the type of libxl_cpuid_policy_list

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 12, 2023 at 05:02:03PM +0100, Anthony PERARD wrote:
> On Tue, Jul 11, 2023 at 11:22:26AM +0200, Roger Pau Monne wrote:
> > -void libxl_cpuid_dispose(libxl_cpuid_policy_list *p_cpuid_list)
> > +void libxl_cpuid_dispose(libxl_cpuid_policy_list *pl)
> >  {
> > -    int i, j;
> > -    libxl_cpuid_policy_list cpuid_list = *p_cpuid_list;
> > +    libxl_cpuid_policy_list policy = *pl;
> >  
> > -    if (cpuid_list == NULL)
> > +    if (policy == NULL)
> >          return;
> > -    for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > -        for (j = 0; j < 4; j++)
> > -            if (cpuid_list[i].policy[j] != NULL) {
> > -                free(cpuid_list[i].policy[j]);
> > -                cpuid_list[i].policy[j] = NULL;
> > -            }
> > +
> > +    if (policy->cpuid) {
> > +        unsigned int i, j;
> > +        struct xc_xend_cpuid *cpuid_list = policy->cpuid;
> > +
> > +        for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) 
> > {
> > +            for (j = 0; j < 4; j++)
> > +                if (cpuid_list[i].policy[j] != NULL) {
> > +                    free(cpuid_list[i].policy[j]);
> > +                    cpuid_list[i].policy[j] = NULL;
> > +                }
> 
> This looks like a lot of work. We could just call
> free(cpuid_list[i].policy[j]) and that's all, as cpuid_list will be gone
> right after the loop.

I wasn't planning on changing the code, as this is just indentation
movement, but will do.

> Also, please add {} for the second "for ()" loop.
> 
> > +        }
> > +        free(policy->cpuid);
> >      }
> 
> Beside some the coding style pointing out, the patch looks fine:
> Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks, Roger.