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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-05-08 at 07:03 +0200, Luis R. Rodriguez wrote:
> > > BTW I see no ldconfig run after make install, where do we want to put it
> > > given we have a few libs ?
> 
> How about this?

Isn't this conventionally the admin's responsibility?

> 
> > >  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.
> 
> Sure, that makes sesnse.

The out label idiom you have used is pretty conventional and will save
faff if some other operation gets added here. I'm happy with the patch
as it is.

> > Also, you can do without 'rc' if you are happy putting the madvise() in the 
> > if statement
> > itself.  'rc' isn't subsequently referenced.
> 
> OK.
> 
>   Luis



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