[PATCH RFC v1] Fix up libxc errno and return mismatch.

[PATCH RFC v1] Fix up libxc errno and return mismatch.

[Konrad Rzeszutek Wilk]

Dear Maintainers of Libxl,

This is all started with me using 'xc_domain_get_tsc_info' as template
and the realizing that libxl should really return -1 for errors
and we should stash the Exx value in errno. As I started fixing this up the
job got bigger and bigger.

There are still some in the migration cases but that can wait for another
burst.

Anyhow please see the patches. Most of them are pretty simple - the
do_memory_op is the most complex as it is used by many of the APIs, as such
if the maintainer does not wish to spend four hours double-checking that
I did not mess up - please just drop that patch from your mind.

 tools/libxc/xc_core_arm.c              | 15 ++++++++--
 tools/libxc/xc_core_x86.c              | 22 +++++++++++---
 tools/libxc/xc_cpupool.c               |  4 +--
 tools/libxc/xc_dom_x86.c               |  7 +++--
 tools/libxc/xc_domain.c                |  6 ++--
 tools/libxc/xc_domain_restore.c        |  2 +-
 tools/libxc/xc_domain_save.c           |  8 ++++-
 tools/libxc/xc_offline_page.c          | 42 +++++++++++++++++---------
 tools/libxc/xc_physdev.c               | 12 +++++---
 tools/libxc/xc_pm.c                    | 54 ++++++++++++++++++++++------------
 tools/libxc/xc_private.c               |  6 ++--
 tools/libxc/xc_tmem.c                  | 16 +++++++---
 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               |  2 +-
 tools/tests/mem-sharing/memshrtool.c   | 12 ++++++--
 tools/xenstat/libxenstat/src/xenstat.c |  5 ++--
 19 files changed, 161 insertions(+), 74 deletions(-)

[PATCH RFC 01/14] 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>
---
 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 RFC 02/14] libxc: xc_core_arch_memory_map_get populate errno

[Konrad Rzeszutek Wilk]

with proper value (ENOMEM) when reporting failures.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_core_arm.c | 1 +
 tools/libxc/xc_core_x86.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index 16508e7..888a3ac 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -55,6 +55,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
     if ( map == NULL )
     {
         PERROR("Could not allocate memory");
+        errno = ENOMEM;
         return -1;
     }
 
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index d8846f1..37b8522 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -60,6 +60,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
     if ( map == NULL )
     {
         PERROR("Could not allocate memory");
+        errno = ENOMEM;
         return -1;
     }
 
-- 
2.1.0

[PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Konrad Rzeszutek Wilk]

Instead put the -Exx in errno.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_domain.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 845d1d7..eb3a5f2 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -771,8 +771,10 @@ int xc_domain_get_tsc_info(xc_interface *xch,
 
     info = xc_hypercall_buffer_alloc(xch, info, sizeof(*info));
     if ( info == NULL )
-        return -ENOMEM;
-
+    {
+        errno = ENOMEM;
+        return -1;
+    }
     domctl.cmd = XEN_DOMCTL_gettscinfo;
     domctl.domain = (domid_t)domid;
     set_xen_guest_handle(domctl.u.tsc_info.out_info, info);
-- 
2.1.0

[PATCH RFC 04/14] libxl: 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>
---
 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 RFC 05/14] libxl: Return negative value and stash error in errno for xc_offline_page API

[Konrad Rzeszutek Wilk]

And also fix the user of it to print the proper error value.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_offline_page.c | 42 ++++++++++++++++++++++++++++--------------
 tools/libxc/xc_private.c      |  3 ++-
 tools/misc/xen-hptool.c       |  6 +++---
 3 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
index 3147203..e255dd9 100644
--- a/tools/libxc/xc_offline_page.c
+++ b/tools/libxc/xc_offline_page.c
@@ -58,12 +58,15 @@ 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;
+        errno = EINVAL;
+        return -1;
     }
 
     sysctl.cmd = XEN_SYSCTL_page_offline_op;
@@ -86,12 +89,15 @@ 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;
+        errno = EINVAL;
+        return -1;
     }
 
     sysctl.cmd = XEN_SYSCTL_page_offline_op;
@@ -114,12 +120,15 @@ 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;
+        errno = EINVAL;
+        return -1;
     }
 
     sysctl.cmd = XEN_SYSCTL_page_offline_op;
