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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.01.15 at 11:28, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> On Wed, 2015-01-14 at 08:46 +0000, Jan Beulich wrote:
>> >>> On 13.01.15 at 17:35, <Ian.Campbell@xxxxxxxxxxxxx> 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.

>> >> +#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

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...

> 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?

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 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.

Sure, easily added.

Jan


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