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

[Bryce Harrington]

On Wed, Apr 13, 2016 at 02:47:06PM +0300, Pekka Paalanen wrote:
> 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?

But we have a major/minor version.  WESTON_DRM_BACKEND_CONFIG_MAJOR_VERSION?
Or is that too long?

> > > > 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

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQIUAwUBVw4xuiNf5bQRqqqnAQio/A/0D4E9Ju5OnowSgjKPiMQuqNFQcPUcaKF4
> B8au7NESgrGgAd8ZOowBwBjEnvX5QW8gkaG2QzurwDetWQepnVzwC6kK50Y22I/q
> PHJuuFf/QM1vQ4Z7d+t8yRr+LjWJlOXl66Eh1773QaqQGTROGmcBdNw8CPnDAOUa
> Zkc++5oKrbeslkZU80pgMYCBXvVgip2fVVAqwJqnPEJ7thOAhcezz7kpPFKi5pHq
> +FhkSDl/Zb3ETd0zd7Yb20xEMiuyLoctZxLLV9Ny4oEB7+MKM7Nx9U7bW3Vm2hi7
> IZS2M7QzE3x4xkRBm6uIF3DXsZAQJDUPsn9b0dxchz7Pp1bajJVb0E3zdocC5os+
> 3SmHJlyiKrX9YNaU7zlPiiDhEJ8K98n2l6gjfOJiGncG96pQ29rJc36H9LBVlpNy
> M1Vo12X3pAOcNu5uOFDFgIZCcPBXTzekkgdB0KVBZBTzFgQ//dYxR/yXciAeCFoJ
> ltat1eyOp/omJp2H3qNhwy8FTc1qqIFbw6AkN/Mrv/F1xINFgBoVk+phMMtoZIZi
> qsQyJYQTVB/EJ+Also8JkAGI2eIWY0+8Fz+cMzKOiDdT3RHHbepMthKnC7RA262f
> SoHEmIYH4djVbOaSSXsolK86qeBvC5rAxGNlVqsPqw8PL4i9lHFKmxys7yLc5pA0
> kgkjSH4uSw==
> =MYas
> -----END PGP SIGNATURE-----