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

[Bryce Harrington]

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?

> > 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?
 
> - 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;
        memcpy(config, config_base, config_base->struct_size);

?
 
> 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.
> 
> > >  
> > >  	b = drm_backend_create(compositor, config);
> > >  	if (b == NULL)
> > > diff --git a/src/compositor.h b/src/compositor.h
> > > index 30462cf..3e52703 100644
> > > --- a/src/compositor.h
> > > +++ b/src/compositor.h
> > > @@ -684,6 +684,22 @@ struct weston_backend_output_config {
> > >   * data.
> > >   */
> > >  struct weston_backend_config {
> > > +	/** Major version for the backend-specific config struct
> > > +	 *
> > > +	 * This version must match exactly what the backend expects, otherwise
> > > +	 * the struct is incompatible.
> > > +	 */
> > > +	uint32_t struct_version;
> > > +
> > > +	/** Minor version of the backend-specific config struct
> > > +	 *
> > > +	 * This must be set to sizeof(struct backend-specific config).
> > > +	 * If the value here is smaller than what the backend expects, the
> > > +	 * extra config members will assume their default values.
> > > +	 *
> > > +	 * A value greater than what the backend expects is incompatible.
> > > +	 */
> > > +	size_t struct_size;
> > >  };
> 
> This hunk should be a prerequisite patch of its own, and the rest
> squashed in the drm porting patch.

Ok.
 
> > >  
> > >  struct weston_backend {
> > > diff --git a/src/main.c b/src/main.c
> > > index 66c054e..7370292 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -748,6 +748,8 @@ load_drm_backend(struct weston_compositor *c, const char *backend,
> > >  					 "gbm-format", &config->base.format,
> > >  					 NULL);
> > >  
> > > +	config->base.base.struct_version = 1;
> > > +	config->base.base.struct_size = sizeof(struct weston_drm_backend_config);
> > >  	config->base.configure_output = drm_configure_output;
> > >  
> > >  	if (load_backend_new(c, backend, &config->base.base) < 0) {
> > >   
> 
> This is good.
> 
> 
> I noted some more things in my patch 2 review comments that would need
> to be done to add the struct versioning support.

Bryce