[PATCH v1] Fix libxc return -E misusage.

[PATCH v1] Fix libxc return -E misusage.

[Konrad Rzeszutek Wilk]

Hey,

Please see the following set of patches which fix the various
usage of return -Exx instead of return -1 for errors (and
stashing the error value in errno).

It also cleans up some of the invalid usage of errno (as the
underlaying calls already stash errno values) - and we just
need to bubble them up.

Lastly it also wraps an errno-invisibility shield against
xc_hypercall_bounce_* calls so that any errors in those
won't over-write the hypercall ones.


 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                |  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          | 36 +++++++++++++----------
 tools/libxc/xc_physdev.c               | 12 +++++---
 tools/libxc/xc_pm.c                    | 54 ++++++++++++++++++++++------------
 tools/libxc/xc_private.c               |  4 +--
 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               |  2 +-
 tools/tests/mem-sharing/memshrtool.c   | 12 ++++++--
 tools/xenstat/libxenstat/src/xenstat.c |  5 ++--
 21 files changed, 158 insertions(+), 73 deletions(-)

[PATCH 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

Re: [PATCH 01/14] libxc: Replaces tabs with spaces in xc_cpupool_freeinfo

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 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>

[PATCH 02/14] libxl: 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>
---
 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..bb6d240 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 calls. */
+    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..f5426a1 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 the 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 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)
-- 
2.1.0

Re: [PATCH 02/14] libxl: Propagate errno from hypercall instead of anything else.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 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..bb6d240 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 calls. */

If you have cause to resend then:

 /* We MUST Propagate the hypercall's ernno, not the unmap call's. */.

> +    /* Ignore the pthread errors. */

s/the //

(both throughout).

Not a big deal but if it needs resending anyway

Ian.

