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 <jbeul...@suse.com>
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;
> }
>
