From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [RFC] libxc: check return values on mmap() and madvise() on xc_alloc_hypercall_buffer() Date: Wed, 07 May 2014 23:10:07 +0100 Message-ID: <536AAF3F.3070700@citrix.com> References: <1399499244-4421-1-git-send-email-mcgrof@do-not-panic.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WiA2S-0007cY-B4 for xen-devel@lists.xenproject.org; Wed, 07 May 2014 22:10:08 +0000 In-Reply-To: <1399499244-4421-1-git-send-email-mcgrof@do-not-panic.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Luis R. Rodriguez" , xen-devel@lists.xenproject.org Cc: "Luis R. Rodriguez" , Ian Jackson , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 07/05/2014 22:47, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" > > On a Thinkpad T4440p with OpenSUSE tumbleweed with v3.15-rc4 > and today's latest xen tip from the git tree strace -f reveals > we end up on a never ending wait shortly after > > write(20, "backend/console/5\0", 18 > > I've tracked this down to a lack of error return values on mmap() and > madvise() on xc_alloc_hypercall_buffer(). This moves us forward. > > Cc: Ian Jackson > Cc: Stefano Stabellini > Cc: Ian Campbell > Signed-off-by: Luis R. Rodriguez > --- Good catch. I am supprised this hasn't blown up in someones face . Out of interest, which bit starts failing given correct error handling here? > > BTW I see no ldconfig run after make install, where do we want to put it > given we have a few libs ? > > tools/libxc/xc_linux_osdep.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > > diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c > index 73860a2..32e5332 100644 > --- a/tools/libxc/xc_linux_osdep.c > +++ b/tools/libxc/xc_linux_osdep.c > @@ -92,14 +92,29 @@ static void *linux_privcmd_alloc_hypercall_buffer(xc_interface *xch, xc_osdep_ha > { > size_t size = npages * XC_PAGE_SIZE; > void *p; > + int rc, saved_errno; > > /* Address returned by mmap is page aligned. */ > p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_LOCKED, -1, 0); > + if ( p == MAP_FAILED ) > + { > + PERROR("xc_alloc_hypercall_buffer: mmap failed"); > + return NULL; > + } > > /* Do not copy the VMA to child process on fork. Avoid the page being COW > on hypercall. */ > - madvise(p, npages * XC_PAGE_SIZE, MADV_DONTFORK); > + rc = madvise(p, npages * XC_PAGE_SIZE, MADV_DONTFORK); > + if ( rc < 0 ) > + goto out; > + This might be cleaner like: if ( rc == 0 ) return p; error cleanup; as it is small enough to do without an out; label. Also, you can do without 'rc' if you are happy putting the madvise() in the if statement itself. 'rc' isn't subsequently referenced. ~Andrew > return p; > +out: > + saved_errno = errno; > + PERROR("xc_alloc_hypercall_buffer: madvise failed"); > + (void)munmap(p, size); > + errno = saved_errno; > + return NULL; > } > > static void linux_privcmd_free_hypercall_buffer(xc_interface *xch, xc_osdep_handle h, void *ptr, int npages)