Re: [PATCH] libxl/x86: check return value of SHADOW_OP_SET_ALLOCATION domctl

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Jun 28, 2021 at 01:47:03PM +0200, Jan Beulich wrote:
> The hypervisor may not have enough memory to satisfy the request.
> 
> Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Especially if the request was mostly fulfilled, guests may have done
> fine despite the failure, so there is a risk of perceived regressions
> here. But not checking the error at all was certainly wrong.
> 
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -531,8 +531,18 @@ int libxl__arch_domain_create(libxl__gc
>      if (d_config->b_info.type != LIBXL_DOMAIN_TYPE_PV) {
>          unsigned long shadow = DIV_ROUNDUP(d_config->b_info.shadow_memkb,
>                                             1024);
> -        xc_shadow_control(ctx->xch, domid, 
> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> -                          NULL, 0, &shadow, 0, NULL);
> +        int rc = xc_shadow_control(ctx->xch, domid,

Could you use 'r' instead of 'rc' ? The later is reserved for libxl
error codes while the former is for system and libxc calls.

> +                                   XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> +                                   NULL, 0, &shadow, 0, NULL);
> +
> +        if (rc) {

xc_shadow_control seems to return "domctl.u.shadow_op.pages" in some
cases, are all non-zero return value errors?

> +            LOGED(ERROR, domid,
> +                  "Failed to set %s allocation: %d (errno:%d)\n",

LOGED already prints prints the meaning of the "errno" value, so we
don't need to log it.

> +                  libxl_defbool_val(d_config->c_info.hap) ? "HAP" : "shadow",
> +                  rc, errno);

Is the return value of xc_shadow_control() actually useful when errno is
already logged?

Thanks,

-- 
Anthony PERARD