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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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