[PATCH 03/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>
---
[v1: errno before using PERROR]
---
 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..34185cf 100644
--- a/tools/libxc/xc_core_arm.c
+++ b/tools/libxc/xc_core_arm.c
@@ -54,6 +54,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
     map = malloc(sizeof(*map));
     if ( map == NULL )
     {
+        errno = ENOMEM;
         PERROR("Could not allocate memory");
         return -1;
     }
diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
index d8846f1..b5d442d 100644
--- a/tools/libxc/xc_core_x86.c
+++ b/tools/libxc/xc_core_x86.c
@@ -59,6 +59,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
     map = malloc(sizeof(*map));
     if ( map == NULL )
     {
+        errno = ENOMEM;
         PERROR("Could not allocate memory");
         return -1;
     }
-- 
2.1.0

Re: [PATCH 03/14] libxc: xc_core_arch_memory_map_get populate errno

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> with proper value (ENOMEM) when reporting failures.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> [v1: errno before using PERROR]
> ---
>  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..34185cf 100644
> --- a/tools/libxc/xc_core_arm.c
> +++ b/tools/libxc/xc_core_arm.c
> @@ -54,6 +54,7 @@ xc_core_arch_memory_map_get(xc_interface *xch, struct xc_core_arch_context *unus
>      map = malloc(sizeof(*map));
>      if ( map == NULL )
>      {
> +        errno = ENOMEM;

http://pubs.opengroup.org/onlinepubs/9699919799/functions/malloc.html#tag_16_311 says that malloc will set errno itself:
        "Otherwise, it shall return a null pointer [CX] [Option Start]
        and set errno to indicate the error"

[PATCH 04/14] 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 appropiate errno values and we just
need to pass them up the stack.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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

Re: [PATCH 04/14] libxc: Fix xc_domain_get_tsc_info to return -1 instead of -Exx.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> We don't need to put fill errno because xc_hypercall_buffer_alloc
> fills the errno with the appropiate errno values and we just
> need to pass them up the stack.

"appropriate"

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

[PATCH 05/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

Re: [PATCH 05/14] libxl: xc_physdev_map return -1 and populate errno.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 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.

IIRC I saw a qemu patch go past to fix the callers?

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

Re: [PATCH 05/14] libxl: xc_physdev_map return -1 and populate errno.

[Konrad Rzeszutek Wilk]

On Wed, Mar 18, 2015 at 04:21:52PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> > 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.
> 
> IIRC I saw a qemu patch go past to fix the callers?
> 

Yes and Stefano has already Acked them.
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Yeey!
> 
> > ---
> >  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;
> 
> 

[PATCH 06/14] libxl: 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>
---
 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

Re: [PATCH 06/14] libxl: Return negative value and propagate errno for xc_offline_page API

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 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>

[PATCH 07/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

Re: [PATCH 07/14] libxl: Fix xc_pm API calls to return negative error and stash error in errno.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 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>

[PATCH 08/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.

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 |  5 +++--
 2 files changed, 13 insertions(+), 6 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..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

Re: [PATCH 08/14] libxl: Fix xc_tmem_control to return proper error.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -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>
> ---
>  tools/libxc/xc_tmem.c                  | 14 ++++++++++----
>  tools/xenstat/libxenstat/src/xenstat.c |  5 +++--
>  2 files changed, 13 insertions(+), 6 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..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);

Why the cast, why not make rc an int since that is what xc_tmem_control
takes and you don't seem to use the full width anyway?

Or alternatively fix the return type of xc_tmem_control.

> -
> +	node->freeable_mb = (rc < 0) ? 0 : rc;

Should rc not get propagated into an error for the caller?

>  	/* malloc(0) is not portable, so allocate a single domain.  This will
>  	 * be resized below. */
>  	node->domains = malloc(sizeof(xenstat_domain));

Re: [PATCH 08/14] libxl: Fix xc_tmem_control to return proper error.

[Konrad Rzeszutek Wilk]

On Wed, Mar 18, 2015 at 04:26:37PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-16 at 11:39 -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>
> > ---
> >  tools/libxc/xc_tmem.c                  | 14 ++++++++++----
> >  tools/xenstat/libxenstat/src/xenstat.c |  5 +++--
> >  2 files changed, 13 insertions(+), 6 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..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);
> 
> Why the cast, why not make rc an int since that is what xc_tmem_control
> takes and you don't seem to use the full width anyway?

Right. int should be enough.
> 
> Or alternatively fix the return type of xc_tmem_control.
> 
> > -
> > +	node->freeable_mb = (rc < 0) ? 0 : rc;
> 
> Should rc not get propagated into an error for the caller?

Nope. If tmem is not enabled (so xc_tmem_control returns -ENOSYS)
freeable_mb should be zero. In this case we would have returned negative
values which is certainly not right.

> 
> >  	/* malloc(0) is not portable, so allocate a single domain.  This will
> >  	 * be resized below. */
> >  	node->domains = malloc(sizeof(xenstat_domain));
> 
> 

Re: [PATCH 08/14] libxl: Fix xc_tmem_control to return proper error.

[Ian Campbell]

On Wed, 2015-03-18 at 13:37 -0400, Konrad Rzeszutek Wilk wrote:
> > Or alternatively fix the return type of xc_tmem_control.
> > 
> > > -
> > > +	node->freeable_mb = (rc < 0) ? 0 : rc;
> > 
> > Should rc not get propagated into an error for the caller?
> 
> Nope. If tmem is not enabled (so xc_tmem_control returns -ENOSYS)

I think after these changes you mean returns -1 with errno == -ENOSYS?

> freeable_mb should be zero. In this case we would have returned negative
> values which is certainly not right.

What about other possible error codes from xc_tmem_control? At the least
it appears it can return with errno=ENOMEM (from the hypercall buffer
usage).

Ian.

[PATCH 09/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 34185cf..654692a 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 b5d442d..426b90d 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

Re: [PATCH 09/14] libxl: Check xc_domain_maximum_gpfn for negative return values

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 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 34185cf..654692a 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)

It would be more natural to return the rc directly and update a pointer
argument for the max gpfn on success.

> diff --git a/tools/libxc/xc_core_x86.c b/tools/libxc/xc_core_x86.c
> index b5d442d..426b90d 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)

Likewise, although perhaps you could consolidate those two into one
common wrapper? And then use it everywhere (there were some open-coded
ones later on).

Or even fix the prototype of xc_domain_maximum_gpfn turning it into said
helper, it doesn't look like it has too many callers.

Ian.

[PATCH 10/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

Re: [PATCH 10/14] libxl: Check xc_maximum_ram_page for negative return values.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> 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;

*max_mfn is unsigned, so the negative would have been lost already.
Perhaps do the same sort of thing with a helper as the last patch?

> +
>      *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) ||

Same here.

>           !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
>      {
>          xc_unmap_domain_meminfo(xch, &minfo);

[PATCH 11/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

Re: [PATCH 11/14] libxl: If xc_domain_add_to_physmap fails, include errno value

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:

$subject says libxl but means libxc.

Actually, now I notice it so do almost all of the patches! Once fixed
any acks I gave (or will give shortly) can still be applied.

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

s/ ,/, /

(hey, I drew a pretty picture)

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

Ian.

[PATCH 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

Re: [PATCH 12/14] libxl: Check xc_sharing_* for proper return values.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> If there is a negative return value - check for that and
> also use errno for the proper error value.

Was xc_sharing_freed_pages fixed earlier in the series (which is
strictly speaking a bisection hazard, but nevermind) or was the existing
check for l == -E... just buggy?

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

Just return 1 please.

> +
> +        printf("used = %ld\n", rc);
> +        rc = xc_sharing_used_frames(xch);
> +        if ( rc < 0 )
> +            return errno;

Likewise.

> +        printf("freed = %ld\n", rc);
>      }
>      else if( !strcasecmp(cmd, "enable") )
>      {

Re: [PATCH 12/14] libxl: Check xc_sharing_* for proper return values.

[Konrad Rzeszutek Wilk]

On Wed, Mar 18, 2015 at 04:36:40PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> > If there is a negative return value - check for that and
> > also use errno for the proper error value.
> 
> Was xc_sharing_freed_pages fixed earlier in the series (which is
> strictly speaking a bisection hazard, but nevermind) or was the existing
> check for l == -E... just buggy?

The existing check for ENOSYS was buggy.
> 
> > 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;
> 
> Just return 1 please.
> 
> > +
> > +        printf("used = %ld\n", rc);
> > +        rc = xc_sharing_used_frames(xch);
> > +        if ( rc < 0 )
> > +            return errno;
> 
> Likewise.
> 
> > +        printf("freed = %ld\n", rc);
> >      }
> >      else if( !strcasecmp(cmd, "enable") )
> >      {
> 
> 

[PATCH 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

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

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:

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

"overwriting"

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

[PATCH 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 0735e23..1222d05 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 14/14] libxl: Fix do_memory_op to return negative value on errors

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> instead of the -Exx values (which should go in errno).
> 
> This patch has HUGE implications.

For such a small patch!

>  There is a lot of APIs
> that are using do_memory_op. Fortunatly most of them

"Fortunately".

> 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

s/libxl/libxc/ in most of them.

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.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 0735e23..1222d05 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) )
>      {

Re: [PATCH v1] Fix libxc return -E misusage.

[Ian Campbell]

On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> Hey,
> 
> Please see the following set of patches which fix the various
> usage of return -Exx instead of return -1 for errors (and
> stashing the error value in errno).

All of them, or just a subset of the callpaths?

If it's all of them then I'm amazed it was only 14 patches!

Either way, well done and thanks for draining this swamp even a bit!

> It also cleans up some of the invalid usage of errno (as the
> underlaying calls already stash errno values) - and we just
> need to bubble them up.
> 
> Lastly it also wraps an errno-invisibility shield against
> xc_hypercall_bounce_* calls so that any errors in those
> won't over-write the hypercall ones.
> 
> 
>  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                |  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          | 36 +++++++++++++----------
>  tools/libxc/xc_physdev.c               | 12 +++++---
>  tools/libxc/xc_pm.c                    | 54 ++++++++++++++++++++++------------
>  tools/libxc/xc_private.c               |  4 +--
>  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               |  2 +-
>  tools/tests/mem-sharing/memshrtool.c   | 12 ++++++--
>  tools/xenstat/libxenstat/src/xenstat.c |  5 ++--
>  21 files changed, 158 insertions(+), 73 deletions(-)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v1] Fix libxc return -E misusage.

[Konrad Rzeszutek Wilk]

On Wed, Mar 18, 2015 at 04:43:07PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-16 at 11:39 -0400, Konrad Rzeszutek Wilk wrote:
> > Hey,
> > 
> > Please see the following set of patches which fix the various
> > usage of return -Exx instead of return -1 for errors (and
> > stashing the error value in errno).
> 
> All of them, or just a subset of the callpaths?

Not all of them. I did do a careful analysis of the
ones that the patches have to make sure I didn't cause
any regressions. But I hadn't looked at some of
the other ones because I got fatigued.
> 
> If it's all of them then I'm amazed it was only 14 patches!

I am sure there are others. When I get another day when to do
spring cleaning I will crank out some other patches.
> 
> Either way, well done and thanks for draining this swamp even a bit!

Hehe
> 
> > It also cleans up some of the invalid usage of errno (as the
> > underlaying calls already stash errno values) - and we just
> > need to bubble them up.
> > 
> > Lastly it also wraps an errno-invisibility shield against
> > xc_hypercall_bounce_* calls so that any errors in those
> > won't over-write the hypercall ones.
> > 
> > 
> >  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                |  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          | 36 +++++++++++++----------
> >  tools/libxc/xc_physdev.c               | 12 +++++---
> >  tools/libxc/xc_pm.c                    | 54 ++++++++++++++++++++++------------
> >  tools/libxc/xc_private.c               |  4 +--
> >  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               |  2 +-
> >  tools/tests/mem-sharing/memshrtool.c   | 12 ++++++--
> >  tools/xenstat/libxenstat/src/xenstat.c |  5 ++--
> >  21 files changed, 158 insertions(+), 73 deletions(-)
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
>