[PATCH v2] Fix libxc return -E misusage.

[PATCH v2] Fix libxc return -E misusage.

[Konrad Rzeszutek Wilk]

Hey

Please see the following patches which fix an subset of
some of the various usages of return -Exx instead of
using -1 (and stashing in errno the error value).

We also clean up some cases where the errno is over-writen
- we want to bubble up the errnor that the underlaying
hypercall had done.

Lastly it also wraps an invisibility layer on xc_hypercall_bounce_*
so that any errors in those won't over-write the hypercall
ones (as we usually do hypercall and then xc_hypercall_bounce_post).

Compared to v1, the patches that have changed the most are:

 [PATCH v2 07/13] libxc: Fix xc_tmem_control to return proper error.
 [PATCH v2 08/13] libxc: Check xc_domain_maximum_gpfn for negative
 [PATCH v2 09/13] libxc: Check xc_maximum_ram_page for negative return
 [PATCH v2 11/13] libxc: Check xc_sharing_* for proper return values.

which have changed per Ian's review.

 tools/libxc/include/xenctrl.h          |  2 +-
 tools/libxc/xc_core_arm.c              | 17 +++++++++--
 tools/libxc/xc_core_x86.c              | 24 ++++++++++++---
 tools/libxc/xc_cpupool.c               |  4 +--
 tools/libxc/xc_dom_x86.c               |  7 +++--
 tools/libxc/xc_domain.c                |  2 +-
 tools/libxc/xc_domain_save.c           |  8 ++++-
 tools/libxc/xc_freebsd_osdep.c         |  3 ++
 tools/libxc/xc_hcall_buf.c             |  6 ++++
 tools/libxc/xc_linux_osdep.c           |  3 ++
 tools/libxc/xc_offline_page.c          | 42 ++++++++++++++------------
 tools/libxc/xc_physdev.c               | 12 +++++---
 tools/libxc/xc_pm.c                    | 54 ++++++++++++++++++++++------------
 tools/libxc/xc_private.c               | 15 +++++++---
 tools/libxc/xc_tmem.c                  | 14 ++++++---
 tools/libxc/xg_save_restore.h          |  3 +-
 tools/libxl/libxl.c                    |  4 +--
 tools/libxl/libxl_x86.c                |  9 ++----
 tools/misc/xen-hptool.c                |  6 ++--
 tools/misc/xen-mfndump.c               |  9 +++---
 tools/tests/mem-sharing/memshrtool.c   | 12 ++++++--
 tools/xenstat/libxenstat/src/xenstat.c |  7 +++--
 22 files changed, 178 insertions(+), 85 deletions(-)

[PATCH v2 01/13] libxc: Replaces tabs with spaces in xc_cpupool_freeinfo

[Konrad Rzeszutek Wilk]

The goto looks very wrong when the rest of the code
has spaces.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_cpupool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
index 6393cfb..828f234 100644
--- a/tools/libxc/xc_cpupool.c
+++ b/tools/libxc/xc_cpupool.c
@@ -190,11 +190,11 @@ xc_cpumap_t xc_cpupool_freeinfo(xc_interface *xch)
     err = do_sysctl_save(xch, &sysctl);
 
     if ( err < 0 )
-	goto out;
+        goto out;
 
     cpumap = xc_cpumap_alloc(xch);
     if (cpumap == NULL)
-	goto out;
+        goto out;
 
     memcpy(cpumap, local, mapsize);
 
-- 
2.1.0

[PATCH v2 02/13] libxc: Propagate errno from hypercall instead of anything else.

[Konrad Rzeszutek Wilk]

After we have done the hypercall - the errno has the failure
code. However our usage of pthread and munmap can trigger them
to manipulate the errno with their failure values. That would
be bad as what we care about is just the hypercall error value.

Another solution to this would be to save the 'errno' from
pthread/munmap/madvise as an extra parameter to be analyzed
later. However the call-sites above us do not care about it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_freebsd_osdep.c | 3 +++
 tools/libxc/xc_hcall_buf.c     | 6 ++++++
 tools/libxc/xc_linux_osdep.c   | 3 +++
 3 files changed, 12 insertions(+)

diff --git a/tools/libxc/xc_freebsd_osdep.c b/tools/libxc/xc_freebsd_osdep.c
index 151d3bf..e6613ef 100644
--- a/tools/libxc/xc_freebsd_osdep.c
+++ b/tools/libxc/xc_freebsd_osdep.c
@@ -125,10 +125,13 @@ static void freebsd_privcmd_free_hypercall_buffer(xc_interface *xch,
                                                   int npages)
 {
 
+    int saved_errno = errno;
     /* Unlock pages */
     munlock(ptr, npages * XC_PAGE_SIZE);
 
     munmap(ptr, npages * XC_PAGE_SIZE);
+    /* We MUST propagate the hypercall errno, not unmap call's. */
+    errno = saved_errno;
 }
 
 static int freebsd_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h,
diff --git a/tools/libxc/xc_hcall_buf.c b/tools/libxc/xc_hcall_buf.c
index e762a93..932b47c 100644
--- a/tools/libxc/xc_hcall_buf.c
+++ b/tools/libxc/xc_hcall_buf.c
@@ -33,16 +33,22 @@ pthread_mutex_t hypercall_buffer_cache_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 static void hypercall_buffer_cache_lock(xc_interface *xch)
 {
+    int saved_errno = errno;
     if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
         return;
     pthread_mutex_lock(&hypercall_buffer_cache_mutex);
+    /* Ignore pthread errors. */
+    errno = saved_errno;
 }
 
 static void hypercall_buffer_cache_unlock(xc_interface *xch)
 {
+    int saved_errno = errno;
     if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
         return;
     pthread_mutex_unlock(&hypercall_buffer_cache_mutex);
+    /* Ignore pthread errors. */
+    errno = saved_errno;
 }
 
 static void *hypercall_buffer_cache_alloc(xc_interface *xch, int nr_pages)
diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index 92f7cac..2687424 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -122,10 +122,13 @@ out:
 
 static void linux_privcmd_free_hypercall_buffer(xc_interface *xch, xc_osdep_handle h, void *ptr, int npages)
 {
+    int saved_errno = errno;
     /* Recover the VMA flags. Maybe it's not necessary */
     madvise(ptr, npages * XC_PAGE_SIZE, MADV_DOFORK);
 
     munmap(ptr, npages * XC_PAGE_SIZE);
+    /* We MUST propagate the hypercall errno, not unmap call's. */
+    errno = saved_errno;
 }
 
 static int linux_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcmd_hypercall_t *hypercall)
-- 
2.1.0

[PATCH v2 03/13] libxc: Fix xc_domain_get_tsc_info to return -1 instead of -Exx.

[Konrad Rzeszutek Wilk]

