<div dir="ltr">Hi,<div class="gmail_extra"><br><div class="gmail_quote">On 14 July 2014 13:51, Hans de Goede <span dir="ltr"><<a href="mailto:hdegoede@redhat.com" target="_blank">hdegoede@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">On 07/14/2014 02:43 PM, Julien Cristau wrote:<br>

> On Mon, Jul 14, 2014 at 14:33:00 +0200, Hans de Goede wrote:<br>>> Can I / we please get a reply from you on this ?<br>
>> As explained below this is not about glamor.h, but about<br>
>> making os.h safe to include which really is an<br>
>> orthogonal problem, The glamor.h usage of os.h was<br>
>> just a (bad) example.<br>
><br>
> The drivers need to include xorg-server.h before any other server<br>
> header.  AFAICT that's true both before and after your patch.<br></div></div></blockquote><div><br></div><div>Yes, this is basically my position. Including anything from <xorg/*.h> without having either xorg-server.h (for drivers) or xorg-config.h (internal) included first results in undefined behaviour. Specifically on 64-bit systems, you lose out horrendously because CARD32 secretly gets promoted via unsigned long to CARD64, as _XSERVER64 is not defined. The lack of all the functional definitions in xorg-server.h also means that your ABI's probably different even without the type differences.</div>
<div><br></div><div>It's a terrible, terrible, idea that results in really hard-to-track-down bug reports. To be fair, I'm not on the end of these bug reports anymore, but it's not fun. Anything we can do to discourage this would be better.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">
</div></div>Well in practice they are not doing that, and they are getting<br>
away with it (iow everything works just fine), except that<br>
os.h starts re-defining system libc functions.<br></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

I really don't want to go and fix every single driver out there.<br></blockquote><div><br></div><div>To be honest, I really don't think it would be every single driver.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

There may be some headers where drivers really *must* include<br>
xorg-server.h first, but os.h is not one of them. So far we've<br>
been getting away with os.h redefining e.g. strndup, but with<br>
the latest glibc things have started to conflict.<br>
<br>
I know X is really really old, so we have a bunch of legacy,<br>
like headers which do not work stand-alone (which is considered<br>
a big no no in modern C development practices). But just because<br>
we have this legacy we should no strive to do better.<br>
<br>
So there are 2 reasons to make these changes to os.h:<br>
<br>
1) It avoids the need to fix every single driver out there<br>
2) It is simply the right thing to do<br></blockquote><div><br></div><div>os.h isn't safe to necessarily safe to include without xorg-server.h, as it drags in misc.h, which has: extern _X_EXPORT void SwapLongs(CARD32 *list, unsigned long count);</div>
<div> </div><div>Again, CARD32 is unsigned long without xorg-server.h, which makes it 64-bit on those systems; including xorg-server.h defines _XSERVER64, which makes CARD32 actually be uint32_t. Not massively harmful, but still not really great. A lot of the authorization functions also use XID types, which have the same variance on 64-bit systems without xorg-server.h.</div>
<div><br></div><div>It might not be strictly harmful in some cases, but it's not the sort of thing I want to encourage. 'Always include xorg-server.h before any X header ever' is a really simple rule to remember for driver developers; muddying the water, particularly when not documented, isn't amazingly helpful IMHO.</div>
<div><br></div><div>Cheers,</div><div>Daniel </div></div><br></div></div>