Re: [Xen-devel] [PATCH 4/5] x86/mem_sharing: check for errors in p2m->set_entry().

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Mar 7, 2013, at 11:10 AM, Tim Deegan <tim@xxxxxxx> wrote:

> At 10:07 -0500 on 07 Mar (1362650835), Andres Lagar-Cavilla wrote:
>> On Mar 7, 2013, at 9:53 AM, Tim Deegan <tim@xxxxxxx> wrote:
>> 
>>> This call ought always to succeed.  Assert that it does rather than
>>> ignoring the return value.
>>> 
>>> Signed-off-by: Tim Deegan <tim@xxxxxxx>
>>> Cc: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>> ---
>>> xen/arch/x86/mm/mem_sharing.c |   12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>>> index 1caa900..0364bb0 100644
>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>> @@ -1273,6 +1273,8 @@ int relinquish_shared_pages(struct domain *d)
>>>        p2m_access_t a;
>>>        p2m_type_t t;
>>>        mfn_t mfn;
>>> +        int set_rc;
>>> +
>>>        if ( atomic_read(&d->shr_pages) == 0 )
>>>            break;
>>>        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
>>> @@ -1281,10 +1283,12 @@ int relinquish_shared_pages(struct domain *d)
>>>            /* Does not fail with ENOMEM given the DESTROY flag */
>>>            BUG_ON(__mem_sharing_unshare_page(d, gfn, 
>>>                    MEM_SHARING_DESTROY_GFN));
>>> -            /* Clear out the p2m entry so no one else may try to 
>>> -             * unshare */
>>> -            p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
>>> -                            p2m_invalid, p2m_access_rwx);
>>> +            /* Clear out the p2m entry so no one else may try to
>>> +             * unshare.  Must succeed: we just read the old entry and
>>> +             * we hold the p2m lock. */
>>> +            set_rc = p2m->set_entry(p2m, gfn, _mfn(0), PAGE_ORDER_4K,
>>> +                                    p2m_invalid, p2m_access_rwx);
>>> +            ASSERT(set_rc != 0);
>> Acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> 
> Thanks.
> 
>> Wouldn't it be slightly cleaner to BUG_ON(p2m->set_entry(..) != 1)?
> 
> I dislike BUG_ON(something_with_side_effects()).  When scanning though
> code, my eye skips over ASSERT()s and BUG_ON()s, assuming they're just
> testing invariants.  Besides, that sort of thinking leads to the much
> more pernicious ASSERT(thing_with_side_effects). :)

Yeah ok. My point was that BUG_ON doesn't get compiled away on non-debug 
builds, but there is a point to avoiding the pattern.

Andres
> 
> Tim.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel