On Tue, Apr 13, 2021 at 04:01:19PM +0200, Roger Pau Monne wrote:
> Also change libxl__cpuid_legacy to propagate the error from
> xc_cpuid_apply_policy into callers.
>
> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> ---
> Changes since 1:
> - Return ERROR_FAIL on error.
> ---
> tools/libs/light/libxl_cpuid.c | 15 +++++++++++----
> tools/libs/light/libxl_create.c | 5 +++--
> tools/libs/light/libxl_dom.c | 2 +-
> tools/libs/light/libxl_internal.h | 4 ++--
> tools/libs/light/libxl_nocpuid.c | 5 +++--
> 5 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index 289c59c7420..539fc4869e6 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -419,11 +419,13 @@ int
> libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
> return 0;
> }
>
> -void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
> - libxl_domain_build_info *info)
> +int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
> + libxl_domain_build_info *info)
> {
> + GC_INIT(ctx);
> bool pae = true;
> bool itsc;
> + int rc;
>
> /*
> * Gross hack. Using libxl_defbool_val() here causes libvirt to crash in
> @@ -462,8 +464,13 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
> bool restore,
> itsc = (libxl_defbool_val(info->disable_migrate) ||
> info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
>
> - xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> - pae, itsc, nested_virt, info->cpuid);
> + rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> + pae, itsc, nested_virt, info->cpuid);
> + if (rc)
> + LOGE(ERROR, "Failed to apply CPUID policy");
Is logging `errno` is going to be accurate enough ? The
xc_cpuid_apply_policy() seems to only set `rc` and never change `errno`
directly. It would often return "-errno" or an error code. There's one
instance where we have "rc = -EOPNOTSUPP" but `errno` doesn't seems to
be change to the same value (when checking "featureset).
So maybe having "LOGEV(ERROR, -rc, ...)" would be better?
And it should be `r` instead of `rc`. The latter is for libxl error
code, the former for system call/libxc.
> +
> + GC_FREE;
> + return rc ? ERROR_FAIL : 0;
The rest looks good.
Thanks,
--
Anthony PERARD