@@ -411,19 +420,22 @@ 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;
+        errno = EFAULT;
+        return -1;
     }
 
     if (!info.shutdown || info.shutdown_reason != SHUTDOWN_suspend)
     {
         ERROR("Can't exchange page unless domain is suspended\n");
-        return -EINVAL;
+        errno = EINVAL;
+        return -1;
     }
 
     if (!is_page_exchangable(xch, domid, mfn, &info))
     {
         ERROR("Could not exchange page\n");
-        return -EINVAL;
+        errno = EINVAL;
+        return -1;
     }
 
     /* Map M2P and obtain gpfn */
@@ -431,7 +443,8 @@ 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;
+        errno = EFAULT;
+        return -1;
     }
     gpfn = m2p_table[mfn];
 
@@ -440,7 +453,8 @@ 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;
+        errno = EFAULT;
+        return -1;
     }
 
     /* For translation macros */
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index df6cd9b..394338a 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -427,11 +427,12 @@ 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) )
     {
         PERROR("Could not bounce memory for mmuext op hypercall");
+        errno = ENOMEM;
         goto out1;
     }
 
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 RFC 06/14] libxl: 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>
---
 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 RFC 07/14] libxl: 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.

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

diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
index 3261e10..3e3246e 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -73,11 +73,15 @@ 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;
+            errno = ENOMEM;
+            return -1;
         }
     }
 
@@ -118,11 +122,15 @@ 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;
+            errno = ENOMEM;
+            return -1;
         }
     }
 
diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index 8072a90..bf257ef 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;
+	long 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,
+	rc = (long)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

[PATCH RFC 08/14] libxl: Check xc_domain_maximum_gpfn for negative return values

[Konrad Rzeszutek Wilk]

Instead of assuming everything is always OK.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_core_arm.c    | 14 +++++++++++---
 tools/libxc/xc_core_x86.c    | 21 +++++++++++++++++----
 tools/libxc/xc_domain_save.c |  8 +++++++-
 3 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
index 888a3ac..88f7ce1 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -31,9 +31,13 @@ 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, int *rc)
 {
-    return xc_domain_maximum_gpfn(xch, domid) + 1;
+    *rc = xc_domain_maximum_gpfn(xch, domid);
+
+    if ( *rc < 0 )
+        return 0;
+    return *rc + 1;
 }
 
 int
@@ -48,9 +52,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);
+    int rc;
+    unsigned long p2m_size = nr_gpfns(xch, info->domid, &rc);
     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 37b8522..f46468d 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -36,9 +36,13 @@ 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, int *rc)
 {
-    return xc_domain_maximum_gpfn(xch, domid) + 1;
+    *rc = xc_domain_maximum_gpfn(xch, domid);
+
+    if ( *rc < 0 )
+        return 0;
+    return *rc + 1;
 }
 
 int
@@ -53,9 +57,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);
+    int rc;
+    unsigned long p2m_size = nr_gpfns(xch, info->domid, &rc);
     xc_core_memory_map_t *map;
 
+    if ( rc < 0 )
+        return -1;
+
     map = malloc(sizeof(*map));
     if ( map == NULL )
     {
@@ -89,7 +97,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);
+    dinfo->p2m_size = nr_gpfns(xch, info->domid, &err);
+    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

[PATCH RFC 09/14] libxl: Check xc_maximum_ram_page for negative return values.

[Konrad Rzeszutek Wilk]

Instead of assuming everything is always OK.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xg_save_restore.h | 3 +++
 tools/misc/xen-mfndump.c      | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index bdd9009..6d26a0a 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -313,6 +313,9 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
 
     *max_mfn = xc_maximum_ram_page(xch);
 
+    if (*max_mfn < 0)
+        return 0;
+
     *hvirt_start = xen_params.virt_start;
 
     if ( xc_domain_get_guest_width(xch, dom, guest_width) != 0)
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 0761f6e..81ef448 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -184,7 +184,7 @@ int dump_ptes_func(int argc, char *argv[])
 
     /* Map M2P and obtain gpfn */
     max_mfn = xc_maximum_ram_page(xch);
-    if ( (mfn > max_mfn) ||
+    if ( (max_mfn < 0 ) || (mfn > max_mfn) ||
          !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
         xc_unmap_domain_meminfo(xch, &minfo);
-- 
2.1.0

[PATCH RFC 10/14] libxl: 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>
---
 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 bf06fe4..20e379c 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -865,7 +865,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->total_pages + i, rc);
+                         ", rc=%d, errno=%d)", __FUNCTION__,
+                         dom->total_pages + i, rc ,errno);
             return rc;
         }
     }
