[PATCH for-4.7 0/4] xsplice fixes

[PATCH for-4.7 0/4] xsplice fixes

[Roger Pau Monne]

Hello,

This series contains bugfixes for xSplice, that popped up when testing it on 
OSes != Linux.

Roger.


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

[PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Roger Pau Monne]

According to the POSIX standard for error codes [0], ENODATA is both
obsolescent (so it may be removed in the future) and optional. Replace it's
usage with ENOENT, which seems like the closest match. Both FreeBSD and
OpenBSD don't have this error code in the native errno.h headers.

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

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
 tools/libxl/libxl.c             |  2 +-
 xen/arch/x86/hvm/hvm.c          |  8 ++++----
 xen/arch/x86/microcode_amd.c    |  4 ++--
 xen/arch/x86/mm/shadow/common.c |  2 +-
 xen/common/device_tree.c        |  2 +-
 xen/common/domctl.c             |  2 +-
 xen/common/version.c            | 10 +++++-----
 xen/drivers/acpi/pmstat.c       |  2 +-
 xen/drivers/acpi/tables.c       |  2 +-
 xen/include/public/errno.h      |  1 -
 10 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index c39d745..0dcc06f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5361,7 +5361,7 @@ static const int libxl__xc_version_wrap(libxl__gc *gc, libxl_version_info *info,
     r = xc_version(CTX->xch, XENVER_build_id, build);
     switch (r) {
     case -EPERM:
-    case -ENODATA:
+    case -ENOENT:
     case 0:
         info->build_id = libxl__strdup(NOGC, "");
         break;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8cb6e9e..980b4a1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1274,14 +1274,14 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
         printk(XENLOG_G_WARNING
                "HVM%d.%d restore: not enough data left to read xsave descriptor\n",
                d->domain_id, vcpuid);
-        return -ENODATA;
+        return -ENOENT;
     }
     if ( desc->length + sizeof (*desc) > h->size - h->cur)
     {
         printk(XENLOG_G_WARNING
                "HVM%d.%d restore: not enough data left to read %u xsave bytes\n",
                d->domain_id, vcpuid, desc->length);
-        return -ENODATA;
+        return -ENOENT;
     }
     if ( desc->length < offsetof(struct hvm_hw_cpu_xsave, save_area) +
                         XSTATE_AREA_MIN_SIZE )
@@ -1402,14 +1402,14 @@ static int hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
         printk(XENLOG_G_WARNING
                "HVM%d.%d restore: not enough data left to read MSR descriptor\n",
                d->domain_id, vcpuid);
-        return -ENODATA;
+        return -ENOENT;
     }
     if ( desc->length + sizeof (*desc) > h->size - h->cur)
     {
         printk(XENLOG_G_WARNING
                "HVM%d.%d restore: not enough data left to read %u MSR bytes\n",
                d->domain_id, vcpuid, desc->length);
-        return -ENODATA;
+        return -ENOENT;
     }
     if ( desc->length < HVM_CPU_MSR_SIZE(1) )
     {
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index a61c926..ca1a026 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -342,7 +342,7 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
         *offset += size;
 
         if ( !size_left )
-            return -ENODATA;
+            return -ENOENT;
     }
 
     return 0;
@@ -454,7 +454,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
             break;
 
         error = container_fast_forward(buf, bufsize - offset, &offset);
