Re: [Xen-devel] [RFC PATCH for-next 04/18] x86/mem_sharing: cleanup code in various locations

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Sep 26, 2019 at 8:20 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 26.09.2019 16:09, Tamas K Lengyel wrote:
> > On Wed, Sep 25, 2019 at 10:15 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 25.09.2019 17:48, Tamas K Lengyel wrote:
> >>> --- a/xen/arch/x86/hvm/hvm.c
> >>> +++ b/xen/arch/x86/hvm/hvm.c
> >>> @@ -1879,12 +1879,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, 
> >>> unsigned long gla,
> >>>      if ( npfec.write_access && (p2mt == p2m_ram_shared) )
> >>>      {
> >>>          ASSERT(p2m_is_hostp2m(p2m));
> >>> -        sharing_enomem =
> >>> -            (mem_sharing_unshare_page(currd, gfn, 0) < 0);
> >>> +        sharing_enomem = mem_sharing_unshare_page(currd, gfn, 0);
> >>
> >> I guess the implication here is that the function can only return
> >> -ENOMEM? Not very forward compatible, but well. However, if you
> >> touch this already, shouldn't you at least make "sharing_enomem"
> >> bool?
> >
> > Correct, there is a BUG_ON for every other rc value but ENOMEM. We
> > could turn it into a bool but I don't see a reason for it, perhaps
> > there will be another rc value in the future that we want to handle
> > gracefully.
>
> At which point the variable's name will no longer be appropriate.
> Hence my request to make it bool; at such a future point the code
> would need touching again anyway if you (understandably) don't
> want to make more than purely cosmetic changes now.

By the way, it is made bool in patch 7 of the series because there is
no need to call this function directly here.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel