| Message ID | 1457014484-2282-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.03.16 at 15:14, <andrew.cooper3@citrix.com> wrote: > @@ -82,16 +109,19 @@ XEN_ERRNO(EISCONN, 106) /* Transport endpoint is already connected */ > XEN_ERRNO(ENOTCONN, 107) /* Transport endpoint is not connected */ > XEN_ERRNO(ETIMEDOUT, 110) /* Connection timed out */ > > -#undef XEN_ERRNO > #endif /* XEN_ERRNO */ > - > -#ifndef __XEN_PUBLIC_ERRNO_H__ > -#define __XEN_PUBLIC_ERRNO_H__ > - > /* ` } */ > > + > +/* > + * Clean up from a default include. Close the enum (for C) and remove the > + * default XEN_ERRNO from scope. > + */ > +#ifdef XEN_ERRNO_DEFAULT_INCLUDE > +#undef XEN_ERRNO_DEFAULT_INCLUDE > +#undef XEN_ERRNO > #ifndef __ASSEMBLY__ > }; > #endif > > -#endif /* __XEN_PUBLIC_ERRNO_H__ */ > +#endif /* XEN_ERRNO_DEFAULT_INCLUDE */ So far, upon reaching the end oft the file XEN_ERRNO is undefined, no matter whether it got defined here or prior to inclusion. I think this property should be retained, but moving the #undef to the very end. Please indicate if that's okay with you, as this doesn't really require another version to be sent. Jan
>>> On 04.03.16 at 13:24, <JBeulich@suse.com> wrote: >>>> On 03.03.16 at 15:14, <andrew.cooper3@citrix.com> wrote: >> @@ -82,16 +109,19 @@ XEN_ERRNO(EISCONN, 106) /* Transport endpoint is already > connected */ >> XEN_ERRNO(ENOTCONN, 107) /* Transport endpoint is not connected */ >> XEN_ERRNO(ETIMEDOUT, 110) /* Connection timed out */ >> >> -#undef XEN_ERRNO >> #endif /* XEN_ERRNO */ >> - >> -#ifndef __XEN_PUBLIC_ERRNO_H__ >> -#define __XEN_PUBLIC_ERRNO_H__ >> - >> /* ` } */ >> >> + >> +/* >> + * Clean up from a default include. Close the enum (for C) and remove the >> + * default XEN_ERRNO from scope. >> + */ >> +#ifdef XEN_ERRNO_DEFAULT_INCLUDE >> +#undef XEN_ERRNO_DEFAULT_INCLUDE >> +#undef XEN_ERRNO >> #ifndef __ASSEMBLY__ >> }; >> #endif >> >> -#endif /* __XEN_PUBLIC_ERRNO_H__ */ >> +#endif /* XEN_ERRNO_DEFAULT_INCLUDE */ > > So far, upon reaching the end oft the file XEN_ERRNO is undefined, > no matter whether it got defined here or prior to inclusion. I think > this property should be retained, but moving the #undef to the > very end. Or rather not removing it from where it's now. Jan
On 04/03/16 12:28, Jan Beulich wrote: >>>> On 04.03.16 at 13:24, <JBeulich@suse.com> wrote: >>>>> On 03.03.16 at 15:14, <andrew.cooper3@citrix.com> wrote: >>> @@ -82,16 +109,19 @@ XEN_ERRNO(EISCONN, 106) /* Transport endpoint is already >> connected */ >>> XEN_ERRNO(ENOTCONN, 107) /* Transport endpoint is not connected */ >>> XEN_ERRNO(ETIMEDOUT, 110) /* Connection timed out */ >>> >>> -#undef XEN_ERRNO >>> #endif /* XEN_ERRNO */ >>> - >>> -#ifndef __XEN_PUBLIC_ERRNO_H__ >>> -#define __XEN_PUBLIC_ERRNO_H__ >>> - >>> /* ` } */ >>> >>> + >>> +/* >>> + * Clean up from a default include. Close the enum (for C) and remove the >>> + * default XEN_ERRNO from scope. >>> + */ >>> +#ifdef XEN_ERRNO_DEFAULT_INCLUDE >>> +#undef XEN_ERRNO_DEFAULT_INCLUDE >>> +#undef XEN_ERRNO >>> #ifndef __ASSEMBLY__ >>> }; >>> #endif >>> >>> -#endif /* __XEN_PUBLIC_ERRNO_H__ */ >>> +#endif /* XEN_ERRNO_DEFAULT_INCLUDE */ >> So far, upon reaching the end oft the file XEN_ERRNO is undefined, >> no matter whether it got defined here or prior to inclusion. I think >> this property should be retained, but moving the #undef to the >> very end. > Or rather not removing it from where it's now. IMO, it is wrong to undef XEN_ERRNO if it was provided from external scope. The only users of this "custom" include in Xen itself, and shortly, hvmloader. I think it is fine as-is. ~Andrew
>>> On 04.03.16 at 13:50, <andrew.cooper3@citrix.com> wrote: > On 04/03/16 12:28, Jan Beulich wrote: >>>>> On 04.03.16 at 13:24, <JBeulich@suse.com> wrote: >>>>>> On 03.03.16 at 15:14, <andrew.cooper3@citrix.com> wrote: >>>> @@ -82,16 +109,19 @@ XEN_ERRNO(EISCONN, 106) /* Transport endpoint is already >>> connected */ >>>> XEN_ERRNO(ENOTCONN, 107) /* Transport endpoint is not connected */ >>>> XEN_ERRNO(ETIMEDOUT, 110) /* Connection timed out */ >>>> >>>> -#undef XEN_ERRNO >>>> #endif /* XEN_ERRNO */ >>>> - >>>> -#ifndef __XEN_PUBLIC_ERRNO_H__ >>>> -#define __XEN_PUBLIC_ERRNO_H__ >>>> - >>>> /* ` } */ >>>> >>>> + >>>> +/* >>>> + * Clean up from a default include. Close the enum (for C) and remove the >>>> + * default XEN_ERRNO from scope. >>>> + */ >>>> +#ifdef XEN_ERRNO_DEFAULT_INCLUDE >>>> +#undef XEN_ERRNO_DEFAULT_INCLUDE >>>> +#undef XEN_ERRNO >>>> #ifndef __ASSEMBLY__ >>>> }; >>>> #endif >>>> >>>> -#endif /* __XEN_PUBLIC_ERRNO_H__ */ >>>> +#endif /* XEN_ERRNO_DEFAULT_INCLUDE */ >>> So far, upon reaching the end oft the file XEN_ERRNO is undefined, >>> no matter whether it got defined here or prior to inclusion. I think >>> this property should be retained, but moving the #undef to the >>> very end. >> Or rather not removing it from where it's now. > > IMO, it is wrong to undef XEN_ERRNO if it was provided from external scope. Well, even if not written down, that's the intended behavior: The use case for the including file to further need that definition is rather hard to see, whereas use cases for the including file wanting to do multiple inclusion are quite easy to construct, and in that case the including file would needlessly be forced to repeatedly #undef the symbol. > The only users of this "custom" include in Xen itself, and shortly, > hvmloader. The only users you know of. I'm definitely having plans to make Linux use it too, to have a build time check that the errno values continue to be in sync. > I think it is fine as-is. I could use the same words, just for the unpatched file: I don't really agree with this part of your change. Jan
Jan Beulich writes ("Re: [Xen-devel] [PATCH v3] xen/errno: Reduce complexity of inclusion"):
> On 04.03.16 at 13:50, <andrew.cooper3@citrix.com> wrote:
> > IMO, it is wrong to undef XEN_ERRNO if it was provided from external scope.
>
> Well, even if not written down, that's the intended behavior: The
> use case for the including file to further need that definition is
> rather hard to see, whereas use cases for the including file
> wanting to do multiple inclusion are quite easy to construct, and
> in that case the including file would needlessly be forced to
> repeatedly #undef the symbol.
This is really a very bikeshed issue, but: I slightly prefer Jan's
argument here to Andrew's.
I think that this overall patch from Andrew is very helpful and I
would like to see this minor issue resolved.
Andrew, would you be able to revise the header to #undef XEN_ERRNO in
this case ? Naturally this would want to be documented, in your
admirable new doc comment.
Thanks,
Ian.
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h index dbac396..88e72a8 100644 --- a/xen/include/public/errno.h +++ b/xen/include/public/errno.h @@ -1,4 +1,30 @@ +/* + * There are two expected ways of including this header. + * + * 1) The "default" case (expected from tools etc). + * + * Simply #include <public/errno.h> + * + * In this circumstance, normal header guards apply and the includer shall get + * an enumeration in the XEN_xxx namespace, appropriate for C or assembly. + * + * 2) The special case where the includer provides a XEN_ERRNO() in scope. + * + * In this case, no inclusion guards apply and the caller is responsible for + * their XEN_ERRNO() being appropriate in the included context. + */ + +#ifndef XEN_ERRNO + +/* + * Includer has not provided a custom XEN_ERRNO(). Arrange for normal header + * guards, an automatic enum (for C code) and constants in the XEN_xxx + * namespace. + */ #ifndef __XEN_PUBLIC_ERRNO_H__ +#define __XEN_PUBLIC_ERRNO_H__ + +#define XEN_ERRNO_DEFAULT_INCLUDE #ifndef __ASSEMBLY__ @@ -11,11 +37,12 @@ enum xen_errno { #endif /* __ASSEMBLY__ */ +#endif /* __XEN_PUBLIC_ERRNO_H__ */ +#endif /* !XEN_ERRNO */ + /* ` enum neg_errnoval { [ -Efoo for each Efoo in the list below ] } */ /* ` enum errnoval { */ -#endif /* __XEN_PUBLIC_ERRNO_H__ */ - #ifdef XEN_ERRNO /* @@ -82,16 +109,19 @@ XEN_ERRNO(EISCONN, 106) /* Transport endpoint is already connected */ XEN_ERRNO(ENOTCONN, 107) /* Transport endpoint is not connected */ XEN_ERRNO(ETIMEDOUT, 110) /* Connection timed out */ -#undef XEN_ERRNO #endif /* XEN_ERRNO */ - -#ifndef __XEN_PUBLIC_ERRNO_H__ -#define __XEN_PUBLIC_ERRNO_H__ - /* ` } */ + +/* + * Clean up from a default include. Close the enum (for C) and remove the + * default XEN_ERRNO from scope. + */ +#ifdef XEN_ERRNO_DEFAULT_INCLUDE +#undef XEN_ERRNO_DEFAULT_INCLUDE +#undef XEN_ERRNO #ifndef __ASSEMBLY__ }; #endif -#endif /* __XEN_PUBLIC_ERRNO_H__ */ +#endif /* XEN_ERRNO_DEFAULT_INCLUDE */ diff --git a/xen/include/xen/errno.h b/xen/include/xen/errno.h index 3178466..69b28dd 100644 --- a/xen/include/xen/errno.h +++ b/xen/include/xen/errno.h @@ -1,18 +1,16 @@ #ifndef __XEN_ERRNO_H__ #define __XEN_ERRNO_H__ -#include <public/errno.h> - #ifndef __ASSEMBLY__ -#define XEN_ERRNO(name, value) name = XEN_##name, +#define XEN_ERRNO(name, value) name = value, enum { #include <public/errno.h> }; #else /* !__ASSEMBLY__ */ -#define XEN_ERRNO(name, value) .equ name, XEN_##name +#define XEN_ERRNO(name, value) .equ name, value #include <public/errno.h> #endif /* __ASSEMBLY__ */
The inclusion rules conditions for errno.h were unnecesserily complicated, and required the includer to jump through hoops if they wished to avoid getting multiple namespaces worth of constants. Simply the logic, and document what is going on. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Doug Goldstein <cardoe@cardoe.com> v3: * Reinstate magic documentation comments * Provide assembly-suitable defaults if appropriate --- xen/include/public/errno.h | 46 ++++++++++++++++++++++++++++++++++++++-------- xen/include/xen/errno.h | 6 ++---- 2 files changed, 40 insertions(+), 12 deletions(-)