[RFC] server-private and public header separation

[Chase Douglas]

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