We don't need to put fill errno because xc_hypercall_buffer_alloc
fills the errno with the appropriate errno values and we just
need to pass them up the stack.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 845d1d7..2fed727 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -771,7 +771,7 @@ int xc_domain_get_tsc_info(xc_interface *xch,
 
     info = xc_hypercall_buffer_alloc(xch, info, sizeof(*info));
     if ( info == NULL )
-        return -ENOMEM;
+        return -1;
 
     domctl.cmd = XEN_DOMCTL_gettscinfo;
     domctl.domain = (domid_t)domid;
-- 
2.1.0

[PATCH v2 04/13] libxc: xc_physdev_map return -1 and populate errno.

[Konrad Rzeszutek Wilk]

The users of these (qemu) check for a negative value
so we are safe in regards to that. However they
also use the return value to inform the user of the
error.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_physdev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_physdev.c b/tools/libxc/xc_physdev.c
index cf02d85..9b064b8 100644
--- a/tools/libxc/xc_physdev.c
+++ b/tools/libxc/xc_physdev.c
@@ -43,8 +43,10 @@ int xc_physdev_map_pirq(xc_interface *xch,
     struct physdev_map_pirq map;
 
     if ( !pirq )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     memset(&map, 0, sizeof(struct physdev_map_pirq));
     map.domid = domid;
     map.type = MAP_PIRQ_TYPE_GSI;
@@ -72,8 +74,10 @@ int xc_physdev_map_pirq_msi(xc_interface *xch,
     struct physdev_map_pirq map;
 
     if ( !pirq )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     memset(&map, 0, sizeof(struct physdev_map_pirq));
     map.domid = domid;
     map.type = MAP_PIRQ_TYPE_MSI;
-- 
2.1.0

[PATCH v2 05/13] libxc: Return negative value and propagate errno for xc_offline_page API

[Konrad Rzeszutek Wilk]

Instead of returning -Exx we now return -1 for error.
We could stash the -Exx values in errno values but why - the
underlaying functions we call all stash the proper errno
value. Hence we just propagate it up wherver it is needed.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_offline_page.c | 36 +++++++++++++++++++++---------------
 tools/libxc/xc_private.c      |  2 +-
 tools/misc/xen-hptool.c       |  6 +++---
 3 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
index 3147203..d46cc5b 100644
--- a/tools/libxc/xc_offline_page.c
+++ b/tools/libxc/xc_offline_page.c
@@ -58,12 +58,14 @@ int xc_mark_page_online(xc_interface *xch, unsigned long start,
     int ret = -1;
 
     if ( !status || (end < start) )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     if ( xc_hypercall_bounce_pre(xch, status) )
     {
         ERROR("Could not bounce memory for xc_mark_page_online\n");
-        return -EINVAL;
+        return -1;
     }
 
     sysctl.cmd = XEN_SYSCTL_page_offline_op;
@@ -86,12 +88,14 @@ int xc_mark_page_offline(xc_interface *xch, unsigned long start,
     int ret = -1;
 
     if ( !status || (end < start) )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     if ( xc_hypercall_bounce_pre(xch, status) )
     {
         ERROR("Could not bounce memory for xc_mark_page_offline");
-        return -EINVAL;
+        return -1;
     }
 
     sysctl.cmd = XEN_SYSCTL_page_offline_op;
@@ -114,12 +118,14 @@ int xc_query_page_offline_status(xc_interface *xch, unsigned long start,
     int ret = -1;
 
     if ( !status || (end < start) )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     if ( xc_hypercall_bounce_pre(xch, status) )
     {
         ERROR("Could not bounce memory for xc_query_page_offline_status\n");
-        return -EINVAL;
+        return -1;
     }
 
     sysctl.cmd = XEN_SYSCTL_page_offline_op;
@@ -411,19 +417,19 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
     if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
     {
         ERROR("Could not get domain info");
-        return -EFAULT;
+        return -1;
     }
 
     if (!info.shutdown || info.shutdown_reason != SHUTDOWN_suspend)
     {
+        errno = EINVAL;
         ERROR("Can't exchange page unless domain is suspended\n");
-        return -EINVAL;
+        return -1;
     }
-
     if (!is_page_exchangable(xch, domid, mfn, &info))
     {
         ERROR("Could not exchange page\n");
-        return -EINVAL;
+        return -1;
     }
 
     /* Map M2P and obtain gpfn */
@@ -431,7 +437,7 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
     if ( !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
         PERROR("Failed to map live M2P table");
-        return -EFAULT;
+        return -1;
     }
     gpfn = m2p_table[mfn];
 
@@ -440,7 +446,7 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
     if ( xc_map_domain_meminfo(xch, domid, &minfo) )
     {
         PERROR("Could not map domain's memory information\n");
-        return -EFAULT;
+        return -1;
     }
 
     /* For translation macros */
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index df6cd9b..0735e23 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -427,7 +427,7 @@ int xc_mmuext_op(
 {
     DECLARE_HYPERCALL;
     DECLARE_HYPERCALL_BOUNCE(op, nr_ops*sizeof(*op), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
-    long ret = -EINVAL;
+    long ret = -1;
 
     if ( xc_hypercall_bounce_pre(xch, op) )
     {
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index 1134603..c7561a9 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -49,7 +49,7 @@ static int hp_mem_online_func(int argc, char *argv[])
     ret = xc_mark_page_online(xch, mfn, mfn, &status);
 
     if (ret < 0)
-        fprintf(stderr, "Onlining page mfn %lx failed, error %x", mfn, ret);
+        fprintf(stderr, "Onlining page mfn %lx failed, error %x", mfn, errno);
     else if (status & (PG_ONLINE_FAILED |PG_ONLINE_BROKEN)) {
         fprintf(stderr, "Onlining page mfn %lx is broken, "
                         "Memory online failed\n", mfn);
@@ -80,7 +80,7 @@ static int hp_mem_query_func(int argc, char *argv[])
     ret = xc_query_page_offline_status(xch, mfn, mfn, &status);
 
     if (ret < 0)
-        fprintf(stderr, "Querying page mfn %lx failed, error %x", mfn, ret);
+        fprintf(stderr, "Querying page mfn %lx failed, error %x", mfn, errno);
     else
     {
 		printf("Memory Status %x: [", status);
@@ -160,7 +160,7 @@ static int hp_mem_offline_func(int argc, char *argv[])
     printf("Prepare to offline MEMORY mfn %lx\n", mfn);
     ret = xc_mark_page_offline(xch, mfn, mfn, &status);
     if (ret < 0) {
-        fprintf(stderr, "Offlining page mfn %lx failed, error %x\n", mfn, ret);
+        fprintf(stderr, "Offlining page mfn %lx failed, error %x\n", mfn, errno);
         if (status & (PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED))
             fprintf(stderr, "XEN_PAGE is not permitted be offlined\n");
         else if (status & (PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM))
-- 
2.1.0

[PATCH v2 06/13] libxc: Fix xc_pm API calls to return negative error and stash error in errno.

[Konrad Rzeszutek Wilk]

Oddly enough the user of this API did the right thing -
check for return being negative and used 'errno' for the
real error.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_pm.c | 54 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index e4e0fb9..5a7148e 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -51,8 +51,10 @@ int xc_pm_get_pxstat(xc_interface *xch, int cpuid, struct xc_px_stat *pxpt)
     int max_px, ret;
 
     if ( !pxpt->trans_pt || !pxpt->pt )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     if ( (ret = xc_pm_get_max_px(xch, cpuid, &max_px)) != 0)
         return ret;
 
@@ -219,8 +221,10 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
         if ( (!user_para->affected_cpus)                    ||
              (!user_para->scaling_available_frequencies)    ||
              (!user_para->scaling_available_governors) )
-            return -EINVAL;
-
+        {
+            errno = EINVAL;
+            return -1;
+        }
         if ( xc_hypercall_bounce_pre(xch, affected_cpus) )
             goto unlock_1;
         if ( xc_hypercall_bounce_pre(xch, scaling_available_frequencies) )
@@ -293,8 +297,10 @@ int xc_set_cpufreq_gov(xc_interface *xch, int cpuid, char *govname)
     char *scaling_governor = sysctl.u.pm_op.u.set_gov.scaling_governor;
 
     if ( !xch || !govname )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = SET_CPUFREQ_GOV;
     sysctl.u.pm_op.cpuid = cpuid;
@@ -310,8 +316,10 @@ int xc_set_cpufreq_para(xc_interface *xch, int cpuid,
     DECLARE_SYSCTL;
 
     if ( !xch )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = SET_CPUFREQ_PARA;
     sysctl.u.pm_op.cpuid = cpuid;
@@ -327,8 +335,10 @@ int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq)
     DECLARE_SYSCTL;
 
     if ( !xch || !avg_freq )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = GET_CPUFREQ_AVGFREQ;
     sysctl.u.pm_op.cpuid = cpuid;
@@ -392,8 +402,10 @@ int xc_get_cpuidle_max_cstate(xc_interface *xch, uint32_t *value)
     DECLARE_SYSCTL;
 
     if ( !xch || !value )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_get_max_cstate;
     sysctl.u.pm_op.cpuid = 0;
@@ -409,8 +421,10 @@ int xc_set_cpuidle_max_cstate(xc_interface *xch, uint32_t value)
     DECLARE_SYSCTL;
 
     if ( !xch )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_set_max_cstate;
     sysctl.u.pm_op.cpuid = 0;
@@ -424,8 +438,10 @@ int xc_enable_turbo(xc_interface *xch, int cpuid)
     DECLARE_SYSCTL;
 
     if ( !xch )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_enable_turbo;
     sysctl.u.pm_op.cpuid = cpuid;
@@ -437,8 +453,10 @@ int xc_disable_turbo(xc_interface *xch, int cpuid)
     DECLARE_SYSCTL;
 
     if ( !xch )
-        return -EINVAL;
-
+    {
+        errno = EINVAL;
+        return -1;
+    }
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_disable_turbo;
     sysctl.u.pm_op.cpuid = cpuid;
-- 
2.1.0

[PATCH v2 07/13] libxc: Fix xc_tmem_control to return proper error.

[Konrad Rzeszutek Wilk]

The API returns now negative values on error and stashes
the error in errno. Fix the user of this API.

The 'xc_hypercall_bounce_pre' can fail - and if so it will
stash its errno values - no need to over-write it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_tmem.c                  | 14 ++++++++++----
 tools/xenstat/libxenstat/src/xenstat.c |  7 ++++---
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
index 3261e10..02797bf 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -73,11 +73,14 @@ int xc_tmem_control(xc_interface *xch,
     if ( subop == TMEMC_LIST && arg1 != 0 )
     {
         if ( buf == NULL )
-            return -EINVAL;
+        {
+            errno = EINVAL;
+            return -1;
+        }
         if ( xc_hypercall_bounce_pre(xch, buf) )
         {
             PERROR("Could not bounce buffer for tmem control hypercall");
-            return -ENOMEM;
+            return -1;
         }
     }
 
@@ -118,11 +121,14 @@ int xc_tmem_control_oid(xc_interface *xch,
     if ( subop == TMEMC_LIST && arg1 != 0 )
     {
         if ( buf == NULL )
-            return -EINVAL;
+        {
+            errno = EINVAL;
+            return -1;
+        }
         if ( xc_hypercall_bounce_pre(xch, buf) )
         {
             PERROR("Could not bounce buffer for tmem control (OID) hypercall");
-            return -ENOMEM;
+            return -1;
         }
     }
 
diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index 8072a90..c1f6511 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -166,6 +166,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
 	xc_domaininfo_t domaininfo[DOMAIN_CHUNK_SIZE];
 	int new_domains;
 	unsigned int i;
+	int rc;
 
 	/* Create the node */
 	node = (xenstat_node *) calloc(1, sizeof(xenstat_node));
@@ -189,9 +190,9 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
 	node->free_mem = ((unsigned long long)physinfo.free_pages)
 	    * handle->page_size;
 
-	node->freeable_mb = (long)xc_tmem_control(handle->xc_handle, -1,
-				TMEMC_QUERY_FREEABLE_MB, -1, 0, 0, 0, NULL);
-
+	rc = xc_tmem_control(handle->xc_handle, -1,
+                         TMEMC_QUERY_FREEABLE_MB, -1, 0, 0, 0, NULL);
+	node->freeable_mb = (rc < 0) ? 0 : rc;
 	/* malloc(0) is not portable, so allocate a single domain.  This will
 	 * be resized below. */
 	node->domains = malloc(sizeof(xenstat_domain));
-- 
2.1.0

Re: [PATCH v2 07/13] libxc: Fix xc_tmem_control to return proper error.

[Ian Campbell]

On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote:
> The API returns now negative values on error and stashes
> the error in errno. Fix the user of this API.
> 
> The 'xc_hypercall_bounce_pre' can fail - and if so it will
> stash its errno values - no need to over-write it.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I'm still a little concerned about xenstat.c's handling of errno!
=-ENOSYS, but not enough to nack.

Re: [PATCH v2 07/13] libxc: Fix xc_tmem_control to return proper error.

[Konrad Rzeszutek Wilk]

On Thu, Mar 19, 2015 at 04:39:49PM +0000, Ian Campbell wrote:
> On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote:
> > The API returns now negative values on error and stashes
> > the error in errno. Fix the user of this API.
> > 
> > The 'xc_hypercall_bounce_pre' can fail - and if so it will
> > stash its errno values - no need to over-write it.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I'm still a little concerned about xenstat.c's handling of errno!
> =-ENOSYS, but not enough to nack.

You mean not handling it :-)

Yeah, there is certainly some more tmem related changes (another
wrapper function) so that it returns 0 when 'tmem' is not enabled
(and not modify 'errno'). But not this week..
> 
> 

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

[Konrad Rzeszutek Wilk]

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)
 {
-    return xc_domain_maximum_gpfn(xch, domid) + 1;
+    int rc = xc_domain_maximum_gpfn(xch, domid);
+
+    if ( rc >= 0 )
+    {
+        *gpfns = rc + 1;
+        rc = 0;
+    }
+    return rc;
 }
 
 int
@@ -48,9 +55,13 @@ 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;
+    int rc = nr_gpfns(xch, info->domid, &p2m_size);
     xc_core_memory_map_t *map;
 
+    if ( rc < 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..02377e8 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -36,9 +36,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)
 {
-    return xc_domain_maximum_gpfn(xch, domid) + 1;
+    int rc = xc_domain_maximum_gpfn(xch, domid);
+
+    if ( rc >= 0 )
+    {
+        *gpfns = rc + 1;
+        rc = 0;
+    }
+    return rc;
 }
 
 int
@@ -53,9 +60,13 @@ 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;
+    int rc = nr_gpfns(xch, info->domid, &p2m_size);
     xc_core_memory_map_t *map;
 
+    if ( rc < 0 )
+        return -1;
+
     map = malloc(sizeof(*map));
     if ( map == NULL )
     {
@@ -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);
+    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;
 
     if ( dinfo->p2m_size > ~XEN_DOMCTL_PFINFO_LTAB_MASK )
     {
-- 
2.1.0

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

[Ian Campbell]

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.

Ian.

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

[Konrad Rzeszutek Wilk]

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?

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

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

[Ian Campbell]

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.

If you want to stick with a wrapper for some reason then xc_private.c
would be an ok choice (its a dumping ground already), or xc_misc.c seems
to have a bunch of not dissimilar functionality in it. I think a new
file would be overkill.

Ian.

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

[Konrad Rzeszutek Wilk]

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.
> 
> If you want to stick with a wrapper for some reason then xc_private.c
> would be an ok choice (its a dumping ground already), or xc_misc.c seems
> to have a bunch of not dissimilar functionality in it. I think a new
> file would be overkill.
> 
> Ian.
> 

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

[Konrad Rzeszutek Wilk]

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

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

[Ian Campbell]

On Fri, 2015-03-20 at 10:49 -0400, Konrad Rzeszutek Wilk wrote:
> 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>

Looks good, only one minor comment:
[...]

> -    *gpfn = (xen_pfn_t)rc + 1;

Perhaps the new parameter to xc_domain_maximum_gpfn should be a
xen_pfn_t?

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

[Konrad Rzeszutek Wilk]

> > All of them are 
> > 
> >  git://xenbits.xen.org/people/konradwilk/xen.git xc_cleanup.v4

..snip..
> Looks good, only one minor comment:
> [...]
> 
> > -    *gpfn = (xen_pfn_t)rc + 1;
> 
> Perhaps the new parameter to xc_domain_maximum_gpfn should be a
> xen_pfn_t?
> 

Here you go. They are at:

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

>From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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 |  2 +-
 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, 30 insertions(+), 29 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index df18292..7853fdb 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1315,7 +1315,7 @@ 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, xen_pfn_t *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..c3f5868 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);
+    xen_pfn_t 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..4552e43 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);
+    xen_pfn_t 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..da88e08 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, xen_pfn_t *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

> 

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

[Ian Campbell]

On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
> From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>

Acked + applied along with the rest of the series, thanks,

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

[Andrew Cooper]

On 20/03/15 17:03, Ian Campbell wrote:
> On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
>>  From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>
> Acked + applied along with the rest of the series, thanks,

This change as unfortunately causes a regression in migration v2, 
because the fenceposting has changed and the function no longer returns 
the maximum gpfn.  It returns one past the maximum gpfn.

It would appear that migration v2 was the only consumer which actually 
want the max gpfn.

Can we either rename the function to accurately name the value it 
returns (although I am out of ideas as to what this might be), or undo 
the fenceposting change so that it continues to return the value it 
claims to return.

~Andrew

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

[Ian Campbell]

On Thu, 2015-03-26 at 21:07 +0000, Andrew Cooper wrote:
> On 20/03/15 17:03, Ian Campbell wrote:
> > On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
> >>  From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>
> > Acked + applied along with the rest of the series, thanks,
> 
> This change as unfortunately causes a regression in migration v2, 
> because the fenceposting has changed and the function no longer returns 
> the maximum gpfn.  It returns one past the maximum gpfn.

Oops, so it does, I noticed that but didn't fully think through the
naming implications once I noticed all the callers being correctly
adjusted, sorry.

> It would appear that migration v2 was the only consumer which actually 
> want the max gpfn.
> 
> Can we either rename the function to accurately name the value it 
> returns (although I am out of ideas as to what this might be), or undo 
> the fenceposting change so that it continues to return the value it 
> claims to return.

Putting the fence posting back, which will involve adjusting the other
callers, is probably best and reflects the underlying hypercall.

I think maximum_gpfn+1 could be described as nr_gpfns if we wanted to go
the renaming route, or make a wrapper which did the +1, returning the
original to the expect semantics.

Konrad, will you take care of this one way or another?

Ian.

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

[Konrad Rzeszutek Wilk]

On Fri, Mar 27, 2015 at 10:52:07AM +0000, Ian Campbell wrote:
> On Thu, 2015-03-26 at 21:07 +0000, Andrew Cooper wrote:
> > On 20/03/15 17:03, Ian Campbell wrote:
> > > On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
> > >>  From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>
> > > Acked + applied along with the rest of the series, thanks,
> > 
> > This change as unfortunately causes a regression in migration v2, 
> > because the fenceposting has changed and the function no longer returns 
> > the maximum gpfn.  It returns one past the maximum gpfn.
> 
> Oops, so it does, I noticed that but didn't fully think through the
> naming implications once I noticed all the callers being correctly
> adjusted, sorry.
> 
> > It would appear that migration v2 was the only consumer which actually 
> > want the max gpfn.
> > 
> > Can we either rename the function to accurately name the value it 
> > returns (although I am out of ideas as to what this might be), or undo 
> > the fenceposting change so that it continues to return the value it 
> > claims to return.
> 
> Putting the fence posting back, which will involve adjusting the other
> callers, is probably best and reflects the underlying hypercall.
> 
> I think maximum_gpfn+1 could be described as nr_gpfns if we wanted to go
> the renaming route, or make a wrapper which did the +1, returning the
> original to the expect semantics.
> 
> Konrad, will you take care of this one way or another?

Yes. I will just add an xc_nr_gpfns and conver the in-tree callers to use
that. And naturally revert the xc_domain_maximum_gpfn.

Andrew, thank you for noticing this!
> 
> Ian.
> 

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

[Konrad Rzeszutek Wilk]

On Thu, Mar 26, 2015 at 09:07:05PM +0000, Andrew Cooper wrote:
> On 20/03/15 17:03, Ian Campbell wrote:
> >On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
> >> From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>
> >Acked + applied along with the rest of the series, thanks,
> 
> This change as unfortunately causes a regression in migration v2, because
> the fenceposting has changed and the function no longer returns the maximum
> gpfn.  It returns one past the maximum gpfn.
> 
> It would appear that migration v2 was the only consumer which actually want
> the max gpfn.
> 
> Can we either rename the function to accurately name the value it returns
> (although I am out of ideas as to what this might be), or undo the
> fenceposting change so that it continues to return the value it claims to
> return.

This should do it? (I didn't fix the ';' issue in here).

>From a7206930f867025234966c7b784bead9b174930b Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 27 Mar 2015 15:36:02 -0400
Subject: [PATCH] libxc: Re-institute the old xc_maximum_ram_page and add
 xc_maximum_gpfn

The commit 1781f00ea62edb72bed4fe1b6959eeed427e988f
"libxc: Check xc_maximum_ram_page for negative return values."
broke migrationv2 (out of tree) as it wanted the raw
value instead of the +1 manipulation the rest of the callers do.

As such we resurrect xc_maximum_ram_page and rename the
version that was added in the above mention commit to
xc_maximum_gpfn.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/include/xenctrl.h | 3 ++-
 tools/libxc/xc_offline_page.c | 2 +-
 tools/libxc/xc_private.c      | 7 ++++++-
 tools/libxc/xg_save_restore.h | 2 +-
 tools/misc/xen-mfndump.c      | 4 ++--
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..c3d22de 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1509,7 +1509,8 @@ int xc_mmuext_op(xc_interface *xch, struct mmuext_op *op, unsigned int nr_ops,
                  domid_t dom);
 
 /* System wide memory properties */
-int xc_maximum_ram_page(xc_interface *xch, unsigned long *max_mfn);
+long xc_maximum_ram_page(xc_interface *xch);
+int xc_maximum_gpfn(xc_interface *xch, unsigned long *max_mfn);
 
 /* Get current total pages allocated to a domain. */
 long xc_get_tot_pages(xc_interface *xch, uint32_t domid);
diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
index b1d169c..4bf35ea 100644
--- a/tools/libxc/xc_offline_page.c
+++ b/tools/libxc/xc_offline_page.c
@@ -433,7 +433,7 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
     }
 
     /* Map M2P and obtain gpfn */
-    rc = xc_maximum_ram_page(xch, &max_mfn);
+    rc = xc_maximum_gpfn(xch, &max_mfn);
     if ( rc || !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
         PERROR("Failed to map live M2P table");
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 2eb44b6..ef0d778 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -535,7 +535,12 @@ int do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len)
     return ret;
 }
 
-int xc_maximum_ram_page(xc_interface *xch, unsigned long *max_mfn)
+long xc_maximum_ram_page(xc_interface *xch)
+{
+    return do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
+}
+
+int xc_maximum_gpfn(xc_interface *xch, unsigned long *max_mfn)
 {
     long rc = do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
 
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index 832c329..c760df8 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -311,7 +311,7 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
     if (xc_version(xch, XENVER_capabilities, &xen_caps) != 0)
         return 0;
 
-    if (xc_maximum_ram_page(xch, max_mfn))
+    if (xc_maximum_gpfn(xch, max_mfn))
         return 0;
 
     *hvirt_start = xen_params.virt_start;
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 0c018e0..12555b8 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -41,7 +41,7 @@ int dump_m2p_func(int argc, char *argv[])
     }
 
     /* Map M2P and obtain gpfn */
-    if ( xc_maximum_ram_page(xch, &max_mfn) < 0 );
+    if ( xc_maximum_gpfn(xch, &max_mfn) < 0 );
     {
         ERROR("Failed to get the maximum mfn");
         return -1;
@@ -182,7 +182,7 @@ int dump_ptes_func(int argc, char *argv[])
     }
 
     /* Map M2P and obtain gpfn */
-    rc = xc_maximum_ram_page(xch, &max_mfn);
+    rc = xc_maximum_gpfn(xch, &max_mfn);
     if ( rc || (mfn > max_mfn) ||
          !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
-- 
2.1.0

> 
> ~Andrew

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

[Andrew Cooper]

On 27/03/15 19:42, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 26, 2015 at 09:07:05PM +0000, Andrew Cooper wrote:
>> On 20/03/15 17:03, Ian Campbell wrote:
>>> On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
>>>> From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>
>>> Acked + applied along with the rest of the series, thanks,
>> This change as unfortunately causes a regression in migration v2, because
>> the fenceposting has changed and the function no longer returns the maximum
>> gpfn.  It returns one past the maximum gpfn.
>>
>> It would appear that migration v2 was the only consumer which actually want
>> the max gpfn.
>>
>> Can we either rename the function to accurately name the value it returns
>> (although I am out of ideas as to what this might be), or undo the
>> fenceposting change so that it continues to return the value it claims to
>> return.
> This should do it? (I didn't fix the ';' issue in here).
>
> From a7206930f867025234966c7b784bead9b174930b Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 27 Mar 2015 15:36:02 -0400
> Subject: [PATCH] libxc: Re-institute the old xc_maximum_ram_page and add
>  xc_maximum_gpfn
>
> The commit 1781f00ea62edb72bed4fe1b6959eeed427e988f
> "libxc: Check xc_maximum_ram_page for negative return values."
> broke migrationv2 (out of tree) as it wanted the raw
> value instead of the +1 manipulation the rest of the callers do.
>
> As such we resurrect xc_maximum_ram_page and rename the
> version that was added in the above mention commit to
> xc_maximum_gpfn.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

It is not xc_maximum_ram_page() which was the problem.  It is
xc_domain_maximum_gpfn() which had its fenceposting changed.

The new xc_maximum_ram_page() is fine, and really does want to keep its
new API.  xc_domain_maximum_gpfn() is also fine keeping its new API, but
wants to not adjust max_pfn by 1.

~Andrew

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

[Konrad Rzeszutek Wilk]

On Fri, Mar 27, 2015 at 08:04:44PM +0000, Andrew Cooper wrote:
> On 27/03/15 19:42, Konrad Rzeszutek Wilk wrote:
> > On Thu, Mar 26, 2015 at 09:07:05PM +0000, Andrew Cooper wrote:
> >> On 20/03/15 17:03, Ian Campbell wrote:
> >>> On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
> >>>> From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>
> >>> Acked + applied along with the rest of the series, thanks,
> >> This change as unfortunately causes a regression in migration v2, because
> >> the fenceposting has changed and the function no longer returns the maximum
> >> gpfn.  It returns one past the maximum gpfn.
> >>
> >> It would appear that migration v2 was the only consumer which actually want
> >> the max gpfn.
> >>
> >> Can we either rename the function to accurately name the value it returns
> >> (although I am out of ideas as to what this might be), or undo the
> >> fenceposting change so that it continues to return the value it claims to
> >> return.
> > This should do it? (I didn't fix the ';' issue in here).
> >
> > From a7206930f867025234966c7b784bead9b174930b Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Fri, 27 Mar 2015 15:36:02 -0400
> > Subject: [PATCH] libxc: Re-institute the old xc_maximum_ram_page and add
> >  xc_maximum_gpfn
> >
> > The commit 1781f00ea62edb72bed4fe1b6959eeed427e988f
> > "libxc: Check xc_maximum_ram_page for negative return values."
> > broke migrationv2 (out of tree) as it wanted the raw
> > value instead of the +1 manipulation the rest of the callers do.
> >
> > As such we resurrect xc_maximum_ram_page and rename the
> > version that was added in the above mention commit to
> > xc_maximum_gpfn.
> >
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> It is not xc_maximum_ram_page() which was the problem.  It is
> xc_domain_maximum_gpfn() which had its fenceposting changed.
> 
> The new xc_maximum_ram_page() is fine, and really does want to keep its
> new API.  xc_domain_maximum_gpfn() is also fine keeping its new API, but
> wants to not adjust max_pfn by 1.
> 

<blushes>

>From dbca252c1ad53a83ec5cf0a0d5aa62ca711bd0c0 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 27 Mar 2015 16:36:36 -0400
Subject: [PATCH] libxc: Re-institute the old xc_domain_maximum_gpfn and add
 xc_domain_nr_gpfns

The commit a8f8a590e02d2d2b717257c0bd9a8b396103bdf4
"libxc: Check xc_domain_maximum_gpfn for negative return values"
broke migrationv2 (out of tree) as it wanted the raw value instead
of the +1 value.

Resurrect the old xc_domain_maximum_gpfn and rename the new
xc_domain_maximum_gpfn to xc_domain_nr_gpfns.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/include/xenctrl.h | 4 +++-
 tools/libxc/xc_core_arm.c     | 2 +-
 tools/libxc/xc_core_x86.c     | 6 +++---
 tools/libxc/xc_domain.c       | 6 +++++-
 tools/libxc/xc_domain_save.c  | 2 +-
 5 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4e9537e..5d7d653 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1315,7 +1315,9 @@ 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, xen_pfn_t *gpfns);
+int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid);
+
+int xc_domain_nr_gpfns(xc_interface *xch, domid_t domid, xen_pfn_t *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 c3f5868..57d4715 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -45,7 +45,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
     xen_pfn_t p2m_size = 0;
     xc_core_memory_map_t *map;
 
-    if ( xc_domain_maximum_gpfn(xch, info->domid, &p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, info->domid, &p2m_size) < 0 )
         return -1;
 
     map = malloc(sizeof(*map));
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index 4552e43..93ebcbb 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -50,7 +50,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
     xen_pfn_t p2m_size = 0;
     xc_core_memory_map_t *map;
 
-    if ( xc_domain_maximum_gpfn(xch, info->domid, &p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, info->domid, &p2m_size) < 0 )
         return -1;
 
     map = malloc(sizeof(*map));
@@ -85,7 +85,7 @@ xc_core_arch_map_p2m_rw(xc_interface *xch, struct domain_info_context *dinfo, xc
     int err;
     int i;
 
-    if ( xc_domain_maximum_gpfn(xch, info->domid, &dinfo->p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, info->domid, &dinfo->p2m_size) < 0 )
     {
         ERROR("Could not get maximum GPFN!");
         goto out;
@@ -212,7 +212,7 @@ int
 xc_core_arch_get_scratch_gpfn(xc_interface *xch, domid_t domid,
                               xen_pfn_t *gpfn)
 {
-    return xc_domain_maximum_gpfn(xch, domid, gpfn);
+    return xc_domain_nr_gpfns(xch, domid, gpfn);
 }
 
 /*
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index da88e08..ddaa872 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -788,8 +788,12 @@ int xc_domain_get_tsc_info(xc_interface *xch,
     return rc;
 }
 
+int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid)
+{
+    return do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
+}
 
-int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns)
+int xc_domain_nr_gpfns(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns)
 {
     int rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
 
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index b611c07..cef6995 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -939,7 +939,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     }
 
     /* Get the size of the P2M table */
-    if ( xc_domain_maximum_gpfn(xch, dom, &dinfo->p2m_size) < 0 )
+    if ( xc_domain_nr_gpfns(xch, dom, &dinfo->p2m_size) < 0 )
     {
         ERROR("Could not get maximum GPFN!");
         goto out;
-- 
2.1.0

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

[Andrew Cooper]

On 27/03/15 20:41, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 27, 2015 at 08:04:44PM +0000, Andrew Cooper wrote:
>> On 27/03/15 19:42, Konrad Rzeszutek Wilk wrote:
>>> On Thu, Mar 26, 2015 at 09:07:05PM +0000, Andrew Cooper wrote:
>>>> On 20/03/15 17:03, Ian Campbell wrote:
>>>>> On Fri, 2015-03-20 at 11:45 -0400, Konrad Rzeszutek Wilk wrote:
>>>>>> From 45bd7cd377b0b8364757cc2bc0bd8d6a13523a97 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>
>>>>> Acked + applied along with the rest of the series, thanks,
>>>> This change as unfortunately causes a regression in migration v2, because
>>>> the fenceposting has changed and the function no longer returns the maximum
>>>> gpfn.  It returns one past the maximum gpfn.
>>>>
>>>> It would appear that migration v2 was the only consumer which actually want
>>>> the max gpfn.
>>>>
>>>> Can we either rename the function to accurately name the value it returns
>>>> (although I am out of ideas as to what this might be), or undo the
>>>> fenceposting change so that it continues to return the value it claims to
>>>> return.
>>> This should do it? (I didn't fix the ';' issue in here).
>>>
>>> From a7206930f867025234966c7b784bead9b174930b Mon Sep 17 00:00:00 2001
>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Date: Fri, 27 Mar 2015 15:36:02 -0400
>>> Subject: [PATCH] libxc: Re-institute the old xc_maximum_ram_page and add
>>>  xc_maximum_gpfn
>>>
>>> The commit 1781f00ea62edb72bed4fe1b6959eeed427e988f
>>> "libxc: Check xc_maximum_ram_page for negative return values."
>>> broke migrationv2 (out of tree) as it wanted the raw
>>> value instead of the +1 manipulation the rest of the callers do.
>>>
>>> As such we resurrect xc_maximum_ram_page and rename the
>>> version that was added in the above mention commit to
>>> xc_maximum_gpfn.
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> It is not xc_maximum_ram_page() which was the problem.  It is
>> xc_domain_maximum_gpfn() which had its fenceposting changed.
>>
>> The new xc_maximum_ram_page() is fine, and really does want to keep its
>> new API.  xc_domain_maximum_gpfn() is also fine keeping its new API, but
>> wants to not adjust max_pfn by 1.
>>
> <blushes>
>
> From dbca252c1ad53a83ec5cf0a0d5aa62ca711bd0c0 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 27 Mar 2015 16:36:36 -0400
> Subject: [PATCH] libxc: Re-institute the old xc_domain_maximum_gpfn and add
>  xc_domain_nr_gpfns
>
> The commit a8f8a590e02d2d2b717257c0bd9a8b396103bdf4
> "libxc: Check xc_domain_maximum_gpfn for negative return values"
> broke migrationv2 (out of tree) as it wanted the raw value instead
> of the +1 value.
>
> Resurrect the old xc_domain_maximum_gpfn and rename the new
> xc_domain_maximum_gpfn to xc_domain_nr_gpfns.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/include/xenctrl.h | 4 +++-
>  tools/libxc/xc_core_arm.c     | 2 +-
>  tools/libxc/xc_core_x86.c     | 6 +++---
>  tools/libxc/xc_domain.c       | 6 +++++-
>  tools/libxc/xc_domain_save.c  | 2 +-
>  5 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 4e9537e..5d7d653 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1315,7 +1315,9 @@ 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, xen_pfn_t *gpfns);
> +int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid);

It is perfectly fine for xc_domain_maximum_gpfn() to keep its *gpfn
pointer, as this is a rather more sane interface which doesn't truncate
the hypervisor long in a 64bit toolstack.

~Andrew

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

[Ian Campbell]

On Fri, 2015-03-27 at 21:17 +0000, Andrew Cooper wrote:
> > -int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns);
> > +int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid);
> 
> It is perfectly fine for xc_domain_maximum_gpfn() to keep its *gpfn
> pointer, as this is a rather more sane interface which doesn't truncate
> the hypervisor long in a 64bit toolstack.

Ack. FAOD the two interfaces should be:
int xc_domain_maximum_gpfn(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns);
int xc_domain_nr_gpfns(xc_interface *xch, domid_t domid, xen_pfn_t *gpfns);

Ian.

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

[Konrad Rzeszutek Wilk]

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

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

[Konrad Rzeszutek Wilk]

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

Runtime tested now too.

I've put the whole lot (including this patch) on 

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

To ease pulling it in.

Thank you!
> 
> 
> 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.
> > 

[PATCH v2 09/13] libxc: Check xc_maximum_ram_page for negative return values.

[Konrad Rzeszutek Wilk]

Instead of assuming everything is always OK. As such
we return now the return value (or zero for success).
The max_mfn is now passed in as the parameter.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_offline_page.c |  6 +++---
 tools/libxc/xc_private.c      | 11 +++++++++--
 tools/libxc/xg_save_restore.h |  3 ++-
 tools/misc/xen-mfndump.c      |  9 ++++-----
 5 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index df18292..14aae84 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1509,7 +1509,7 @@ int xc_mmuext_op(xc_interface *xch, struct mmuext_op *op, unsigned int nr_ops,
                  domid_t dom);
 
 /* System wide memory properties */
-long xc_maximum_ram_page(xc_interface *xch);
+int xc_maximum_ram_page(xc_interface *xch, unsigned long *max_mfn);
 
 /* Get current total pages allocated to a domain. */
 long xc_get_tot_pages(xc_interface *xch, uint32_t domid);
diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
index d46cc5b..b1d169c 100644
--- a/tools/libxc/xc_offline_page.c
+++ b/tools/libxc/xc_offline_page.c
@@ -412,7 +412,7 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
     uint32_t status;
     xen_pfn_t new_mfn, gpfn;
     xen_pfn_t *m2p_table;
-    int max_mfn;
+    unsigned long max_mfn;
 
     if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 )
     {
@@ -433,8 +433,8 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
     }
 
     /* Map M2P and obtain gpfn */
-    max_mfn = xc_maximum_ram_page(xch);
-    if ( !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
+    rc = xc_maximum_ram_page(xch, &max_mfn);
+    if ( rc || !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
         PERROR("Failed to map live M2P table");
         return -1;
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 0735e23..f4f2748 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -535,9 +535,16 @@ int do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len)
     return ret;
 }
 
-long xc_maximum_ram_page(xc_interface *xch)
+int xc_maximum_ram_page(xc_interface *xch, unsigned long *max_mfn)
 {
-    return do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
+    long rc = do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
+
+    if ( rc >= 0 )
+    {
+        *max_mfn = rc;
+        rc = 0;
+    }
+    return rc;
 }
 
 long long xc_domain_get_cpu_usage( xc_interface *xch, domid_t domid, int vcpu )
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index bdd9009..832c329 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -311,7 +311,8 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
     if (xc_version(xch, XENVER_capabilities, &xen_caps) != 0)
         return 0;
 
-    *max_mfn = xc_maximum_ram_page(xch);
+    if (xc_maximum_ram_page(xch, max_mfn))
+        return 0;
 
     *hvirt_start = xen_params.virt_start;
 
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 0761f6e..0c018e0 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -31,7 +31,7 @@ int help_func(int argc, char *argv[])
 int dump_m2p_func(int argc, char *argv[])
 {
     unsigned long i;
-    long max_mfn;
+    unsigned long max_mfn;
     xen_pfn_t *m2p_table;
 
     if ( argc > 0 )
@@ -41,8 +41,7 @@ int dump_m2p_func(int argc, char *argv[])
     }
 
     /* Map M2P and obtain gpfn */
-    max_mfn = xc_maximum_ram_page(xch);
-    if ( max_mfn < 0 )
+    if ( xc_maximum_ram_page(xch, &max_mfn) < 0 );
     {
         ERROR("Failed to get the maximum mfn");
         return -1;
@@ -183,8 +182,8 @@ int dump_ptes_func(int argc, char *argv[])
     }
 
     /* Map M2P and obtain gpfn */
-    max_mfn = xc_maximum_ram_page(xch);
-    if ( (mfn > max_mfn) ||
+    rc = xc_maximum_ram_page(xch, &max_mfn);
+    if ( rc || (mfn > max_mfn) ||
          !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
         xc_unmap_domain_meminfo(xch, &minfo);
-- 
2.1.0

Re: [PATCH v2 09/13] libxc: Check xc_maximum_ram_page for negative return values.

[Ian Campbell]

On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote:
> Instead of assuming everything is always OK. As such
> we return now the return value (or zero for success).
> The max_mfn is now passed in as the parameter.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 10/13] libxc: If xc_domain_add_to_physmap fails, include errno value

[Konrad Rzeszutek Wilk]

Instead of just the return value.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index af0c9f4..3301f53 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -946,7 +946,8 @@ static int map_grant_table_frames(struct xc_dom_image *dom)
             }
             xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
                          "%s: mapping grant tables failed " "(pfn=0x%" PRIpfn
-                         ", rc=%d)", __FUNCTION__, dom->p2m_size + i, rc);
+                         ", rc=%d, errno=%d)", __FUNCTION__, dom->p2m_size + i,
+                         rc, errno);
             return rc;
         }
     }
@@ -999,8 +1000,8 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
         if ( rc != 0 )
         {
             xc_dom_panic(dom->xch, XC_INTERNAL_ERROR, "%s: mapping"
-                         " shared_info failed (pfn=0x%" PRIpfn ", rc=%d)",
-                         __FUNCTION__, dom->shared_info_pfn, rc);
+                         " shared_info failed (pfn=0x%" PRIpfn ", rc=%d, errno: %d)",
+                         __FUNCTION__, dom->shared_info_pfn, rc, errno);
             return rc;
         }
 
-- 
2.1.0

[PATCH v2 11/13] libxc: Check xc_sharing_* for proper return values.

[Konrad Rzeszutek Wilk]

If there is a negative return value - check for that and
also use errno for the proper error value.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c                  |  4 ++--
 tools/tests/mem-sharing/memshrtool.c | 12 ++++++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ad69a8a..55d2340 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5024,7 +5024,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
     physinfo->scrub_pages = xcphysinfo.scrub_pages;
     physinfo->outstanding_pages = xcphysinfo.outstanding_pages;
     l = xc_sharing_freed_pages(ctx->xch);
-    if (l == -ENOSYS) {
+    if (l < 0 && errno == ENOSYS) {
         l = 0;
     } else if (l < 0) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, l,
@@ -5033,7 +5033,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
     }
     physinfo->sharing_freed_pages = l;
     l = xc_sharing_used_frames(ctx->xch);
-    if (l == -ENOSYS) {
+    if (l < 0 && errno == ENOSYS) {
         l = 0;
     } else if (l < 0) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, l,
diff --git a/tools/tests/mem-sharing/memshrtool.c b/tools/tests/mem-sharing/memshrtool.c
index db44294..6454bc3 100644
--- a/tools/tests/mem-sharing/memshrtool.c
+++ b/tools/tests/mem-sharing/memshrtool.c
@@ -55,11 +55,19 @@ int main(int argc, const char** argv)
 
     if( !strcasecmp(cmd, "info") )
     {
+        long rc;
         if( argc != 2 )
             return usage(argv[0]);
 
-        printf("used = %ld\n", xc_sharing_used_frames(xch));
-        printf("freed = %ld\n", xc_sharing_freed_pages(xch));
+        rc = xc_sharing_freed_pages(xch);
+        if ( rc < 0 )
+            return 1;
+
+        printf("used = %ld\n", rc);
+        rc = xc_sharing_used_frames(xch);
+        if ( rc < 0 )
+            return 1;
+        printf("freed = %ld\n", rc);
     }
     else if( !strcasecmp(cmd, "enable") )
     {
-- 
2.1.0

Re: [PATCH v2 11/13] libxc: Check xc_sharing_* for proper return values.

[Ian Campbell]

On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote:
> If there is a negative return value - check for that and
> also use errno for the proper error value.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 12/13] libxl: Don't assign return value to errno for E820 get/set xc_ calls

[Konrad Rzeszutek Wilk]

We should be using the errno that the hypercall left
instead of overwriting it with the return value.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_x86.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index fd45ead..376b477 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -215,10 +215,8 @@ static int e820_host_sanitize(libxl__gc *gc,
     int rc;
 
     rc = xc_get_machine_memory_map(CTX->xch, map, *nr);
-    if (rc < 0) {
-        errno = rc;
+    if (rc < 0)
         return ERROR_FAIL;
-    }
 
     *nr = rc;
 
@@ -251,10 +249,9 @@ static int libxl__e820_alloc(libxl__gc *gc, uint32_t domid,
 
     rc = xc_domain_set_memory_map(ctx->xch, domid, map, nr);
 
-    if (rc < 0) {
-        errno  = rc;
+    if (rc < 0)
         return ERROR_FAIL;
-    }
+
     return 0;
 }
 
-- 
2.1.0

[PATCH v2 13/13] libxc: Fix do_memory_op to return negative value on errors

[Konrad Rzeszutek Wilk]

instead of the -Exx values (which should go in errno).

This patch has HUGE implications. There is a lot of APIs
that are using do_memory_op. Fortunately most of them
check for 'if (do_memory_op(..) < 0)' so will function
properly. However there were some which printed the return
value to the user. They have been fixed in:

 libxc: Don't assign return value to errno for E820 get/set xc_ calls.
 libxc: Check xc_sharing_* for proper return values.
 libxc: If xc_domain_add_to_physmap fails, include errno value
 libxc: Check xc_maximum_ram_page for negative return values.
 libxc: Check xc_domain_maximum_gpfn for negative return values

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_private.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index f4f2748..2eb44b6 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -516,7 +516,7 @@ int do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len)
 {
     DECLARE_HYPERCALL;
     DECLARE_HYPERCALL_BOUNCE(arg, len, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
-    long ret = -EINVAL;
+    long ret = -1;
 
     if ( xc_hypercall_bounce_pre(xch, arg) )
     {
-- 
2.1.0

Re: [PATCH v2 13/13] libxc: Fix do_memory_op to return negative value on errors

[Ian Campbell]

On Wed, 2015-03-18 at 20:24 -0400, Konrad Rzeszutek Wilk wrote:
> instead of the -Exx values (which should go in errno).
> 
> This patch has HUGE implications. There is a lot of APIs
> that are using do_memory_op. Fortunately most of them
> check for 'if (do_memory_op(..) < 0)' so will function
> properly. However there were some which printed the return
> value to the user. They have been fixed in:
> 
>  libxc: Don't assign return value to errno for E820 get/set xc_ calls.
>  libxc: Check xc_sharing_* for proper return values.
>  libxc: If xc_domain_add_to_physmap fails, include errno value
>  libxc: Check xc_maximum_ram_page for negative return values.
>  libxc: Check xc_domain_maximum_gpfn for negative return values
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>