[RFC wayland] Track protocol object versions inside wl_proxy.
Derek Foreman
derekf at osg.samsung.com
Wed Nov 25 10:46:33 PST 2015
On 25/11/15 12:25 PM, Jason Ekstrand wrote:
> On Wed, Nov 25, 2015 at 10:15 AM, Derek Foreman <derekf at osg.samsung.com> wrote:
>> On 13/11/15 12:21 PM, Jason Ekstrand wrote:
>>> On Fri, Nov 13, 2015 at 3:18 AM, Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>>>> So do i understand correctly that if the app creating the
>>>> wl_compositor was built against a libwayland without this patch the
>>>> version returned in mesa by wl_proxy_get_version() for every proxy
>>>> will be always 1?
>>>
>>> Yes, see also the second e-mail link below. I proposed to make
>>> wl_display version 0 so that libraries can detect whether the client
>>> was built against old or new libwayland.
>>>
>>> --Jason
>>
>> But the generated protocol headers won't have wl_surface_get_version()
>> in that case, so build would fail?
>>
>> I'm finally trying to build a client that shows this failure mode, and
>> it's starting to feel quite contrived...
>
> The issue that arises is if you have a client built against an old
> version of libwayland that uses a library built against a newer
> version. Let's make this extremely concrete:
>
> Suppose that we use this wl_surface_get_version() in mesa's EGL
> implementation to determine whether or not we have a buffer_damage
> request. We do that, merge it into mesa master, and start shipping it
> in distros. Then some old client comes along that was built against
> libwayland 1.3. It can run against libwayland 1.7 just fine because
> everything's backwards compaible. It can also run against our
> brand-new mesa because the GL and EGL apis are backwards compatible.
> However, whatever wayland objects it passes in to EGL will be created
> using the old api that doesn't have _versioned functions. The result
> of this is that all objects appear to be version 1 because that's what
> wl_display starts at.
>
> This is ok for buffer_damage() because that's just adding a request.
> However, if there's something more subtle that happens when an object
> changes version such that treating a version 3 object as a version 1
> object may not always be correct, then we have a problem. While, in
> general, we should try to avoid these kinds of changes, they can
> happen so we should let the user of wl_proxy_get_version() be able to
> distinguish between version 1 and "I don't know".
>
> Does that make more sense? It's quite subtle.
Ugh. Ok. That's a bit tricky.
I absolutely don't have any better ideas than your suggestion of setting
wl_display's proxy_version to 0 and letting all the old-client-created
objects show as version 0.
Thanks for the excellent explanation,
Derek
>> Is there a reason we'd want to use wl_proxy_get_version() directly? I
>> can't imagine having a proxy, needing to know its version, but not
>> knowing exactly what it's a proxy for... Maybe I'm just not being
>> imaginative enough though.
>
> No, wrappers are fine.
>
>> If we make foo_get_version()'s presence known through #defines then the
>> absence of an appropriate foo_get_version() (ie: old header) could
>> indicate we need to do whatever kind of fallback.
>
> Sure. For that matter, the client can get that from a version #define
> or from pkg-config. That's a standard "am I building with a recent
> version" issue.
>
> --Jason
>
>> Thanks,
>> Derek
>>
>>>> 2015-11-12 22:01 GMT+02:00 Derek Foreman <derekf at osg.samsung.com>:
>>>>> From: Jason Ekstrand <jason at jlekstrand.net>
>>>>>
>>>>> This provides a standardized mechanism for tracking protocol object
>>>>> versions in client code. The wl_display object is created with version 1.
>>>>> Every time an object is created from within wl_registry_bind, it gets the
>>>>> bound version. Every other time an object is created, it simply inherits
>>>>> it's version from the parent object that created it.
>>>>>
>>>>> ---
>>>>> Sooo, seems to finally fix the EGLSwapBuffersWithDamage problem, we also
>>>>> need this patch. However, it's not complete.
>>>>>
>>>>> I've rebased it and updated it. Here's the original posting:
>>>>> http://lists.freedesktop.org/archives/wayland-devel/2014-April/014004.html
>>>>>
>>>>> Of special importance is:
>>>>> http://lists.freedesktop.org/archives/wayland-devel/2014-April/014011.html
>>>>>
>>>>> And the proposed solution to the new backwards compatibility issue. If we
>>>>> get a consensus on a way forward, I can pick this up and finish it off...
>>>>>
>>>>> src/scanner.c | 29 ++++++++----
>>>>> src/wayland-client-core.h | 14 ++++++
>>>>> src/wayland-client.c | 112 +++++++++++++++++++++++++++++++++++++++++++---
>>>>> 3 files changed, 140 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/src/scanner.c b/src/scanner.c
>>>>> index 8ecdd56..850e72a 100644
>>>>> --- a/src/scanner.c
>>>>> +++ b/src/scanner.c
>>>>> @@ -889,6 +889,14 @@ emit_stubs(struct wl_list *message_list, struct interface *interface)
>>>>> interface->name, interface->name, interface->name,
>>>>> interface->name);
>>>>>
>>>>> + printf("static inline uint32_t\n"
>>>>> + "%s_get_version(struct %s *%s)\n"
>>>>> + "{\n"
>>>>> + "\treturn wl_proxy_get_version((struct wl_proxy *) %s);\n"
>>>>> + "}\n\n",
>>>>> + interface->name, interface->name, interface->name,
>>>>> + interface->name);
>>>>> +
>>>>> has_destructor = 0;
>>>>> has_destroy = 0;
>>>>> wl_list_for_each(m, message_list, link) {
>>>>> @@ -960,20 +968,25 @@ emit_stubs(struct wl_list *message_list, struct interface *interface)
>>>>>
>>>>> printf(")\n"
>>>>> "{\n");
>>>>> - if (ret) {
>>>>> + if (ret && ret->interface_name == NULL) {
>>>>> printf("\tstruct wl_proxy *%s;\n\n"
>>>>> - "\t%s = wl_proxy_marshal_constructor("
>>>>> + "\t%s = wl_proxy_marshal_constructor_versioned("
>>>>> "(struct wl_proxy *) %s,\n"
>>>>> - "\t\t\t %s_%s, ",
>>>>> + "\t\t\t %s_%s, interface, version",
>>>>> ret->name, ret->name,
>>>>> interface->name,
>>>>> interface->uppercase_name,
>>>>> m->uppercase_name);
>>>>> -
>>>>> - if (ret->interface_name == NULL)
>>>>> - printf("interface");
>>>>> - else
>>>>> - printf("&%s_interface", ret->interface_name);
>>>>> + } else if (ret) {
>>>>> + printf("\tstruct wl_proxy *%s;\n\n"
>>>>> + "\t%s = wl_proxy_marshal_constructor("
>>>>> + "(struct wl_proxy *) %s,\n"
>>>>> + "\t\t\t %s_%s, &%s_interface",
>>>>> + ret->name, ret->name,
>>>>> + interface->name,
>>>>> + interface->uppercase_name,
>>>>> + m->uppercase_name,
>>>>> + ret->interface_name);
>>>>> } else {
>>>>> printf("\twl_proxy_marshal((struct wl_proxy *) %s,\n"
>>>>> "\t\t\t %s_%s",
>>>>> diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h
>>>>> index 8b4b4b8..ed41773 100644
>>>>> --- a/src/wayland-client-core.h
>>>>> +++ b/src/wayland-client-core.h
>>>>> @@ -138,10 +138,21 @@ wl_proxy_marshal_constructor(struct wl_proxy *proxy,
>>>>> const struct wl_interface *interface,
>>>>> ...);
>>>>>
>>>>> +struct wl_proxy *wl_proxy_marshal_constructor_versioned(struct wl_proxy *proxy,
>>>>> + uint32_t opcode,
>>>>> + const struct wl_interface *interface,
>>>>> + uint32_t version,
>>>>> + ...);
>>>>> struct wl_proxy *
>>>>> wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
>>>>> uint32_t opcode, union wl_argument *args,
>>>>> const struct wl_interface *interface);
>>>>> +struct wl_proxy *
>>>>> +wl_proxy_marshal_array_constructor_versioned(struct wl_proxy *proxy,
>>>>> + uint32_t opcode,
>>>>> + union wl_argument *args,
>>>>> + const struct wl_interface *interface,
>>>>> + uint32_t version);
>>>>>
>>>>> void
>>>>> wl_proxy_destroy(struct wl_proxy *proxy);
>>>>> @@ -165,6 +176,9 @@ void *
>>>>> wl_proxy_get_user_data(struct wl_proxy *proxy);
>>>>>
>>>>> uint32_t
>>>>> +wl_proxy_get_version(struct wl_proxy *proxy);
>>>>> +
>>>>> +uint32_t
>>>>> wl_proxy_get_id(struct wl_proxy *proxy);
>>>>>
>>>>> const char *
>>>>> diff --git a/src/wayland-client.c b/src/wayland-client.c
>>>>> index b1c600f..3fa342f 100644
>>>>> --- a/src/wayland-client.c
>>>>> +++ b/src/wayland-client.c
>>>>> @@ -62,6 +62,7 @@ struct wl_proxy {
>>>>> int refcount;
>>>>> void *user_data;
>>>>> wl_dispatcher_func_t dispatcher;
>>>>> + uint32_t version;
>>>>> };
>>>>>
>>>>> struct wl_global {
>>>>> @@ -326,7 +327,8 @@ wl_display_create_queue(struct wl_display *display)
>>>>> }
>>>>>
>>>>> static struct wl_proxy *
>>>>> -proxy_create(struct wl_proxy *factory, const struct wl_interface *interface)
>>>>> +proxy_create(struct wl_proxy *factory, const struct wl_interface *interface,
>>>>> + uint32_t version)
>>>>> {
>>>>> struct wl_proxy *proxy;
>>>>> struct wl_display *display = factory->display;
>>>>> @@ -341,6 +343,7 @@ proxy_create(struct wl_proxy *factory, const struct wl_interface *interface)
>>>>> proxy->display = display;
>>>>> proxy->queue = factory->queue;
>>>>> proxy->refcount = 1;
>>>>> + proxy->version = version;
>>>>>
>>>>> proxy->object.id = wl_map_insert_new(&display->objects, 0, proxy);
>>>>>
>>>>> @@ -373,7 +376,7 @@ wl_proxy_create(struct wl_proxy *factory, const struct wl_interface *interface)
>>>>> struct wl_proxy *proxy;
>>>>>
>>>>> pthread_mutex_lock(&display->mutex);
>>>>> - proxy = proxy_create(factory, interface);
>>>>> + proxy = proxy_create(factory, interface, factory->version);
>>>>> pthread_mutex_unlock(&display->mutex);
>>>>>
>>>>> return proxy;
>>>>> @@ -398,6 +401,7 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
>>>>> proxy->display = display;
>>>>> proxy->queue = factory->queue;
>>>>> proxy->refcount = 1;
>>>>> + proxy->version = factory->version;
>>>>>
>>>>> wl_map_insert_at(&display->objects, 0, id, proxy);
>>>>>
>>>>> @@ -529,7 +533,7 @@ wl_proxy_add_dispatcher(struct wl_proxy *proxy,
>>>>> static struct wl_proxy *
>>>>> create_outgoing_proxy(struct wl_proxy *proxy, const struct wl_message *message,
>>>>> union wl_argument *args,
>>>>> - const struct wl_interface *interface)
>>>>> + const struct wl_interface *interface, uint32_t version)
>>>>> {
>>>>> int i, count;
>>>>> const char *signature;
>>>>> @@ -543,7 +547,7 @@ create_outgoing_proxy(struct wl_proxy *proxy, const struct wl_message *message,
>>>>>
>>>>> switch (arg.type) {
>>>>> case 'n':
>>>>> - new_proxy = proxy_create(proxy, interface);
>>>>> + new_proxy = proxy_create(proxy, interface, version);
>>>>> if (new_proxy == NULL)
>>>>> return NULL;
>>>>>
>>>>> @@ -568,7 +572,8 @@ create_outgoing_proxy(struct wl_proxy *proxy, const struct wl_message *message,
>>>>> *
>>>>> * For new-id arguments, this function will allocate a new wl_proxy
>>>>> * and send the ID to the server. The new wl_proxy will be returned
>>>>> - * on success or NULL on errror with errno set accordingly.
>>>>> + * on success or NULL on errror with errno set accordingly. The newly
>>>>> + * created proxy will inherit their version from their parent.
>>>>> *
>>>>> * \note This is intended to be used by language bindings and not in
>>>>> * non-generated code.
>>>>> @@ -582,6 +587,43 @@ wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
>>>>> uint32_t opcode, union wl_argument *args,
>>>>> const struct wl_interface *interface)
>>>>> {
>>>>> + return wl_proxy_marshal_array_constructor_versioned(proxy, opcode,
>>>>> + args, interface,
>>>>> + proxy->version);
>>>>> +}
>>>>> +
>>>>> +
>>>>> +/** Prepare a request to be sent to the compositor
>>>>> + *
>>>>> + * \param proxy The proxy object
>>>>> + * \param opcode Opcode of the request to be sent
>>>>> + * \param args Extra arguments for the given request
>>>>> + * \param interface The interface to use for the new proxy
>>>>> + * \param version The protocol object version for the new proxy
>>>>> + *
>>>>> + * Translates the request given by opcode and the extra arguments into the
>>>>> + * wire format and write it to the connection buffer. This version takes an
>>>>> + * array of the union type wl_argument.
>>>>> + *
>>>>> + * For new-id arguments, this function will allocate a new wl_proxy
>>>>> + * and send the ID to the server. The new wl_proxy will be returned
>>>>> + * on success or NULL on errror with errno set accordingly. The newly
>>>>> + * created proxy will have the version specified.
>>>>> + *
>>>>> + * \note This is intended to be used by language bindings and not in
>>>>> + * non-generated code.
>>>>> + *
>>>>> + * \sa wl_proxy_marshal()
>>>>> + *
>>>>> + * \memberof wl_proxy
>>>>> + */
>>>>> +WL_EXPORT struct wl_proxy *
>>>>> +wl_proxy_marshal_array_constructor_versioned(struct wl_proxy *proxy,
>>>>> + uint32_t opcode,
>>>>> + union wl_argument *args,
>>>>> + const struct wl_interface *interface,
>>>>> + uint32_t version)
>>>>> +{
>>>>> struct wl_closure *closure;
>>>>> struct wl_proxy *new_proxy = NULL;
>>>>> const struct wl_message *message;
>>>>> @@ -591,7 +633,8 @@ wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
>>>>> message = &proxy->object.interface->methods[opcode];
>>>>> if (interface) {
>>>>> new_proxy = create_outgoing_proxy(proxy, message,
>>>>> - args, interface);
>>>>> + args, interface,
>>>>> + version);
>>>>> if (new_proxy == NULL)
>>>>> goto err_unlock;
>>>>> }
>>>>> @@ -663,7 +706,8 @@ wl_proxy_marshal(struct wl_proxy *proxy, uint32_t opcode, ...)
>>>>> *
>>>>> * For new-id arguments, this function will allocate a new wl_proxy
>>>>> * and send the ID to the server. The new wl_proxy will be returned
>>>>> - * on success or NULL on errror with errno set accordingly.
>>>>> + * on success or NULL on errror with errno set accordingly. The newly
>>>>> + * created proxy will inherit their version from their parent.
>>>>> *
>>>>> * \note This should not normally be used by non-generated code.
>>>>> *
>>>>> @@ -685,6 +729,46 @@ wl_proxy_marshal_constructor(struct wl_proxy *proxy, uint32_t opcode,
>>>>> args, interface);
>>>>> }
>>>>>
>>>>> +
>>>>> +/** Prepare a request to be sent to the compositor
>>>>> + *
>>>>> + * \param proxy The proxy object
>>>>> + * \param opcode Opcode of the request to be sent
>>>>> + * \param interface The interface to use for the new proxy
>>>>> + * \param version The protocol object version of the new proxy
>>>>> + * \param ... Extra arguments for the given request
>>>>> + * \return A new wl_proxy for the new_id argument or NULL on error
>>>>> + *
>>>>> + * Translates the request given by opcode and the extra arguments into the
>>>>> + * wire format and write it to the connection buffer.
>>>>> + *
>>>>> + * For new-id arguments, this function will allocate a new wl_proxy
>>>>> + * and send the ID to the server. The new wl_proxy will be returned
>>>>> + * on success or NULL on errror with errno set accordingly. The newly
>>>>> + * created proxy will have the version specified.
>>>>> + *
>>>>> + * \note This should not normally be used by non-generated code.
>>>>> + *
>>>>> + * \memberof wl_proxy
>>>>> + */
>>>>> +WL_EXPORT struct wl_proxy *
>>>>> +wl_proxy_marshal_constructor_versioned(struct wl_proxy *proxy, uint32_t opcode,
>>>>> + const struct wl_interface *interface,
>>>>> + uint32_t version, ...)
>>>>> +{
>>>>> + union wl_argument args[WL_CLOSURE_MAX_ARGS];
>>>>> + va_list ap;
>>>>> +
>>>>> + va_start(ap, version);
>>>>> + wl_argument_from_va_list(proxy->object.interface->methods[opcode].signature,
>>>>> + args, WL_CLOSURE_MAX_ARGS, ap);
>>>>> + va_end(ap);
>>>>> +
>>>>> + return wl_proxy_marshal_array_constructor_versioned(proxy, opcode,
>>>>> + args, interface,
>>>>> + version);
>>>>> +}
>>>>> +
>>>>> /** Prepare a request to be sent to the compositor
>>>>> *
>>>>> * \param proxy The proxy object
>>>>> @@ -848,6 +932,7 @@ wl_display_connect_to_fd(int fd)
>>>>> display->proxy.queue = &display->default_queue;
>>>>> display->proxy.flags = 0;
>>>>> display->proxy.refcount = 1;
>>>>> + display->proxy.version = 1;
>>>>>
>>>>> display->connection = wl_connection_create(display->fd);
>>>>> if (display->connection == NULL)
>>>>> @@ -1839,6 +1924,19 @@ wl_proxy_get_user_data(struct wl_proxy *proxy)
>>>>> return proxy->user_data;
>>>>> }
>>>>>
>>>>> +/** Get the protocol object version of a proxy object
>>>>> + *
>>>>> + * \param proxy The proxy object
>>>>> + * \return The protocol object version of the proxy
>>>>> + *
>>>>> + * \memberof wl_proxy
>>>>> + */
>>>>> +WL_EXPORT uint32_t
>>>>> +wl_proxy_get_version(struct wl_proxy *proxy)
>>>>> +{
>>>>> + return proxy->version;
>>>>> +}
>>>>> +
>>>>> /** Get the id of a proxy object
>>>>> *
>>>>> * \param proxy The proxy object
>>>>> --
>>>>> 2.6.2
>>>>>
>>>>> _______________________________________________
>>>>> wayland-devel mailing list
>>>>> wayland-devel at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>> _______________________________________________
>>> wayland-devel mailing list
>>> wayland-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>>
>>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list