[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Fri, 2 Jul 2021 17:14:40 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hC94+a97j6kslkLN67tYNOJce5dy4+/q6ruAJmaPGJE=; b=EiiCxLKvOiPViHbuCAVonkDNUu5gXIP/bu9v/tA0nSdD+adpKO3Gnkj0E6713kI0MlO9gzy+mxMkna7MZYjb9dMAZtRqPhEURMn+vXwq+TBLz1pviHGj7CpR3hlW31fFqrU0p8VZVFNQDjPoWrDoecV4A1jh2G/cTtJOkZ/xStFYONhYmQ6W98oxsUyw1avX+LooSemVM7PxoD2GV6ot8hLqGhLvfQp3B25iZORiAX/CmTl+0b1jUDzyRjFVKcZ6G2cVN24HoVHfHwC0J9T6U7gvXDhfPG28aAnwkwH4oUOBmmHOjnLa8/uIMtT9xuRkzjogKpV+G+uNXfnH9v6g1A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=a8QTSxu/aRY6ivBtt2kvuaxGpn7UYZx8BwtrJpUSeCT0JCU9JoEli8F6gboIrlfP+AM9sADEhRTTSWql9bU9xcTwWC0rSdsn2UTgmzR39F4IAFqnZWEEcJZLFT5J86HJwnQ7iAY7/qXUi50NFrMPHxjqCpB/4Emd6fprTbFIsXSAoZ2gWn66pS3PHOyCU10S4XRKXGpj0T2eRGbTzFfqz64oF+/hKXF7yEU7G2pI0yxwb7CcRnr0E43ZVvDx7tmmbvuJ5cGHnpkfzu8lZWk+qiYzMGAkZlyyji0F3dyus38TzN1PJ4WqhjzviAy+s4gsGDnw328CQfdm1llZ2jtF0A==
  • Authentication-results: citrix.com; dkim=none (message not signed) header.d=none;citrix.com; dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Delivery-date: Fri, 02 Jul 2021 15:14:46 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.