From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [PATCH 4/5] x86/mem_sharing: check for errors in p2m->set_entry(). Date: Thu, 7 Mar 2013 16:10:33 +0000 Message-ID: <20130307161033.GD30110@ocelot.phlegethon.org> References: <1362667983-24938-1-git-send-email-tim@xen.org> <1362667983-24938-5-git-send-email-tim@xen.org> <9CDC4FDE-C556-47AE-A140-2785B3F064BE@gridcentric.ca> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <9CDC4FDE-C556-47AE-A140-2785B3F064BE@gridcentric.ca> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andres Lagar-Cavilla Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org At 10:07 -0500 on 07 Mar (1362650835), Andres Lagar-Cavilla wrote: > On Mar 7, 2013, at 9:53 AM, Tim Deegan wrote: > > > This call ought always to succeed. Assert that it does rather than > > ignoring the return value. > > > > Signed-off-by: Tim Deegan > > Cc: Andres Lagar-Cavilla > > --- > > 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 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). :) Tim.