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 11:18 +0000, Jan Beulich wrote:
> >>> On 14.01.15 at 11:28, <Ian.Campbell@eu.citrix.com> wrote:
> > 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?
> 
> Could do, but no-one is required to use the enum as a type. I just
> wanted to _allow_ people using it if they want.

I was thinking to avoid presenting them with the temptation.

> >> >> +#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 ...
     enum xen_errno {
> > #endif

> I don't think that would work when including public/errno.h a second
> time. Or maybe I'm not getting how you envision things to be...

I forgot the enum stuff above, I've inserted it in the quote.

The second time around XEN_ERRNO would be defined and so none of that
stuff would happen, just the list of XEN_ERRNO(value..) would be
evaluated (in the context of xen/errno.h's "enum {").

Thinking about it now I guess some more trickery would be needed for the
closing } of the enum xen_errno, which could well end up being a cure
worse than the disease.

> To be honest I prefer a solution with just a single new public header,
> even if it ends up using some trickery (so long as it doesn't violate
> any language requirements).

I lean the other way, but not enough to continue arguing, so: ok.

Ian.