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

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

[Jan Beulich]

The hypervisor may not have enough memory to satisfy the request.

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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",
+                  rc, errno);
+            ret = ERROR_FAIL;
+            goto out;
+        }
     }
 
     if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&

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

[Andrew Cooper]

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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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@citrix.com>

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

[Anthony PERARD]

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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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

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

[Jan Beulich]

On 01.07.2021 11:36, Anthony PERARD wrote:
> On Mon, Jun 28, 2021 at 01:47:03PM +0200, Jan Beulich wrote:
>> --- 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.

Of course I can, but I did look at the rest of the function and
found that it uses "ret" for the purpose of what you now say
"rc" ought to be used for. Seeing "ret", I decided to avoid it
(knowing you use different names for different kinds of return
values). While I've switched to "r" for now, I'd be rather
inclined to re-use "ret" instead. (Or actually, as per the
remark further down, I can get away without any local variable
then.)

>> +                                   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?

Indeed it does, but (a) we pass in zero here and (b) this
operation doesn't alter (nor even care about) the value. So I'd
prefer to stick to what I have, but if you tell me to switch to
"... < 0", I will.

>> +            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.

I see. Please note that again I took neighboring code (a few lines
down) for reference. Judging from other call sites (not the one
right below here) I infer I also shouldn't have \n in the format
string?

>> +                  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?

I don't know. Again what I had matches what can be found a few
lines down in the same function. But looking at other uses (in
other files) I'm getting the impression that it's useless -
dropped.

Jan

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

[Anthony PERARD]

On Fri, Jul 02, 2021 at 02:29:31PM +0200, Jan Beulich wrote:
> On 01.07.2021 11:36, Anthony PERARD wrote:
> > On Mon, Jun 28, 2021 at 01:47:03PM +0200, Jan Beulich wrote:
> >> --- 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.
> 
> Of course I can, but I did look at the rest of the function and
> found that it uses "ret" for the purpose of what you now say
> "rc" ought to be used for. Seeing "ret", I decided to avoid it
> (knowing you use different names for different kinds of return
> values). While I've switched to "r" for now, I'd be rather
> inclined to re-use "ret" instead. (Or actually, as per the
> remark further down, I can get away without any local variable
> then.)

I know there's quite a few (many?) coding style issue in libxl. I'm
trying to prevent new issue without asking to fix the existing one.
The use of "ret" is an already existing issue, so I'm fine with it been
use in this patch for libxl error code in the function.

BTW, you still need to store the return value of xc_shadow_control()
into a "r" variable before checking it for error.

> 
> >> +                                   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?
> 
> Indeed it does, but (a) we pass in zero here and (b) this
> operation doesn't alter (nor even care about) the value. So I'd
> prefer to stick to what I have, but if you tell me to switch to
> "... < 0", I will.

That's fine, no need to change.

> 
> >> +            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.
> 
> I see. Please note that again I took neighboring code (a few lines
> down) for reference. Judging from other call sites (not the one
> right below here) I infer I also shouldn't have \n in the format
> string?

Ah, indeed, the '\n' isn't needed.

> >> +                  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?
> 
> I don't know. Again what I had matches what can be found a few
> lines down in the same function. But looking at other uses (in
> other files) I'm getting the impression that it's useless -
> dropped.

Whether or not the return value is useful to be logged depends only on
xc_shadow_control(). But thanks.

Thanks,

-- 
Anthony PERARD

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

[Jan Beulich]

On 02.07.2021 16:46, Anthony PERARD wrote:
> On Fri, Jul 02, 2021 at 02:29:31PM +0200, Jan Beulich wrote:
>> On 01.07.2021 11:36, Anthony PERARD wrote:
>>> On Mon, Jun 28, 2021 at 01:47:03PM +0200, Jan Beulich wrote:
>>>> --- 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.
>>
>> Of course I can, but I did look at the rest of the function and
>> found that it uses "ret" for the purpose of what you now say
>> "rc" ought to be used for. Seeing "ret", I decided to avoid it
>> (knowing you use different names for different kinds of return
>> values). While I've switched to "r" for now, I'd be rather
>> inclined to re-use "ret" instead. (Or actually, as per the
>> remark further down, I can get away without any local variable
>> then.)
> 
> I know there's quite a few (many?) coding style issue in libxl. I'm
> trying to prevent new issue without asking to fix the existing one.
> The use of "ret" is an already existing issue, so I'm fine with it been
> use in this patch for libxl error code in the function.
> 
> BTW, you still need to store the return value of xc_shadow_control()
> into a "r" variable before checking it for error.

