Re: [PATCH v2 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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.pau@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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