From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH RFC] make error codes a formal part of the ABI Date: Wed, 14 Jan 2015 10:28:11 +0000 Message-ID: <1421231291.19103.192.camel@eu.citrix.com> References: <54B5540A02000078000547D4@mail.emea.novell.com> <1421166952.19103.148.camel@eu.citrix.com> <54B63AE60200007800054A1E@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YBLBS-00049d-3S for xen-devel@lists.xenproject.org; Wed, 14 Jan 2015 10:28:18 +0000 In-Reply-To: <54B63AE60200007800054A1E@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , Stefano Stabellini , Andrew Cooper , Tim Deegan , xen-devel , Ian Jackson List-Id: xen-devel@lists.xenproject.org On Wed, 2015-01-14 at 08:46 +0000, Jan Beulich wrote: > >>> On 13.01.15 at 17:35, wrote: > > On Tue, 2015-01-13 at 16:21 +0000, Jan Beulich wrote: > >> --- /dev/null > >> +++ b/xen/include/public/errno.h > >> @@ -0,0 +1,94 @@ > >> +#ifndef __XEN_PUBLIC_ERRNO_H__ > >> + > >> +#ifndef __ASSEMBLY__ > >> + > >> +#define XEN_ERRNO(name, value) XEN_##name = value, > >> +enum xen_errno { > > > > The switch to an enum doesn't seem related to the main purpose of the > > patch, unless I'm missing something? > > No, this is an integral part of the change: A macro can't be used to > generate preprocessor directives (i.e. #define-s). Oh right, yes. Given the ABI pitfalls of enums (size etc) perhaps make it anonymous? > >> +#else /* !__ASSEMBLY__ */ > >> + > >> +#define XEN_ERRNO(name, value) .equ XEN_##name, value > > > > So here public/errno.h defines it's own XEN_ERRNO for ASM vs none. But > > then later xen/errno.h also defines it before including the public > > version. Also the enum xen_errno seems to be similarly duplicated. (I > > suspect you changed your mind and forgot to save one or the other > > file?). I think the includer chooses the namespace approach makes most > > sense. > > No, this again is intentional and - imo - necessary: A plain > #include ought to suffice to get all XEN_E* > definitions. That's not so much for Xen's internal purposes, but more > for actual consumers of the public headers. For Xen's internal > purposes, a plain #include ought to suffice and > produce (at least) the non-XEN_-prefixed values. Hence xen/errno.h > has to double-include public/errno.h, once without overriding > XEN_ERRNO() and then a second time with doing so. Oh I see now that you are relying on the multi-inclusion guard to also guard the definition of the default version of XEN_ERRNO macro, which is a bit tricksy (and wasn't obvious until I applied the patch and looked at the result). Can you make this more explicit while leaving the guard with its usual semantics? e.g. #ifndef XEN_ERRNO #if .. Asm ... #define ... #else #define ... #endif and in xen/errno.h: #include /* We explicitly want a second include with separate names. */ #undef __XEN_PUBLIC_ERRNO_H__ #if ... asm... #define ... #include <...> #else #define ... enum { #include <...> } #endif If you don't like the #undef then perhaps move the value list into public/errno-values.h as a bare list which requires XEN_ERRNO to be defined by the includer and has no guard of its own, so it can be included from both the public and private errno.h with the correct context? > > (I suppose someone needs to patch libxc et al to actually use this) > > The primary consumer, as said in the description, is meant to be > hvmloader. But yes, other tools parts may also want to follow > that. /me wonders how libxc has been getting away with out this until now, especially on non-Linux platforms. Fixing libxc to correctly disambiguate xen errno's from genuine syscalls ones is going to suck... Oh well, not your problem at least, lucky you ;-) > > >> + > >> +#endif /* __ASSEMBLY__ */ > >> + > >> +/* ` enum neg_errnoval { [ -Efoo for each Efoo in the list below ] } */ > >> +/* ` enum errnoval { */ > >> + > >> +#endif /* __XEN_PUBLIC_ERRNO_H__ */ > >> + > >> +#ifdef XEN_ERRNO > >> + > >> +XEN_ERRNO(EPERM, 1) /* Operation not permitted */ > >> +XEN_ERRNO(ENOENT, 2) /* No such file or directory */ > >> +XEN_ERRNO(ESRCH, 3) /* No such process */ > >> +#ifdef __XEN__ > >> +XEN_ERRNO(EINTR, 4) /* Interrupted system call */ > >> +#endif > > > > I take it this is because something prevents this value ever getting > > exposes to userspace? (Continuations?). > > Yes. The thus framed values are supposed to never reach the caller > of a hypercall. > > > I think keeping that away from > > guest API is a good idea, but if it's completely internal perhaps we > > should move it up into a region which we reserve for ourselves? > > That would be at the risk of (later) creating conflicting definitions. I > specifically wanted to preserve the original values and ordering. Good reason. Perhaps #ifdef __XEN__ /* Internal only, should never be exposed to the guest */ since there isn't so many of them to make that onerous. Ian.