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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/06/2021 12:47, 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,
> +                                   XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> +                                   NULL, 0, &shadow, 0, NULL);
> +
> +        if (rc) {
> +            LOGED(ERROR, domid,
> +                  "Failed to set %s allocation: %d (errno:%d)\n",
> +                  libxl_defbool_val(d_config->c_info.hap) ? "HAP" : "shadow",

The error message wants to include the value of shadow, just in case the
cause of the failure is because the value is stupidly high.

Having traced the number through, the local variable wants to be named
shadow_mb to try and make the units clearer.  (Also - why on earth do we
have a hypercall which takes integer units of MB, and a memkb field in
the config file...)

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>