Re: [PATCH v3 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls

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

On Fri, May 24, 2024 at 11:32:50AM +0100, Alejandro Vallejo wrote:
> On 23/05/2024 11:21, Roger Pau Monné wrote:
> > On Thu, May 23, 2024 at 10:41:29AM +0100, Alejandro Vallejo wrote:
> >> -int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *p,
> >> -                            xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
> >> -                            xen_msr_entry_t *msrs, uint32_t *nr_msrs)
> >> +int xc_cpu_policy_serialise(xc_interface *xch, xc_cpu_policy_t *p)
> >>  {
> >> +    unsigned int nr_leaves = ARRAY_SIZE(p->leaves);
> >> +    unsigned int nr_msrs = ARRAY_SIZE(p->msrs);
> >>      int rc;
> >>  
> >> -    if ( leaves )
> >> +    rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
> >> +    if ( rc )
> >>      {
> >> -        rc = x86_cpuid_copy_to_buffer(&p->policy, leaves, nr_leaves);
> >> -        if ( rc )
> >> -        {
> >> -            ERROR("Failed to serialize CPUID policy");
> >> -            errno = -rc;
> >> -            return -1;
> >> -        }
> >> +        ERROR("Failed to serialize CPUID policy");
> >> +        errno = -rc;
> >> +        return -1;
> >>      }
> >>  
> >> -    if ( msrs )
> >> +    p->nr_leaves = nr_leaves;
> > 
> > Nit: FWIW, I think you could avoid having to introduce local
> > nr_{leaves,msrs} variables and just use p->nr_{leaves,msrs}?  By
> > setting them to ARRAY_SIZE() at the top of the function and then
> > letting x86_{cpuid,msr}_copy_to_buffer() adjust as necessary.
> > 
> > Thanks, Roger.
> 
> The intent was to avoid mutating the policy object in the error cases
> during deserialise. Then I adjusted the serialise case to have symmetry.

It's currently unavoidable for the policy to be likely mutated even in
case of error, as x86_{cpuid,msr}_copy_to_buffer() are two separate
operations, and hence the first succeeding but the second failing will
already result in the policy being mutated on error.

> It's true the preservation is not meaningful in the serialise case
> because at that point the serialised form is already corrupted.
> 
> I don't mind either way. Seeing how I'm sending one final version with
> the comments of patch2 I'll just adjust as you proposed.

I'm fine either way (hence why prefix it with "nit:") albeit I have a
preference for not introducing the local variables if they are not
needed.

Thanks, Roger.