On 03/22/2012 11:07 PM, Peter Hutterer wrote: > On Thu, Mar 22, 2012 at 10:48:53PM -0700, Chase Douglas wrote: >> On 03/22/2012 09:25 PM, Peter Hutterer wrote: >>> This is just a quickfire patch to show the principle, it has not been tested >>> much. Plus, it's more of an idea right now, not sure if I'll find the time >>> to do it. >>> >>> Right now, we export virtually everything including the gory bits of every >>> struct. Which causes us to break the ABI whenever we so much as look at a >>> struct (see the per-device-idlecounters for example). >>> >>> This patch introduces a new define __XSERVER__ that we can use in the sdk >>> headers. Obviously the real integration will be a tad harder as there are >>> other headers that are installed outside of include/. But the gist is that >>> anything between ifdef __XSERVER__ disappears on install, (in)effectively >>> hiding it. >>> >>> It's going to be painful defining what goes in and what doesn't, but maybe, >>> just maybe, it'll be useful in the long term. >>> >>> Is that something we want? >>> --- >>> include/Makefile.am | 8 ++++++++ >>> include/dix-config.h.in | 3 +++ >>> include/inputstr.h | 27 +++++++++++++++------------ >>> 3 files changed, 26 insertions(+), 12 deletions(-) >>> >>> diff --git a/include/Makefile.am b/include/Makefile.am >>> index 972f403..3e183db 100644 >>> --- a/include/Makefile.am >>> +++ b/include/Makefile.am >>> @@ -71,3 +71,11 @@ EXTRA_DIST = \ >>> eventconvert.h eventstr.h inpututils.h \ >>> protocol-versions.h \ >>> xsha1.h >>> + >>> + >>> +unifdef_headers: $(sdk_HEADERS) >>> + for file in $(sdk_HEADERS); do \ >>> + (cd $(DESTDIR)$(sdkdir) && unifdef -U__XSERVER__ -o $$file.tmp >>> $$file; mv $$file.tmp $$file) \ >>> + done >>> + >>> +install-data-hook: unifdef_headers >>> diff --git a/include/dix-config.h.in b/include/dix-config.h.in >>> index 3fb6413..9b362c6 100644 >>> --- a/include/dix-config.h.in >>> +++ b/include/dix-config.h.in >>> @@ -3,6 +3,9 @@ >>> #ifndef _DIX_CONFIG_H_ >>> #define _DIX_CONFIG_H_ >>> >>> +/* XServer-internal */ >>> +#define __XSERVER__ 1 >>> + >>> /* Support BigRequests extension */ >>> #undef BIGREQS >>> >>> diff --git a/include/inputstr.h b/include/inputstr.h >>> index 5a38924..a4f549c 100644 >>> --- a/include/inputstr.h >>> +++ b/include/inputstr.h >>> @@ -529,19 +529,8 @@ typedef struct _SpriteInfoRec { >>> typedef struct _DeviceIntRec { >>> DeviceRec public; >>> DeviceIntPtr next; >>> - Bool startup; /* true if needs to be turned on at >>> - server initialization time */ >>> - DeviceProc deviceProc; /* proc(DevicePtr, DEVICE_xx). It is >>> - used to initialize, turn on, or >>> - turn off the device */ >>> - Bool inited; /* TRUE if INIT returns Success */ >>> - Bool enabled; /* TRUE if ON returns Success */ >>> - Bool coreEvents; /* TRUE if device also sends core */ >>> - GrabInfoRec deviceGrab; /* grab on the device */ >>> - int type; /* MASTER_POINTER, MASTER_KEYBOARD, SLAVE >>> */ >>> - Atom xinput_type; >>> - char *name; >>> int id; >>> + char *name; >>> KeyClassPtr key; >>> ValuatorClassPtr valuator; >>> TouchClassPtr touch; >>> @@ -554,6 +543,19 @@ typedef struct _DeviceIntRec { >>> StringFeedbackPtr stringfeed; >>> BellFeedbackPtr bell; >>> LedFeedbackPtr leds; >>> + >>> +#ifdef __XSERVER__ >>> + Bool startup; /* true if needs to be turned on at >>> + server initialization time */ >>> + DeviceProc deviceProc; /* proc(DevicePtr, DEVICE_xx). It is >>> + used to initialize, turn on, or >>> + turn off the device */ >>> + Bool inited; /* TRUE if INIT returns Success */ >>> + Bool enabled; /* TRUE if ON returns Success */ >>> + Bool coreEvents; /* TRUE if device also sends core */ >>> + GrabInfoRec deviceGrab; /* grab on the device */ >>> + int type; /* MASTER_POINTER, MASTER_KEYBOARD, SLAVE >>> */ >>> + Atom xinput_type; >>> struct _XkbInterest *xkb_interest; >>> char *config_info; /* used by the hotplug layer */ >>> ClassesPtr unused_classes; /* for master devices */ >>> @@ -593,6 +595,7 @@ typedef struct _DeviceIntRec { >>> int xtest_master_id; >>> >>> struct _SyncCounter *idle_counter; >>> +#endif >>> } DeviceIntRec; >>> >>> typedef struct { >> >> This implementation would make the struct size look different between >> the server and the client. I think that would likely end up causing some >> bad memory corruption bugs sometime down the line. >> >> I would suggest using the pimpl idiom, where you have a public struct >> with a pointer to a private data struct. The pointer is still public, >> but the data hidden behind the pointer isn't. The private structure >> definition could be provided in a struct within the __XSERVER__ >> conditional macros so it's filtered out at install time. >> >> I do think this is valuable. The scope of the ABI today is *huge* >> because we don't hide anything. Almost all of the input ABI could be >> hidden, I suspect. > > it's also a tradeoff of work vs benefit. ifdef-ing out parts of the struct > is largely free minus the addition of ifdefs and makefile changes. > Hiding non-public fields in a private struct means going through the server > and changing most (but not all) dev to dev->priv. which can be rather > painful until we find a way to automate it. > it'll also have the joy of conflicts with other patches going in at the same > time, so it would be something where the server has to stand still for a > bit.
Yeah, it's a non-trivial amount of work. However, I think the approach above has many pitfalls that are likely to ensnare us at some future point down the road. And memory corruption issues are the hardest to figure out. > and tbh, if we're already going the pimpl path we might as well just export > everything as opaque structs and provide setter/getter functions. At which > point we have an actual API. One could dream? :) -- Chase _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel