[PATCH wayland 4/7 v3] Add support for custom dispatchers

[Pekka Paalanen]

Hi Jason,

you forgot to CC wayland again. ;-)

On Wed, 20 Feb 2013 10:23:28 -0600
Jason Ekstrand <jason at jlekstrand.net> wrote:

> On Wed, Feb 20, 2013 at 3:45 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Tue, 19 Feb 2013 16:09:21 -0600
> > Jason Ekstrand <jason at jlekstrand.net> wrote:
> >
> >>
> >> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> >> ---
> >> This version contains the documentation changes as requested by Pekka Paalanen
> >> as well as the if-statement change suggested by Bill Spitzak.
> >>
> >>  src/connection.c        | 15 +++++++++----
> >>  src/wayland-client.c    | 13 +++++++++--
> >>  src/wayland-private.h   |  3 ++-
> >>  src/wayland-server.c    |  9 +++++++-
> >>  src/wayland-util.h      | 35 +++++++++++++++++++++++++++--
> >>  tests/dispatcher-test.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  6 files changed, 125 insertions(+), 10 deletions(-)
> >>
> > ...
> >> diff --git a/src/wayland-util.h b/src/wayland-util.h
> >> index df7b384..2d3367f 100644
> >> --- a/src/wayland-util.h
> >> +++ b/src/wayland-util.h
> >> @@ -46,18 +46,49 @@ struct wl_message {
> >>       const struct wl_interface **types;
> >>  };
> >>
> >> +union wl_argument;
> >> +struct wl_object;
> >> +
> >> +typedef void (*wl_interface_dispatcher_func_t)(struct wl_object *, uint32_t,
> >> +                                            const struct wl_message *,
> >> +                                            void *, union wl_argument *);
> >> +/**
> >> + * wl_interface - object method interface
> >> + *
> >> + * This structure has been extended since libwayland version 1.  The version
> >> + * field now represents both an interface version and a structure version.
> >> + * The upper 16 bits represent the version of the structure with the original
> >> + * version being version 0.  The lower 16 bits represent the interface version
> >> + * as specified in the interface XML file.  This way, as long as the version
> >> + * number is something sane and we always check the version number before
> >> + * reading any fields that are not in the original structure, we should be
> >> + * fine.
> >> + *
> >> + * Special care must, however, be taken with the auto-generated instances of
> >> + * wl_interface created by the scanner.  Because each of the generated
> >> + * interfaces is a global extern variable, the memory for them may be allocated
> >> + * by the client-code and not by the library.  Because the client code
> >> + * allocates only allocates enough memory for the size it knows about, any
> >> + * fields unknown to the client cannot be trusted. Therefore, increasing the
> >> + * interface version of wl_interface globals generated by the scanner
> >> + * constitutes an ABI break.
> >
> > Alright, this looks better. An extra 'allocates'.
> > Don't you mean struct version for the ABI break?
> 
> Pekka,
> Thanks for you're comments.  I'm a little out of my element here, but
> I'll try as best as I can.

Me too.

> > This does make me wonder, is it possible for the global to be allocated
> > in one size, and then initialized according to a bigger size, leading
> > to overwriting some memory? I mean at runtime when linking a library,
> > in case globals are copied. Does that even happen? The dynamic linker
> > won't look at the struct version. My gut is saying that this must be
> > already handled properly and there's no need to worry, but I've been
> > surprised before.
> >
> > I also have hard time understanding the API break. If we have an
> > application built with struct version 0, and libwayland built with
> > struct version 1, does that break?
> 
> I looked at the assembly generated from wayland-protocol.c and I think
> I have a better idea of what's going on.  Because it's declared as
> extern const, it gets storred in the .data segment of the library.
> During the dynamic linking stage at runtime, the executable may make a
> copy of the interface data.  However, because it doesn't know the
> right size for the structure, it only copies the first part of it.
> The result is that added fields are either inaccessible or garbage.
> Like I said, I'm a little out of my element here so this is
> part-conjecture.
> 
> The reason this causes a break is that, if the structure version is
> set to 1, libwayland will look at event_dispatcher and
> method_dispatcher and use them if they are not null.  If they are
> inaccessible or filled with a garbage value, we have a problem.
> 
> > Especially considering the case, if the application has a generated
> > *-protocol.c file of its own protocol extension, which will then
> > reference core interface globals like wl_surface_interface.
> 
> This shouldn't be a problem as it's not crossing a dynamic linking boundary.

This statement is a little unobvious. The app contains a .c file that
references a global (&wl_surface_interface) in libwayland. However, as
the code that actually looks at the struct version and extended fields
is also in libwayland, and not in the app, it's not really crossing the
dynamic linking boundary. Is that what you mean?

Let's bump the complexity: add a libfeature to the mix, providing
another protocol extension. The app is now using interfaces from
libwayland and libfeature. Let's assume:
- libwayland is built with struct version 1
- libfeature is built with struct version 1
- the app is built with struct version 0

If the app does a copy of the libfeature globals according to struct
version 0, the extended fields are missing, but the struct version
field is still 1. Kaboom?

I can only raise more questions without answers:
- is this actually fragile or not?
- do we care about supporting libfeature updates without rebuilding the
  app?

Sorry for playing devil's advocate, but this just sounds like it might
be important. OTOH, we are still in childhood of Wayland, and if we
never need to bump the struct version again, maybe we don't have to
care about libfeature as there might be none yet?

So. While we are bumping the struct wl_interface size, maybe we should
also add some reserved unused fields for the future?

> > Maybe we are saved by the fact that all the globals are const? Wait...
> > struct wl_interfaces is not a const anymore, since you need to be able
> > to assign the dispatcher function pointers at runtime, do you not?
> 
> no
> 
> > Btw. shouldn't you modify wayland-scanner to not make them const then?
> 
> They are still const
> 
> > Could you explain or demonstrate how you actually plug in your custom
> > dispatchers? I couldn't find any hint of that in the v3 patch set,
> > did I just miss it?
> 
> It's intended that dispatchers will be assigned by scanner code or
> something similar generated by language bindings.  At some point here,
> I'm going to modify scanner.c so that it can optionally generate a
> wayland-protocol.c file that uses dispatchers and doesn't require
> libffi.  Obviously, you still need libffi in most release code, but it
> would mean that I don't have to build libffi for 4 different Android
> platforms :-/.
> 
> > Ah, but is the whole point, that you will re-generate your own set of
> > all wl_interface structs with your own scanner, which will plug in the
> > right dispatcher functions, and everything can still be const?
> 
> yup.
> 
> >> + */
> >>  struct wl_interface {
> >>       const char *name;
> >> -     int version;
> >> +     uint32_t version;
> >>       int method_count;
> >>       const struct wl_message *methods;
> >>       int event_count;
> >>       const struct wl_message *events;
> >> +
> >> +     /* Added in version 1 */
> >> +     wl_interface_dispatcher_func_t method_dispatcher;
> >> +     wl_interface_dispatcher_func_t event_dispatcher;
> >>  };


Thanks,
pq