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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Mar 20, 2015 at 10:34:34AM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 20, 2015 at 09:48:08AM +0000, Ian Campbell wrote:
> > On Thu, 2015-03-19 at 14:54 -0400, Konrad Rzeszutek Wilk wrote:
> > > 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@oracle.com>
> > > > > ---
> > > > >  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?
> > 
> > I also suggested just changing the interface of xc_domain_maximum_gpfn,
> > in which case it can stay in xc_domain.c. TBH there seems little point
> > in xc_domain_maximum_gpfn if all callers are using a wrapper, so I think
> > I'd advocate this approach.
> 
> Duh, that would be much simpler. Let me respin a patch for that.

Running through testing with it.

All of them are 

 git://xenbits.xen.org/people/konradwilk/xen.git xc_cleanup.v4


>From 319763b12a8c44722f5f170476e0d2afe03408c2 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 13 Mar 2015 14:57:44 -0400
Subject: [PATCH] libxc: Check xc_domain_maximum_gpfn for negative return
 values

Instead of assuming everything is always OK. We stash
the gpfns value as an parameter. Since we use it in three
of places we might as well update xc_domain_maximum_gpfn
to do the right thing.

Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/include/xenctrl.h |  3 ++-
 tools/libxc/xc_core_arm.c     | 11 ++++-------
 tools/libxc/xc_core_x86.c     | 29 +++++++++++------------------
 tools/libxc/xc_domain.c       | 11 +++++++++--
 tools/libxc/xc_domain_save.c  |  6 +++++-
 5 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index df18292..59d2c70 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1315,7 +1315,8 @@ int xc_domain_get_tsc_info(xc_interface *xch,
 
 int xc_domain_disable_migrate(xc_interface *xch, uint32_t domid);
 
-int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid);
+int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid,
+                           unsigned long *gpfns);
 
 int xc_domain_increase_reservation(xc_interface *xch,
                                    uint32_t domid,
diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index 16508e7..5329249 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -30,12 +30,6 @@ xc_core_arch_gpfn_may_present(struct xc_core_arch_context *arch_ctxt,
     return 0;
 }
 
-
-static int nr_gpfns(xc_interface *xch, domid_t domid)
-{
-    return xc_domain_maximum_gpfn(xch, domid) + 1;
-}
-
 int
 xc_core_arch_auto_translated_physmap(const xc_dominfo_t *info)
 {
@@ -48,9 +42,12 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
                             xc_core_memory_map_t **mapp,
                             unsigned int *nr_entries)
 {
-    unsigned long p2m_size = nr_gpfns(xch, info->domid);
+    unsigned long p2m_size = 0;
     xc_core_memory_map_t *map;
 
+    if ( xc_domain_maximum_gpfn(xch, info->domid, &p2m_size) < 0 )
+        return -1;
+
     map = malloc(sizeof(*map));
     if ( map == NULL )
     {
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index d8846f1..1d7c6aa 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -35,12 +35,6 @@ xc_core_arch_gpfn_may_present(struct xc_core_arch_context *arch_ctxt,
     return 1;
 }
 
-
-static int nr_gpfns(xc_interface *xch, domid_t domid)
-{
-    return xc_domain_maximum_gpfn(xch, domid) + 1;
-}
-
 int
 xc_core_arch_auto_translated_physmap(const xc_dominfo_t *info)
 {
@@ -53,9 +47,12 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
                             xc_core_memory_map_t **mapp,
                             unsigned int *nr_entries)
 {
-    unsigned long p2m_size = nr_gpfns(xch, info->domid);
+    unsigned long p2m_size = 0;
     xc_core_memory_map_t *map;
 
+    if ( xc_domain_maximum_gpfn(xch, info->domid, &p2m_size) < 0 )
+        return -1;
+
     map = malloc(sizeof(*map));
     if ( map == NULL )
     {
@@ -88,7 +85,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);
+    if ( xc_domain_maximum_gpfn(xch, info->domid, &dinfo->p2m_size) < 0 )
+    {
+        ERROR("Could not get maximum GPFN!");
+        goto out;
+    }
+
     if ( dinfo->p2m_size < info->nr_pages  )
     {
         ERROR("p2m_size < nr_pages -1 (%lx < %lx", dinfo->p2m_size, info->nr_pages - 1);
@@ -210,16 +212,7 @@ int
 xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
                               xen_pfn_t *gpfn)
 {
-    int rc;
-
-    rc = xc_domain_maximum_gpfn(xch, domid);
-
-    if ( rc < 0 )
-        return rc;
-
-    *gpfn = (xen_pfn_t)rc + 1;
-
-    return 0;
+    return xc_domain_maximum_gpfn(xch, domid, gpfn);
 }
 
 /*
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 2fed727..9d41918 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -789,9 +789,16 @@ int xc_domain_get_tsc_info(xc_interface *xch,
 }
 
 
-int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid)
+int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid, unsigned long *gpfns)
 {
-    return do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
+    int rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
+
+    if ( rc >= 0 )
+    {
+        *gpfns = rc + 1;
+        rc = 0;
+    }
+    return rc;
 }
 
 int xc_domain_increase_reservation(xc_interface *xch,
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 254fdb3..b611c07 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -939,7 +939,11 @@ 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;
+    if ( xc_domain_maximum_gpfn(xch, dom, &dinfo->p2m_size) < 0 )
+    {
+        ERROR("Could not get maximum GPFN!");
+        goto out;
+    }
 
     if ( dinfo->p2m_size > ~XEN_DOMCTL_PFINFO_LTAB_MASK )
     {
-- 
2.1.0