On Wed, Apr 13, 2016 at 02:47:06PM +0300, Pekka Paalanen wrote:
> On Tue, 12 Apr 2016 21:44:10 -0700
> Bryce Harrington <br...@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 <gschw...@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 <gschw...@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 <br...@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-----
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
