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

[Bryce Harrington]

On Tue, Apr 12, 2016 at 09:44:10PM -0700, Bryce Harrington 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?
> 
> > > 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);
> 
> ?

(with a struct and a set of parens thrown in there obviously...
this seems to work.)
  
> > 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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel