Re: [PATCH RFC] make error codes a formal part of the ABI

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

On Wed, 2015-01-14 at 08:46 +0000, Jan Beulich wrote:
> >>> On 13.01.15 at 17:35, <Ian.Campbell@eu.citrix.com> 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 <public/errno.h> 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 <xen/errno.h> 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 <public/errno.h>
        
        /* 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.