[PATCH 4/5] compositor: Version the backend configuration structures

[Pekka Paalanen]

On Tue, 12 Apr 2016 21:44:10 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Wed, Apr 06, 2016 at 11:43:54AM +0300, Pekka Paalanen wrote:
> > On Mon, 21 Mar 2016 23:11:29 +0100
> > Benoit Gschwind <gschwind at gnu-log.net> wrote:
> >   
> > > Hello,
> > > 
> > > I think that struct_version and struct_size should not be belong
> > > compositor.h. I think those versioning should be back-end detail, and
> > > each back-end should provide a major version number through #define in
> > > the backend header.  
> > 
> > Hi!
> > 
> > No, the struct fields do belong in compositor.h. These fields are
> > common to all backend-specific structs, and must be handled the same
> > everywhere, so they make perfect sense in compositor.h, in the
> > definition of struct weston_backend_config.
> > 
> > However, you are right in that backends must define the struct_version
> > values in a backend-specific header. That #define can (only) be used for
> > build time compatibility checks in #if directives in main.c.  
> 
> Agreed.  How should this #define be named?

Hi,

umm... WESTON_DRM_BACKEND_CONFIG_VERSION?

> > > Reviewed-by: Benoit Gschwind <gschwind at gnu-log.net>
> > > 
> > > Le 10/03/2016 01:49, Bryce Harrington a écrit :  
> > > > With this struct versioning, it is possible to add new options without
> > > > breaking the ABI, as long as all additions are made to the end of a
> > > > struct and nothing existing is modified or removed.  When things are
> > > > added, the structure's size will increase, and we'll use this size as
> > > > our minor version number.  If existing things need to be changed, then
> > > > the major version, struct_version, is incremented to indicate the ABI
> > > > break.
> > > > 
> > > > From our call site in main we record these major and minor version as
> > > > struct_version and struct_size.  The backend then verifies these against
> > > > its own assumptions.  So long as the backend's struct is equal or larger
> > > > than what was passed in and the major versions are equal, we're good;
> > > > but if it is larger, then this is a fatal error.
> > > > 
> > > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > > > ---
> > > >  src/compositor-drm.c | 10 ++++++++--
> > > >  src/compositor.h     | 16 ++++++++++++++++
> > > >  src/main.c           |  2 ++
> > > >  3 files changed, 26 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > > > index 5ddedb9..9bce285 100644
> > > > --- a/src/compositor-drm.c
> > > > +++ b/src/compositor-drm.c
> > > > @@ -3203,8 +3203,14 @@ backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> > > >               struct weston_backend_config *config_base)
> > > >  {
> > > >  	struct drm_backend *b;
> > > > -	struct weston_drm_backend_config *config =
> > > > -				(struct weston_drm_backend_config *)config_base;
> > > > +	struct weston_drm_backend_config *config;
> > > > +
> > > > +	if (config_base == NULL ||
> > > > +	    config_base->struct_version != 1 ||
> > > > +	    config_base->struct_size > sizeof(struct weston_drm_backend_config))
> > > > +		return -1;
> > > > +
> > > > +	config = (struct weston_drm_backend_config *)config_base;  
> > 
> > Like Quentin has suggested in IRC, this should be done as follows:  
> 
> Is there a log of that discussion?

Umh, it happened a while ago. Not really much else to say than it's a
nice trick to augment an incoming struct into a version used internally
in a library. That's the reason to pick sizeof() instead of an arbitrary
minor_version.

> > - allocate a private struct weston_drm_backend_config
> > - init the private config with all defaults
> > - copy the first struct_size bytes from the passed-in config to the
> >   private config
> > 
> > This allows the backend to add more fields to the end with default
> > values, and maintain compatiblity with the older main.c.  
> 
> So like in place of the cast line, do you mean something like:
> 
>         config = zalloc(sizeof weston_drm_backend_config);
>         if (config == NULL)
> 	   return -1;

Add init_to_defaults(config); here.

>         memcpy(config, config_base, config_base->struct_size);
> 
> ?

Then memcpy only overwrites the passed in data and leaves the rest as
defaults.

This is the generic way. There might be shortcuts to be made if
everything is already copied into yet other variables anyway.

> > The reason is that sizeof(struct weston_drm_backend_config) is
> > different between the caller and callee, and we still need to get the
> > defaults in somehow. Using this copy trick allows the version check to
> > be in just one place, and all other code can trust that all the fields
> > are properly initialized (not dependent on version).
> > 
> > It is a shallow copy, which is a bit awkward, but I'm not sure there is
> > a simple and better way.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160413/4022106d/attachment.sig>