Re: [Xen-devel] [PATCH v2 08/13] libxc: Check xc_domain_maximum_gpfn for negative return values

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Mar 19, 2015 at 04:47:58PM +0000, Ian Campbell wrote:
> On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote:
> > Instead of assuming everything is always OK. We stash
> > the gpfns value as an parameter.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > ---
> >  tools/libxc/xc_core_arm.c    | 17 ++++++++++++++---
> >  tools/libxc/xc_core_x86.c    | 24 ++++++++++++++++++++----
> >  tools/libxc/xc_domain_save.c |  8 +++++++-
> >  3 files changed, 41 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
> > index 16508e7..26cec04 100644
> > --- a/tools/libxc/xc_core_arm.c
> > +++ b/tools/libxc/xc_core_arm.c
> > @@ -31,9 +31,16 @@ xc_core_arch_gpfn_may_present(struct 
> > xc_core_arch_context *arch_ctxt,
> >  }
> >  
> > 
> > -static int nr_gpfns(xc_interface *xch, domid_t domid)
> > +static int nr_gpfns(xc_interface *xch, domid_t domid, unsigned long *gpfns)
> 
> You didn't fancy merging the two versions of this then ;-)

I was not sure where you would want to put them. xc_private looks
like the best place, but perhaps it should be in an new file?

> > diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> > index d8846f1..02377e8 100644
> > --- a/tools/libxc/xc_core_x86.c
> > +++ b/tools/libxc/xc_core_x86.c
> 
> > @@ -88,7 +99,12 @@ xc_core_arch_map_p2m_rw(xc_interface *xch, struct 
> > domain_info_context *dinfo, xc
> >      int err;
> >      int i;
> >  
> > -    dinfo->p2m_size = nr_gpfns(xch, info->domid);
> > +    err = nr_gpfns(xch, info->domid, &dinfo->p2m_size);
> 
> Please could you avoid reusing err here, the reason is that it's sole
> use now is to save errno over the cleanup path, whereas here it looks
> like it is going to be used for something but it isn't.
> 
>  if ( nr_gpfns(...)  < 0 )
> 
> is ok per the Xen coding style if you don't actually need the return
> code.
> 
> Or
> 
>     ret = nr_gpfns()
>     if ( ret < 0 )
>         error, goto out
> 
>     ret = -1;
>     .. the rest
> 
> would be ok too I guess. (coding style here allows
>     if ( (ret = nr_gpfns(...)) < 0 )
> too FWIW).
> 
> > +    if ( err < 0 )
> > +    {
> > +        ERROR("nr_gpfns returns errno: %d.", errno);
> > +        goto out;
> > +    }
> >      if ( dinfo->p2m_size < info->nr_pages  )
> >      {
> >          ERROR("p2m_size < nr_pages -1 (%lx < %lx", dinfo->p2m_size, 
> > info->nr_pages - 1);
> > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> > index 254fdb3..6346c12 100644
> > --- a/tools/libxc/xc_domain_save.c
> > +++ b/tools/libxc/xc_domain_save.c
> > @@ -939,7 +939,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, 
> > uint32_t dom, uint32_t max_iter
> >      }
> >  
> >      /* Get the size of the P2M table */
> > -    dinfo->p2m_size = xc_domain_maximum_gpfn(xch, dom) + 1;
> > +    rc = xc_domain_maximum_gpfn(xch, dom);
> > +    if ( rc < 0 )
> > +    {
> > +        ERROR("Could not get maximum GPFN!");
> > +        goto out;
> > +    }
> > +    dinfo->p2m_size = rc + 1;
> 
> Shame this can't use the same helper as the others.

But if we do stick that 'nr_gpfns' in xc_private.c it could!
> 
> Ian.
> 

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