[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: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Date: Fri, 2 Jul 2021 16:51:38 +0100
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • 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:51:50 +0000
  • Ironport-hdrordr: A9a23:PmKBF6iKTgOr8h62UT8Hcq/G4nBQXtgji2hC6mlwRA09TySZ// rOoB0+726StN9xYgBFpTnuAsW9qB/nmqKdpLNhW4tKPzOW3VdATrsSjrcKqgeIc0aVm9K1l5 0QEZSWYOeAdGSS5vyb3ODXKbgd/OU=
  • Ironport-sdr: 5FoqbxpJvQGHXpeWq5UWrpKAmtCthWJJNpCeswrd1d7pWx0OOKa/ZXTwLhcwOGYxOAQ+gSgXs6 8ux9AbSoaLOnQNXjAUTfpYGka1zRj1uNydN8tUF/CRJUxaUya8W4ebsI57CaAlUcQqhpuER9O0 t3lJBz851hpY/cRinxO/dy0kyeLyg/dwoVggGwozafZ3HkrhxZoGynJqNcCY9n7IMDbvTiftAG DZi1TYerS3BMJEnTsVcB2xpL/6SLsWl643LwV92jQ5ypxZNEjr3bNnW190dmRYmnRZQYg+2kpZ 3Ak=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

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