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 &&
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>
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.