-        if ( error == -ENODATA )
+        if ( error == -ENOENT )
         {
             ASSERT(offset == bufsize);
             break;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 4e06c13..6ce76e2 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3729,7 +3729,7 @@ int shadow_track_dirty_vram(struct domain *d,
         dirty_vram->last_dirty = NOW();
 
         /* Tell the caller that this time we could not track dirty bits. */
-        rc = -ENODATA;
+        rc = -ENOENT;
     }
     else if (dirty_vram->last_dirty == -1)
         /* still completely clean, just copy our empty bitmap */
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 0ed86a7..9d1896e 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -204,7 +204,7 @@ int dt_property_read_string(const struct dt_device_node *np,
     if ( !pp )
         return -EINVAL;
     if ( !pp->value )
-        return -ENODATA;
+        return -ENOENT;
     if ( strnlen(pp->value, pp->length) >= pp->length )
         return -EILSEQ;
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e43904e..d07114a 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -901,7 +901,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
              v == current ) /* no vcpu_pause() */
             goto getvcpucontext_out;
 
-        ret = -ENODATA;
+        ret = -ENOENT;
         if ( !v->is_initialised )
             goto getvcpucontext_out;
 
diff --git a/xen/common/version.c b/xen/common/version.c
index 0f96849..1bffb3e 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -74,7 +74,7 @@ static unsigned int build_id_len __read_mostly;
 int xen_build_id(const void **p, unsigned int *len)
 {
     if ( !build_id_len )
-        return -ENODATA;
+        return -ENOENT;
 
     *len = build_id_len;
     *p = build_id_p;
@@ -91,7 +91,7 @@ int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
 {
     /* Check if we really have a build-id. */
     if ( NT_GNU_BUILD_ID != n->type )
-        return -ENODATA;
+        return -ENOENT;
 
     if ( n_sz <= sizeof(*n) )
         return -EINVAL;
@@ -107,7 +107,7 @@ int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
 
     /* Sanity check, name should be "GNU" for ld-generated build-id. */
     if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
-        return -ENODATA;
+        return -ENOENT;
 
     if ( len )
         *len = n->descsz;
@@ -124,11 +124,11 @@ static int __init xen_build_init(void)
 
     /* --build-id invoked with wrong parameters. */
     if ( __note_gnu_build_id_end <= &n[0] )
-        return -ENODATA;
+        return -ENOENT;
 
     /* Check for full Note header. */
     if ( &n[1] > __note_gnu_build_id_end )
-        return -ENODATA;;
+        return -ENOENT;
 
     sz = (void *)__note_gnu_build_id_end - (void *)n;
 
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index 892260d..7ed59b0 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -95,7 +95,7 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
         if ( !pxpt || !pxpt->u.pt || !pxpt->u.trans_pt )
         {
             spin_unlock(cpufreq_statistic_lock);
-            return -ENODATA;
+            return -ENOENT;
         }
 
         pxpt->u.usable = pmpt->perf.state_count - pmpt->perf.platform_limit;
diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
index dd2031f..685c2cd 100644
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -319,7 +319,7 @@ acpi_parse_entries(char *id, unsigned long table_size,
 		if (entry->length < sizeof(*entry)) {
 			printk(KERN_ERR PREFIX "[%4.4s:%#x] Invalid length\n",
 			       id, entry_id);
-			return -ENODATA;
+			return -ENOENT;
 		}
 
 		if (entry->type == entry_id
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index ebb853a..070f834 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -93,7 +93,6 @@ XEN_ERRNO(ENAMETOOLONG,	36)	/* File name too long */
 XEN_ERRNO(ENOLCK,	37)	/* No record locks available */
 XEN_ERRNO(ENOTEMPTY,	39)	/* Directory not empty */
 XEN_ERRNO(ENOSYS,	38)	/* Function not implemented */
-XEN_ERRNO(ENODATA,	61)	/* No data available */
 XEN_ERRNO(ETIME,	62)	/* Timer expired */
 XEN_ERRNO(EBADMSG,	74)	/* Not a data message */
 XEN_ERRNO(EOVERFLOW,	75)	/* Value too large for defined data type */
-- 
2.6.4 (Apple Git-63)


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

[PATCH for-4.7 2/4] tools/xsplice: corrently use errno

[Roger Pau Monne]

Some error paths incorrectly used rc instead of errno.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/misc/xen-xsplice.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c
index 0f1ab5a..695be7f 100644
--- a/tools/misc/xen-xsplice.c
+++ b/tools/misc/xen-xsplice.c
@@ -273,7 +273,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
     if ( rc )
     {
         fprintf(stderr, "%s failed to get status (rc=%d, %s)!\n",
-                name, -rc, strerror(-rc));
+                name, errno, strerror(errno));
         return -1;
     }
     if ( status.rc == -EAGAIN )
@@ -295,7 +295,8 @@ int action_func(int argc, char *argv[], unsigned int idx)
         rc = action_options[idx].function(xch, name, 0);
         if ( rc )
         {
-            fprintf(stderr, "%s failed with %d(%s)\n", name, -rc, strerror(-rc));
+            fprintf(stderr, "%s failed with %d(%s)\n", name, errno,
+                    strerror(errno));
             return -1;
         }
     }
-- 
2.6.4 (Apple Git-63)


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

[PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones.

[Roger Pau Monne]

Avoid using system errno values when comparing with Xen errno values.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Using errno values inside of hypercall structs is not right IMHO, but there
are already several occurrences of this. Although I'm adding the correct XEN_
prefixes here, it's very likely that new additions/modifications to this
file will not take this into account, breaking it for OSes != Linux.
---
 tools/misc/xen-xsplice.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c
index 695be7f..b31115c 100644
--- a/tools/misc/xen-xsplice.c
+++ b/tools/misc/xen-xsplice.c
@@ -13,6 +13,8 @@
 #include <xenctrl.h>
 #include <xenstore.h>
 
+#include <xen/errno.h>
+
 static xc_interface *xch;
 
 void show_help(void)
@@ -233,7 +235,7 @@ struct {
         .function = xc_xsplice_revert,
     },
     {   .allow = XSPLICE_STATE_CHECKED,
-        .expected = -ENOENT,
+        .expected = -XEN_ENOENT,
         .name = "unload",
         .function = xc_xsplice_unload,
     },
@@ -276,7 +278,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
                 name, errno, strerror(errno));
         return -1;
     }
-    if ( status.rc == -EAGAIN )
+    if ( status.rc == -XEN_EAGAIN )
     {
         fprintf(stderr, "%s failed. Operation already in progress\n", name);
         return -1;
@@ -319,7 +321,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
 
         if ( status.state != original_state )
             break;
-        if ( status.rc && status.rc != -EAGAIN )
+        if ( status.rc && status.rc != -XEN_EAGAIN )
         {
             rc = status.rc;
             break;
-- 
2.6.4 (Apple Git-63)


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

[PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload

[Roger Pau Monne]

FreeBSD linker sets the OS ABI to ELFOSABI_FREEBSD, but the payload can
still be loaded without issues.

All the ELF OS ABIs follow the System V calling convention, and the OS ABI
doesn't really matter because Xen is a standalone kernel.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/common/xsplice_elf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
index e403a0e..de68d29 100644
--- a/xen/common/xsplice_elf.c
+++ b/xen/common/xsplice_elf.c
@@ -397,7 +397,6 @@ static int xsplice_header_check(const struct xsplice_elf *elf)
     if ( hdr->e_version != EV_CURRENT ||
          hdr->e_ident[EI_VERSION] != EV_CURRENT ||
          hdr->e_ident[EI_ABIVERSION] != 0 ||
-         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
          hdr->e_type != ET_REL ||
          hdr->e_phnum != 0 )
     {
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Jan Beulich]

>>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
> According to the POSIX standard for error codes [0], ENODATA is both
> obsolescent (so it may be removed in the future) and optional.

It being optional still doesn't preclude us using it.

> Replace it's
> usage with ENOENT, which seems like the closest match. Both FreeBSD and
> OpenBSD don't have this error code in the native errno.h headers.

There's no rule saying that Xen's errno set must match any other OS'es.
That's one of the reasons why we (finally) separated ours.

> --- a/xen/include/public/errno.h
> +++ b/xen/include/public/errno.h
> @@ -93,7 +93,6 @@ XEN_ERRNO(ENAMETOOLONG,	36)	/* File name too long */
>  XEN_ERRNO(ENOLCK,	37)	/* No record locks available */
>  XEN_ERRNO(ENOTEMPTY,	39)	/* Directory not empty */
>  XEN_ERRNO(ENOSYS,	38)	/* Function not implemented */
> -XEN_ERRNO(ENODATA,	61)	/* No data available */
>  XEN_ERRNO(ETIME,	62)	/* Timer expired */
>  XEN_ERRNO(EBADMSG,	74)	/* Not a data message */
>  XEN_ERRNO(EOVERFLOW,	75)	/* Value too large for defined data type */

And in absolutely no case can you unconditionally remove _anything_
from other than the tools-only parts of the public interface.

Jan


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

Re: [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload

[Jan Beulich]

>>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
> FreeBSD linker sets the OS ABI to ELFOSABI_FREEBSD, but the payload can
> still be loaded without issues.
> 
> All the ELF OS ABIs follow the System V calling convention, and the OS ABI
> doesn't really matter because Xen is a standalone kernel.

Well, first of all our name is wrong. The correct one is
ELFOSABI_NONE, as I did also write in one of the xSplice patch
reviews. And this _is_ the correct thing to expect here, as other
settings may imply behavioral changes. If other ABIs are also
fine, they can be added, but we can't ignore that field.

Jan

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/common/xsplice_elf.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
> index e403a0e..de68d29 100644
> --- a/xen/common/xsplice_elf.c
> +++ b/xen/common/xsplice_elf.c
> @@ -397,7 +397,6 @@ static int xsplice_header_check(const struct xsplice_elf 
> *elf)
>      if ( hdr->e_version != EV_CURRENT ||
>           hdr->e_ident[EI_VERSION] != EV_CURRENT ||
>           hdr->e_ident[EI_ABIVERSION] != 0 ||
> -         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
>           hdr->e_type != ET_REL ||
>           hdr->e_phnum != 0 )
>      {
> -- 
> 2.6.4 (Apple Git-63)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 



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

Re: [PATCH for-4.7 2/4] tools/xsplice: corrently use errno

[Wei Liu]

On Fri, Apr 29, 2016 at 04:21:18PM +0200, Roger Pau Monne wrote:
> Some error paths incorrectly used rc instead of errno.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/misc/xen-xsplice.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c
> index 0f1ab5a..695be7f 100644
> --- a/tools/misc/xen-xsplice.c
> +++ b/tools/misc/xen-xsplice.c
> @@ -273,7 +273,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
>      if ( rc )
>      {
>          fprintf(stderr, "%s failed to get status (rc=%d, %s)!\n",

I think it is better to also change rc= to errno= here.

> -                name, -rc, strerror(-rc));
> +                name, errno, strerror(errno));
>          return -1;
>      }
>      if ( status.rc == -EAGAIN )
> @@ -295,7 +295,8 @@ int action_func(int argc, char *argv[], unsigned int idx)
>          rc = action_options[idx].function(xch, name, 0);
>          if ( rc )
>          {
> -            fprintf(stderr, "%s failed with %d(%s)\n", name, -rc, strerror(-rc));
> +            fprintf(stderr, "%s failed with %d(%s)\n", name, errno,
> +                    strerror(errno));
>              return -1;
>          }
>      }
> -- 
> 2.6.4 (Apple Git-63)
> 

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

Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones.

[Wei Liu]

On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote:
> Avoid using system errno values when comparing with Xen errno values.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> Using errno values inside of hypercall structs is not right IMHO, but there
> are already several occurrences of this. Although I'm adding the correct XEN_
> prefixes here, it's very likely that new additions/modifications to this
> file will not take this into account, breaking it for OSes != Linux.

This seems to be a rather thorny issue.

I have a gut feeling that returning XEN_ errno to userspace program is
layering violation. They should always be translated to OS level errno
by privcmd driver.

Aren't FreeBSD and NetBSD already doing that?

Wei.

> ---
>  tools/misc/xen-xsplice.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c
> index 695be7f..b31115c 100644
> --- a/tools/misc/xen-xsplice.c
> +++ b/tools/misc/xen-xsplice.c
> @@ -13,6 +13,8 @@
>  #include <xenctrl.h>
>  #include <xenstore.h>
>  
> +#include <xen/errno.h>
> +
>  static xc_interface *xch;
>  
>  void show_help(void)
> @@ -233,7 +235,7 @@ struct {
>          .function = xc_xsplice_revert,
>      },
>      {   .allow = XSPLICE_STATE_CHECKED,
> -        .expected = -ENOENT,
> +        .expected = -XEN_ENOENT,
>          .name = "unload",
>          .function = xc_xsplice_unload,
>      },
> @@ -276,7 +278,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
>                  name, errno, strerror(errno));
>          return -1;
>      }
> -    if ( status.rc == -EAGAIN )
> +    if ( status.rc == -XEN_EAGAIN )
>      {
>          fprintf(stderr, "%s failed. Operation already in progress\n", name);
>          return -1;
> @@ -319,7 +321,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
>  
>          if ( status.state != original_state )
>              break;
> -        if ( status.rc && status.rc != -EAGAIN )
> +        if ( status.rc && status.rc != -XEN_EAGAIN )
>          {
>              rc = status.rc;
>              break;
> -- 
> 2.6.4 (Apple Git-63)
> 

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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
> > According to the POSIX standard for error codes [0], ENODATA is both
> > obsolescent (so it may be removed in the future) and optional.
> 
> It being optional still doesn't preclude us using it.
> 
> > Replace it's
> > usage with ENOENT, which seems like the closest match. Both FreeBSD and
> > OpenBSD don't have this error code in the native errno.h headers.
> 
> There's no rule saying that Xen's errno set must match any other OS'es.
> That's one of the reasons why we (finally) separated ours.

I understand that, but doing this means that then on the toolstack side we 
need to start doing ifdefery in order to match values that are not present 
in the native OS. For example a check was added to libxl against 
XENVER_build_id returning ENODATA, this means that then on libxl I would 
have to add a:

#ifdef __FreeBSD__
#define ENODATA ENOENT
#endif

I think this adds more complexity, when we could solve it by only using 
error codes that are part of the POSIX standard, and that are present on all 
UNIX systems.

> > --- a/xen/include/public/errno.h
> > +++ b/xen/include/public/errno.h
> > @@ -93,7 +93,6 @@ XEN_ERRNO(ENAMETOOLONG,	36)	/* File name too long */
> >  XEN_ERRNO(ENOLCK,	37)	/* No record locks available */
> >  XEN_ERRNO(ENOTEMPTY,	39)	/* Directory not empty */
> >  XEN_ERRNO(ENOSYS,	38)	/* Function not implemented */
> > -XEN_ERRNO(ENODATA,	61)	/* No data available */
> >  XEN_ERRNO(ETIME,	62)	/* Timer expired */
> >  XEN_ERRNO(EBADMSG,	74)	/* Not a data message */
> >  XEN_ERRNO(EOVERFLOW,	75)	/* Value too large for defined data type */
> 
> And in absolutely no case can you unconditionally remove _anything_
> from other than the tools-only parts of the public interface.

Right, the problem with leaving it there is that I'm quite certain it's 
going to be used again as a return value from a hypercall.

Roger.

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

Re: [PATCH for-4.7 2/4] tools/xsplice: corrently use errno

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 03:57:23PM +0100, Wei Liu wrote:
> On Fri, Apr 29, 2016 at 04:21:18PM +0200, Roger Pau Monne wrote:
> > Some error paths incorrectly used rc instead of errno.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/misc/xen-xsplice.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c
> > index 0f1ab5a..695be7f 100644
> > --- a/tools/misc/xen-xsplice.c
> > +++ b/tools/misc/xen-xsplice.c
> > @@ -273,7 +273,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
> >      if ( rc )
> >      {
> >          fprintf(stderr, "%s failed to get status (rc=%d, %s)!\n",
> 
> I think it is better to also change rc= to errno= here.

Thanks. Yes, in fact I think I prefer the format used elsewhere in this 
file, which is:

%d(%s)

Roger.

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

Re: [PATCH for-4.7 2/4] tools/xsplice: corrently use errno

[Wei Liu]

On Fri, Apr 29, 2016 at 05:07:40PM +0200, Roger Pau Monne wrote:
> On Fri, Apr 29, 2016 at 03:57:23PM +0100, Wei Liu wrote:
> > On Fri, Apr 29, 2016 at 04:21:18PM +0200, Roger Pau Monne wrote:
> > > Some error paths incorrectly used rc instead of errno.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/misc/xen-xsplice.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c
> > > index 0f1ab5a..695be7f 100644
> > > --- a/tools/misc/xen-xsplice.c
> > > +++ b/tools/misc/xen-xsplice.c
> > > @@ -273,7 +273,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
> > >      if ( rc )
> > >      {
> > >          fprintf(stderr, "%s failed to get status (rc=%d, %s)!\n",
> > 
> > I think it is better to also change rc= to errno= here.
> 
> Thanks. Yes, in fact I think I prefer the format used elsewhere in this 
> file, which is:
> 
> %d(%s)
> 

That's fine, too.

Wei.

> Roger.

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

Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones.

[Andrew Cooper]

On 29/04/16 16:02, Wei Liu wrote:
> On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote:
>> Avoid using system errno values when comparing with Xen errno values.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>> Using errno values inside of hypercall structs is not right IMHO, but there
>> are already several occurrences of this. Although I'm adding the correct XEN_
>> prefixes here, it's very likely that new additions/modifications to this
>> file will not take this into account, breaking it for OSes != Linux.
> This seems to be a rather thorny issue.
>
> I have a gut feeling that returning XEN_ errno to userspace program is
> layering violation. They should always be translated to OS level errno
> by privcmd driver.
>
> Aren't FreeBSD and NetBSD already doing that?

It is not practical for the privcmd driver to do this translation, as
most hypercalls through the privcmd driver are toolstack calls, and have
an unstable layout.

Another thorny issue is that the ioctl() call for privcmd returns errors
via errno, which may be in system errno space, or xen errno space
depending on the source of the errno.

This is very large swamp in desperate need of some attention.

~Andrew

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

Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones.

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
> On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote:
> > Avoid using system errno values when comparing with Xen errno values.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Using errno values inside of hypercall structs is not right IMHO, but there
> > are already several occurrences of this. Although I'm adding the correct XEN_
> > prefixes here, it's very likely that new additions/modifications to this
> > file will not take this into account, breaking it for OSes != Linux.
> 
> This seems to be a rather thorny issue.
> 
> I have a gut feeling that returning XEN_ errno to userspace program is
> layering violation. They should always be translated to OS level errno
> by privcmd driver.

Yes, the error value returned from the hypercall executed is indeed 
translated into the native OS error space. The problem here is that those 
error codes are returned _inside_ of the specific hypercall struct, which 
sadly privcmd doesn't know anything about.

And of course teaching privcmd about every possible hypercall struct is 
simply impossible, since some of them are not stable (eg: domctls)
 
> Aren't FreeBSD and NetBSD already doing that?

As said above, this is only done for direct return codes, everything inside 
of the struct passed to the hypercall is returned as-is.

This is a complete mess, and TBH, I don't have a clever idea about how to 
solve it.

Roger.

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

Re: [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 08:48:39AM -0600, Jan Beulich wrote:
> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
> > FreeBSD linker sets the OS ABI to ELFOSABI_FREEBSD, but the payload can
> > still be loaded without issues.
> > 
> > All the ELF OS ABIs follow the System V calling convention, and the OS ABI
> > doesn't really matter because Xen is a standalone kernel.
> 
> Well, first of all our name is wrong. The correct one is
> ELFOSABI_NONE, as I did also write in one of the xSplice patch
> reviews. And this _is_ the correct thing to expect here, as other
> settings may imply behavioral changes. If other ABIs are also
> fine, they can be added, but we can't ignore that field.

Thanks for the review. I'm going to change it to ELFOSABI_NONE and then 
add ELFOSABI_FREEBSD as a supported OSABI also.

Roger.

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

Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones.

[Wei Liu]

On Fri, Apr 29, 2016 at 04:11:47PM +0100, Andrew Cooper wrote:
> On 29/04/16 16:02, Wei Liu wrote:
> > On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote:
> >> Avoid using system errno values when comparing with Xen errno values.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> ---
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Wei Liu <wei.liu2@citrix.com>
> >> ---
> >> Using errno values inside of hypercall structs is not right IMHO, but there
> >> are already several occurrences of this. Although I'm adding the correct XEN_
> >> prefixes here, it's very likely that new additions/modifications to this
> >> file will not take this into account, breaking it for OSes != Linux.
> > This seems to be a rather thorny issue.
> >
> > I have a gut feeling that returning XEN_ errno to userspace program is
> > layering violation. They should always be translated to OS level errno
> > by privcmd driver.
> >
> > Aren't FreeBSD and NetBSD already doing that?
> 
> It is not practical for the privcmd driver to do this translation, as
> most hypercalls through the privcmd driver are toolstack calls, and have
> an unstable layout.
> 

If what you mean is what Roger said in his other reply then I agree
translating in privcmd is not doable.

> Another thorny issue is that the ioctl() call for privcmd returns errors
> via errno, which may be in system errno space, or xen errno space
> depending on the source of the errno.
> 
> This is very large swamp in desperate need of some attention.
> 

Indeed.

Wei.

> ~Andrew

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

Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones.

[Wei Liu]

On Fri, Apr 29, 2016 at 05:12:51PM +0200, Roger Pau Monne wrote:
> On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
> > On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote:
> > > Avoid using system errno values when comparing with Xen errno values.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > > Using errno values inside of hypercall structs is not right IMHO, but there
> > > are already several occurrences of this. Although I'm adding the correct XEN_
> > > prefixes here, it's very likely that new additions/modifications to this
> > > file will not take this into account, breaking it for OSes != Linux.
> > 
> > This seems to be a rather thorny issue.
> > 
> > I have a gut feeling that returning XEN_ errno to userspace program is
> > layering violation. They should always be translated to OS level errno
> > by privcmd driver.
> 
> Yes, the error value returned from the hypercall executed is indeed 
> translated into the native OS error space. The problem here is that those 
> error codes are returned _inside_ of the specific hypercall struct, which 
> sadly privcmd doesn't know anything about.
> 
> And of course teaching privcmd about every possible hypercall struct is 
> simply impossible, since some of them are not stable (eg: domctls)
>  
> > Aren't FreeBSD and NetBSD already doing that?
> 
> As said above, this is only done for direct return codes, everything inside 
> of the struct passed to the hypercall is returned as-is.
> 
> This is a complete mess, and TBH, I don't have a clever idea about how to 
> solve it.
> 

Me neither. Maybe a new thread should be started to discuss this.

Wei.

> Roger.

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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Jan Beulich]

>>> On 29.04.16 at 17:06, <roger.pau@citrix.com> wrote:
> On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
>> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
>> > According to the POSIX standard for error codes [0], ENODATA is both
>> > obsolescent (so it may be removed in the future) and optional.
>> 
>> It being optional still doesn't preclude us using it.
>> 
>> > Replace it's
>> > usage with ENOENT, which seems like the closest match. Both FreeBSD and
>> > OpenBSD don't have this error code in the native errno.h headers.
>> 
>> There's no rule saying that Xen's errno set must match any other OS'es.
>> That's one of the reasons why we (finally) separated ours.
> 
> I understand that, but doing this means that then on the toolstack side we 
> need to start doing ifdefery in order to match values that are not present 
> in the native OS. For example a check was added to libxl against 
> XENVER_build_id returning ENODATA, this means that then on libxl I would 
> have to add a:
> 
> #ifdef __FreeBSD__
> #define ENODATA ENOENT
> #endif

You ought to be using XEN_NODATA there anyway.

Jan


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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 10:19:41AM -0600, Jan Beulich wrote:
> >>> On 29.04.16 at 17:06, <roger.pau@citrix.com> wrote:
> > On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
> >> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
> >> > According to the POSIX standard for error codes [0], ENODATA is both
> >> > obsolescent (so it may be removed in the future) and optional.
> >> 
> >> It being optional still doesn't preclude us using it.
> >> 
> >> > Replace it's
> >> > usage with ENOENT, which seems like the closest match. Both FreeBSD and
> >> > OpenBSD don't have this error code in the native errno.h headers.
> >> 
> >> There's no rule saying that Xen's errno set must match any other OS'es.
> >> That's one of the reasons why we (finally) separated ours.
> > 
> > I understand that, but doing this means that then on the toolstack side we 
> > need to start doing ifdefery in order to match values that are not present 
> > in the native OS. For example a check was added to libxl against 
> > XENVER_build_id returning ENODATA, this means that then on libxl I would 
> > have to add a:
> > 
> > #ifdef __FreeBSD__
> > #define ENODATA ENOENT
> > #endif
> 
> You ought to be using XEN_NODATA there anyway.

No, the privcmd driver is the one that performs the translation from Xen 
error codes into the OS error code space, so the tools just see error codes 
in the OS space. No tools at all use XEN_* error codes directly.

This is because privcmd does the translation, and because it would be 
impossible to differentiate if the errno returned by the ioctl belongs to 
Xen space or to the OS space.

Roger.

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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Jan Beulich]

>>> On 29.04.16 at 18:34, <roger.pau@citrix.com> wrote:
> On Fri, Apr 29, 2016 at 10:19:41AM -0600, Jan Beulich wrote:
>> >>> On 29.04.16 at 17:06, <roger.pau@citrix.com> wrote:
>> > On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
>> >> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
>> >> > According to the POSIX standard for error codes [0], ENODATA is both
>> >> > obsolescent (so it may be removed in the future) and optional.
>> >> 
>> >> It being optional still doesn't preclude us using it.
>> >> 
>> >> > Replace it's
>> >> > usage with ENOENT, which seems like the closest match. Both FreeBSD and
>> >> > OpenBSD don't have this error code in the native errno.h headers.
>> >> 
>> >> There's no rule saying that Xen's errno set must match any other OS'es.
>> >> That's one of the reasons why we (finally) separated ours.
>> > 
>> > I understand that, but doing this means that then on the toolstack side we 
>> > need to start doing ifdefery in order to match values that are not present 
>> > in the native OS. For example a check was added to libxl against 
>> > XENVER_build_id returning ENODATA, this means that then on libxl I would 
>> > have to add a:
>> > 
>> > #ifdef __FreeBSD__
>> > #define ENODATA ENOENT
>> > #endif
>> 
>> You ought to be using XEN_NODATA there anyway.
> 
> No, the privcmd driver is the one that performs the translation from Xen 
> error codes into the OS error code space, so the tools just see error codes 
> in the OS space. No tools at all use XEN_* error codes directly.

That's the supposed behavior for return values, but not for
structure contents. The structures are Xen-specific, so them
holding Xen-specific values is known to the callers, and they
should (be made) cope.

> This is because privcmd does the translation, and because it would be 
> impossible to differentiate if the errno returned by the ioctl belongs to 
> Xen space or to the OS space.

Exactly - I agree for the return values.

Jan


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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Roger Pau Monne]

On Fri, Apr 29, 2016 at 10:42:06AM -0600, Jan Beulich wrote:
> >>> On 29.04.16 at 18:34, <roger.pau@citrix.com> wrote:
> > On Fri, Apr 29, 2016 at 10:19:41AM -0600, Jan Beulich wrote:
> >> >>> On 29.04.16 at 17:06, <roger.pau@citrix.com> wrote:
> >> > On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
> >> >> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
> >> >> > According to the POSIX standard for error codes [0], ENODATA is both
> >> >> > obsolescent (so it may be removed in the future) and optional.
> >> >> 
> >> >> It being optional still doesn't preclude us using it.
> >> >> 
> >> >> > Replace it's
> >> >> > usage with ENOENT, which seems like the closest match. Both FreeBSD and
> >> >> > OpenBSD don't have this error code in the native errno.h headers.
> >> >> 
> >> >> There's no rule saying that Xen's errno set must match any other OS'es.
> >> >> That's one of the reasons why we (finally) separated ours.
> >> > 
> >> > I understand that, but doing this means that then on the toolstack side we 
> >> > need to start doing ifdefery in order to match values that are not present 
> >> > in the native OS. For example a check was added to libxl against 
> >> > XENVER_build_id returning ENODATA, this means that then on libxl I would 
> >> > have to add a:
> >> > 
> >> > #ifdef __FreeBSD__
> >> > #define ENODATA ENOENT
> >> > #endif
> >> 
> >> You ought to be using XEN_NODATA there anyway.
> > 
> > No, the privcmd driver is the one that performs the translation from Xen 
> > error codes into the OS error code space, so the tools just see error codes 
> > in the OS space. No tools at all use XEN_* error codes directly.
> 
> That's the supposed behavior for return values, but not for
> structure contents. The structures are Xen-specific, so them
> holding Xen-specific values is known to the callers, and they
> should (be made) cope.

And the usage of ENODATA I'm trying to fix here is from the direct return of 
an hypercall (__HYPERVISOR_xen_version). I don't mind adding this define, I 
just think it would be better to simply replace the usage of ENODATA with 
something else, so I could avoid adding more ifdefery to the tools.

Would you be fine with me just adjusting xen_build_id (in 
xen/common/version.c) to return ENOENT instead of ENODATA?

Roger.

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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Jan Beulich]

>>> On 29.04.16 at 18:52, <roger.pau@citrix.com> wrote:
> On Fri, Apr 29, 2016 at 10:42:06AM -0600, Jan Beulich wrote:
>> >>> On 29.04.16 at 18:34, <roger.pau@citrix.com> wrote:
>> > On Fri, Apr 29, 2016 at 10:19:41AM -0600, Jan Beulich wrote:
>> >> >>> On 29.04.16 at 17:06, <roger.pau@citrix.com> wrote:
>> >> > On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
>> >> >> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
>> >> >> > According to the POSIX standard for error codes [0], ENODATA is both
>> >> >> > obsolescent (so it may be removed in the future) and optional.
>> >> >> 
>> >> >> It being optional still doesn't preclude us using it.
>> >> >> 
>> >> >> > Replace it's
>> >> >> > usage with ENOENT, which seems like the closest match. Both FreeBSD and
>> >> >> > OpenBSD don't have this error code in the native errno.h headers.
>> >> >> 
>> >> >> There's no rule saying that Xen's errno set must match any other OS'es.
>> >> >> That's one of the reasons why we (finally) separated ours.
>> >> > 
>> >> > I understand that, but doing this means that then on the toolstack side we 
> 
>> >> > need to start doing ifdefery in order to match values that are not present 
> 
>> >> > in the native OS. For example a check was added to libxl against 
>> >> > XENVER_build_id returning ENODATA, this means that then on libxl I would 
>> >> > have to add a:
>> >> > 
>> >> > #ifdef __FreeBSD__
>> >> > #define ENODATA ENOENT
>> >> > #endif
>> >> 
>> >> You ought to be using XEN_NODATA there anyway.
>> > 
>> > No, the privcmd driver is the one that performs the translation from Xen 
>> > error codes into the OS error code space, so the tools just see error codes 
> 
>> > in the OS space. No tools at all use XEN_* error codes directly.
>> 
>> That's the supposed behavior for return values, but not for
>> structure contents. The structures are Xen-specific, so them
>> holding Xen-specific values is known to the callers, and they
>> should (be made) cope.
> 
> And the usage of ENODATA I'm trying to fix here is from the direct return of 
> 
> an hypercall (__HYPERVISOR_xen_version). I don't mind adding this define, I 
> just think it would be better to simply replace the usage of ENODATA with 
> something else, so I could avoid adding more ifdefery to the tools.
> 
> Would you be fine with me just adjusting xen_build_id (in 
> xen/common/version.c) to return ENOENT instead of ENODATA?

Well, I wouldn't be particularly happy, but I'd also not be as heavily
opposed as to removing that error code altogether. In general (and
I probably should have said this in the first reply, despite there
having been the other more important reason to object) I don't view
ENOENT as a good replacement for ENODATA. The two really mean
different things, but in this particular case it would seem a reasonable
replacement.

But how would that help you? Would you mean to monitor future
patches for not again introducing some use of ENODATA that the
tool stack wants to explicitly check for? That would be quite tedious
a task...

Jan


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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Roger Pau Monne]

On Mon, May 02, 2016 at 12:22:35AM -0600, Jan Beulich wrote:
> >>> On 29.04.16 at 18:52, <roger.pau@citrix.com> wrote:
> > On Fri, Apr 29, 2016 at 10:42:06AM -0600, Jan Beulich wrote:
> >> >>> On 29.04.16 at 18:34, <roger.pau@citrix.com> wrote:
> >> > On Fri, Apr 29, 2016 at 10:19:41AM -0600, Jan Beulich wrote:
> >> >> >>> On 29.04.16 at 17:06, <roger.pau@citrix.com> wrote:
> >> >> > On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
> >> >> >> >>> On 29.04.16 at 16:21, <roger.pau@citrix.com> wrote:
> >> >> >> > According to the POSIX standard for error codes [0], ENODATA is both
> >> >> >> > obsolescent (so it may be removed in the future) and optional.
> >> >> >> 
> >> >> >> It being optional still doesn't preclude us using it.
> >> >> >> 
> >> >> >> > Replace it's
> >> >> >> > usage with ENOENT, which seems like the closest match. Both FreeBSD and
> >> >> >> > OpenBSD don't have this error code in the native errno.h headers.
> >> >> >> 
> >> >> >> There's no rule saying that Xen's errno set must match any other OS'es.
> >> >> >> That's one of the reasons why we (finally) separated ours.
> >> >> > 
> >> >> > I understand that, but doing this means that then on the toolstack side we 
> > 
> >> >> > need to start doing ifdefery in order to match values that are not present 
> > 
> >> >> > in the native OS. For example a check was added to libxl against 
> >> >> > XENVER_build_id returning ENODATA, this means that then on libxl I would 
> >> >> > have to add a:
> >> >> > 
> >> >> > #ifdef __FreeBSD__
> >> >> > #define ENODATA ENOENT
> >> >> > #endif
> >> >> 
> >> >> You ought to be using XEN_NODATA there anyway.
> >> > 
> >> > No, the privcmd driver is the one that performs the translation from Xen 
> >> > error codes into the OS error code space, so the tools just see error codes 
> > 
> >> > in the OS space. No tools at all use XEN_* error codes directly.
> >> 
> >> That's the supposed behavior for return values, but not for
> >> structure contents. The structures are Xen-specific, so them
> >> holding Xen-specific values is known to the callers, and they
> >> should (be made) cope.
> > 
> > And the usage of ENODATA I'm trying to fix here is from the direct return of 
> > 
> > an hypercall (__HYPERVISOR_xen_version). I don't mind adding this define, I 
> > just think it would be better to simply replace the usage of ENODATA with 
> > something else, so I could avoid adding more ifdefery to the tools.
> > 
> > Would you be fine with me just adjusting xen_build_id (in 
> > xen/common/version.c) to return ENOENT instead of ENODATA?
> 
> Well, I wouldn't be particularly happy, but I'd also not be as heavily
> opposed as to removing that error code altogether. In general (and
> I probably should have said this in the first reply, despite there
> having been the other more important reason to object) I don't view
> ENOENT as a good replacement for ENODATA. The two really mean
> different things, but in this particular case it would seem a reasonable
> replacement.

Right. But since the privcmd driver does the error code translation I think 
ENOENT is the closest match to ENODATA when doing automatic error 
translation (I'm open to suggestions if someone knows a better replacement 
for it).

> But how would that help you? Would you mean to monitor future
> patches for not again introducing some use of ENODATA that the
> tool stack wants to explicitly check for? That would be quite tedious
> a task...

Yes it is, but TBH I cannot find any other solution. Adding ENODATA to the 
FreeBSD list of error codes seems quite pointless when there's no in-tree 
user of it.

Roger.

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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Jan Beulich]

>>> On 02.05.16 at 10:55, <roger.pau@citrix.com> wrote:
> On Mon, May 02, 2016 at 12:22:35AM -0600, Jan Beulich wrote:
>> But how would that help you? Would you mean to monitor future
>> patches for not again introducing some use of ENODATA that the
>> tool stack wants to explicitly check for? That would be quite tedious
>> a task...
> 
> Yes it is, but TBH I cannot find any other solution. Adding ENODATA to the 
> FreeBSD list of error codes seems quite pointless when there's no in-tree 
> user of it.

That's a slightly strange way of looking at it: Shouldn't a component
sitting on top of another component be capable of dealing with all
output of that lower component, i.e. also any of the error codes that
one may produce? In which case "no in-tree user" simply becomes a
non-argument imo... (For comparison, consider a user mode library
not itself using EINVAL: Would you say it's okay for the library to not
care about EINVAL coming back from system calls?)

Jan


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

Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones.

[Wei Liu]

On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote:
> Avoid using system errno values when comparing with Xen errno values.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> Using errno values inside of hypercall structs is not right IMHO, but there
> are already several occurrences of this. Although I'm adding the correct XEN_
> prefixes here, it's very likely that new additions/modifications to this
> file will not take this into account, breaking it for OSes != Linux.

While the discussion of errno is on-going, I think we should accept this
patch for the time being.

Wei.

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

Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

[Roger Pau Monne]

On Mon, May 02, 2016 at 03:06:21AM -0600, Jan Beulich wrote:
> >>> On 02.05.16 at 10:55, <roger.pau@citrix.com> wrote:
> > On Mon, May 02, 2016 at 12:22:35AM -0600, Jan Beulich wrote:
> >> But how would that help you? Would you mean to monitor future
> >> patches for not again introducing some use of ENODATA that the
> >> tool stack wants to explicitly check for? That would be quite tedious
> >> a task...
> > 
> > Yes it is, but TBH I cannot find any other solution. Adding ENODATA to the 
> > FreeBSD list of error codes seems quite pointless when there's no in-tree 
> > user of it.
> 
> That's a slightly strange way of looking at it: Shouldn't a component
> sitting on top of another component be capable of dealing with all
> output of that lower component, i.e. also any of the error codes that
> one may produce? In which case "no in-tree user" simply becomes a
> non-argument imo... (For comparison, consider a user mode library
> not itself using EINVAL: Would you say it's okay for the library to not
> care about EINVAL coming back from system calls?)

If it wasn't marked as optional and obsolescent I would have added it to 
FreeBSD without questioning, but the fact that the POSIX standard says that 
strictly conforming POSIX applications should not use it makes me wonder 
whether that is the right to do. ENODATA is the only error code that Xen 
uses that's not part of the core POSIX error codes.

IMHO Xen should be a strictly conforming POSIX application, it being a 
kernel that shares a common interface between a wide variety of different 
OSes.

I don't really want to make this any longer than necessary, so I think I'm 
going to try to get ENODATA added to the FreeBSD errno.h header, but I 
wouldn't be surprised if I found some resistance there, and TBH, I don't 
think I will be able to make a good point.

Roger.

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