* [PATCH for-4.7 0/4] xsplice fixes
@ 2016-04-29 14:21 Roger Pau Monne
2016-04-29 14:21 ` [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code Roger Pau Monne
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Roger Pau Monne @ 2016-04-29 14:21 UTC (permalink / raw)
To: xen-devel
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
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 14:21 [PATCH for-4.7 0/4] xsplice fixes Roger Pau Monne @ 2016-04-29 14:21 ` Roger Pau Monne 2016-04-29 14:44 ` Jan Beulich 2016-04-29 14:21 ` [PATCH for-4.7 2/4] tools/xsplice: corrently use errno Roger Pau Monne ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 14:21 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Tim Deegan, Julien Grall, Jan Beulich, Ian Jackson, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 14:21 ` [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code Roger Pau Monne @ 2016-04-29 14:44 ` Jan Beulich 2016-04-29 15:06 ` Roger Pau Monne 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2016-04-29 14:44 UTC (permalink / raw) To: Roger Pau Monne Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 14:44 ` Jan Beulich @ 2016-04-29 15:06 ` Roger Pau Monne 2016-04-29 16:19 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 15:06 UTC (permalink / raw) To: Jan Beulich Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 15:06 ` Roger Pau Monne @ 2016-04-29 16:19 ` Jan Beulich 2016-04-29 16:34 ` Roger Pau Monne 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2016-04-29 16:19 UTC (permalink / raw) To: Roger Pau Monne Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 16:19 ` Jan Beulich @ 2016-04-29 16:34 ` Roger Pau Monne 2016-04-29 16:42 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 16:34 UTC (permalink / raw) To: Jan Beulich Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 16:34 ` Roger Pau Monne @ 2016-04-29 16:42 ` Jan Beulich 2016-04-29 16:52 ` Roger Pau Monne 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2016-04-29 16:42 UTC (permalink / raw) To: Roger Pau Monne Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 16:42 ` Jan Beulich @ 2016-04-29 16:52 ` Roger Pau Monne 2016-05-02 6:22 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 16:52 UTC (permalink / raw) To: Jan Beulich Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-04-29 16:52 ` Roger Pau Monne @ 2016-05-02 6:22 ` Jan Beulich 2016-05-02 8:55 ` Roger Pau Monne 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2016-05-02 6:22 UTC (permalink / raw) To: Roger Pau Monne Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-05-02 6:22 ` Jan Beulich @ 2016-05-02 8:55 ` Roger Pau Monne 2016-05-02 9:06 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-05-02 8:55 UTC (permalink / raw) To: Jan Beulich Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-05-02 8:55 ` Roger Pau Monne @ 2016-05-02 9:06 ` Jan Beulich 2016-05-02 11:06 ` Roger Pau Monne 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2016-05-02 9:06 UTC (permalink / raw) To: Roger Pau Monne Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code 2016-05-02 9:06 ` Jan Beulich @ 2016-05-02 11:06 ` Roger Pau Monne 0 siblings, 0 replies; 26+ messages in thread From: Roger Pau Monne @ 2016-05-02 11:06 UTC (permalink / raw) To: Jan Beulich Cc: StefanoStabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH for-4.7 2/4] tools/xsplice: corrently use errno 2016-04-29 14:21 [PATCH for-4.7 0/4] xsplice fixes Roger Pau Monne 2016-04-29 14:21 ` [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code Roger Pau Monne @ 2016-04-29 14:21 ` Roger Pau Monne 2016-04-29 14:57 ` Wei Liu 2016-04-29 14:21 ` [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones Roger Pau Monne 2016-04-29 14:21 ` [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload Roger Pau Monne 3 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 14:21 UTC (permalink / raw) To: xen-devel; +Cc: Ross Lagerwall, Ian Jackson, Wei Liu, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 2/4] tools/xsplice: corrently use errno 2016-04-29 14:21 ` [PATCH for-4.7 2/4] tools/xsplice: corrently use errno Roger Pau Monne @ 2016-04-29 14:57 ` Wei Liu 2016-04-29 15:07 ` Roger Pau Monne 0 siblings, 1 reply; 26+ messages in thread From: Wei Liu @ 2016-04-29 14:57 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Ross Lagerwall, Ian Jackson, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 2/4] tools/xsplice: corrently use errno 2016-04-29 14:57 ` Wei Liu @ 2016-04-29 15:07 ` Roger Pau Monne 2016-04-29 15:08 ` Wei Liu 0 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 15:07 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ross Lagerwall, Ian Jackson 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 2/4] tools/xsplice: corrently use errno 2016-04-29 15:07 ` Roger Pau Monne @ 2016-04-29 15:08 ` Wei Liu 0 siblings, 0 replies; 26+ messages in thread From: Wei Liu @ 2016-04-29 15:08 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Ross Lagerwall, Wei Liu, Ian Jackson 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones. 2016-04-29 14:21 [PATCH for-4.7 0/4] xsplice fixes Roger Pau Monne 2016-04-29 14:21 ` [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code Roger Pau Monne 2016-04-29 14:21 ` [PATCH for-4.7 2/4] tools/xsplice: corrently use errno Roger Pau Monne @ 2016-04-29 14:21 ` Roger Pau Monne 2016-04-29 15:02 ` Wei Liu 2016-05-02 11:00 ` Wei Liu 2016-04-29 14:21 ` [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload Roger Pau Monne 3 siblings, 2 replies; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 14:21 UTC (permalink / raw) To: xen-devel; +Cc: Ross Lagerwall, Ian Jackson, Wei Liu, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones. 2016-04-29 14:21 ` [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones Roger Pau Monne @ 2016-04-29 15:02 ` Wei Liu 2016-04-29 15:11 ` Andrew Cooper 2016-04-29 15:12 ` Roger Pau Monne 2016-05-02 11:00 ` Wei Liu 1 sibling, 2 replies; 26+ messages in thread From: Wei Liu @ 2016-04-29 15:02 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Ross Lagerwall, Ian Jackson, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones. 2016-04-29 15:02 ` Wei Liu @ 2016-04-29 15:11 ` Andrew Cooper 2016-04-29 15:28 ` Wei Liu 2016-04-29 15:12 ` Roger Pau Monne 1 sibling, 1 reply; 26+ messages in thread From: Andrew Cooper @ 2016-04-29 15:11 UTC (permalink / raw) To: Wei Liu, Roger Pau Monne; +Cc: xen-devel, Ian Jackson, Ross Lagerwall 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones. 2016-04-29 15:11 ` Andrew Cooper @ 2016-04-29 15:28 ` Wei Liu 0 siblings, 0 replies; 26+ messages in thread From: Wei Liu @ 2016-04-29 15:28 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel, Ross Lagerwall, Wei Liu, Ian Jackson, Roger Pau Monne 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones. 2016-04-29 15:02 ` Wei Liu 2016-04-29 15:11 ` Andrew Cooper @ 2016-04-29 15:12 ` Roger Pau Monne 2016-04-29 15:30 ` Wei Liu 1 sibling, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 15:12 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ross Lagerwall, Ian Jackson 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones. 2016-04-29 15:12 ` Roger Pau Monne @ 2016-04-29 15:30 ` Wei Liu 0 siblings, 0 replies; 26+ messages in thread From: Wei Liu @ 2016-04-29 15:30 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Ross Lagerwall, Wei Liu, Ian Jackson 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones. 2016-04-29 14:21 ` [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones Roger Pau Monne 2016-04-29 15:02 ` Wei Liu @ 2016-05-02 11:00 ` Wei Liu 1 sibling, 0 replies; 26+ messages in thread From: Wei Liu @ 2016-05-02 11:00 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Ross Lagerwall, Ian Jackson, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload 2016-04-29 14:21 [PATCH for-4.7 0/4] xsplice fixes Roger Pau Monne ` (2 preceding siblings ...) 2016-04-29 14:21 ` [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones Roger Pau Monne @ 2016-04-29 14:21 ` Roger Pau Monne 2016-04-29 14:48 ` Jan Beulich 3 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 14:21 UTC (permalink / raw) To: xen-devel; +Cc: Ross Lagerwall, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload 2016-04-29 14:21 ` [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload Roger Pau Monne @ 2016-04-29 14:48 ` Jan Beulich 2016-04-29 15:16 ` Roger Pau Monne 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2016-04-29 14:48 UTC (permalink / raw) To: Roger Pau Monne; +Cc: Ross Lagerwall, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload 2016-04-29 14:48 ` Jan Beulich @ 2016-04-29 15:16 ` Roger Pau Monne 0 siblings, 0 replies; 26+ messages in thread From: Roger Pau Monne @ 2016-04-29 15:16 UTC (permalink / raw) To: Jan Beulich; +Cc: Ross Lagerwall, xen-devel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2016-05-02 11:06 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-29 14:21 [PATCH for-4.7 0/4] xsplice fixes Roger Pau Monne 2016-04-29 14:21 ` [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code Roger Pau Monne 2016-04-29 14:44 ` Jan Beulich 2016-04-29 15:06 ` Roger Pau Monne 2016-04-29 16:19 ` Jan Beulich 2016-04-29 16:34 ` Roger Pau Monne 2016-04-29 16:42 ` Jan Beulich 2016-04-29 16:52 ` Roger Pau Monne 2016-05-02 6:22 ` Jan Beulich 2016-05-02 8:55 ` Roger Pau Monne 2016-05-02 9:06 ` Jan Beulich 2016-05-02 11:06 ` Roger Pau Monne 2016-04-29 14:21 ` [PATCH for-4.7 2/4] tools/xsplice: corrently use errno Roger Pau Monne 2016-04-29 14:57 ` Wei Liu 2016-04-29 15:07 ` Roger Pau Monne 2016-04-29 15:08 ` Wei Liu 2016-04-29 14:21 ` [PATCH for-4.7 3/4] tools/xsplice: fix mixing system errno values with Xen ones Roger Pau Monne 2016-04-29 15:02 ` Wei Liu 2016-04-29 15:11 ` Andrew Cooper 2016-04-29 15:28 ` Wei Liu 2016-04-29 15:12 ` Roger Pau Monne 2016-04-29 15:30 ` Wei Liu 2016-05-02 11:00 ` Wei Liu 2016-04-29 14:21 ` [PATCH for-4.7 4/4] xen/xsplice: remove OSABI check when loading a payload Roger Pau Monne 2016-04-29 14:48 ` Jan Beulich 2016-04-29 15:16 ` Roger Pau Monne
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).