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

[Jan Beulich <jbeulich@xxxxxxxx>]

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