Are you saying that

        if (xc_shadow_control(ctx->xch, domid,
                              XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
                              NULL, 0, &shadow_mb, 0, NULL)) {

is not acceptable, style-wise? If indeed you are, please disambiguate
your statement above regarding the use of "ret": May I or may I not
use it? IOW do I need to introduce "r", or can I get away with the
existing local variables.

Jan

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

[Jan Beulich]

On 02.07.2021 17:12, Jan Beulich wrote:
> On 02.07.2021 16:46, Anthony PERARD wrote:
>> On Fri, Jul 02, 2021 at 02:29:31PM +0200, Jan Beulich wrote:
>>> On 01.07.2021 11:36, Anthony PERARD wrote:
>>>> On Mon, Jun 28, 2021 at 01:47:03PM +0200, Jan Beulich wrote:
>>>>> --- 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.
>>>
>>> Of course I can, but I did look at the rest of the function and
>>> found that it uses "ret" for the purpose of what you now say
>>> "rc" ought to be used for. Seeing "ret", I decided to avoid it
>>> (knowing you use different names for different kinds of return
>>> values). While I've switched to "r" for now, I'd be rather
>>> inclined to re-use "ret" instead. (Or actually, as per the
>>> remark further down, I can get away without any local variable
>>> then.)
>>
>> I know there's quite a few (many?) coding style issue in libxl. I'm
>> trying to prevent new issue without asking to fix the existing one.
>> The use of "ret" is an already existing issue, so I'm fine with it been
>> use in this patch for libxl error code in the function.
>>
>> BTW, you still need to store the return value of xc_shadow_control()
>> into a "r" variable before checking it for error.
> 
> Are you saying that
> 
>         if (xc_shadow_control(ctx->xch, domid,
>                               XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
>                               NULL, 0, &shadow_mb, 0, NULL)) {
> 
> is not acceptable, style-wise?

Oh, there is indeed such a rule under "ERROR HANDLING". Which means ...

> If indeed you are, please disambiguate
> your statement above regarding the use of "ret": May I or may I not
> use it? IOW do I need to introduce "r", or can I get away with the
> existing local variables.

... I need this to be clarified.

Jan

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

[Anthony PERARD]

On Fri, Jul 02, 2021 at 05:14:40PM +0200, Jan Beulich wrote:
> On 02.07.2021 17:12, Jan Beulich wrote:
> > On 02.07.2021 16:46, Anthony PERARD wrote:
> >> On Fri, Jul 02, 2021 at 02:29:31PM +0200, Jan Beulich wrote:
> >>> On 01.07.2021 11:36, Anthony PERARD wrote:
> >>>> On Mon, Jun 28, 2021 at 01:47:03PM +0200, Jan Beulich wrote:
> >>>>> --- 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.
> >>>
> >>> Of course I can, but I did look at the rest of the function and
> >>> found that it uses "ret" for the purpose of what you now say
> >>> "rc" ought to be used for. Seeing "ret", I decided to avoid it
> >>> (knowing you use different names for different kinds of return
> >>> values). While I've switched to "r" for now, I'd be rather
> >>> inclined to re-use "ret" instead. (Or actually, as per the
> >>> remark further down, I can get away without any local variable
> >>> then.)
> >>
> >> I know there's quite a few (many?) coding style issue in libxl. I'm
> >> trying to prevent new issue without asking to fix the existing one.
> >> The use of "ret" is an already existing issue, so I'm fine with it been
> >> use in this patch for libxl error code in the function.
> >>
> >> BTW, you still need to store the return value of xc_shadow_control()
> >> into a "r" variable before checking it for error.
> > 
> > Are you saying that
> > 
> >         if (xc_shadow_control(ctx->xch, domid,
> >                               XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION,
> >                               NULL, 0, &shadow_mb, 0, NULL)) {
> > 
> > is not acceptable, style-wise?
> 
> Oh, there is indeed such a rule under "ERROR HANDLING". Which means ...
> 
> > If indeed you are, please disambiguate
> > your statement above regarding the use of "ret": May I or may I not
> > use it? IOW do I need to introduce "r", or can I get away with the
> > existing local variables.
> 
> ... I need this to be clarified.

You need to introduce the "r" local variable, to store xc_shadow_control
return value.
Then, set "ret" to ERROR_FAIL before "goto out;".

Hope that's clearer.

Cheers,

-- 
Anthony PERARD