[PATCH for-4.6 0/3] Get rid of non-POSIX error codes

[PATCH for-4.6 0/3] Get rid of non-POSIX error codes

[Roger Pau Monne]

This patch series gets rid of non-POSIX error codes in the hypervisor and 
the toolstack. This is needed for OS compatibility.

I think the patch series is fairly small and non-intrusive, hence the 
for-4.6 tag.

Thanks, Roger.

[PATCH for-4.6 1/3] xen/x86/libxl: replace non-POSIX error codes used by PSR code

[Roger Pau Monne]

PSR was using EBADSLT and EUSERS which are not POSIX error codes, replace
them with EINVAL and EOVERFLOW respectively.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxl/libxl_psr.c | 6 +++---
 xen/arch/x86/psr.c      | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 2a07777..f64406a 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -31,7 +31,7 @@ static void libxl__psr_log_err_msg(libxl__gc *gc, int err)
     case ESRCH:
         msg = "invalid domain ID";
         break;
-    case EBADSLT:
+    case EINVAL:
         msg = "socket is not supported";
         break;
     case EFAULT:
@@ -59,7 +59,7 @@ static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int err)
     case ENOENT:
         msg = "CMT is not attached to this domain";
         break;
-    case EUSERS:
+    case EOVERFLOW:
         msg = "no free RMID available";
         break;
     default:
@@ -81,7 +81,7 @@ static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int err)
     case ENOENT:
         msg = "CAT is not enabled on the socket";
         break;
-    case EUSERS:
+    case EOVERFLOW:
         msg = "no free COS available";
         break;
     case EEXIST:
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 861683f..0185c45 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -176,7 +176,7 @@ int psr_alloc_rmid(struct domain *d)
     if ( rmid > psr_cmt->rmid_max )
     {
         d->arch.psr_rmid = 0;
-        return -EUSERS;
+        return -EOVERFLOW;
     }
 
     d->arch.psr_rmid = rmid;
@@ -251,7 +251,7 @@ static struct psr_cat_socket_info *get_cat_socket_info(unsigned int socket)
         return ERR_PTR(-ENODEV);
 
     if ( socket >= nr_sockets )
-        return ERR_PTR(-EBADSLT);
+        return ERR_PTR(-EINVAL);
 
     if ( !test_bit(socket, cat_socket_enable) )
         return ERR_PTR(-ENOENT);
@@ -332,7 +332,7 @@ static int write_l3_cbm(unsigned int socket, unsigned int cos, uint64_t cbm)
         unsigned int cpu = get_socket_cpu(socket);
 
         if ( cpu >= nr_cpu_ids )
-            return -EBADSLT;
+            return -EINVAL;
         on_selected_cpus(cpumask_of(cpu), do_write_l3_cbm, &info, 1);
     }
 
@@ -381,7 +381,7 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm)
     if ( !found )
     {
         spin_unlock(&info->cbm_lock);
-        return -EUSERS;
+        return -EOVERFLOW;
     }
 
     cos = found - map;
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH for-4.6 1/3] xen/x86/libxl: replace non-POSIX error codes used by PSR code

[Jan Beulich]

>>> On 21.07.15 at 17:56, <roger.pau@citrix.com> wrote:
> PSR was using EBADSLT and EUSERS which are not POSIX error codes, replace
> them with EINVAL and EOVERFLOW respectively.

