[PATCH] x86/domctl: Improve XEN_DOMCTL_hypercall_init error handling

[PATCH] x86/domctl: Improve XEN_DOMCTL_hypercall_init error handling

[Andrew Cooper]

EACCES cannot be distinguished against an incorrect DOMCTL_INTERFACE_VERSION,
and will cause an incorrect "need to rebuild the user-space tool set?" message
from libxc.  EINVAL is a perfectly reasonable alternative.

On the libxc side, put the useful piece of information in the error message,
rathe than the -1 from do_domctl().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_boot.c |    6 +++---
 xen/arch/x86/domctl.c     |    4 +---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index a141eb5..f82db2d 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -57,9 +57,9 @@ static int setup_hypercall_page(struct xc_dom_image *dom)
     domctl.u.hypercall_init.gmfn = xc_dom_p2m_guest(dom, pfn);
     rc = do_domctl(dom->xch, &domctl);
     if ( rc != 0 )
-        xc_dom_panic(dom->xch,
-                     XC_INTERNAL_ERROR, "%s: HYPERCALL_INIT failed (rc=%d)",
-                     __FUNCTION__, rc);
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "%s: HYPERCALL_INIT failed: %d - %s)",
+                     __FUNCTION__, errno, strerror(errno));
     return rc;
 }
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 3e5bef1..3ffb083 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -386,16 +386,14 @@ long arch_do_domctl(
 
         page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
 
-        ret = -EACCES;
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
+            ret = -EINVAL;
             if ( page )
                 put_page(page);
             break;
         }
 
-        ret = 0;
-
         hypercall_page = __map_domain_page(page);
         hypercall_page_initialise(d, hypercall_page);
         unmap_domain_page(hypercall_page);
-- 
1.7.10.4

Re: [PATCH] x86/domctl: Improve XEN_DOMCTL_hypercall_init error handling

[Jan Beulich]

>>> On 11.03.15 at 13:56, <andrew.cooper3@citrix.com> wrote:
> EACCES cannot be distinguished against an incorrect DOMCTL_INTERFACE_VERSION,
> and will cause an incorrect "need to rebuild the user-space tool set?" 
> message
> from libxc.  EINVAL is a perfectly reasonable alternative.

I'm okay with this for the !page case, but I think -EPERM or some
such would be better suited when the page is non-writable but
otherwise valid.

Jan

[PATCH v2 1/6] tools/libxl: Introduce min and max macros

[Andrew Cooper]

This is the same set used by libxc.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

---
v2: Don't use reserved identifiers in min_t/max_t
---
 tools/libxl/libxl_internal.h |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..fcbec7f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -108,6 +108,22 @@
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
+#define min(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x < _y) ? _x : _y; })
+#define max(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x > _y) ? _x : _y; })
+
+#define min_t(type, x, y)                                               \
+    ({ const type _x = (x); const type _y = (y); _x < _y ? _x: _y; })
+#define max_t(type, x, y)                                               \
+    ({ const type _x = (x); const type _y = (y); _x > _y ? _x: _y; })
+
 #define LIBXL__LOGGING_ENABLED
 
 #ifdef LIBXL__LOGGING_ENABLED
-- 
1.7.10.4

Re: [PATCH v2 1/6] tools/libxl: Introduce min and max macros

[Andrew Cooper]

On 12/03/15 11:01, Andrew Cooper wrote:
> This is the same set used by libxc.
>

Please ignore this - I had an accident with git-send-email.

~Andrew

[PATCH v2] x86/domctl: Improve XEN_DOMCTL_hypercall_init error handling

[Andrew Cooper]

EACCES cannot be distinguished against an incorrect DOMCTL_INTERFACE_VERSION,
and will cause an incorrect "need to rebuild the user-space tool set?" message
from libxc.

On the libxc side, put the useful piece of information in the error message,
rathe than the -1 from do_domctl().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

---
v2: Split error between "no such frame" and "bad type"
---
 tools/libxc/xc_dom_boot.c |    6 +++---
 xen/arch/x86/domctl.c     |    8 +++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index a141eb5..f82db2d 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -57,9 +57,9 @@ static int setup_hypercall_page(struct xc_dom_image *dom)
     domctl.u.hypercall_init.gmfn = xc_dom_p2m_guest(dom, pfn);
     rc = do_domctl(dom->xch, &domctl);
     if ( rc != 0 )
