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

[Jan Beulich <jbeulich@xxxxxxxx>]

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