[PATCH wayland 4/7 v3] Add support for custom dispatchers
Jason Ekstrand
jason at jlekstrand.net
Thu Feb 21 07:33:28 PST 2013
On Thu, Feb 21, 2013 at 1:44 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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?
Yeah, that's a problem waiting to happen.
> 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?
I think being in the childhood of Wayland is actually an advantage
here because now is the time to find these issues and fix them. :-)
> 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
Looking at everything, I can really only see 2 solutions, one of which
I already know Krsitian doesn't like but I'm going to put it back on
the table anyway.
First is to wait until version 2 and change the scanner so that it
emits the wl_interface instances as static const and then emits a
static const struct wl_interface * that other code/libraries can use.
This way we are only ever passing pointers to/from the library and
future structure changes should be safe.
The second (Kristian doesn't like this one) is to put the dispatchers
directly into wl_object. I'll make the argument as to why it doesn't
break ABI too badly further down. First, the pros and cons from a
conceptual/architectural perspective.
>From a conceptual perspective, it means that dispatchers are part of
the implementation, not part of the library. While I think the
primary use-case is still language bindings (which will probably
generate their own wl_interfaces anyway), it may still have an
advantage to regular devs. For instance, it would greatly reduce the
amount of boiler-plate code required to get C++ off the ground with
just the C version of libwayland. It also reduces two potential
sources of confusion:
1) Having multiple copies of wl_interface that have the same name but
are still technically different.
2) Having to keep track of event dispatchers vs. method dispatchers.
Since any given wl_object is either server-side or client-side and
never both, we don't have to worry about this.
On the other hand, it means we can never make dispatchers the default
with a single change to the scanner. I don't know how much people
care about it, but it makes it harder to get rid of libffi. Also, it
may make more sense for dispatchers to be tied to the interface; I
know you can make that argument.
Now time to convince you all that this doesn't break ABI too bad...
First, compositors. If structures are nicely memset to zero right
after allocation, all it will need is a recompile. Otherwise, we're
looking at a small weston patch (I can do that). Obviously this will
break ABI on the compositor side, but that should be acceptable for
1.1.
Client side, however, we should be safe. Technically, the wl_object
structure is exposed through wayland-util.h to the clients. However,
the only way clients can communicate with libwayland is through
wl_display and wl_proxy which are both opaque pointers. Therefore, we
can change the internals all we want and the clients will never
notice. If we want to add client-side dispatchers support, we simply
add another wl_proxy_add_listener variant that takes a dispatcher and
we can do so without breaking ABI.
Feel free to tell me that's a bad idea; it just seems the safest and
best long-term to me. Although, even if we do add dispatchers to
wl_object, we may want to do the scanner change (first idea) too when
we get to version 2.
--Jason
More information about the wayland-devel
mailing list