[RFC] server-private and public header separation

[Chase Douglas]

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.

-- Chase