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

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

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 ;-)
> > 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.

How about this (compile tested but not yet runtime tested):


>From 92085d29b7e2906095a2bc6849b5a17b478e5c79 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 v3] 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 stick it in a common file for
all three of them to use.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_core_arm.c    | 11 ++++-------
 tools/libxc/xc_core_x86.c    | 18 ++++++++++--------
 tools/libxc/xc_domain_save.c |  6 +++++-
 tools/libxc/xc_private.c     | 12 ++++++++++++
 tools/libxc/xc_private.h     |  2 ++
 5 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index 16508e7..846bc6c 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_nr_gpfns(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..2f5ffea 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_nr_gpfns(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_nr_gpfns(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);
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 254fdb3..b410273 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_nr_gpfns(xch, dom, &dinfo->p2m_size) < 0 )
+    {
+        ERROR("Could not get maximum GPFN!");
+        goto out;
+    }
 
     if ( dinfo->p2m_size > ~XEN_DOMCTL_PFINFO_LTAB_MASK )
     {
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 0735e23..0eb49ee 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -540,6 +540,18 @@ long xc_maximum_ram_page(xc_interface *xch)
     return do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
 }
 
+int xc_nr_gpfns(xc_interface *xch, domid_t domid, unsigned long *gpfns)
+{
+    int rc = xc_domain_maximum_gpfn(xch, domid);
+
+    if ( rc >= 0 )
+    {
+        *gpfns = rc + 1;
+        rc = 0;
+    }
+    return rc;
+}
+
 long long xc_domain_get_cpu_usage( xc_interface *xch, domid_t domid, int vcpu )
 {
     DECLARE_DOMCTL;
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 45b8644..4b7f001 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -364,6 +364,8 @@ static inline int do_multicall_op(xc_interface *xch,
 
 int do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len);
 
+int xc_nr_gpfns(xc_interface *xch, domid_t domid, unsigned long *gpfns);
+
 void *xc_map_foreign_ranges(xc_interface *xch, uint32_t dom,
                             size_t size, int prot, size_t chunksize,
                             privcmd_mmap_entry_t entries[], int nentries);
-- 
2.1.0

> 
> Ian.
>