Considering that we use EINVAL for almost everything (well beyond
parameter checking I'm afraid), I don't think using this value for
something intended to yield a specific user mode error message is
really a good choice. Looking at the two respective hypervisor side
changes - how about e.g. using EDOM, EBADF, or ENXIO instead?

Jan

Re: [PATCH for-4.6 1/3] xen/x86/libxl: replace non-POSIX error codes used by PSR code

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH for-4.6 1/3] xen/x86/libxl: replace non-POSIX error codes used by PSR code"):
> On 21.07.15 at 17:56, <roger.pau@citrix.com> wrote:
> > PSR was using EBADSLT and EUSERS which are not POSIX error codes, replace
> > them with EINVAL and EOVERFLOW respectively.
> 
> Considering that we use EINVAL for almost everything (well beyond
> parameter checking I'm afraid), I don't think using this value for
> something intended to yield a specific user mode error message is
> really a good choice. Looking at the two respective hypervisor side
> changes - how about e.g. using EDOM, EBADF, or ENXIO instead?

EBADF is rather poor because it's the same error code you would get if
your privcmd fd had been closed.

Ian.

Re: [PATCH for-4.6 1/3] xen/x86/libxl: replace non-POSIX error codes used by PSR code

[Roger Pau Monné]

El 21/07/15 a les 18.11, Jan Beulich ha escrit:
>>>> On 21.07.15 at 17:56, <roger.pau@citrix.com> wrote:
>> PSR was using EBADSLT and EUSERS which are not POSIX error codes, replace
>> them with EINVAL and EOVERFLOW respectively.
> 
> Considering that we use EINVAL for almost everything (well beyond
> parameter checking I'm afraid), I don't think using this value for
> something intended to yield a specific user mode error message is
> really a good choice. Looking at the two respective hypervisor side
> changes - how about e.g. using EDOM, EBADF, or ENXIO instead?

As suggested in a later email, replacing EBADSLT with ENOTSOCK seems
better here.

Roger.

[PATCH for-4.6 2/3] xen: replace non-POSIX error codes

[Roger Pau Monne]

Some DOMCTLs returned non-POSIX error codes, replace them with POSIX
compilant values instead. EBADRQC and EBADSLT are replaced by EINVAL, while
EUSERS is replaced with EOVERFLOW.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Nothing in libxc or libxl seems to check for those specific error codes, so
I guess it's fine to replace them with whatever we want.
---
 xen/arch/x86/mm/paging.c | 2 +-
 xen/common/domain.c      | 4 ++--
 xen/common/hvm/save.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 7089155..1c3504d 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -766,7 +766,7 @@ long paging_domctl_continuation(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 
     if ( op.interface_version != XEN_DOMCTL_INTERFACE_VERSION ||
          op.cmd != XEN_DOMCTL_shadow_op )
-        return -EBADRQC;
+        return -EINVAL;
 
     d = rcu_lock_domain_by_id(op.domain);
     if ( d == NULL )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8efef5c..791166b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -900,7 +900,7 @@ int vcpu_pause_by_systemcontroller(struct vcpu *v)
         new = old + 1;
 
         if ( new > 255 )
-            return -EUSERS;
+            return -EOVERFLOW;
 
         prev = cmpxchg(&v->controller_pause_count, old, new);
     } while ( prev != old );
@@ -980,7 +980,7 @@ int __domain_pause_by_systemcontroller(struct domain *d,
          * toolstack overflowing d->pause_count with many repeated hypercalls.
          */
         if ( new > 255 )
-            return -EUSERS;
+            return -EOVERFLOW;
 
         prev = cmpxchg(&d->controller_pause_count, old, new);
     } while ( prev != old );
diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index da6e668..db85fee 100644
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -114,7 +114,7 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
         uint32_t off;
         const struct hvm_save_descriptor *desc;
 
-        rv = -EBADSLT;
+        rv = -EINVAL;
         for ( off = 0; off < (ctxt.cur - sizeof(*desc)); off += desc->length )
         {
             desc = (void *)(ctxt.data + off);
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH for-4.6 2/3] xen: replace non-POSIX error codes

[Jan Beulich]

>>> On 21.07.15 at 17:56, <roger.pau@citrix.com> wrote:
> Some DOMCTLs returned non-POSIX error codes, replace them with POSIX
> compilant values instead. EBADRQC and EBADSLT are replaced by EINVAL, while
> EUSERS is replaced with EOVERFLOW.

Same here basically - I'd appreciate if we could use EINVAL only as
a last resort error value, to make errors distinguishable.

Jan

Re: [PATCH for-4.6 2/3] xen: replace non-POSIX error codes

[George Dunlap]

On Tue, Jul 21, 2015 at 5:15 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 21.07.15 at 17:56, <roger.pau@citrix.com> wrote:
>> Some DOMCTLs returned non-POSIX error codes, replace them with POSIX
>> compilant values instead. EBADRQC and EBADSLT are replaced by EINVAL, while
>> EUSERS is replaced with EOVERFLOW.
>
> Same here basically - I'd appreciate if we could use EINVAL only as
> a last resort error value, to make errors distinguishable.

What would be more helpful is if you suggested an alternate error code
for the two EINVAL changes in this patch.

In the case of the first one, it looks like this is an "internal"
hypercall used only to do continuation of paging domctls.  Guests
should never call this directly; -EINVAL seems perfectly suitable
here.

 -George

Re: [PATCH for-4.6 2/3] xen: replace non-POSIX error codes

[George Dunlap]

On Wed, Jul 22, 2015 at 10:43 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Tue, Jul 21, 2015 at 5:15 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 21.07.15 at 17:56, <roger.pau@citrix.com> wrote:
>>> Some DOMCTLs returned non-POSIX error codes, replace them with POSIX
>>> compilant values instead. EBADRQC and EBADSLT are replaced by EINVAL, while
>>> EUSERS is replaced with EOVERFLOW.
>>
>> Same here basically - I'd appreciate if we could use EINVAL only as
>> a last resort error value, to make errors distinguishable.
>
> What would be more helpful is if you suggested an alternate error code
> for the two EINVAL changes in this patch.

Sorry, I see you did suggest alternates in the other mail.  I retract
my complaint...

> In the case of the first one, it looks like this is an "internal"
> hypercall used only to do continuation of paging domctls.  Guests
> should never call this directly; -EINVAL seems perfectly suitable
> here.

But still think EINVAL is probably fine for this case.

 -George

[PATCH for-4.6 3/3] xen: remove non-POSIX error codes

[Roger Pau Monne]

Xen was using some non-POSIX error codes that are removed in this patch. For
future reference, the list of POSIX error codes has been obtained from:

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

The error codes already present and defined as optional (XSR), have been
left in place.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/errno.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index 9a7e411..3498727 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -54,8 +54,6 @@ XEN_ERRNO(EDEADLK,	35)	/* Resource deadlock would occur */
 XEN_ERRNO(ENAMETOOLONG,	36)	/* File name too long */
 XEN_ERRNO(ENOLCK,	37)	/* No record locks available */
 XEN_ERRNO(ENOSYS,	38)	/* Function not implemented */
-XEN_ERRNO(EBADRQC,	56)	/* Invalid request code */
-XEN_ERRNO(EBADSLT,	57)	/* Invalid slot */
 XEN_ERRNO(ENODATA,	61)	/* No data available */
 XEN_ERRNO(ETIME,	62)	/* Timer expired */
 XEN_ERRNO(EBADMSG,	74)	/* Not a data message */
@@ -64,15 +62,12 @@ XEN_ERRNO(EILSEQ,	84)	/* Illegal byte sequence */
 #ifdef __XEN__ /* Internal only, should never be exposed to the guest. */
 XEN_ERRNO(ERESTART,	85)	/* Interrupted system call should be restarted */
 #endif
-XEN_ERRNO(EUSERS,	87)	/* Too many users */
 XEN_ERRNO(EOPNOTSUPP,	95)	/* Operation not supported on transport endpoint */
 XEN_ERRNO(EADDRINUSE,	98)	/* Address already in use */
 XEN_ERRNO(EADDRNOTAVAIL, 99)	/* Cannot assign requested address */
 XEN_ERRNO(ENOBUFS,	105)	/* No buffer space available */
 XEN_ERRNO(EISCONN,	106)	/* Transport endpoint is already connected */
 XEN_ERRNO(ENOTCONN,	107)	/* Transport endpoint is not connected */
-XEN_ERRNO(ESHUTDOWN,	108)	/* Cannot send after transport endpoint shutdown */
-XEN_ERRNO(ETOOMANYREFS,	109)	/* Too many references: cannot splice */
 XEN_ERRNO(ETIMEDOUT,	110)	/* Connection timed out */
 
 #undef XEN_ERRNO
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH for-4.6 3/3] xen: remove non-POSIX error codes

[Jan Beulich]

>>> On 21.07.15 at 17:56, <roger.pau@citrix.com> wrote:
> --- a/xen/include/public/errno.h
> +++ b/xen/include/public/errno.h
> @@ -54,8 +54,6 @@ XEN_ERRNO(EDEADLK,	35)	/* Resource deadlock would occur */
>  XEN_ERRNO(ENAMETOOLONG,	36)	/* File name too long */
>  XEN_ERRNO(ENOLCK,	37)	/* No record locks available */
>  XEN_ERRNO(ENOSYS,	38)	/* Function not implemented */
> -XEN_ERRNO(EBADRQC,	56)	/* Invalid request code */
> -XEN_ERRNO(EBADSLT,	57)	/* Invalid slot */
>  XEN_ERRNO(ENODATA,	61)	/* No data available */
>  XEN_ERRNO(ETIME,	62)	/* Timer expired */
>  XEN_ERRNO(EBADMSG,	74)	/* Not a data message */
> @@ -64,15 +62,12 @@ XEN_ERRNO(EILSEQ,	84)	/* Illegal byte sequence */
>  #ifdef __XEN__ /* Internal only, should never be exposed to the guest. */
>  XEN_ERRNO(ERESTART,	85)	/* Interrupted system call should be restarted */
>  #endif
> -XEN_ERRNO(EUSERS,	87)	/* Too many users */
>  XEN_ERRNO(EOPNOTSUPP,	95)	/* Operation not supported on transport endpoint */
>  XEN_ERRNO(EADDRINUSE,	98)	/* Address already in use */
>  XEN_ERRNO(EADDRNOTAVAIL, 99)	/* Cannot assign requested address */
>  XEN_ERRNO(ENOBUFS,	105)	/* No buffer space available */
>  XEN_ERRNO(EISCONN,	106)	/* Transport endpoint is already connected */
>  XEN_ERRNO(ENOTCONN,	107)	/* Transport endpoint is not connected */
> -XEN_ERRNO(ESHUTDOWN,	108)	/* Cannot send after transport endpoint shutdown */
> -XEN_ERRNO(ETOOMANYREFS,	109)	/* Too many references: cannot splice */
>  XEN_ERRNO(ETIMEDOUT,	110)	/* Connection timed out */

Considering that I stripped several values when putting together
the change that introduced this header, shouldn't we perhaps
consider re-adding a few that could be used as replacements?
ENOTSOCK (possibly usable in patch 1) would be one such
candidate.

Jan