[RFC] server-private and public header separation

[Peter Hutterer]

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.

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.

Cheers,
  Peter