@@ -918,8 +919,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 RFC 11/14] libxl: Print xc_domain_decrease_reservation proper errno value.

[Konrad Rzeszutek Wilk]

Instead of just the return value. The other calls check for
the negative value and don't print any of the data - so this
patch just updates on call-site to include the proper error value.

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

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index a382701..89e5a29 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -2020,7 +2020,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
         {
             if ( (frc = xc_domain_decrease_reservation(xch, dom, nr_frees, 0, tailbuf.u.pv.pfntab)) != nr_frees )
             {
-                PERROR("Could not decrease reservation : %d", frc);
+                PERROR("Could not decrease reservation : %d", errno);
                 goto out;
             }
             else
-- 
2.1.0

[PATCH RFC 12/14] libxl: 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 94b4d59..99a99e9 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..d1dd8a2 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 errno;
+
+        printf("used = %ld\n", rc);
+        rc = xc_sharing_used_frames(xch);
+        if ( rc < 0 )
+            return errno;
+        printf("freed = %ld\n", rc);
     }
     else if( !strcasecmp(cmd, "enable") )
     {
-- 
2.1.0

[PATCH RFC 13/14] 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 overwritting it with the return value.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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 9ceb373..fe2ff10 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -224,10 +224,9 @@ static int libxl__e820_alloc(libxl__gc *gc, uint32_t domid,
         return ERROR_INVAL;
 
     rc = xc_get_machine_memory_map(ctx->xch, map, E820MAX);
-    if (rc < 0) {
-        errno = rc;
+    if (rc < 0)
         return ERROR_FAIL;
-    }
+
     nr = rc;
     rc = e820_sanitize(ctx, map, &nr, b_info->target_memkb,
                        (b_info->max_memkb - b_info->target_memkb) +
@@ -237,10 +236,8 @@ 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 RFC 14/14] libxl: 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. Fortunatly 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:

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

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

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 394338a..45903e8 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -517,11 +517,12 @@ 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) )
     {
         PERROR("Could not bounce memory for XENMEM hypercall");
+        errno = ENOMEM;
         goto out1;
     }
 
-- 
2.1.0

Re: [PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Konrad Rzeszutek Wilk]

On Fri, Mar 13, 2015 at 03:31:12PM -0400, Konrad Rzeszutek Wilk wrote:
> Instead put the -Exx in errno.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/xc_domain.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 845d1d7..eb3a5f2 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -771,8 +771,10 @@ int xc_domain_get_tsc_info(xc_interface *xch,
>  
>      info = xc_hypercall_buffer_alloc(xch, info, sizeof(*info));
>      if ( info == NULL )
> -        return -ENOMEM;
> -
> +    {
> +        errno = ENOMEM;
> +        return -1;
> +    }

While I was doing that I saw a lot of other code doing:


if ( xc_hypercall_bounce_pre (..) )
	return -1;

Should they all be fixed up to do:
	'errno = ENOMEM'

before returning -1?

Re: [PATCH RFC 02/14] libxc: xc_core_arch_memory_map_get populate errno

[Andrew Cooper]

On 13/03/15 19:31, Konrad Rzeszutek Wilk wrote:
> with proper value (ENOMEM) when reporting failures.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/xc_core_arm.c | 1 +
>  tools/libxc/xc_core_x86.c | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
> index 16508e7..888a3ac 100644
> --- a/tools/libxc/xc_core_arm.c
> +++ b/tools/libxc/xc_core_arm.c
> @@ -55,6 +55,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
>      if ( map == NULL )
>      {
>          PERROR("Could not allocate memory");
> +        errno = ENOMEM;

PERROR catches and prints errno, so set ENOMEM before the PERROR() call
to correct the error message.

~Andrew

>          return -1;
>      }
>  
> diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> index d8846f1..37b8522 100644
> --- a/tools/libxc/xc_core_x86.c
> +++ b/tools/libxc/xc_core_x86.c
> @@ -60,6 +60,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
>      if ( map == NULL )
>      {
>          PERROR("Could not allocate memory");
> +        errno = ENOMEM;
>          return -1;
>      }
>  

Re: [PATCH RFC 02/14] libxc: xc_core_arch_memory_map_get populate errno

[Konrad Rzeszutek Wilk]

On Fri, Mar 13, 2015 at 08:20:32PM +0000, Andrew Cooper wrote:
> On 13/03/15 19:31, Konrad Rzeszutek Wilk wrote:
> > with proper value (ENOMEM) when reporting failures.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxc/xc_core_arm.c | 1 +
> >  tools/libxc/xc_core_x86.c | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/tools/libxc/xc_core_arm.c b/tools/libxc/xc_core_arm.c
> > index 16508e7..888a3ac 100644
> > --- a/tools/libxc/xc_core_arm.c
> > +++ b/tools/libxc/xc_core_arm.c
> > @@ -55,6 +55,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
> >      if ( map == NULL )
> >      {
> >          PERROR("Could not allocate memory");
> > +        errno = ENOMEM;
> 
> PERROR catches and prints errno, so set ENOMEM before the PERROR() call
> to correct the error message.


Oooh. Right. I fixed it up this patch and the other ones.

Thank you!
> 
> ~Andrew
> 
> >          return -1;
> >      }
> >  
> > diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> > index d8846f1..37b8522 100644
> > --- a/tools/libxc/xc_core_x86.c
> > +++ b/tools/libxc/xc_core_x86.c
> > @@ -60,6 +60,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
> >      if ( map == NULL )
> >      {
> >          PERROR("Could not allocate memory");
> > +        errno = ENOMEM;
> >          return -1;
> >      }
> >  
> 

Re: [PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Ian Campbell]

On Fri, 2015-03-13 at 15:40 -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 13, 2015 at 03:31:12PM -0400, Konrad Rzeszutek Wilk wrote:
> > Instead put the -Exx in errno.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxc/xc_domain.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > index 845d1d7..eb3a5f2 100644
> > --- a/tools/libxc/xc_domain.c
> > +++ b/tools/libxc/xc_domain.c
> > @@ -771,8 +771,10 @@ int xc_domain_get_tsc_info(xc_interface *xch,
> >  
> >      info = xc_hypercall_buffer_alloc(xch, info, sizeof(*info));
> >      if ( info == NULL )
> > -        return -ENOMEM;
> > -
> > +    {
> > +        errno = ENOMEM;
> > +        return -1;
> > +    }
> 
> While I was doing that I saw a lot of other code doing:
> 
> 
> if ( xc_hypercall_bounce_pre (..) )
> 	return -1;
> 
> Should they all be fixed up to do:
> 	'errno = ENOMEM'
> 
> before returning -1?

I think xc_hypercall_bounce_pre should probably set an appropriate errno
on failure rather than making all the callers have to think about it. It
may indirectly already do so via mmap? I didn't actually go look...


Ian.

Re: [PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Konrad Rzeszutek Wilk]

On Sat, Mar 14, 2015 at 07:55:39AM +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 15:40 -0400, Konrad Rzeszutek Wilk wrote:
> > On Fri, Mar 13, 2015 at 03:31:12PM -0400, Konrad Rzeszutek Wilk wrote:
> > > Instead put the -Exx in errno.
> > > 
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > ---
> > >  tools/libxc/xc_domain.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > > index 845d1d7..eb3a5f2 100644
> > > --- a/tools/libxc/xc_domain.c
> > > +++ b/tools/libxc/xc_domain.c
> > > @@ -771,8 +771,10 @@ int xc_domain_get_tsc_info(xc_interface *xch,
> > >  
> > >      info = xc_hypercall_buffer_alloc(xch, info, sizeof(*info));
> > >      if ( info == NULL )
> > > -        return -ENOMEM;
> > > -
> > > +    {
> > > +        errno = ENOMEM;
> > > +        return -1;
> > > +    }
> > 
> > While I was doing that I saw a lot of other code doing:
> > 
> > 
> > if ( xc_hypercall_bounce_pre (..) )
> > 	return -1;
> > 
> > Should they all be fixed up to do:
> > 	'errno = ENOMEM'
> > 
> > before returning -1?
> 
> I think xc_hypercall_bounce_pre should probably set an appropriate errno
> on failure rather than making all the callers have to think about it. It

Doh!

> may indirectly already do so via mmap? I didn't actually go look...

Ugh. We seem to be doing pthread manipulation which can return values
(which we don't check) and put values in errno.

Then the 'mmap' which can fail with an variety of errno values.
Ditto for 'madvise'.

I think we are actually safe - we do put in errno values in.


This patch should be more of removing the 'errno' and just return -1.


Hm, the pthread manipulation is worrying. We do a lot of:

 rc = some_hypercall[]
 (Linux stashes the errnor value in)

 xc_hypercall_bounce_post(..)
 [if hte pthread fails it will stash an errno value in!]


And we end up over-writting the proper errno value (say EACCESS!).

This ought to do it:

commit 45dc0277aed59a891e8bae7bab52dfaf96b5b308
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Mon Mar 16 09:57:05 2015 -0400

    libxl: Propagate errno from hypercall instead of anything else.
    
    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>

diff --git a/tools/libxc/xc_freebsd_osdep.c b/tools/libxc/xc_freebsd_osdep.c
index 151d3bf..5ada50d 100644
--- a/tools/libxc/xc_freebsd_osdep.c
+++ b/tools/libxc/xc_freebsd_osdep.c
@@ -125,10 +125,12 @@ 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);
+    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..b3b5416 100644
--- a/tools/libxc/xc_hcall_buf.c
+++ b/tools/libxc/xc_hcall_buf.c
@@ -33,16 +33,24 @@ pthread_mutex_t hypercall_buffer_cache_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 static void hypercall_buffer_cache_lock(xc_interface *xch)
 {
+    int saved_errno;
     if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
         return;
+    saved_errno = errno;
     pthread_mutex_lock(&hypercall_buffer_cache_mutex);
+    /* Ignore the pthread errors. */
+    errno = saved_errno;
 }
 
 static void hypercall_buffer_cache_unlock(xc_interface *xch)
 {
+    int saved_errno;
     if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
         return;
+    saved_errno = errno;
     pthread_mutex_unlock(&hypercall_buffer_cache_mutex);
+    /* Ignore the 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 a19e4b6..15d772b 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 calls. */
+    errno = saved_errno;
 }
 
 static int linux_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcmd_hypercall_t *hypercall)
> 
> 
> Ian.
> 

Re: [PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Ian Campbell]

On Mon, 2015-03-16 at 10:11 -0400, Konrad Rzeszutek Wilk wrote:
>  static void hypercall_buffer_cache_lock(xc_interface *xch)
>  {
> +    int saved_errno;
>      if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
>          return;
> +    saved_errno = errno;

You can do the init on declaration.

> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> index a19e4b6..15d772b 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 calls. */

Hrm, in general you would want to propagate failure here if the mmap had
succeeded, otherwise you want the mmap failure. Which would involve
pushing the saving into the callers.

But since this function is (for better or worse) void (mainly because it
may trigger out of band due to the cache) I think you are right to
preserve the errno as you've done.

> +    errno = saved_errno;
>  }
>  
>  static int linux_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcmd_hypercall_t *hypercall)
> > 
> > 
> > Ian.
> > 

Re: [PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Konrad Rzeszutek Wilk]

On Mon, Mar 16, 2015 at 02:23:22PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-16 at 10:11 -0400, Konrad Rzeszutek Wilk wrote:
> >  static void hypercall_buffer_cache_lock(xc_interface *xch)
> >  {
> > +    int saved_errno;
> >      if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
> >          return;
> > +    saved_errno = errno;
> 
> You can do the init on declaration.
> 
> > diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> > index a19e4b6..15d772b 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 calls. */
> 
> Hrm, in general you would want to propagate failure here if the mmap had
> succeeded, otherwise you want the mmap failure. Which would involve
> pushing the saving into the callers.
> 
> But since this function is (for better or worse) void (mainly because it
> may trigger out of band due to the cache) I think you are right to
> preserve the errno as you've done.

OK. Will repost the whole series with the above mentioned fix as v1.

Do you want me to post it now (v1) or are you right now looking at the
patchset and would want to finish that?

> 
> > +    errno = saved_errno;
> >  }
> >  
> >  static int linux_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcmd_hypercall_t *hypercall)
> > > 
> > > 
> > > Ian.
> > > 
> 
> 

Re: [PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Ian Campbell]

On Mon, 2015-03-16 at 10:38 -0400, Konrad Rzeszutek Wilk wrote:
> Do you want me to post it now (v1) or are you right now looking at the
> patchset and would want to finish that?

I'm battling yakks right now not reading the series, so you may as well
send it!

> 
> > 
> > > +    errno = saved_errno;
> > >  }
> > >  
> > >  static int linux_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, privcmd_hypercall_t *hypercall)
> > > > 
> > > > 
> > > > Ian.
> > > > 
> > 
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel