Re: [RFC] libxc: check return values on mmap() and madvise() on xc_alloc_hypercall_buffer()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/05/2014 22:47, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> 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 <unfinished ...>
>
> 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 <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---

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)