-        xc_dom_panic(dom->xch,
-                     XC_INTERNAL_ERROR, "%s: HYPERCALL_INIT failed (rc=%d)",
-                     __FUNCTION__, rc);
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "%s: HYPERCALL_INIT failed: %d - %s)",
+                     __FUNCTION__, errno, strerror(errno));
     return rc;
 }
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 3e5bef1..d4f6ccf 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -386,16 +386,18 @@ long arch_do_domctl(
 
         page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
 
-        ret = -EACCES;
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
             if ( page )
+            {
+                ret = -EPERM;
                 put_page(page);
+            }
+            else
+                ret = -EINVAL;
             break;
         }
 
-        ret = 0;
-
         hypercall_page = __map_domain_page(page);
         hypercall_page_initialise(d, hypercall_page);
         unmap_domain_page(hypercall_page);
-- 
1.7.10.4

Re: [PATCH v2] x86/domctl: Improve XEN_DOMCTL_hypercall_init error handling

[Jan Beulich]

>>> On 12.03.15 at 12:03, <andrew.cooper3@citrix.com> wrote:
> EACCES cannot be distinguished against an incorrect DOMCTL_INTERFACE_VERSION,
> and will cause an incorrect "need to rebuild the user-space tool set?" 
> message
> from libxc.
> 
> On the libxc side, put the useful piece of information in the error message,
> rathe than the -1 from do_domctl().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2] x86/domctl: Improve XEN_DOMCTL_hypercall_init error handling

[Wei Liu]

On Thu, Mar 12, 2015 at 11:03:22AM +0000, Andrew Cooper wrote:
> EACCES cannot be distinguished against an incorrect DOMCTL_INTERFACE_VERSION,
> and will cause an incorrect "need to rebuild the user-space tool set?" message
> from libxc.
> 
> On the libxc side, put the useful piece of information in the error message,
> rathe than the -1 from do_domctl().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH v2 0/6] tools/libxl: Improvements to libxl__datacopier

[Andrew Cooper]

To support migration v2, we need far more flexibility out of the datacopier.

This series adds the ability to read from an fd into a local buffer and to
copy a specific number of bytes rather than to EOF.  It also contains bugfixes
related to writing from a local buffer, and POLLHUP handling.

Changes in v2:
 * Rewrite most commit message (but no code change)
 * Rework the POLLHUP fix from scratch

Andrew Cooper (2):
  tools/libxl: Introduce min and max macros
  tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

Ross Lagerwall (3):
  tools/libxl: Allow adding larger amounts of prefixdata to datacopier
  tools/libxl: Allow limiting amount copied by datacopier
  tools/libxl: Extend datacopier to support reading into a buffer

Wen Congyang (1):
  tools/libxl: Update datacopier to support sending data only

 tools/libxl/libxl_aoutils.c    |  121 +++++++++++++++++++++++++---------------
 tools/libxl/libxl_bootloader.c |   24 +++++++-
 tools/libxl/libxl_dom.c        |    1 +
 tools/libxl/libxl_internal.h   |   35 ++++++++++--
 4 files changed, 129 insertions(+), 52 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/6] tools/libxl: Introduce min and max macros

[Andrew Cooper]

This is the same set used by libxc.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

---
v2: Don't use reserved identifiers in min_t/max_t
---
 tools/libxl/libxl_internal.h |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..fcbec7f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -108,6 +108,22 @@
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
+#define min(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x < _y) ? _x : _y; })
+#define max(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x > _y) ? _x : _y; })
+
+#define min_t(type, x, y)                                               \
+    ({ const type _x = (x); const type _y = (y); _x < _y ? _x: _y; })
+#define max_t(type, x, y)                                               \
+    ({ const type _x = (x); const type _y = (y); _x > _y ? _x: _y; })
+
 #define LIBXL__LOGGING_ENABLED
 
 #ifdef LIBXL__LOGGING_ENABLED
-- 
1.7.10.4

Re: [PATCH v2 1/6] tools/libxl: Introduce min and max macros

[Ian Campbell]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> This is the same set used by libxc.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> ---
> v2: Don't use reserved identifiers in min_t/max_t
> ---
>  tools/libxl/libxl_internal.h |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 934465a..fcbec7f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -108,6 +108,22 @@
>  
>  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>  
> +#define min(X, Y) ({                             \
> +            const typeof (X) _x = (X);           \
> +            const typeof (Y) _y = (Y);           \

You've avoided the reserved-by-C names here, but IIRC a single
underscore is reserved by POSIX.

However since you've just copied from libxc, I think we can live with
this until someone find the energy to fix them all:
        Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.