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

[Pekka Paalanen]

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.

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

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

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.

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


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/20160406/01a1265f/attachment.sig>