Ping: [PATCH] libxc/PM: correct (not just) error handling in xc_get_cpufreq_para()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2025 14:32, Jan Beulich wrote:
> From their introduction all xc_hypercall_bounce_pre() uses, when they
> failed, would properly cause exit from the function including cleanup,
> yet without informing the caller of the failure. Purge the unlock_1
> label for being both pointless and mis-named.
> 
> An earlier attempt to switch to the usual split between return value and
> errno wasn't quite complete.
> 
> HWP work made the cleanup of the "available governors" array
> conditional, neglecting the fact that the condition used may not be the
> condition that was used to allocate the buffer (as the structure field
> is updated upon getting back EAGAIN). Throughout the function, use the
> local variable being introduced to address that.
> 
> Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall 
> buffers")
> Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error 
> and stash error in errno")
> Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

May I ask for an ack or comments towards what needs changing?

Thanks, Jan

> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc
>      DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors,
>                        user_para->scaling_available_governors,
>                        user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), 
> XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> -
> -    bool has_num = user_para->cpu_num &&
> -                     user_para->freq_num &&
> -                     user_para->gov_num;
> +    unsigned int in_gov_num = user_para->gov_num;
> +    bool has_num = user_para->cpu_num && user_para->freq_num;
>  
>      if ( has_num )
>      {
>          if ( (!user_para->affected_cpus)                    ||
>               (!user_para->scaling_available_frequencies)    ||
> -             (user_para->gov_num && !user_para->scaling_available_governors) 
> )
> +             (in_gov_num && !user_para->scaling_available_governors) )
>          {
>              errno = EINVAL;
>              return -1;
>          }
> -        if ( xc_hypercall_bounce_pre(xch, affected_cpus) )
> -            goto unlock_1;
> -        if ( xc_hypercall_bounce_pre(xch, scaling_available_frequencies) )
> +        ret = xc_hypercall_bounce_pre(xch, affected_cpus);
> +        if ( ret )
> +            return ret;
> +        ret = xc_hypercall_bounce_pre(xch, scaling_available_frequencies);
> +        if ( ret )
>              goto unlock_2;
> -        if ( user_para->gov_num &&
> -             xc_hypercall_bounce_pre(xch, scaling_available_governors) )
> +        if ( in_gov_num )
> +            ret = xc_hypercall_bounce_pre(xch, scaling_available_governors);
> +        if ( ret )
>              goto unlock_3;
>  
>          set_xen_guest_handle(sys_para->affected_cpus, affected_cpus);
>          set_xen_guest_handle(sys_para->scaling_available_frequencies, 
> scaling_available_frequencies);
> -        if ( user_para->gov_num )
> +        if ( in_gov_num )
>              set_xen_guest_handle(sys_para->scaling_available_governors,
>                                   scaling_available_governors);
>      }
> @@ -246,7 +247,7 @@ int xc_get_cpufreq_para(xc_interface *xc
>      sysctl.u.pm_op.cpuid = cpuid;
>      sys_para->cpu_num  = user_para->cpu_num;
>      sys_para->freq_num = user_para->freq_num;
> -    sys_para->gov_num  = user_para->gov_num;
> +    sys_para->gov_num  = in_gov_num;
>  
>      ret = xc_sysctl(xch, &sysctl);
>      if ( ret )
> @@ -256,12 +257,11 @@ int xc_get_cpufreq_para(xc_interface *xc
>              user_para->cpu_num  = sys_para->cpu_num;
>              user_para->freq_num = sys_para->freq_num;
>              user_para->gov_num  = sys_para->gov_num;
> -            ret = -errno;
>          }
>  
>          if ( has_num )
>              goto unlock_4;
> -        goto unlock_1;
> +        return ret;
>      }
>      else
>      {
> @@ -298,13 +298,13 @@ int xc_get_cpufreq_para(xc_interface *xc
>      }
>  
>  unlock_4:
> -    if ( user_para->gov_num )
> +    if ( in_gov_num )
>          xc_hypercall_bounce_post(xch, scaling_available_governors);
>  unlock_3:
>      xc_hypercall_bounce_post(xch, scaling_available_frequencies);
>  unlock_2:
>      xc_hypercall_bounce_post(xch, affected_cpus);
> -unlock_1:
> +
>      return ret;
>  }
>