On Wed, Apr 06, 2016 at 11:43:54AM +0300, Pekka Paalanen wrote: > On Mon, 21 Mar 2016 23:11:29 +0100 > Benoit Gschwind <[email protected]> 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 <[email protected]> > > > > 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 <[email protected]> > > > --- > > > 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
