[PATCH wayland 0/7] Add support for custom dispatchers

[Jason Ekstrand]

A couple of questions:

1) How would you like to handle the ABI issues?  If we create version
0 wl_interface objects in the scanner, everything seems to work fine.
The clients complain a bit a bout sizes of objects changing in the
version they were originally linked against, but the still work.
However, creating version 1 objects with a null dispatcher causes
segmentation faults.  I'm not 100% sure why, it has something to do
with bits of the dynamic linking process I don't understand.

2) I'm still not 100% satisfied with this method of extending
wl_interface.  While I don't want to require interfaces to be static
(that breaks lots of API binding stuff), it seems like the dispatcher
is more of an implementation detail.  Perhaps what we should do is to
keep going with the "upper 16 bits is a structure version" thing and,
instead of extending wl_interface at this point, replace the
implementation field in wl_object with a pointer to something like
this:

struct wl_implementation {
        void (* dispatcher)(struct wl_object *, uint32_t, const struct
wl_message*, void *, union wl_argument *);
        void *data;
}

This has the disadvantage of meaning that servers wishing to build
against newer versions of wayland would have to do some re-writing.
However, client-side stuff is isolated enough, that it shouldn't be a
problem.

The reason for this is, in my use-case, I need to write different
dispatchers for client-side vs. server-side.  This isn't needed in C
since struct wl_client * and void * are both just pointers and the
exact type is completely ignored by ffi.  However, for language
bindings that need to be able to do some translation, you need
different dispatchers for one vs. the other.  This would also make it
more feasible for someone to write their own dispatcher for a specific
object instead of using the default one generated by the scanner.

There's more comments below.
Thanks,
--Jason

On Thu, Feb 7, 2013 at 8:07 PM, Kristian Høgsberg <hoegsberg at gmail.com> wrote:
> On Fri, Feb 01, 2013 at 10:09:41AM -0600, Jason Ekstrand wrote:
>> Ths patch group contains my additions for custom dispatcher support inside
>> libwayland.  This would allow someone to handle event and resource function
>> calling at runtime rather than through a list of function pointers and libffi.
>> The primary use case for these modifications is to enable much more flexible
>> binding to non-C languages.
>>
>> As far as I am concerned, these patches are ready for commit.  All unit tests
>> pass including new ones.  Also, weston successfully builds and runs against
>> this version.
>>
>> Concerning ABI: I can't see why any programs built against earlier versions
>> should break with these changes.  If patch 5 (for wayland-scanner) is omitted,
>> earlier version of weston built against 1.0.4 work in spite of complaining
>> about different sized structures in the shared object.  With patch 5, they
>> segfault because the version is above 1 but the dispatcher is not null.  I'm
>> not seeing why this happens but it may be that I don't understand ABI's well
>> enough.
>
> Hi Jason,
>
> Just a quick comment on this: I quite like the direction this is
> taking, the marshalling code is simpler overall and the libffi code is
> better isolated.  The only concern I have is that we seem to be doing
> quite a few more copies of all the arguments now.  That's likely not a
> concern in the bigger picture, but it seems like in one case we should
> be able to allocate a wl_closure up front and then vmarshal straight
> into the arg array there instead of a temp array.

I can look into trying to do the argument array creation in place.  It
will require breaking the code up a little differently, but we can
probably optimize that step just a bit.  Most of the time I do it that
way for the sake of avoiding code duplication.  I also do that in the
case of wl_resource_post_event where it's either code duplication or
copying the wl_argument array.  Which way should the compromise go?

>
> Kristian
>
>> Jason Ekstrand (7):
>>   Make a #define for WL_CLOSURE_MAX_ARGS instead of a magic number.
>>   Add a default dispatcher function and related test
>>   Convert wl_closure to use wl_argument and enable dispatchers
>>   Fill out dummy objects in tests so dispatchers will work
>>   Make the scanner generate version 1 wl_interface structures
>>   Add wl_arguments forms of wl_resource_post_event and
>>     wl_resource_queue_event
>>   Test for proper usage of custom vs. default dispatchers
>>
>>  src/connection.c         | 421 ++++++++++++++++++++++++++++-------------------
>>  src/scanner.c            |   3 +-
>>  src/wayland-client.c     |  29 ++--
>>  src/wayland-private.h    |  20 ++-
>>  src/wayland-server.c     |  69 ++++----
>>  src/wayland-server.h     |   4 +
>>  src/wayland-util.h       |  38 ++++-
>>  tests/.gitignore         |   1 +
>>  tests/Makefile.am        |   2 +
>>  tests/connection-test.c  |  35 +++-
>>  tests/dispatcher-test.c  | 163 ++++++++++++++++++
>>  tests/os-wrappers-test.c |  11 +-
>>  12 files changed, 566 insertions(+), 230 deletions(-)
>>  create mode 100644 tests/dispatcher-test.c
>>
>> --
>> 1.8.1
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel