Discussion:
[PATCH 0/3][RFC] introducing inert objects to allow to release server-side objects
(too old to reply)
David FORT
2015-02-25 14:03:32 UTC
Permalink
This serie of patches tries to address the problem of removing a seat and really deleting
it server-side. First I have added the corresponding method in the protocol definition.
Second I have modified the scanner to emit some code to create inert objects: objects
with requests that do nothing, and only return inert objects.
This is a preliminary work and comments are more than welcome.

David FORT (3):
Modify wayland-scanner to generate inert objects
Add a release request on wl_seat
wayland.xml: fixed a typo

protocol/wayland.xml | 8 ++-
src/scanner.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++++---
wayland-scanner.mk | 3 +
3 files changed, 173 insertions(+), 11 deletions(-)
--
1.9.1
David FORT
2015-02-25 14:03:33 UTC
Permalink
From: Hardening <***@gmail.com>

---
protocol/wayland.xml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 2a49805..4fb8035 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1389,7 +1389,7 @@
<arg name="id" type="new_id" interface="wl_touch"/>
</request>

- <!-- Version 2 of additions -->
+ <!-- Version 2 additions -->

<event name="name" since="2">
<description summary="unique identifier for this seat">
--
1.9.1
Bryce Harrington
2015-02-25 20:38:48 UTC
Permalink
Post by David FORT
---
protocol/wayland.xml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Pushed this trivial fix.

f3a5ca9..99d2775 master -> master
Post by David FORT
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 2a49805..4fb8035 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1389,7 +1389,7 @@
<arg name="id" type="new_id" interface="wl_touch"/>
</request>
- <!-- Version 2 of additions -->
+ <!-- Version 2 additions -->
<event name="name" since="2">
<description summary="unique identifier for this seat">
--
1.9.1
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
David FORT
2015-02-25 14:03:35 UTC
Permalink
This is required if we want to correctly remove a wl_seat server-side.
---
protocol/wayland.xml | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 4fb8035..8f63ebf 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1400,6 +1400,12 @@
<arg name="name" type="string"/>
</event>

+ <!-- Version 3 additions -->
+
+ <request name="release" type="destructor" since="3">
+ <description summary="release the seat object"/>
+ </request>
+
</interface>

<interface name="wl_pointer" version="3">
--
1.9.1
Derek Foreman
2015-09-23 16:17:33 UTC
Permalink
Post by David FORT
This is required if we want to correctly remove a wl_seat server-side.
---
protocol/wayland.xml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 4fb8035..8f63ebf 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1400,6 +1400,12 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 3 additions -->
+
+ <request name="release" type="destructor" since="3">
+ <description summary="release the seat object"/>
+ </request>
+
wl_seat appears to currently be version="4", so I guess we'd have to
bump the since= and the version in the comment... (and probably the
version of wl_seat as well)

Unfortunately, I don't think this is necessary or sufficient to solve
the problem at hand (RDP compositor connections are new seats, once apps
bind these seats they're essentially leaked forever even after RDP
client disconnect)

It's unnecessary because we already can remove seats once all clients
using them close (I don't feel this is realistic, though)

It's insufficient because we don't have a way to tell a client the seat
it currently has isn't going to provide any more input ever.

However, I think if we're ever going to kill this problem we'll need
this destructor, so if the version numbers are updated appropriately:

Reviewed-by: Derek Foreman <***@osg.samsung.com>

We'll still need a way to tell the client the seat it has is
dead/inert/whatever. I don't think we can rely on the existing
capabilities being empty, since with libinput backed seats it may be
worthwhile for a toolkit/app to keep those seats to retain state in case
the devices are plugged back in.

That is, there's a difference between a disconnected RDP client and a
libinput backed seat with no devices left in it.

Would a new seat event (say, "removed") solve this sufficiently?
Post by David FORT
</interface>
<interface name="wl_pointer" version="3">
Jonas Ådahl
2015-09-23 23:59:06 UTC
Permalink
Post by Derek Foreman
Post by David FORT
This is required if we want to correctly remove a wl_seat server-side.
---
protocol/wayland.xml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 4fb8035..8f63ebf 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1400,6 +1400,12 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 3 additions -->
+
+ <request name="release" type="destructor" since="3">
+ <description summary="release the seat object"/>
+ </request>
+
wl_seat appears to currently be version="4", so I guess we'd have to
bump the since= and the version in the comment... (and probably the
version of wl_seat as well)
Yes. And we should bump the version of wl_pointer, wl_touch and
wl_keyboard as part of it, just to make things clearer.
Post by Derek Foreman
Unfortunately, I don't think this is necessary or sufficient to solve
the problem at hand (RDP compositor connections are new seats, once apps
bind these seats they're essentially leaked forever even after RDP
client disconnect)
It's unnecessary because we already can remove seats once all clients
using them close (I don't feel this is realistic, though)
It's insufficient because we don't have a way to tell a client the seat
it currently has isn't going to provide any more input ever.
However, I think if we're ever going to kill this problem we'll need
We'll still need a way to tell the client the seat it has is
dead/inert/whatever. I don't think we can rely on the existing
capabilities being empty, since with libinput backed seats it may be
worthwhile for a toolkit/app to keep those seats to retain state in case
the devices are plugged back in.
That is, there's a difference between a disconnected RDP client and a
libinput backed seat with no devices left in it.
Would a new seat event (say, "removed") solve this sufficiently?
Wouldn't the global advertisement being removed be enough? Every seat is
advertised individually in the registry, and removing it there should
IMO be interpreted as there no longer any use for objects that was bound
to the that global.


Jonas
Post by Derek Foreman
Post by David FORT
</interface>
<interface name="wl_pointer" version="3">
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Hardening
2015-09-27 20:51:40 UTC
Permalink
Post by Jonas Ådahl
Post by Derek Foreman
Post by David FORT
This is required if we want to correctly remove a wl_seat server-side.
---
protocol/wayland.xml | 6 ++++++
1 file changed, 6 insertions(+)
Sorry very late reply.

[...]
Post by Jonas Ådahl
Post by Derek Foreman
Post by David FORT
+ <!-- Version 3 additions -->
+
+ <request name="release" type="destructor" since="3">
+ <description summary="release the seat object"/>
+ </request>
+
wl_seat appears to currently be version="4", so I guess we'd have to
bump the since= and the version in the comment... (and probably the
version of wl_seat as well)
Yes. And we should bump the version of wl_pointer, wl_touch and
wl_keyboard as part of it, just to make things clearer.
Does it mean that that wl_pointer, wl_touch and wl_keyboard should all
have the same version as wl_seat ?
Post by Jonas Ådahl
Post by Derek Foreman
Unfortunately, I don't think this is necessary or sufficient to solve
the problem at hand (RDP compositor connections are new seats, once apps
bind these seats they're essentially leaked forever even after RDP
client disconnect)
[...]
Post by Jonas Ådahl
Post by Derek Foreman
That is, there's a difference between a disconnected RDP client and a
libinput backed seat with no devices left in it.
Would a new seat event (say, "removed") solve this sufficiently?
Wouldn't the global advertisement being removed be enough? Every seat is
advertised individually in the registry, and removing it there should
IMO be interpreted as there no longer any use for objects that was bound
to the that global.
Nope because the client can send a request on the seat while the
compositor is announcing the global removal. The release request is
there so that the client can say tto he compositor "understood I won't
use that object anymore". That allows the compositor to ref count users
and free it at the end.
--
David FORT
website: http://www.hardening-consulting.com/
Jonas Ådahl
2015-09-29 07:49:31 UTC
Permalink
Post by Hardening
Post by Jonas Ådahl
Post by Derek Foreman
Post by David FORT
This is required if we want to correctly remove a wl_seat server-side.
---
protocol/wayland.xml | 6 ++++++
1 file changed, 6 insertions(+)
Sorry very late reply.
[...]
Post by Jonas Ådahl
Post by Derek Foreman
Post by David FORT
+ <!-- Version 3 additions -->
+
+ <request name="release" type="destructor" since="3">
+ <description summary="release the seat object"/>
+ </request>
+
wl_seat appears to currently be version="4", so I guess we'd have to
bump the since= and the version in the comment... (and probably the
version of wl_seat as well)
Yes. And we should bump the version of wl_pointer, wl_touch and
wl_keyboard as part of it, just to make things clearer.
Does it mean that that wl_pointer, wl_touch and wl_keyboard should all
have the same version as wl_seat ?
Yes they always have the same version. It is not enforced in the XML,
or by the scanner, but it's that way anyway just because how version
inheritance work.
Post by Hardening
Post by Jonas Ådahl
Post by Derek Foreman
Unfortunately, I don't think this is necessary or sufficient to solve
the problem at hand (RDP compositor connections are new seats, once apps
bind these seats they're essentially leaked forever even after RDP
client disconnect)
[...]
Post by Jonas Ådahl
Post by Derek Foreman
That is, there's a difference between a disconnected RDP client and a
libinput backed seat with no devices left in it.
Would a new seat event (say, "removed") solve this sufficiently?
Wouldn't the global advertisement being removed be enough? Every seat is
advertised individually in the registry, and removing it there should
IMO be interpreted as there no longer any use for objects that was bound
to the that global.
Nope because the client can send a request on the seat while the
compositor is announcing the global removal. The release request is
there so that the client can say tto he compositor "understood I won't
use that object anymore". That allows the compositor to ref count users
and free it at the end.
That is what I wrote. The original question, if I did not misunderstand,
was whether a "remove" event would be needed. The "release" request is
for the server not to leak the object though, that is true.


Jonas
Post by Hardening
--
David FORT
website: http://www.hardening-consulting.com/
David FORT
2015-09-29 07:43:01 UTC
Permalink
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means it won't use that object anymore). When all clients have released the
wl_seat, it can be destroyed by the compositor.
---
protocol/wayland.xml | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..2c7a675 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>

- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,16 @@
<arg name="name" type="string"/>
</event>

+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ This request is called by the client to inform that it will not address the seat
+ object. The compositor can destroy this seat when all clients have called
+ this request.
+ </description>
+ </request>
+
</interface>

<interface name="wl_pointer" version="3">
--
1.9.1
Jonas Ådahl
2015-09-29 07:58:21 UTC
Permalink
Post by David FORT
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means it won't use that object anymore). When all clients have released the
wl_seat, it can be destroyed by the compositor.
---
protocol/wayland.xml | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..2c7a675 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>
- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
Could you bump the other interfaces that will get this version anyway
as well? As mentioned, they will have this version anyway, when created.
Post by David FORT
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,16 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ This request is called by the client to inform that it will not address the seat
+ object. The compositor can destroy this seat when all clients have called
+ this request.
I don't think we should dictate how the implementation should look like.
The compositor may have destroyed the seat already, leaving only defunct
wl_seat objects around.

Not sure what "address" means here. "Use" might be more clear, since we
use that terminology elsewhere. Also using "this" instead of "the" to
address the object might be more clear as well.


Jonas
Post by David FORT
+ </description>
+ </request>
+
</interface>
<interface name="wl_pointer" version="3">
--
1.9.1
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Hardening
2015-09-29 08:16:46 UTC
Permalink
Post by Jonas Ådahl
Post by David FORT
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means it won't use that object anymore). When all clients have released the
wl_seat, it can be destroyed by the compositor.
---
protocol/wayland.xml | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..2c7a675 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>
- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
Could you bump the other interfaces that will get this version anyway
as well? As mentioned, they will have this version anyway, when created.
Perhaps I missed something but I have just discussed this with Pekka,
and he told me not to bump the version from wl_pointer and al...
Post by Jonas Ådahl
Post by David FORT
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,16 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ This request is called by the client to inform that it will not address the seat
+ object. The compositor can destroy this seat when all clients have called
+ this request.
I don't think we should dictate how the implementation should look like.
The compositor may have destroyed the seat already, leaving only defunct
wl_seat objects around.
I didn't mean to dictate the implementation, I'll try to find something
more neutral.
Post by Jonas Ådahl
Not sure what "address" means here. "Use" might be more clear, since we
use that terminology elsewhere. Also using "this" instead of "the" to
address the object might be more clear as well.
Ok I will change the description this way.
--
David FORT
website: http://www.hardening-consulting.com/
David FORT
2015-09-30 20:17:48 UTC
Permalink
Compared to the first version, I changed the description according to
Jonas's comment. No real consensus have emerged on IRC or on the mailing
list about version numbering. So I choosed to keep them as it is done now.

David FORT (1):
Add a release request on wl_seat

protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
--
1.9.1
David FORT
2015-09-30 20:17:49 UTC
Permalink
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means I won't use that object anymore). When all clients have released the
wl_seat, it can be destroyed by the compositor.
---
protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..6764565 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>

- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,15 @@
<arg name="name" type="string"/>
</event>

+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ This request is called by the client to inform that it will not use the seat
+ object.
+ </description>
+ </request>
+
</interface>

<interface name="wl_pointer" version="3">
--
1.9.1
Jonas Ådahl
2015-10-01 07:50:35 UTC
Permalink
Post by David FORT
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means I won't use that object anymore). When all clients have released the
wl_seat, it can be destroyed by the compositor.
Missing Signed-off-by.

The last part of the commit message is not really true. A compositor can
destroy its "seat" long before any client has acknowledged it's gone (and
deal with this fact in the interface implementation), so this is really
just about being able to destroy the wl_resource and nothing else. There
is no need to wait for all clients to do anything really (Sorry I
missed the commit message part in the last patch).
Post by David FORT
---
protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..6764565 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>
- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,15 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ This request is called by the client to inform that it will not use the seat
+ object.
nit: "The" seat object sounds like there is only one seat object. But
this request is about that the client will stop using *this* seat object.
Less important but IMHO, it would also sound better if it was more clear
that it will not use this seat *anymore*.

With the commit message fixed and the nit above fixed, this is
Reviewed-by: Jonas Ådahl <***@gmail.com>


Jonas
Post by David FORT
+ </description>
+ </request>
+
</interface>
<interface name="wl_pointer" version="3">
--
1.9.1
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
David FORT
2015-10-02 12:20:11 UTC
Permalink
This version takes in account last Jonas Ådahl's remarks.


David FORT (1):
wayland: add a release request on wl_seat

protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
--
1.9.1
David FORT
2015-10-02 12:20:12 UTC
Permalink
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means I won't use that object anymore).
---
protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..59819e9 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>

- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,15 @@
<arg name="name" type="string"/>
</event>

+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ Using this request client can tell the server that it is not going to
+ use the seat object anymore.
+ </description>
+ </request>
+
</interface>

<interface name="wl_pointer" version="3">
--
1.9.1
Derek Foreman
2015-10-05 17:24:19 UTC
Permalink
Post by David FORT
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means I won't use that object anymore).
Acked-by: Derek Foreman <***@osg.samsung.com>

Looks good to me, though you appear to have used a tab to indent one
line and spaces on the line just after?

This actually seems to fit with the current state of that file - does
anyone know what the intended formatting paradigm is here? :)
Post by David FORT
---
protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..59819e9 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>
- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,15 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ Using this request client can tell the server that it is not going to
+ use the seat object anymore.
+ </description>
+ </request>
+
</interface>
<interface name="wl_pointer" version="3">
Bryce Harrington
2015-10-08 00:44:05 UTC
Permalink
Post by Derek Foreman
Post by David FORT
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means I won't use that object anymore).
Looks good to me, though you appear to have used a tab to indent one
line and spaces on the line just after?
This actually seems to fit with the current state of that file - does
anyone know what the intended formatting paradigm is here? :)
Last time I studied it, it appeared the .xml files use 4-space
indentations, substituted with tab characters to fill to column 8.

I'm not sure we're really all that strict on the indentation for the
.xml files like we are with the .c files, and there's likely a fair bit
of inconsistency in the existing files. But I try to clean up
formatting when my editor flags it. This is what I use in my
.dir-locals.el for editing wayland files:

;; The 'nil' configuration applies to all modes.
((nil .
;; Globally
((indent-tabs-mode . t)
(c-basic-offset . 4)
(tab-width . 4)
))

;; Mode overrides
(c-mode . (
;; Highlight leading space characters
(eval . (highlight-regexp "^ +[^*]"))
(c-basic-offset . 8)
(tab-width . 8)
)
))

Honestly I don't know why we have one indent rule for .c files and
apparently something different for .xml, but there you go and spare
me the emacs snark, I can tell what you're thinking right now.

Bryce
Post by Derek Foreman
Post by David FORT
---
protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..59819e9 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>
- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,15 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ Using this request client can tell the server that it is not going to
+ use the seat object anymore.
+ </description>
+ </request>
+
</interface>
<interface name="wl_pointer" version="3">
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Bryce Harrington
2015-10-08 01:55:47 UTC
Permalink
Post by Derek Foreman
Post by David FORT
This is required if we want to correctly remove a wl_seat compositor-side. A
wl_seat is announced as a global object, then it is bound by the client. When
the compositor wants to remove the seat, it shall announce the global removal of
the object. The client can then call the release request on the wl_seat (which
means I won't use that object anymore).
Looks good to me, though you appear to have used a tab to indent one
line and spaces on the line just after?
This actually seems to fit with the current state of that file - does
anyone know what the intended formatting paradigm is here? :)
I've gone ahead and pushed this since it's gotten adequate review IMHO.

remote: Updating patchwork state for http://patchwork.freedesktop.org/project/wayland/list/
remote: I: patch #60935 updated using rev b6809e5a8051fd193fe4b041a3245c22432195db
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/wayland/wayland
99c34e5..b6809e5 master -> master

Whitespace cleanup for indentation of the xml files would be fine as a followup.

Bryce
Post by Derek Foreman
Post by David FORT
---
protocol/wayland.xml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index 42c9309..59819e9 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1329,7 +1329,7 @@
</request>
</interface>
- <interface name="wl_seat" version="4">
+ <interface name="wl_seat" version="5">
<description summary="group of input devices">
A seat is a group of keyboards, pointer and touch devices. This
object is published as a global during start up, or when such a
@@ -1400,6 +1400,15 @@
<arg name="name" type="string"/>
</event>
+ <!-- Version 5 additions -->
+
+ <request name="release" type="destructor" since="5">
+ <description summary="release the seat object">
+ Using this request client can tell the server that it is not going to
+ use the seat object anymore.
+ </description>
+ </request>
+
</interface>
<interface name="wl_pointer" version="3">
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
David FORT
2015-02-25 14:03:34 UTC
Permalink
As stated in the very good blog post[1] of Pekka Paalanen, server-side we can have
sometime troubles with object that the server has deleted but that the client
is still requesting. This patch complements the scanner to create some code
that will return inert objects, ie objects that will do nothing and will never
return anything else but an inert object. An example is a get_pointer() on
a wl_seat, with an unplugged mouse (and so no pointer). Instead of keeping alive
the old pointer, we could bind that inert object and wait that the client release
it (which should happen very quickly as a wl_global_remove should be in the wire).

[1]: http://ppaalanen.blogspot.fi/2014/07/wayland-protocol-design-object-lifespan.html
---
src/scanner.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++----
wayland-scanner.mk | 3 +
2 files changed, 166 insertions(+), 10 deletions(-)

diff --git a/src/scanner.c b/src/scanner.c
index 1f1e59a..dbdd1f6 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -39,7 +39,7 @@ enum side {
static int
usage(int ret)
{
- fprintf(stderr, "usage: ./scanner [client-header|server-header|code]\n");
+ fprintf(stderr, "usage: ./scanner [client-header|server-header|code|server-inert]\n");
fprintf(stderr, "\n");
fprintf(stderr, "Converts XML protocol descriptions supplied on "
"stdin to client headers,\n"
@@ -626,6 +626,18 @@ emit_type(struct arg *a)
}
}

+static struct arg *
+get_return_type(struct message *m)
+{
+ struct arg *a, *ret = NULL;
+
+ wl_list_for_each(a, &m->arg_list, link) {
+ if (a->type == NEW_ID)
+ ret = a;
+ }
+ return ret;
+}
+
static void
emit_stubs(struct wl_list *message_list, struct interface *interface)
{
@@ -688,11 +700,7 @@ emit_stubs(struct wl_list *message_list, struct interface *interface)
continue;
}

- ret = NULL;
- wl_list_for_each(a, &m->arg_list, link) {
- if (a->type == NEW_ID)
- ret = a;
- }
+ ret = get_return_type(m);

if (ret && ret->interface_name == NULL)
printf("static inline void *\n");
@@ -1103,8 +1111,7 @@ emit_types(struct protocol *protocol, struct wl_list *message_list)
case NEW_ID:
case OBJECT:
if (a->interface_name)
- printf("\t&%s_interface,\n",
- a->interface_name);
+ printf("\t&%s_interface,\n", a->interface_name);
else
printf("\tNULL,\n");
break;
@@ -1248,6 +1255,140 @@ emit_code(struct protocol *protocol)
}
}

+static void
+emit_inert_request(struct protocol *protocol, struct interface *interface, struct wl_list *message_list)
+{
+ struct message *m;
+ struct arg *a, *ret;
+ const char *newid_name;
+
+ wl_list_for_each(m, message_list, link) {
+ ret = get_return_type(m);
+
+ /* forward declaration for the returned object */
+ if (ret && ret->interface_name) {
+ printf("static void\n"
+ "create_inert_%s(struct wl_client *client, uint32_t version, uint32_t id);\n"
+ "\n",
+ ret->interface_name);
+ }
+
+ printf("static inline void inert_%s_%s(struct wl_client *client, struct wl_resource *resource",
+ interface->name, m->name
+ );
+
+ wl_list_for_each(a, &m->arg_list, link) {
+ if (a->type == NEW_ID) {
+ newid_name = a->name;
+
+ if (a->interface_name == NULL) {
+ printf(", const struct wl_interface *interface"
+ ", uint32_t version");
+ continue;
+ }
+ }
+
+ if (a->type == OBJECT) {
+ printf(", struct wl_resource *");
+ } else {
+ printf(", ");
+ emit_type(a);
+ }
+ printf("%s", a->name);
+ }
+
+ printf(")\n"
+ "{\n");
+
+ if (ret && ret->interface_name) {
+ printf("\tcreate_inert_%s(client, wl_resource_get_version(resource), %s);\n",
+ ret->interface_name, newid_name);
+ } else if (m->destructor) {
+ printf("\twl_resource_destroy(resource);\n");
+ }
+ printf("}\n\n");
+ }
+}
+
+static void
+emit_inert_interface(struct protocol *protocol, struct interface *i, struct wl_array *types) {
+ struct message *m;
+
+
+ if(wl_list_length(&i->request_list)) {
+ emit_inert_request(protocol, i, &i->request_list);
+
+ printf ("static const struct %s_interface inert_%s_implementation = {\n",
+ i->name, i->name);
+ wl_list_for_each(m, &i->request_list, link) {
+ printf("\tinert_%s_%s,\n", i->name, m->name);
+ }
+ printf ("};\n"
+ "\n");
+ }
+
+ printf("static inline void\n"
+ "create_inert_%s(struct wl_client *client, uint32_t version, uint32_t id) {\n",
+ i->name
+ );
+
+ if(wl_list_length(&i->request_list)) {
+ /* emit the method body only when there is requests on the object */
+ printf("\tstruct wl_resource *resource;\n"
+ "\tresource = wl_resource_create(client, &%s_interface, MIN(version, %d), id);\n"
+ "\twl_resource_set_implementation(resource, &inert_%s_implementation, NULL, wl_resource_destroy);\n",
+ i->name, i->version, i->name);
+ }
+
+ printf("}\n"
+ "\n");
+
+}
+
+
+static
+void emit_inert(struct protocol *protocol) {
+ struct interface *i;
+ struct wl_array types;
+
+ if (protocol->copyright)
+ format_copyright(protocol->copyright);
+
+ printf("#ifndef __%s_SERVER_INERT__\n"
+ "#define __%s_SERVER_INERT__\n"
+ "\n"
+ "#include \"%s-server-protocol.h\"\n"
+ "\n"
+ "#ifdef __cplusplus\n"
+ "extern \"C\" {\n"
+ "#endif\n"
+ "\n"
+ "#ifndef MIN\n"
+ "#define MIN(x,y) (((x) < (y)) ? (x) : (y))\n"
+ "#endif\n"
+ "\n",
+ protocol->uppercase_name,
+ protocol->uppercase_name,
+ //protocol->name,
+ protocol->name
+ );
+
+ wl_list_for_each(i, &protocol->interface_list, link) {
+ if (!strcmp(i->name, "wl_registry") || !strcmp(i->name, "wl_display"))
+ continue;
+
+ emit_inert_interface(protocol, i, &types);
+ }
+
+ printf("\n"
+ "#ifdef __cplusplus\n"
+ "}\n"
+ "#endif\n"
+ "\n"
+ "#endif\n");
+
+}
+
int main(int argc, char *argv[])
{
struct parse_context ctx;
@@ -1257,17 +1398,26 @@ int main(int argc, char *argv[])
enum {
CLIENT_HEADER,
SERVER_HEADER,
+ SERVER_INERT,
CODE,
} mode;

- if (argc != 2)
+ if (argc < 2) {
usage(EXIT_FAILURE);
- else if (strcmp(argv[1], "help") == 0 || strcmp(argv[1], "--help") == 0)
+ }
+
+ if (argc > 2) {
+ stdin = fopen(argv[2], "r");
+ }
+
+ if (strcmp(argv[1], "help") == 0 || strcmp(argv[1], "--help") == 0)
usage(EXIT_SUCCESS);
else if (strcmp(argv[1], "client-header") == 0)
mode = CLIENT_HEADER;
else if (strcmp(argv[1], "server-header") == 0)
mode = SERVER_HEADER;
+ else if (strcmp(argv[1], "server-inert") == 0)
+ mode = SERVER_INERT;
else if (strcmp(argv[1], "code") == 0)
mode = CODE;
else
@@ -1317,6 +1467,9 @@ int main(int argc, char *argv[])
case SERVER_HEADER:
emit_header(&protocol, SERVER);
break;
+ case SERVER_INERT:
+ emit_inert(&protocol);
+ break;
case CODE:
emit_code(&protocol);
break;
diff --git a/wayland-scanner.mk b/wayland-scanner.mk
index 0a72062..fd994e0 100644
--- a/wayland-scanner.mk
+++ b/wayland-scanner.mk
@@ -6,3 +6,6 @@

%-client-protocol.h : $(wayland_protocoldir)/%.xml
$(AM_V_GEN)$(wayland_scanner) client-header < $< > $@
+
+%-server-inert.h: $(wayland_protocoldir)/%.xml
+ $(AM_V_GEN)$(wayland_scanner) server-inert < $< > $@
\ No newline at end of file
--
1.9.1
Marek Chalupa
2015-03-12 12:31:30 UTC
Permalink
I wonder if the same effect could be achieved by few smaller changes. What
I'm thinking about is something like:

wl_resource_set_intact()

So far I've got three ways how to achieve that:
1) change scanner so that it saves opcode of destructor events in
wl_interface, so when we invoke a closure, we can bail out if the resource
is intact and it is not a destructor
2) do the same but with wl_message

These first two options would break ABI, so I'm not fond of these.

3) add a sign to the siganture that the message is a destructor.

I went ahead and tried the last one locally. Generally, it work like this:

get_closure()
if (resource_is_intact && message_is_not_destructor)
return
else
invoke

(message_is_not_destructor <=> signature does not contain 'D')

This approach does not break ABI and is nicely compatible with old
protocols. It only adds one function wl_resource_set_intact() (thus changes
API) and the protocols must be re-generated and re-linked, but since it is
backward-compatible it is not crucial dependency (old protocols will work,
but message_is_not_destructor always return true)

What do you think?

Cheers,
Marek
Post by David FORT
As stated in the very good blog post[1] of Pekka Paalanen, server-side we can have
sometime troubles with object that the server has deleted but that the client
is still requesting. This patch complements the scanner to create some code
that will return inert objects, ie objects that will do nothing and will never
return anything else but an inert object. An example is a get_pointer() on
a wl_seat, with an unplugged mouse (and so no pointer). Instead of keeping alive
the old pointer, we could bind that inert object and wait that the client release
it (which should happen very quickly as a wl_global_remove should be in the wire).
http://ppaalanen.blogspot.fi/2014/07/wayland-protocol-design-object-lifespan.html
---
src/scanner.c | 173
+++++++++++++++++++++++++++++++++++++++++++++++++----
wayland-scanner.mk | 3 +
2 files changed, 166 insertions(+), 10 deletions(-)
diff --git a/src/scanner.c b/src/scanner.c
index 1f1e59a..dbdd1f6 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -39,7 +39,7 @@ enum side {
static int
usage(int ret)
{
- fprintf(stderr, "usage: ./scanner
[client-header|server-header|code]\n");
+ fprintf(stderr, "usage: ./scanner
[client-header|server-header|code|server-inert]\n");
fprintf(stderr, "\n");
fprintf(stderr, "Converts XML protocol descriptions supplied on "
"stdin to client headers,\n"
@@ -626,6 +626,18 @@ emit_type(struct arg *a)
}
}
+static struct arg *
+get_return_type(struct message *m)
+{
+ struct arg *a, *ret = NULL;
+
+ wl_list_for_each(a, &m->arg_list, link) {
+ if (a->type == NEW_ID)
+ ret = a;
+ }
+ return ret;
+}
+
static void
emit_stubs(struct wl_list *message_list, struct interface *interface)
{
@@ -688,11 +700,7 @@ emit_stubs(struct wl_list *message_list, struct interface *interface)
continue;
}
- ret = NULL;
- wl_list_for_each(a, &m->arg_list, link) {
- if (a->type == NEW_ID)
- ret = a;
- }
+ ret = get_return_type(m);
if (ret && ret->interface_name == NULL)
printf("static inline void *\n");
@@ -1103,8 +1111,7 @@ emit_types(struct protocol *protocol, struct wl_list *message_list)
if (a->interface_name)
- printf("\t&%s_interface,\n",
- a->interface_name);
+ printf("\t&%s_interface,\n",
a->interface_name);
else
printf("\tNULL,\n");
break;
@@ -1248,6 +1255,140 @@ emit_code(struct protocol *protocol)
}
}
+static void
+emit_inert_request(struct protocol *protocol, struct interface
*interface, struct wl_list *message_list)
+{
+ struct message *m;
+ struct arg *a, *ret;
+ const char *newid_name;
+
+ wl_list_for_each(m, message_list, link) {
+ ret = get_return_type(m);
+
+ /* forward declaration for the returned object */
+ if (ret && ret->interface_name) {
+ printf("static void\n"
+ "create_inert_%s(struct wl_client
*client, uint32_t version, uint32_t id);\n"
+ "\n",
+ ret->interface_name);
+ }
+
+ printf("static inline void inert_%s_%s(struct wl_client
*client, struct wl_resource *resource",
+ interface->name, m->name
+ );
+
+ wl_list_for_each(a, &m->arg_list, link) {
+ if (a->type == NEW_ID) {
+ newid_name = a->name;
+
+ if (a->interface_name == NULL) {
+ printf(", const struct
wl_interface *interface"
+ ", uint32_t version");
+ continue;
+ }
+ }
+
+ if (a->type == OBJECT) {
+ printf(", struct wl_resource *");
+ } else {
+ printf(", ");
+ emit_type(a);
+ }
+ printf("%s", a->name);
+ }
+
+ printf(")\n"
+ "{\n");
+
+ if (ret && ret->interface_name) {
+ printf("\tcreate_inert_%s(client,
wl_resource_get_version(resource), %s);\n",
+ ret->interface_name, newid_name);
+ } else if (m->destructor) {
+ printf("\twl_resource_destroy(resource);\n");
+ }
+ printf("}\n\n");
+ }
+}
+
+static void
+emit_inert_interface(struct protocol *protocol, struct interface *i,
struct wl_array *types) {
+ struct message *m;
+
+
+ if(wl_list_length(&i->request_list)) {
+ emit_inert_request(protocol, i, &i->request_list);
+
+ printf ("static const struct %s_interface
inert_%s_implementation = {\n",
+ i->name, i->name);
+ wl_list_for_each(m, &i->request_list, link) {
+ printf("\tinert_%s_%s,\n", i->name, m->name);
+ }
+ printf ("};\n"
+ "\n");
+ }
+
+ printf("static inline void\n"
+ "create_inert_%s(struct wl_client *client,
uint32_t version, uint32_t id) {\n",
+ i->name
+ );
+
+ if(wl_list_length(&i->request_list)) {
+ /* emit the method body only when there is requests on the
object */
+ printf("\tstruct wl_resource *resource;\n"
+ "\tresource = wl_resource_create(client,
&%s_interface, MIN(version, %d), id);\n"
+
"\twl_resource_set_implementation(resource, &inert_%s_implementation,
NULL, wl_resource_destroy);\n",
+ i->name, i->version, i->name);
+ }
+
+ printf("}\n"
+ "\n");
+
+}
+
+
+static
+void emit_inert(struct protocol *protocol) {
+ struct interface *i;
+ struct wl_array types;
+
+ if (protocol->copyright)
+ format_copyright(protocol->copyright);
+
+ printf("#ifndef __%s_SERVER_INERT__\n"
+ "#define __%s_SERVER_INERT__\n"
+ "\n"
+ "#include \"%s-server-protocol.h\"\n"
+ "\n"
+ "#ifdef __cplusplus\n"
+ "extern \"C\" {\n"
+ "#endif\n"
+ "\n"
+ "#ifndef MIN\n"
+ "#define MIN(x,y) (((x) < (y)) ? (x) : (y))\n"
+ "#endif\n"
+ "\n",
+ protocol->uppercase_name,
+ protocol->uppercase_name,
+ //protocol->name,
+ protocol->name
+ );
+
+ wl_list_for_each(i, &protocol->interface_list, link) {
+ if (!strcmp(i->name, "wl_registry") || !strcmp(i->name,
"wl_display"))
+ continue;
+
+ emit_inert_interface(protocol, i, &types);
+ }
+
+ printf("\n"
+ "#ifdef __cplusplus\n"
+ "}\n"
+ "#endif\n"
+ "\n"
+ "#endif\n");
+
+}
+
int main(int argc, char *argv[])
{
struct parse_context ctx;
@@ -1257,17 +1398,26 @@ int main(int argc, char *argv[])
enum {
CLIENT_HEADER,
SERVER_HEADER,
+ SERVER_INERT,
CODE,
} mode;
- if (argc != 2)
+ if (argc < 2) {
usage(EXIT_FAILURE);
- else if (strcmp(argv[1], "help") == 0 || strcmp(argv[1], "--help")
== 0)
+ }
+
+ if (argc > 2) {
+ stdin = fopen(argv[2], "r");
+ }
+
+ if (strcmp(argv[1], "help") == 0 || strcmp(argv[1], "--help") == 0)
usage(EXIT_SUCCESS);
else if (strcmp(argv[1], "client-header") == 0)
mode = CLIENT_HEADER;
else if (strcmp(argv[1], "server-header") == 0)
mode = SERVER_HEADER;
+ else if (strcmp(argv[1], "server-inert") == 0)
+ mode = SERVER_INERT;
else if (strcmp(argv[1], "code") == 0)
mode = CODE;
else
@@ -1317,6 +1467,9 @@ int main(int argc, char *argv[])
emit_header(&protocol, SERVER);
break;
+ emit_inert(&protocol);
+ break;
emit_code(&protocol);
break;
diff --git a/wayland-scanner.mk b/wayland-scanner.mk
index 0a72062..fd994e0 100644
--- a/wayland-scanner.mk
+++ b/wayland-scanner.mk
@@ -6,3 +6,6 @@
%-client-protocol.h : $(wayland_protocoldir)/%.xml
+
+%-server-inert.h: $(wayland_protocoldir)/%.xml
\ No newline at end of file
--
1.9.1
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Marek Chalupa
2015-03-19 08:11:47 UTC
Permalink
When server looses some capability (like pointer or keyboard),
it takes some time to get this information to clients.
When client sends request with new_id argument to the object
that has been just destroyed on server-side (client
does not know about it yet), we still have to create the resource.
If we wouldn't do it then the client will get invalid id error once it
tries to use the new object. But if we create it, then we have to
take care that all the requests but destructor are ignored,
because we do not have the server-side object anymore.
(eventually, client will destroy the resource, because
it will get the information about server-side object destruction)

This patch solves this ugly race by adding wl_resource_set_intact()
function that marks the resource as intact. When resource is intact
it ignores all requests and events but destructors. The trick is in
adding flag into the request's siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on intact
resource.

Programmer then can mark newly created resource as intact when
this race come up instead of defining new implementation of
resource just for this rare case.

Signed-off-by: Marek Chalupa <***@gmail.com>
---
src/connection.c | 17 ++++++++++++++---
src/scanner.c | 5 +++++
src/wayland-client.c | 6 +++---
src/wayland-private.h | 3 ++-
src/wayland-server.c | 41 ++++++++++++++++++++++++++++++++++-------
src/wayland-server.h | 6 ++++++
6 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 2545194..a8fab12 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -415,6 +415,9 @@ get_next_argument(const char *signature, struct argument_details *details)
details->nullable = 0;
for(; *signature; ++signature) {
switch(*signature) {
+ case 'D':
+ /* skip destructor flag */
+ break;
case 'i':
case 'u':
case 'f':
@@ -457,8 +460,14 @@ int
wl_message_get_since(const struct wl_message *message)
{
int since;
+ const char *sig = message->signature;
+
+ /* destructor flag is always the first letter in
+ * signature */
+ if (*sig == 'D')
+ ++sig;

- since = atoi(message->signature);
+ since = atoi(sig);

if (since == 0)
since = 1;
@@ -1175,7 +1184,8 @@ wl_closure_queue(struct wl_closure *closure, struct wl_connection *connection)
}

void
-wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
+wl_closure_print(struct wl_closure *closure, struct wl_object *target,
+ int send, int intact)
{
int i;
struct argument_details arg;
@@ -1186,9 +1196,10 @@ wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
clock_gettime(CLOCK_REALTIME, &tp);
time = (tp.tv_sec * 1000000L) + (tp.tv_nsec / 1000);

- fprintf(stderr, "[%10.3f] %s%s@%u.%s(",
+ fprintf(stderr, "[%10.3f] %s%s%s@%u.%s(",
time / 1000.0,
send ? " -> " : "",
+ intact ? "INTACT " : "",
target->interface->name, target->id,
closure->message->name);

diff --git a/src/scanner.c b/src/scanner.c
index 1f1e59a..515fe1c 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -1133,6 +1133,11 @@ emit_messages(struct wl_list *message_list,
wl_list_for_each(m, message_list, link) {
printf("\t{ \"%s\", \"", m->name);

+ /* make destructor flag the first letter in
+ * signature */
+ if (m->destructor)
+ printf("D");
+
if (m->since > 1)
printf("%d", m->since);

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 0f1405c..30b0b66 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -598,7 +598,7 @@ wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
}

if (debug_client)
- wl_closure_print(closure, &proxy->object, true);
+ wl_closure_print(closure, &proxy->object, true, false);

if (wl_closure_send(closure, proxy->display->connection)) {
wl_log("Error sending request: %m\n");
@@ -1157,13 +1157,13 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)

if (proxy->dispatcher) {
if (debug_client)
- wl_closure_print(closure, &proxy->object, false);
+ wl_closure_print(closure, &proxy->object, false, false);

wl_closure_dispatch(closure, proxy->dispatcher,
&proxy->object, opcode);
} else if (proxy->object.implementation) {
if (debug_client)
- wl_closure_print(closure, &proxy->object, false);
+ wl_closure_print(closure, &proxy->object, false, false);

wl_closure_invoke(closure, WL_CLOSURE_INVOKE_CLIENT,
&proxy->object, opcode, proxy->user_data);
diff --git a/src/wayland-private.h b/src/wayland-private.h
index db76081..00e34e0 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -160,7 +160,8 @@ wl_closure_send(struct wl_closure *closure, struct wl_connection *connection);
int
wl_closure_queue(struct wl_closure *closure, struct wl_connection *connection);
void
-wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send);
+wl_closure_print(struct wl_closure *closure, struct wl_object *target,
+ int send, int intact);
void
wl_closure_destroy(struct wl_closure *closure);

diff --git a/src/wayland-server.c b/src/wayland-server.c
index 0558634..b777711 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -116,6 +116,7 @@ struct wl_resource {
void *data;
int version;
wl_dispatcher_func_t dispatcher;
+ int intact;
};

static int debug_server = 0;
@@ -139,7 +140,7 @@ wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
resource->client->error = 1;

if (debug_server)
- wl_closure_print(closure, object, true);
+ wl_closure_print(closure, object, true, resource->intact);

wl_closure_destroy(closure);
}
@@ -179,7 +180,7 @@ wl_resource_queue_event_array(struct wl_resource *resource, uint32_t opcode,
resource->client->error = 1;

if (debug_server)
- wl_closure_print(closure, object, true);
+ wl_closure_print(closure, object, true, resource->intact);

wl_closure_destroy(closure);
}
@@ -228,6 +229,12 @@ wl_resource_post_error(struct wl_resource *resource,
}

static int
+wl_message_is_destructor(const struct wl_message *message)
+{
+ return message->signature[0] == 'D';
+}
+
+static int
wl_client_connection_data(int fd, uint32_t mask, void *data)
{
struct wl_client *client = data;
@@ -327,7 +334,17 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
}

if (debug_server)
- wl_closure_print(closure, object, false);
+ wl_closure_print(closure, object,
+ false, resource->intact);
+
+ /* intact resources dispatch only destructors */
+ if (resource->intact &&
+ !wl_message_is_destructor(message)) {
+ /* Let user know why the request "vanished" */
+ wl_log("Skipping non-destructor on intact resource\n");
+ wl_closure_destroy(closure);
+ continue;
+ }

if ((resource_flags & WL_MAP_ENTRY_LEGACY) ||
resource->dispatcher == NULL) {
@@ -1276,6 +1293,18 @@ wl_resource_set_implementation(struct wl_resource *resource,
}

WL_EXPORT void
+wl_resource_set_intact(struct wl_resource *resource)
+{
+ resource->intact = 1;
+}
+
+WL_EXPORT int
+wl_resource_get_intact(struct wl_resource *resource)
+{
+ return resource->intact;
+}
+
+WL_EXPORT void
wl_resource_set_dispatcher(struct wl_resource *resource,
wl_dispatcher_func_t dispatcher,
const void *implementation,
@@ -1298,20 +1327,18 @@ wl_resource_create(struct wl_client *client,
if (resource == NULL)
return NULL;

+ memset(resource, 0, sizeof *resource);
+
if (id == 0)
id = wl_map_insert_new(&client->objects, 0, NULL);

resource->object.id = id;
resource->object.interface = interface;
- resource->object.implementation = NULL;

wl_signal_init(&resource->destroy_signal);

- resource->destroy = NULL;
resource->client = client;
- resource->data = NULL;
resource->version = version;
- resource->dispatcher = NULL;

if (wl_map_insert_at(&client->objects, 0, id, resource) < 0) {
wl_resource_post_error(client->display_resource,
diff --git a/src/wayland-server.h b/src/wayland-server.h
index af2f03d..8dad02c 100644
--- a/src/wayland-server.h
+++ b/src/wayland-server.h
@@ -353,6 +353,12 @@ struct wl_resource *
wl_resource_create(struct wl_client *client,
const struct wl_interface *interface,
int version, uint32_t id);
+
+void
+wl_resource_set_intact(struct wl_resource *resource);
+int
+wl_resource_get_intact(struct wl_resource *resource);
+
void
wl_resource_set_implementation(struct wl_resource *resource,
const void *implementation,
--
2.1.0
Marek Chalupa
2015-03-19 08:11:48 UTC
Permalink
Test if the requests of intact objects are ignored, except for
destructors.

Signed-off-by: Marek Chalupa <***@gmail.com>
---
tests/resources-test.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 123 insertions(+)

diff --git a/tests/resources-test.c b/tests/resources-test.c
index a6ce3ae..7313434 100644
--- a/tests/resources-test.c
+++ b/tests/resources-test.c
@@ -1,5 +1,6 @@
/*
* Copyright © 2013 Marek Chalupa
+ * Copyright © 2015 Red Hat, Inc.
*
* Permission to use, copy, modify, distribute, and sell this software and its
* documentation for any purpose is hereby granted without fee, provided that
@@ -23,9 +24,13 @@
#include <assert.h>
#include <sys/socket.h>
#include <unistd.h>
+#include <string.h>
+
+#define WL_HIDE_DEPRECATED 1

#include "wayland-server.h"
#include "test-runner.h"
+#include "test-compositor.h"

TEST(create_resource_tst)
{
@@ -163,3 +168,121 @@ TEST(create_resource_with_same_id)
wl_display_destroy(display);
close(s[1]);
}
+
+static void
+handle_globals(void *data, struct wl_registry *registry,
+ uint32_t id, const char *intf, uint32_t ver)
+{
+ struct wl_shm_pool **pool = data;
+
+ if (strcmp(intf, "wl_shm_pool") == 0) {
+ (*pool) = wl_registry_bind(registry, id,
+ &wl_shm_pool_interface, ver);
+ assert(pool);
+ }
+}
+
+static const struct wl_registry_listener registry_listener = {
+ handle_globals, NULL
+};
+
+static void
+intact_resource_main(void)
+{
+ struct client *cli = client_connect();
+ struct wl_shm_pool *pool;
+ struct wl_registry *reg = wl_display_get_registry(cli->wl_display);
+ assert(reg);
+
+ wl_registry_add_listener(reg, &registry_listener, &pool);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+ assert(pool && "Did not bind to the pool");
+
+ wl_registry_destroy(reg);
+
+ /* let the display make the pool resource intact */
+ stop_display(cli, 1);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ /* these requests should be ignored */
+ wl_shm_pool_resize(pool, 100);
+ wl_shm_pool_resize(pool, 200);
+
+ /* this one should be not */
+ wl_shm_pool_destroy(pool);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ client_disconnect(cli);
+}
+
+static void
+pool_resize(struct wl_client *client,
+ struct wl_resource *resource,
+ int32_t size)
+{
+ assert(0 && "This event should be never called");
+}
+
+static int destroyed = 0;
+
+static void
+pool_destroy(struct wl_client *client,
+ struct wl_resource *resource)
+{
+ destroyed = 1;
+}
+
+static const struct wl_shm_pool_interface pool_implementation = {
+ .resize = pool_resize,
+ .destroy = pool_destroy
+};
+
+static void
+pool_bind(struct wl_client *client, void *data,
+ uint32_t version, uint32_t id)
+{
+ struct display *d = data;
+ struct client_info *ci;
+ struct wl_resource *res;
+
+ wl_list_for_each(ci, &d->clients, link)
+ if (ci->wl_client == client)
+ break;
+
+ res = wl_resource_create(client, &wl_shm_pool_interface,
+ version, id);
+ assert(res);
+ wl_resource_set_implementation(res, &pool_implementation, NULL, NULL);
+
+ ci->data = res;
+}
+
+TEST(intact_resource)
+{
+ struct client_info *ci;
+ struct wl_resource *res;
+ struct display *d = display_create();
+ /* we need some interface with destructor. wl_pointer/keyboard
+ * has destructor, but we'd need to implement wl_seat for them too,
+ * so choose rather wl_shm_pool */
+ wl_global_create(d->wl_display, &wl_shm_pool_interface,
+ wl_shm_pool_interface.version, d, pool_bind);
+
+ ci = client_create(d, intact_resource_main);
+
+ test_set_timeout(3);
+ display_run(d);
+
+ /* display has been stopped, make resource intact */
+ res = ci->data;
+ assert(res && "Client did not bind to the global");
+
+ assert(wl_resource_get_intact(res) == 0);
+ wl_resource_set_intact(res);
+ assert(wl_resource_get_intact(res) == 1);
+
+ display_resume(d);
+
+ assert(destroyed == 1 && "Destructor was not called");
+ display_destroy(d);
+}
--
2.1.0
Bill Spitzak
2015-03-19 18:34:09 UTC
Permalink
I think the name "intact" is really confusing. Can it be instead called
"destroyed"?
Post by Marek Chalupa
When server looses some capability (like pointer or keyboard),
it takes some time to get this information to clients.
When client sends request with new_id argument to the object
that has been just destroyed on server-side (client
does not know about it yet), we still have to create the resource.
If we wouldn't do it then the client will get invalid id error once it
tries to use the new object. But if we create it, then we have to
take care that all the requests but destructor are ignored,
because we do not have the server-side object anymore.
(eventually, client will destroy the resource, because
it will get the information about server-side object destruction)
This patch solves this ugly race by adding wl_resource_set_intact()
function that marks the resource as intact. When resource is intact
it ignores all requests and events but destructors. The trick is in
adding flag into the request's siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on intact
resource.
Programmer then can mark newly created resource as intact when
this race come up instead of defining new implementation of
resource just for this rare case.
Marek Chalupa
2015-03-23 10:00:15 UTC
Permalink
Destroyed seems confusing to me too, since the resource is not really
destroyed. What about inert or unalive ?
Post by Bill Spitzak
I think the name "intact" is really confusing. Can it be instead called
"destroyed"?
Post by Marek Chalupa
When server looses some capability (like pointer or keyboard),
it takes some time to get this information to clients.
When client sends request with new_id argument to the object
that has been just destroyed on server-side (client
does not know about it yet), we still have to create the resource.
If we wouldn't do it then the client will get invalid id error once it
tries to use the new object. But if we create it, then we have to
take care that all the requests but destructor are ignored,
because we do not have the server-side object anymore.
(eventually, client will destroy the resource, because
it will get the information about server-side object destruction)
This patch solves this ugly race by adding wl_resource_set_intact()
function that marks the resource as intact. When resource is intact
it ignores all requests and events but destructors. The trick is in
adding flag into the request's siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on intact
resource.
Programmer then can mark newly created resource as intact when
this race come up instead of defining new implementation of
resource just for this rare case.
Giulio Camuffo
2015-03-23 11:22:00 UTC
Permalink
Post by Marek Chalupa
Destroyed seems confusing to me too, since the resource is not really
destroyed. What about inert or unalive ?
Inert is ok, imho. On the client side they are called zombie
objects... maybe for consistency it'd be better to keep that here too.
Post by Marek Chalupa
Post by Bill Spitzak
I think the name "intact" is really confusing. Can it be instead called
"destroyed"?
Post by Marek Chalupa
When server looses some capability (like pointer or keyboard),
it takes some time to get this information to clients.
When client sends request with new_id argument to the object
that has been just destroyed on server-side (client
does not know about it yet), we still have to create the resource.
If we wouldn't do it then the client will get invalid id error once it
tries to use the new object. But if we create it, then we have to
take care that all the requests but destructor are ignored,
because we do not have the server-side object anymore.
(eventually, client will destroy the resource, because
it will get the information about server-side object destruction)
This patch solves this ugly race by adding wl_resource_set_intact()
function that marks the resource as intact. When resource is intact
it ignores all requests and events but destructors. The trick is in
adding flag into the request's siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on intact
resource.
Programmer then can mark newly created resource as intact when
this race come up instead of defining new implementation of
resource just for this rare case.
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Bill Spitzak
2015-03-23 15:00:26 UTC
Permalink
Post by Giulio Camuffo
Post by Marek Chalupa
Destroyed seems confusing to me too, since the resource is not really
destroyed. What about inert or unalive ?
Inert is ok, imho. On the client side they are called zombie
objects... maybe for consistency it'd be better to keep that here too.
"zombie" seems like a really good idea. There is lots of precedent for
that too, not just Unix processes but discussion of weak pointers.
Pekka Paalanen
2015-11-19 09:37:23 UTC
Permalink
On Mon, 23 Mar 2015 13:22:00 +0200
Post by Giulio Camuffo
Post by Marek Chalupa
Destroyed seems confusing to me too, since the resource is not really
destroyed. What about inert or unalive ?
Inert is ok, imho. On the client side they are called zombie
objects... maybe for consistency it'd be better to keep that here too.
Sorry for being so late, but "zombie" is actually a used term in
libwayland. It is not synonymous to "inert".

A zombie object is an object that has been destroyed on the client
side, but the server has not yet acknowledged it and may be sending
events through it. These events will be silently dropped in
libwayland-client.

An inert object is a real existing protocol object on both sides of the
connection. An inert object is only semantically "dead" - it depends on
the interface specification on what it actually means.


Thanks,
pq

Hardening
2015-03-24 09:47:43 UTC
Permalink
Post by Marek Chalupa
When server looses some capability (like pointer or keyboard),
it takes some time to get this information to clients.
When client sends request with new_id argument to the object
that has been just destroyed on server-side (client
does not know about it yet), we still have to create the resource.
If we wouldn't do it then the client will get invalid id error once it
tries to use the new object. But if we create it, then we have to
take care that all the requests but destructor are ignored,
because we do not have the server-side object anymore.
(eventually, client will destroy the resource, because
it will get the information about server-side object destruction)
This patch solves this ugly race by adding wl_resource_set_intact()
function that marks the resource as intact. When resource is intact
it ignores all requests and events but destructors. The trick is in
adding flag into the request's siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on intact
resource.
Hello Marek,
nice to have some feedback on this subject. I like your implementation
(and you have a unitary test which is really nice), anyway I feel like
it doesn't handle the case of inert objects that are requested with a
method that "return" an object. For example an inert seat that is
requested for get_keyboard(), in this case I think should we should bind
an inert keyboard. I have worked on this subject to solve that exact
problem. Today the seat can't be released because: we don't have the
method to do it, and we have the race problem that prevent us from
freeing the seat.

Regards.
--
David FORT
website: http://www.hardening-consulting.com/
Marek Chalupa
2015-03-27 14:54:48 UTC
Permalink
Post by Hardening
Post by Marek Chalupa
When server looses some capability (like pointer or keyboard),
it takes some time to get this information to clients.
When client sends request with new_id argument to the object
that has been just destroyed on server-side (client
does not know about it yet), we still have to create the resource.
If we wouldn't do it then the client will get invalid id error once it
tries to use the new object. But if we create it, then we have to
take care that all the requests but destructor are ignored,
because we do not have the server-side object anymore.
(eventually, client will destroy the resource, because
it will get the information about server-side object destruction)
This patch solves this ugly race by adding wl_resource_set_intact()
function that marks the resource as intact. When resource is intact
it ignores all requests and events but destructors. The trick is in
adding flag into the request's siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on intact
resource.
Hello Marek,
nice to have some feedback on this subject. I like your implementation
(and you have a unitary test which is really nice), anyway I feel like
it doesn't handle the case of inert objects that are requested with a
method that "return" an object. For example an inert seat that is
requested for get_keyboard(), in this case I think should we should bind
an inert keyboard. I have worked on this subject to solve that exact
problem. Today the seat can't be released because: we don't have the
method to do it, and we have the race problem that prevent us from
freeing the seat.
Regards.
Yeah, you're right. When the object is inert, then it won't call anything
but destructor, which would break new_id messages on this object... OK,
this could be solved by looking into the signature for new_id arg, so the
new condition would be:

if (is_inert && (!message_is_destructor || !has_new_id))
do_not_invoke

And then programmer must check if the resource is innert when creating new
resource for new id (which would not be much pain, since usually we don't
want to do anything with inert resource, so we'd check for it anyway).

Anyway, In you're approach there's this problem solved by inheriting the
inertness from parent interface, which is really nice.
Post by Hardening
--
David FORT
website: http://www.hardening-consulting.com/
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Marek Chalupa
2015-04-10 13:44:36 UTC
Permalink
Hi,

so I changed my patches so that they can handle the new_id use-case. I've
been testing them now for some time and it looks working.
Unfortunately, the change needed to dig more into wayland, so it is more
bug-prone. This is still just an alternative to your patches.
A benefit I see in my patches is that if we'll ever need make some another
similar changes, this series prepares a ground for it.
Post by Marek Chalupa
Post by Hardening
Post by Marek Chalupa
When server looses some capability (like pointer or keyboard),
it takes some time to get this information to clients.
When client sends request with new_id argument to the object
that has been just destroyed on server-side (client
does not know about it yet), we still have to create the resource.
If we wouldn't do it then the client will get invalid id error once it
tries to use the new object. But if we create it, then we have to
take care that all the requests but destructor are ignored,
because we do not have the server-side object anymore.
(eventually, client will destroy the resource, because
it will get the information about server-side object destruction)
This patch solves this ugly race by adding wl_resource_set_intact()
function that marks the resource as intact. When resource is intact
it ignores all requests and events but destructors. The trick is in
adding flag into the request's siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on intact
resource.
Hello Marek,
nice to have some feedback on this subject. I like your implementation
(and you have a unitary test which is really nice), anyway I feel like
it doesn't handle the case of inert objects that are requested with a
method that "return" an object. For example an inert seat that is
requested for get_keyboard(), in this case I think should we should bind
an inert keyboard. I have worked on this subject to solve that exact
problem. Today the seat can't be released because: we don't have the
method to do it, and we have the race problem that prevent us from
freeing the seat.
Regards.
Yeah, you're right. When the object is inert, then it won't call anything
but destructor, which would break new_id messages on this object... OK,
this could be solved by looking into the signature for new_id arg, so the
if (is_inert && (!message_is_destructor || !has_new_id))
do_not_invoke
And then programmer must check if the resource is innert when creating new
resource for new id (which would not be much pain, since usually we don't
want to do anything with inert resource, so we'd check for it anyway).
Anyway, In you're approach there's this problem solved by inheriting the
inertness from parent interface, which is really nice.
Post by Hardening
--
David FORT
website: http://www.hardening-consulting.com/
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Marek Chalupa
2015-04-10 13:45:38 UTC
Permalink
wl_proxy can have flags like destroyed or deleted_id that are used
to handle races. It turned out that having similar flags with wl_resources
would be handy too. wl_proxy and wl_resource share the same base
which is wl_object. Add flags to wl_object and remove them from
wl_proxy, so that we can use the same flags in both, server and client
objects.

Signed-off-by: Marek Chalupa <***@gmail.com>
---
src/wayland-client.c | 18 ++++++------------
src/wayland-private.h | 6 ++++++
2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index ed108e1..ff2b61a 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -46,16 +46,10 @@

/** \cond */

-enum wl_proxy_flag {
- WL_PROXY_FLAG_ID_DELETED = (1 << 0),
- WL_PROXY_FLAG_DESTROYED = (1 << 1)
-};
-
struct wl_proxy {
struct wl_object object;
struct wl_display *display;
struct wl_event_queue *queue;
- uint32_t flags;
int refcount;
void *user_data;
wl_dispatcher_func_t dispatcher;
@@ -234,7 +228,7 @@ decrease_closure_args_refcount(struct wl_closure *closure)
case 'o':
proxy = (struct wl_proxy *) closure->args[i].o;
if (proxy) {
- if (proxy->flags & WL_PROXY_FLAG_DESTROYED)
+ if (proxy->object.flags & WL_OBJECT_FLAG_DESTROYED)
closure->args[i].o = NULL;

proxy->refcount--;
@@ -402,7 +396,7 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
void
proxy_destroy(struct wl_proxy *proxy)
{
- if (proxy->flags & WL_PROXY_FLAG_ID_DELETED)
+ if (proxy->object.flags & WL_OBJECT_FLAG_ID_DELETED)
wl_map_remove(&proxy->display->objects, proxy->object.id);
else if (proxy->object.id < WL_SERVER_ID_START)
wl_map_insert_at(&proxy->display->objects, 0,
@@ -412,7 +406,7 @@ proxy_destroy(struct wl_proxy *proxy)
proxy->object.id, NULL);


- proxy->flags |= WL_PROXY_FLAG_DESTROYED;
+ proxy->object.flags |= WL_OBJECT_FLAG_DESTROYED;

proxy->refcount--;
if (!proxy->refcount)
@@ -728,7 +722,7 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)
wl_log("error: received delete_id for unknown id (%u)\n", id);

if (proxy && proxy != WL_ZOMBIE_OBJECT)
- proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
+ proxy->object.flags |= WL_OBJECT_FLAG_ID_DELETED;
else
wl_map_remove(&display->objects, id);

@@ -835,11 +829,11 @@ wl_display_connect_to_fd(int fd)
display->proxy.object.interface = &wl_display_interface;
display->proxy.object.id =
wl_map_insert_new(&display->objects, 0, display);
+ display->proxy.object.flags = 0;
display->proxy.display = display;
display->proxy.object.implementation = (void(**)(void)) &display_listener;
display->proxy.user_data = display;
display->proxy.queue = &display->default_queue;
- display->proxy.flags = 0;
display->proxy.refcount = 1;

display->connection = wl_connection_create(display->fd);
@@ -1142,7 +1136,7 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)

decrease_closure_args_refcount(closure);
proxy = closure->proxy;
- proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
+ proxy_destroyed = !!(proxy->object.flags & WL_OBJECT_FLAG_DESTROYED);

proxy->refcount--;
if (proxy_destroyed) {
diff --git a/src/wayland-private.h b/src/wayland-private.h
index db76081..ab48889 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -42,10 +42,16 @@
#define WL_SERVER_ID_START 0xff000000
#define WL_CLOSURE_MAX_ARGS 20

+enum wl_object_flags {
+ WL_OBJECT_FLAG_DESTROYED = 1 << 0,
+ WL_OBJECT_FLAG_ID_DELETED = 1 << 1
+};
+
struct wl_object {
const struct wl_interface *interface;
const void *implementation;
uint32_t id;
+ uint32_t flags;
};

extern struct wl_object global_zombie_object;
--
2.1.0
Marek Chalupa
2015-04-10 13:45:39 UTC
Permalink
We can get the proxy/resource by (wl_)container_of macro where needed.
Storing bare wl_object allows us to store even some objects
that do not have assigned proxy or resource and thus make some
reasoning about objects on lower level (like when we demarshal
the connection)

Signed-off-by: Marek Chalupa <***@gmail.com>
---
src/wayland-client.c | 18 ++++++++++++------
src/wayland-server.c | 26 ++++++++++++++++++++------
2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index ff2b61a..92256a1 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -103,6 +103,13 @@ struct wl_display {

static int debug_client = 0;

+static struct wl_proxy *
+wl_map_lookup_proxy(struct wl_map *objects, uint32_t id)
+{
+ struct wl_object *object = wl_map_lookup(objects, id);
+ return container_of(object, struct wl_proxy, object);
+}
+
/**
* This helper function wakes up all threads that are
* waiting for display->reader_cond (i. e. when reading is done,
@@ -331,7 +338,7 @@ proxy_create(struct wl_proxy *factory, const struct wl_interface *interface)
proxy->queue = factory->queue;
proxy->refcount = 1;

- proxy->object.id = wl_map_insert_new(&display->objects, 0, proxy);
+ proxy->object.id = wl_map_insert_new(&display->objects, 0, &proxy->object);

return proxy;
}
@@ -388,7 +395,7 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
proxy->queue = factory->queue;
proxy->refcount = 1;

- wl_map_insert_at(&display->objects, 0, id, proxy);
+ wl_map_insert_at(&display->objects, 0, id, &proxy->object);

return proxy;
}
@@ -716,8 +723,7 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)

pthread_mutex_lock(&display->mutex);

- proxy = wl_map_lookup(&display->objects, id);
-
+ proxy = wl_map_lookup_proxy(&display->objects, id);
if (!proxy)
wl_log("error: received delete_id for unknown id (%u)\n", id);

@@ -828,7 +834,7 @@ wl_display_connect_to_fd(int fd)

display->proxy.object.interface = &wl_display_interface;
display->proxy.object.id =
- wl_map_insert_new(&display->objects, 0, display);
+ wl_map_insert_new(&display->objects, 0, &display->proxy.object);
display->proxy.object.flags = 0;
display->proxy.display = display;
display->proxy.object.implementation = (void(**)(void)) &display_listener;
@@ -1079,7 +1085,7 @@ queue_event(struct wl_display *display, int len)
if (len < size)
return 0;

- proxy = wl_map_lookup(&display->objects, id);
+ proxy = wl_map_lookup_proxy(&display->objects, id);
if (proxy == WL_ZOMBIE_OBJECT) {
wl_connection_consume(display->connection, size);
return size;
diff --git a/src/wayland-server.c b/src/wayland-server.c
index ecbae68..d34e07c 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -120,6 +120,13 @@ struct wl_resource {

static int debug_server = 0;

+static struct wl_resource *
+wl_map_lookup_resource(struct wl_map *objects, uint32_t id)
+{
+ struct wl_object *object = wl_map_lookup(objects, id);
+ return container_of(object, struct wl_resource, object);
+}
+
WL_EXPORT void
wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
union wl_argument *args)
@@ -273,7 +280,7 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
if (len < size)
break;

- resource = wl_map_lookup(&client->objects, p[0]);
+ resource = wl_map_lookup_resource(&client->objects, p[0]);
resource_flags = wl_map_lookup_flags(&client->objects, p[0]);
if (resource == NULL) {
wl_resource_post_error(client->display_resource,
@@ -502,7 +509,7 @@ wl_client_get_credentials(struct wl_client *client,
WL_EXPORT struct wl_resource *
wl_client_get_object(struct wl_client *client, uint32_t id)
{
- return wl_map_lookup(&client->objects, id);
+ return wl_map_lookup_resource(&client->objects, id);
}

WL_EXPORT void
@@ -536,6 +543,13 @@ destroy_resource(void *element, void *data)
free(resource);
}

+static inline void
+destroy_resource_for_object(void *element, void *data)
+{
+ /* element is of type wl_object * */
+ destroy_resource(container_of(element, struct wl_resource, object), data);
+}
+
WL_EXPORT void
wl_resource_destroy(struct wl_resource *resource)
{
@@ -668,7 +682,7 @@ wl_client_destroy(struct wl_client *client)
wl_signal_emit(&client->destroy_signal, client);

wl_client_flush(client);
- wl_map_for_each(&client->objects, destroy_resource, &serial);
+ wl_map_for_each(&client->objects, destroy_resource_for_object, &serial);
wl_map_release(&client->objects);
wl_event_source_remove(client->source);
close(wl_connection_destroy(client->connection));
@@ -1314,7 +1328,7 @@ wl_resource_create(struct wl_client *client,
resource->version = version;
resource->dispatcher = NULL;

- if (wl_map_insert_at(&client->objects, 0, id, resource) < 0) {
+ if (wl_map_insert_at(&client->objects, 0, id, &resource->object) < 0) {
wl_resource_post_error(client->display_resource,
WL_DISPLAY_ERROR_INVALID_OBJECT,
"invalid new id %d", id);
@@ -1396,9 +1410,9 @@ wl_client_add_resource(struct wl_client *client,
if (resource->object.id == 0) {
resource->object.id =
wl_map_insert_new(&client->objects,
- WL_MAP_ENTRY_LEGACY, resource);
+ WL_MAP_ENTRY_LEGACY, &resource->object);
} else if (wl_map_insert_at(&client->objects, WL_MAP_ENTRY_LEGACY,
- resource->object.id, resource) < 0) {
+ resource->object.id, &resource->object) < 0) {
wl_resource_post_error(client->display_resource,
WL_DISPLAY_ERROR_INVALID_OBJECT,
"invalid new id %d",
--
2.1.0
Marek Chalupa
2015-04-10 13:45:40 UTC
Permalink
When client sends request with new_id argument to the object
that has been destroyed on server-side just before that (client
may not know about it, because it has not got the information
via wire yet), we have to create the resource. If we wouldn't do it then
client will get invalid error id once it tries to use the new object.
But if we create it, then we have to take care that all the requests
but destructor are ignored, because we do not have the server-side
object anymore. Client will destroy the resource right away, because
it gets the information about server-side object destruction.

This patch solves this ugly race by adding wl_resource_set_inert()
function that marks the resource as inert. When resource is inert
it ignores all requests and events but destructor. The trick is in
adding flag into the request/even siganture that says: "hey! I'm
destructor". Server then can skip non-destructor actions on inert
resource.

When an object asks for creating new object from inert object,
wayland will silently create zombie object without invoking
appropriate handler, so once programmer marks resource as inert,
wayland will take care of the rest.

Programmer then can mark newly created resource as inert when
this race come up instead of defining new implementation of
resource just for this rare case.

v2: s/intact/inert
requests with new_id on inert objects create zombie objects

Signed-off-by: Marek Chalupa <***@gmail.com>
---
src/connection.c | 61 +++++++++++++++++++++-
src/scanner.c | 5 ++
src/wayland-client.c | 7 +++
src/wayland-private.h | 16 +++++-
src/wayland-server.c | 142 +++++++++++++++++++++++++++++++++++++++++---------
src/wayland-server.h | 6 +++
6 files changed, 207 insertions(+), 30 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 2545194..87ada61 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -415,6 +415,9 @@ get_next_argument(const char *signature, struct argument_details *details)
details->nullable = 0;
for(; *signature; ++signature) {
switch(*signature) {
+ case 'D':
+ /* skip destructor flag */
+ break;
case 'i':
case 'u':
case 'f':
@@ -457,8 +460,14 @@ int
wl_message_get_since(const struct wl_message *message)
{
int since;
+ const char *sig = message->signature;
+
+ /* destructor flag is always the first letter in
+ * signature */
+ if (*sig == 'D')
+ ++sig;

- since = atoi(message->signature);
+ since = atoi(sig);

if (since == 0)
since = 1;
@@ -608,6 +617,20 @@ wl_closure_vmarshal(struct wl_object *sender, uint32_t opcode, va_list ap,
return wl_closure_marshal(sender, opcode, args, message);
}

+static struct wl_object *
+create_inert_object(const struct wl_interface *intf, uint32_t id)
+{
+ struct wl_object *obj = calloc(1, sizeof *obj);
+ if (!obj)
+ return NULL;
+
+ obj->interface = intf;
+ obj->id = id;
+ obj->flags = (WL_OBJECT_FLAG_INERT | WL_OBJECT_FLAG_INERT_INHERENTLY);
+
+ return obj;
+}
+
struct wl_closure *
wl_connection_demarshal(struct wl_connection *connection,
uint32_t size,
@@ -622,6 +645,8 @@ wl_connection_demarshal(struct wl_connection *connection,
struct argument_details arg;
struct wl_closure *closure;
struct wl_array *array, *array_extra;
+ struct wl_object *sender, *iobj;
+ const struct wl_interface *interface;

count = arg_count_for_signature(message->signature);
if (count > WL_CLOSURE_MAX_ARGS) {
@@ -732,6 +757,37 @@ wl_connection_demarshal(struct wl_connection *connection,
goto err;
}

+ sender = wl_map_lookup(objects, closure->sender_id);
+ if(!sender) {
+ wl_log("got message from unknown object (%d)\n",
+ closure->sender_id);
+ goto err;
+ }
+
+ if (sender->flags & WL_OBJECT_FLAG_INERT) {
+ interface = message->types[i];
+ if (!interface) {
+ wl_log("new_id argument in message %s(%s)"
+ " has NULL interface\n",
+ message->name, message->signature);
+ errno = EINVAL;
+ goto err;
+ }
+
+ iobj = create_inert_object(interface, id);
+ if (!iobj) {
+ errno = ENOMEM;
+ goto err;
+ }
+
+ if(wl_map_insert_at(objects, 0, id, iobj) < 0) {
+ /* we reserved new id just few lines above,
+ * so this insert_at must never fail. If
+ * it does, it is a bug in wayland */
+ assert(0 && "BUG in wl_map implementation");
+ }
+ }
+
break;
case 'a':
length = *p++;
@@ -1186,9 +1242,10 @@ wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
clock_gettime(CLOCK_REALTIME, &tp);
time = (tp.tv_sec * 1000000L) + (tp.tv_nsec / 1000);

- fprintf(stderr, "[%10.3f] %s%s@%u.%s(",
+ fprintf(stderr, "[%10.3f] %s%s%s@%u.%s(",
time / 1000.0,
send ? " -> " : "",
+ (target->flags & WL_OBJECT_FLAG_INERT) ? "INERT " : "",
target->interface->name, target->id,
closure->message->name);

diff --git a/src/scanner.c b/src/scanner.c
index adc9aa3..e526e50 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -1138,6 +1138,11 @@ emit_messages(struct wl_list *message_list,
wl_list_for_each(m, message_list, link) {
printf("\t{ \"%s\", \"", m->name);

+ /* make destructor flag the first letter in
+ * signature */
+ if (m->destructor)
+ printf("D");
+
if (m->since > 1)
printf("%d", m->since);

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 92256a1..a3258c4 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -107,6 +107,13 @@ static struct wl_proxy *
wl_map_lookup_proxy(struct wl_map *objects, uint32_t id)
{
struct wl_object *object = wl_map_lookup(objects, id);
+
+ /* in the case that offset of object in proxy is not 0
+ * and the proxy is WL_ZOMBIE_OBJECT (which is not embedded
+ * in any proxy), we could run into troubles. */
+ if (object == WL_ZOMBIE_OBJECT)
+ return (struct wl_proxy *) WL_ZOMBIE_OBJECT;
+
return container_of(object, struct wl_proxy, object);
}

diff --git a/src/wayland-private.h b/src/wayland-private.h
index ab48889..5a52d00 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -43,8 +43,20 @@
#define WL_CLOSURE_MAX_ARGS 20

enum wl_object_flags {
- WL_OBJECT_FLAG_DESTROYED = 1 << 0,
- WL_OBJECT_FLAG_ID_DELETED = 1 << 1
+ /* wl_object has been destroy by a destroy function, but
+ * lives in references of events/requests */
+ WL_OBJECT_FLAG_DESTROYED = 1 << 0,
+
+ /* client got delete_id event for this object */
+ WL_OBJECT_FLAG_ID_DELETED = 1 << 1,
+
+ /* client created this object, but server do not has a
+ * counter part anymore. This object does not dispatch
+ * anything but destructors */
+ WL_OBJECT_FLAG_INERT = 1 << 2,
+
+ /* object is created from inert object */
+ WL_OBJECT_FLAG_INERT_INHERENTLY = 1 << 3
};

struct wl_object {
diff --git a/src/wayland-server.c b/src/wayland-server.c
index d34e07c..c1256a3 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -124,6 +124,11 @@ static struct wl_resource *
wl_map_lookup_resource(struct wl_map *objects, uint32_t id)
{
struct wl_object *object = wl_map_lookup(objects, id);
+
+ /* inherently inert object are not (in any) resource */
+ if (!object || (object->flags & WL_OBJECT_FLAG_INERT_INHERENTLY))
+ return NULL;
+
return container_of(object, struct wl_resource, object);
}

@@ -134,6 +139,9 @@ wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
struct wl_closure *closure;
struct wl_object *object = &resource->object;

+ if (object->flags & WL_OBJECT_FLAG_INERT)
+ return;
+
closure = wl_closure_marshal(object, opcode, args,
&object->interface->events[opcode]);

@@ -235,6 +243,15 @@ wl_resource_post_error(struct wl_resource *resource,
}

static int
+wl_message_is_destructor(const struct wl_message *message)
+{
+ return message->signature[0] == 'D';
+}
+
+static void
+client_delete_id(struct wl_client *client, uint32_t id);
+
+static int
wl_client_connection_data(int fd, uint32_t mask, void *data)
{
struct wl_client *client = data;
@@ -280,16 +297,18 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
if (len < size)
break;

- resource = wl_map_lookup_resource(&client->objects, p[0]);
resource_flags = wl_map_lookup_flags(&client->objects, p[0]);
- if (resource == NULL) {
+ object = wl_map_lookup(&client->objects, p[0]);
+ if (object == NULL) {
wl_resource_post_error(client->display_resource,
WL_DISPLAY_ERROR_INVALID_OBJECT,
"invalid object %u", p[0]);
break;
}

- object = &resource->object;
+ /* this would be wayland bug */
+ assert(object->interface && "Object without interface");
+
if (opcode >= object->interface->method_count) {
wl_resource_post_error(client->display_resource,
WL_DISPLAY_ERROR_INVALID_METHOD,
@@ -300,9 +319,18 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
break;
}

+ /* inherently inert objects are not embedded in any
+ * resource. However, we cannot just bail out here.
+ * We need the closure to be created, because there
+ * will be possibly created new inherently inert objects */
+ if (object->flags & WL_OBJECT_FLAG_INERT_INHERENTLY)
+ resource = NULL;
+ else
+ resource = wl_container_of(object, resource, object);
+
message = &object->interface->methods[opcode];
since = wl_message_get_since(message);
- if (!(resource_flags & WL_MAP_ENTRY_LEGACY) &&
+ if (!(resource_flags & WL_MAP_ENTRY_LEGACY) && resource &&
resource->version > 0 && resource->version < since) {
wl_resource_post_error(client->display_resource,
WL_DISPLAY_ERROR_INVALID_METHOD,
@@ -314,13 +342,13 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
break;
}

-
closure = wl_connection_demarshal(client->connection, size,
&client->objects, message);
len -= size;

if (closure == NULL && errno == ENOMEM) {
- wl_resource_post_no_memory(resource);
+ if (resource)
+ wl_resource_post_no_memory(resource);
break;
} else if (closure == NULL ||
wl_closure_lookup_objects(closure, &client->objects) < 0) {
@@ -337,13 +365,48 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
if (debug_server)
wl_closure_print(closure, object, false);

- if ((resource_flags & WL_MAP_ENTRY_LEGACY) ||
- resource->dispatcher == NULL) {
- wl_closure_invoke(closure, WL_CLOSURE_INVOKE_SERVER,
- object, opcode, client);
+ /* inert resources dispatch only destructors */
+ if ((object->flags & WL_OBJECT_FLAG_INERT) &&
+ !wl_message_is_destructor(message)) {
+ /* Let user know why the request "vanished" */
+ wl_log("Skipping non-destructor action on inert resource"
+ " (%s@%u.%s)\n", object->interface->name, object->id,
+ message->name);
+ wl_closure_destroy(closure);
+ continue;
+ }
+
+ /* if everything is ok, or if this is destructor of inert object,
+ * dispatch the request */
+ if (resource) {
+ if ((object->flags & WL_OBJECT_FLAG_INERT)
+ && !object->implementation) {
+ wl_log("No implementation at inert object %s@%u."
+ " Blame compositor!\n",
+ object->interface->name, object->id);
+ wl_closure_destroy(closure);
+ continue;
+ }
+
+ if ((resource_flags & WL_MAP_ENTRY_LEGACY) ||
+ resource->dispatcher == NULL) {
+ wl_closure_invoke(closure, WL_CLOSURE_INVOKE_SERVER,
+ object, opcode, client);
+ } else {
+ wl_closure_dispatch(closure, resource->dispatcher,
+ object, opcode);
+ }
} else {
- wl_closure_dispatch(closure, resource->dispatcher,
- object, opcode);
+ /* we are here, but do not have resource => this is a
+ * destructor of inherently inert object.
+ * Just send delete_id - destructor is not set anyway */
+ assert(object->flags & WL_OBJECT_FLAG_INERT_INHERENTLY);
+
+ client_delete_id(client, object->id);
+
+ /* inherently inert objects are allocated dynamically.
+ * We can free them at this moment */
+ free(object);
}

wl_closure_destroy(closure);
@@ -530,9 +593,11 @@ static void
destroy_resource(void *element, void *data)
{
struct wl_resource *resource = element;
- struct wl_client *client = resource->client;
+ struct wl_client *client;
uint32_t flags;

+ client = resource->client;
+
wl_signal_emit(&resource->destroy_signal, resource);

flags = wl_map_lookup_flags(&client->objects, resource->object.id);
@@ -546,19 +611,21 @@ destroy_resource(void *element, void *data)
static inline void
destroy_resource_for_object(void *element, void *data)
{
- /* element is of type wl_object * */
+ struct wl_object *object = element;
+
+ /* if this is inherently inert object, we cannot destroy
+ * it as a resource. We just need to free the memory */
+ if (object->flags & WL_OBJECT_FLAG_INERT_INHERENTLY) {
+ free(object);
+ return;
+ }
+
destroy_resource(container_of(element, struct wl_resource, object), data);
}

-WL_EXPORT void
-wl_resource_destroy(struct wl_resource *resource)
+static void
+client_delete_id(struct wl_client *client, uint32_t id)
{
- struct wl_client *client = resource->client;
- uint32_t id;
-
- id = resource->object.id;
- destroy_resource(resource, NULL);
-
if (id < WL_SERVER_ID_START) {
if (client->display_resource) {
wl_resource_queue_event(client->display_resource,
@@ -570,6 +637,18 @@ wl_resource_destroy(struct wl_resource *resource)
}
}

+WL_EXPORT void
+wl_resource_destroy(struct wl_resource *resource)
+{
+ struct wl_client *client = resource->client;
+ uint32_t id;
+
+ id = resource->object.id;
+ destroy_resource(resource, NULL);
+
+ client_delete_id(client, id);
+}
+
WL_EXPORT uint32_t
wl_resource_get_id(struct wl_resource *resource)
{
@@ -1291,6 +1370,18 @@ wl_resource_set_implementation(struct wl_resource *resource,
}

WL_EXPORT void
+wl_resource_set_inert(struct wl_resource *resource)
+{
+ resource->object.flags |= WL_OBJECT_FLAG_INERT;
+}
+
+WL_EXPORT int
+wl_resource_is_inert(struct wl_resource *resource)
+{
+ return (resource->object.flags & WL_OBJECT_FLAG_INERT);
+}
+
+WL_EXPORT void
wl_resource_set_dispatcher(struct wl_resource *resource,
wl_dispatcher_func_t dispatcher,
const void *implementation,
@@ -1313,20 +1404,19 @@ wl_resource_create(struct wl_client *client,
if (resource == NULL)
return NULL;

+ memset(resource, 0, sizeof *resource);
+
if (id == 0)
id = wl_map_insert_new(&client->objects, 0, NULL);

resource->object.id = id;
resource->object.interface = interface;
- resource->object.implementation = NULL;
+ resource->object.flags = 0;

wl_signal_init(&resource->destroy_signal);

- resource->destroy = NULL;
resource->client = client;
- resource->data = NULL;
resource->version = version;
- resource->dispatcher = NULL;

if (wl_map_insert_at(&client->objects, 0, id, &resource->object) < 0) {
wl_resource_post_error(client->display_resource,
diff --git a/src/wayland-server.h b/src/wayland-server.h
index af2f03d..41d4c25 100644
--- a/src/wayland-server.h
+++ b/src/wayland-server.h
@@ -353,6 +353,12 @@ struct wl_resource *
wl_resource_create(struct wl_client *client,
const struct wl_interface *interface,
int version, uint32_t id);
+
+void
+wl_resource_set_inert(struct wl_resource *resource);
+int
+wl_resource_is_inert(struct wl_resource *resource);
+
void
wl_resource_set_implementation(struct wl_resource *resource,
const void *implementation,
--
2.1.0
Bill Spitzak
2015-04-10 19:56:36 UTC
Permalink
Post by Marek Chalupa
When an object asks for creating new object from inert object,
wayland will silently create zombie object without invoking
appropriate handler, so once programmer marks resource as inert,
wayland will take care of the rest.
I think you should use the word "zombie" everywhere, rather than
sometimes calling it "inert" and other times "zombie". My guess this
would be a simple global search and replace on your patch.

I assume the new object returned is exactly like any other zombie, in
that if a program manages to ask *it* for a third object, that third one
is also a zombie?
Marek Chalupa
2015-04-10 13:45:41 UTC
Permalink
Signed-off-by: Marek Chalupa <***@gmail.com>
---
tests/resources-test.c | 238 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 238 insertions(+)

diff --git a/tests/resources-test.c b/tests/resources-test.c
index a6ce3ae..889f1bb 100644
--- a/tests/resources-test.c
+++ b/tests/resources-test.c
@@ -1,5 +1,6 @@
/*
* Copyright © 2013 Marek Chalupa
+ * Copyright © 2015 Red Hat, Inc.
*
* Permission to use, copy, modify, distribute, and sell this software and its
* documentation for any purpose is hereby granted without fee, provided that
@@ -23,9 +24,13 @@
#include <assert.h>
#include <sys/socket.h>
#include <unistd.h>
+#include <string.h>
+
+#define WL_HIDE_DEPRECATED 1

#include "wayland-server.h"
#include "test-runner.h"
+#include "test-compositor.h"

TEST(create_resource_tst)
{
@@ -59,6 +64,10 @@ TEST(create_resource_tst)
wl_resource_set_user_data(res, (void *) 0xbee);
assert(wl_resource_get_user_data(res) == (void *) 0xbee);

+ assert(!wl_resource_is_inert(res));
+ wl_resource_set_inert(res);
+ assert(wl_resource_is_inert(res));
+
wl_resource_destroy(res);
wl_client_destroy(client);
wl_display_destroy(display);
@@ -132,6 +141,57 @@ TEST(destroy_res_tst)
close(s[1]);
}

+TEST(destroy_inert_res_tst)
+{
+ struct wl_display *display;
+ struct wl_client *client;
+ struct wl_resource *res;
+ int s[2];
+
+ notify_called = 0;
+ _Bool destroyed = 0;
+ struct wl_listener destroy_listener = {
+ .notify = &destroy_notify
+ };
+
+ assert(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, s) == 0);
+ display = wl_display_create();
+ assert(display);
+ client = wl_client_create(display, s[0]);
+ assert(client);
+
+ res = wl_resource_create(client, &wl_display_interface, 4, 0);
+ assert(res);
+ wl_resource_set_implementation(res, NULL, &destroyed, res_destroy_func);
+ wl_resource_add_destroy_listener(res, &destroy_listener);
+
+ wl_resource_set_inert(res);
+ assert(wl_resource_is_inert(res));
+
+ wl_resource_destroy(res);
+ assert(destroyed);
+ assert(notify_called); /* check if signal was emitted */
+ assert(wl_client_get_object(client, wl_resource_get_id(res)) == NULL);
+
+ res = wl_resource_create(client, &wl_display_interface, 2, 0);
+ assert(res);
+
+ destroyed = 0;
+ notify_called = 0;
+ wl_resource_set_destructor(res, res_destroy_func);
+ wl_resource_set_user_data(res, &destroyed);
+ wl_resource_add_destroy_listener(res, &destroy_listener);
+ wl_resource_set_inert(res);
+
+ /* client should destroy the resource upon its destruction normally */
+ wl_client_destroy(client);
+ assert(destroyed);
+ assert(notify_called);
+
+ wl_display_destroy(display);
+ close(s[1]);
+}
+
TEST(create_resource_with_same_id)
{
struct wl_display *display;
@@ -163,3 +223,181 @@ TEST(create_resource_with_same_id)
wl_display_destroy(display);
close(s[1]);
}
+
+static void
+handle_globals(void *data, struct wl_registry *registry,
+ uint32_t id, const char *intf, uint32_t ver)
+{
+ struct wl_shm_pool **pool = data;
+
+ if (strcmp(intf, "wl_shm_pool") == 0) {
+ (*pool) = wl_registry_bind(registry, id,
+ &wl_shm_pool_interface, ver);
+ assert(pool);
+ }
+}
+
+static const struct wl_registry_listener registry_listener = {
+ handle_globals, NULL
+};
+
+static void
+inert_resource_main(void)
+{
+ struct client *cli = client_connect();
+ struct wl_shm_pool *pool;
+ struct wl_registry *reg = wl_display_get_registry(cli->wl_display);
+ assert(reg);
+
+ wl_registry_add_listener(reg, &registry_listener, &pool);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+ assert(pool && "Did not bind to the pool");
+
+ wl_registry_destroy(reg);
+
+ /* let the display make the pool resource inert */
+ stop_display(cli, 1);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ /* these requests should be ignored */
+ wl_shm_pool_resize(pool, 100);
+ wl_shm_pool_resize(pool, 200);
+
+ /* this one should be not */
+ wl_shm_pool_destroy(pool);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ client_disconnect(cli);
+}
+
+static void
+pool_resize(struct wl_client *client,
+ struct wl_resource *resource,
+ int32_t size)
+{
+ assert(0 && "This event should be never called");
+}
+
+static int destroyed = 0;
+
+static void
+pool_destroy(struct wl_client *client,
+ struct wl_resource *resource)
+{
+ destroyed = 1;
+}
+
+static const struct wl_shm_pool_interface pool_implementation = {
+ .resize = pool_resize,
+ .destroy = pool_destroy
+};
+
+static void
+pool_bind(struct wl_client *client, void *data,
+ uint32_t version, uint32_t id)
+{
+ struct display *d = data;
+ struct client_info *ci;
+ struct wl_resource *res;
+
+ wl_list_for_each(ci, &d->clients, link)
+ if (ci->wl_client == client)
+ break;
+
+ res = wl_resource_create(client, &wl_shm_pool_interface,
+ version, id);
+ assert(res);
+ wl_resource_set_implementation(res, &pool_implementation, NULL, NULL);
+
+ ci->data = res;
+}
+
+TEST(inert_resource)
+{
+ struct client_info *ci;
+ struct wl_resource *res;
+ struct display *d = display_create();
+ /* we need some interface with destructor. wl_pointer/keyboard
+ * has destructor, but we'd need to implement wl_seat for them too,
+ * so choose rather wl_shm_pool */
+ wl_global_create(d->wl_display, &wl_shm_pool_interface,
+ wl_shm_pool_interface.version, d, pool_bind);
+
+ ci = client_create(d, inert_resource_main);
+ display_run(d);
+
+ /* display has been stopped, make resource inert */
+ res = ci->data;
+ assert(res && "Client did not bind to the global");
+
+ assert(!wl_resource_is_inert(res));
+ wl_resource_set_inert(res);
+ assert(wl_resource_is_inert(res));
+
+ display_resume(d);
+
+ assert(destroyed == 1 && "Destructor was not called");
+ display_destroy(d);
+}
+
+static void
+inert_parent_resource_main(void)
+{
+ struct client *cli = client_connect();
+ struct wl_shm_pool *pool;
+ struct wl_registry *reg = wl_display_get_registry(cli->wl_display);
+ struct wl_buffer *buffer;
+ assert(reg);
+
+ wl_registry_add_listener(reg, &registry_listener, &pool);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+ assert(pool && "Did not bind to the pool");
+
+ wl_registry_destroy(reg);
+
+ /* let the display make the pool resource inert */
+ stop_display(cli, 1);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ /* these requests should be ignored, but the new_id object
+ * should be created */
+ buffer = wl_shm_pool_create_buffer(pool, 0, 100, 100, 4, 0);
+ assert(buffer);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ wl_buffer_destroy(buffer);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ /* this one should be not */
+ wl_shm_pool_destroy(pool);
+ assert(wl_display_roundtrip(cli->wl_display) != -1);
+
+ client_disconnect(cli);
+}
+
+/* test creating objects on inert object */
+TEST(inert_parent_resource)
+{
+ struct client_info *ci;
+ struct wl_resource *res;
+ struct display *d = display_create();
+
+ wl_global_create(d->wl_display, &wl_shm_pool_interface,
+ wl_shm_pool_interface.version, d, pool_bind);
+
+ ci = client_create(d, inert_parent_resource_main);
+ display_run(d);
+
+ /* display has been stopped, make resource inert */
+ res = ci->data;
+ assert(res && "Client did not bind to the global");
+
+ assert(!wl_resource_is_inert(res));
+ wl_resource_set_inert(res);
+ assert(wl_resource_is_inert(res));
+
+ display_resume(d);
+
+ assert(destroyed == 1 && "Destructor was not called");
+ display_destroy(d);
+}
--
2.1.0
Daniel Stone
2015-04-10 16:12:36 UTC
Permalink
Hi,
Post by Marek Chalupa
wl_proxy can have flags like destroyed or deleted_id that are used
to handle races. It turned out that having similar flags with wl_resources
would be handy too. wl_proxy and wl_resource share the same base
which is wl_object. Add flags to wl_object and remove them from
wl_proxy, so that we can use the same flags in both, server and client
objects.
NAK. wayland-server.h exposes wl_buffer as a non-opaque type (albeit
with a wrapper), and we allow EGL implementations in particular to use
the full details of that type as ABI. Adding this shifts everything
else in wl_resource and wl_buffer down by 4 bytes. For that reason, I
don't think it can even be added to wl_resource.

Sorry.

Cheers,
Daniel
Post by Marek Chalupa
---
src/wayland-client.c | 18 ++++++------------
src/wayland-private.h | 6 ++++++
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index ed108e1..ff2b61a 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -46,16 +46,10 @@
/** \cond */
-enum wl_proxy_flag {
- WL_PROXY_FLAG_ID_DELETED = (1 << 0),
- WL_PROXY_FLAG_DESTROYED = (1 << 1)
-};
-
struct wl_proxy {
struct wl_object object;
struct wl_display *display;
struct wl_event_queue *queue;
- uint32_t flags;
int refcount;
void *user_data;
wl_dispatcher_func_t dispatcher;
@@ -234,7 +228,7 @@ decrease_closure_args_refcount(struct wl_closure *closure)
proxy = (struct wl_proxy *) closure->args[i].o;
if (proxy) {
- if (proxy->flags & WL_PROXY_FLAG_DESTROYED)
+ if (proxy->object.flags & WL_OBJECT_FLAG_DESTROYED)
closure->args[i].o = NULL;
proxy->refcount--;
@@ -402,7 +396,7 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
void
proxy_destroy(struct wl_proxy *proxy)
{
- if (proxy->flags & WL_PROXY_FLAG_ID_DELETED)
+ if (proxy->object.flags & WL_OBJECT_FLAG_ID_DELETED)
wl_map_remove(&proxy->display->objects, proxy->object.id);
else if (proxy->object.id < WL_SERVER_ID_START)
wl_map_insert_at(&proxy->display->objects, 0,
@@ -412,7 +406,7 @@ proxy_destroy(struct wl_proxy *proxy)
proxy->object.id, NULL);
- proxy->flags |= WL_PROXY_FLAG_DESTROYED;
+ proxy->object.flags |= WL_OBJECT_FLAG_DESTROYED;
proxy->refcount--;
if (!proxy->refcount)
@@ -728,7 +722,7 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)
wl_log("error: received delete_id for unknown id (%u)\n", id);
if (proxy && proxy != WL_ZOMBIE_OBJECT)
- proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
+ proxy->object.flags |= WL_OBJECT_FLAG_ID_DELETED;
else
wl_map_remove(&display->objects, id);
@@ -835,11 +829,11 @@ wl_display_connect_to_fd(int fd)
display->proxy.object.interface = &wl_display_interface;
display->proxy.object.id =
wl_map_insert_new(&display->objects, 0, display);
+ display->proxy.object.flags = 0;
display->proxy.display = display;
display->proxy.object.implementation = (void(**)(void)) &display_listener;
display->proxy.user_data = display;
display->proxy.queue = &display->default_queue;
- display->proxy.flags = 0;
display->proxy.refcount = 1;
display->connection = wl_connection_create(display->fd);
@@ -1142,7 +1136,7 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
decrease_closure_args_refcount(closure);
proxy = closure->proxy;
- proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
+ proxy_destroyed = !!(proxy->object.flags & WL_OBJECT_FLAG_DESTROYED);
proxy->refcount--;
if (proxy_destroyed) {
diff --git a/src/wayland-private.h b/src/wayland-private.h
index db76081..ab48889 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -42,10 +42,16 @@
#define WL_SERVER_ID_START 0xff000000
#define WL_CLOSURE_MAX_ARGS 20
+enum wl_object_flags {
+ WL_OBJECT_FLAG_DESTROYED = 1 << 0,
+ WL_OBJECT_FLAG_ID_DELETED = 1 << 1
+};
+
struct wl_object {
const struct wl_interface *interface;
const void *implementation;
uint32_t id;
+ uint32_t flags;
};
extern struct wl_object global_zombie_object;
--
2.1.0
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Marek Chalupa
2015-04-10 13:56:22 UTC
Permalink
When we get request for new object without having the device,
mark the resource as inert

Signed-off-by: Marek Chalupa <***@gmail.com>
---
src/input.c | 34 +++++++++++++++++++++++++++-------
1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/input.c b/src/input.c
index 142c670..01d6a37 100644
--- a/src/input.c
+++ b/src/input.c
@@ -43,7 +43,9 @@ empty_region(pixman_region32_t *region)

static void unbind_resource(struct wl_resource *resource)
{
- wl_list_remove(wl_resource_get_link(resource));
+ /* we do not insert inert resources into lists */
+ if (!wl_resource_is_inert(resource))
+ wl_list_remove(wl_resource_get_link(resource));
}

WL_EXPORT void
@@ -1714,12 +1716,18 @@ seat_get_pointer(struct wl_client *client, struct wl_resource *resource,
return;
}

+ wl_resource_set_implementation(cr, &pointer_interface, seat->pointer,
+ unbind_resource);
+
+ if (seat->pointer_device_count == 0) {
+ wl_resource_set_inert(cr);
+ return;
+ }
+
/* May be moved to focused list later by either
* weston_pointer_set_focus or directly if this client is already
* focused */
wl_list_insert(&seat->pointer->resource_list, wl_resource_get_link(cr));
- wl_resource_set_implementation(cr, &pointer_interface, seat->pointer,
- unbind_resource);

if (seat->pointer->focus && seat->pointer->focus->surface->resource &&
wl_resource_get_client(seat->pointer->focus->surface->resource) == client) {
@@ -1787,12 +1795,18 @@ seat_get_keyboard(struct wl_client *client, struct wl_resource *resource,
return;
}

+ wl_resource_set_implementation(cr, &keyboard_interface,
+ seat, unbind_resource);
+
+ if (seat->keyboard_device_count == 0) {
+ wl_resource_set_inert(cr);
+ return;
+ }
+
/* May be moved to focused list later by either
* weston_keyboard_set_focus or directly if this client is already
* focused */
wl_list_insert(&seat->keyboard->resource_list, wl_resource_get_link(cr));
- wl_resource_set_implementation(cr, &keyboard_interface,
- seat, unbind_resource);

if (wl_resource_get_version(cr) >= WL_KEYBOARD_REPEAT_INFO_SINCE_VERSION) {
wl_keyboard_send_repeat_info(cr,
@@ -1866,6 +1880,14 @@ seat_get_touch(struct wl_client *client, struct wl_resource *resource,
return;
}

+ wl_resource_set_implementation(cr, &touch_interface,
+ seat, unbind_resource);
+
+ if (seat->touch_device_count == 0) {
+ wl_resource_set_inert(cr);
+ return;
+ }
+
if (seat->touch->focus &&
wl_resource_get_client(seat->touch->focus->surface->resource) == client) {
wl_list_insert(&seat->touch->resource_list,
@@ -1874,8 +1896,6 @@ seat_get_touch(struct wl_client *client, struct wl_resource *resource,
wl_list_insert(&seat->touch->focus_resource_list,
wl_resource_get_link(cr));
}
- wl_resource_set_implementation(cr, &touch_interface,
- seat, unbind_resource);
}

static const struct wl_seat_interface seat_interface = {
--
2.1.0
Derek Foreman
2015-09-23 16:33:23 UTC
Permalink
This one's been waiting for a long time...

I'm currently pretending to make an effort to clean up patchwork a bit. ;)
Post by David FORT
As stated in the very good blog post[1] of Pekka Paalanen, server-side we can have
sometime troubles with object that the server has deleted but that the client
is still requesting. This patch complements the scanner to create some code
that will return inert objects, ie objects that will do nothing and will never
return anything else but an inert object. An example is a get_pointer() on
a wl_seat, with an unplugged mouse (and so no pointer). Instead of keeping alive
the old pointer, we could bind that inert object and wait that the client release
it (which should happen very quickly as a wl_global_remove should be in the wire).
Personally, I'd like to NAK this one :/

Adding a way to hand out already-dead objects kind of makes me nervous
(perhaps for no good reason).

I think we have to keep the old pointers alive in weston for libinput
backed pointers because we want to retain their position if they go
away/come back.

This could happen on a USB bus reset and not on an intentional
unplug/plug event.

So this doesn't actually simplify compositor code?

The capabilities event from the seat should tell you that your pointer
isn't valid, and that already works.

I guess it's not clear to me what problem this is actually solving.
Post by David FORT
[1]: http://ppaalanen.blogspot.fi/2014/07/wayland-protocol-design-object-lifespan.html
---
src/scanner.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++----
wayland-scanner.mk | 3 +
2 files changed, 166 insertions(+), 10 deletions(-)
diff --git a/src/scanner.c b/src/scanner.c
index 1f1e59a..dbdd1f6 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -39,7 +39,7 @@ enum side {
static int
usage(int ret)
{
- fprintf(stderr, "usage: ./scanner [client-header|server-header|code]\n");
+ fprintf(stderr, "usage: ./scanner [client-header|server-header|code|server-inert]\n");
fprintf(stderr, "\n");
fprintf(stderr, "Converts XML protocol descriptions supplied on "
"stdin to client headers,\n"
@@ -626,6 +626,18 @@ emit_type(struct arg *a)
}
}
+static struct arg *
+get_return_type(struct message *m)
+{
+ struct arg *a, *ret = NULL;
+
+ wl_list_for_each(a, &m->arg_list, link) {
+ if (a->type == NEW_ID)
+ ret = a;
+ }
+ return ret;
+}
+
static void
emit_stubs(struct wl_list *message_list, struct interface *interface)
{
@@ -688,11 +700,7 @@ emit_stubs(struct wl_list *message_list, struct interface *interface)
continue;
}
- ret = NULL;
- wl_list_for_each(a, &m->arg_list, link) {
- if (a->type == NEW_ID)
- ret = a;
- }
+ ret = get_return_type(m);
if (ret && ret->interface_name == NULL)
printf("static inline void *\n");
@@ -1103,8 +1111,7 @@ emit_types(struct protocol *protocol, struct wl_list *message_list)
if (a->interface_name)
- printf("\t&%s_interface,\n",
- a->interface_name);
+ printf("\t&%s_interface,\n", a->interface_name);
else
printf("\tNULL,\n");
break;
@@ -1248,6 +1255,140 @@ emit_code(struct protocol *protocol)
}
}
+static void
+emit_inert_request(struct protocol *protocol, struct interface *interface, struct wl_list *message_list)
+{
+ struct message *m;
+ struct arg *a, *ret;
+ const char *newid_name;
+
+ wl_list_for_each(m, message_list, link) {
+ ret = get_return_type(m);
+
+ /* forward declaration for the returned object */
+ if (ret && ret->interface_name) {
+ printf("static void\n"
+ "create_inert_%s(struct wl_client *client, uint32_t version, uint32_t id);\n"
+ "\n",
+ ret->interface_name);
+ }
+
+ printf("static inline void inert_%s_%s(struct wl_client *client, struct wl_resource *resource",
+ interface->name, m->name
+ );
+
+ wl_list_for_each(a, &m->arg_list, link) {
+ if (a->type == NEW_ID) {
+ newid_name = a->name;
+
+ if (a->interface_name == NULL) {
+ printf(", const struct wl_interface *interface"
+ ", uint32_t version");
+ continue;
+ }
+ }
+
+ if (a->type == OBJECT) {
+ printf(", struct wl_resource *");
+ } else {
+ printf(", ");
+ emit_type(a);
+ }
+ printf("%s", a->name);
+ }
+
+ printf(")\n"
+ "{\n");
+
+ if (ret && ret->interface_name) {
+ printf("\tcreate_inert_%s(client, wl_resource_get_version(resource), %s);\n",
+ ret->interface_name, newid_name);
+ } else if (m->destructor) {
+ printf("\twl_resource_destroy(resource);\n");
+ }
+ printf("}\n\n");
+ }
+}
+
+static void
+emit_inert_interface(struct protocol *protocol, struct interface *i, struct wl_array *types) {
+ struct message *m;
+
+
+ if(wl_list_length(&i->request_list)) {
+ emit_inert_request(protocol, i, &i->request_list);
+
+ printf ("static const struct %s_interface inert_%s_implementation = {\n",
+ i->name, i->name);
+ wl_list_for_each(m, &i->request_list, link) {
+ printf("\tinert_%s_%s,\n", i->name, m->name);
+ }
+ printf ("};\n"
+ "\n");
+ }
+
+ printf("static inline void\n"
+ "create_inert_%s(struct wl_client *client, uint32_t version, uint32_t id) {\n",
+ i->name
+ );
+
+ if(wl_list_length(&i->request_list)) {
+ /* emit the method body only when there is requests on the object */
+ printf("\tstruct wl_resource *resource;\n"
+ "\tresource = wl_resource_create(client, &%s_interface, MIN(version, %d), id);\n"
+ "\twl_resource_set_implementation(resource, &inert_%s_implementation, NULL, wl_resource_destroy);\n",
+ i->name, i->version, i->name);
+ }
+
+ printf("}\n"
+ "\n");
+
+}
+
+
+static
+void emit_inert(struct protocol *protocol) {
+ struct interface *i;
+ struct wl_array types;
+
+ if (protocol->copyright)
+ format_copyright(protocol->copyright);
+
+ printf("#ifndef __%s_SERVER_INERT__\n"
+ "#define __%s_SERVER_INERT__\n"
+ "\n"
+ "#include \"%s-server-protocol.h\"\n"
+ "\n"
+ "#ifdef __cplusplus\n"
+ "extern \"C\" {\n"
+ "#endif\n"
+ "\n"
+ "#ifndef MIN\n"
+ "#define MIN(x,y) (((x) < (y)) ? (x) : (y))\n"
+ "#endif\n"
+ "\n",
+ protocol->uppercase_name,
+ protocol->uppercase_name,
+ //protocol->name,
+ protocol->name
+ );
+
+ wl_list_for_each(i, &protocol->interface_list, link) {
+ if (!strcmp(i->name, "wl_registry") || !strcmp(i->name, "wl_display"))
+ continue;
+
+ emit_inert_interface(protocol, i, &types);
+ }
+
+ printf("\n"
+ "#ifdef __cplusplus\n"
+ "}\n"
+ "#endif\n"
+ "\n"
+ "#endif\n");
+
+}
+
int main(int argc, char *argv[])
{
struct parse_context ctx;
@@ -1257,17 +1398,26 @@ int main(int argc, char *argv[])
enum {
CLIENT_HEADER,
SERVER_HEADER,
+ SERVER_INERT,
CODE,
} mode;
- if (argc != 2)
+ if (argc < 2) {
usage(EXIT_FAILURE);
- else if (strcmp(argv[1], "help") == 0 || strcmp(argv[1], "--help") == 0)
+ }
+
+ if (argc > 2) {
+ stdin = fopen(argv[2], "r");
+ }
+
+ if (strcmp(argv[1], "help") == 0 || strcmp(argv[1], "--help") == 0)
usage(EXIT_SUCCESS);
else if (strcmp(argv[1], "client-header") == 0)
mode = CLIENT_HEADER;
else if (strcmp(argv[1], "server-header") == 0)
mode = SERVER_HEADER;
+ else if (strcmp(argv[1], "server-inert") == 0)
+ mode = SERVER_INERT;
else if (strcmp(argv[1], "code") == 0)
mode = CODE;
else
@@ -1317,6 +1467,9 @@ int main(int argc, char *argv[])
emit_header(&protocol, SERVER);
break;
+ emit_inert(&protocol);
+ break;
emit_code(&protocol);
break;
diff --git a/wayland-scanner.mk b/wayland-scanner.mk
index 0a72062..fd994e0 100644
--- a/wayland-scanner.mk
+++ b/wayland-scanner.mk
@@ -6,3 +6,6 @@
%-client-protocol.h : $(wayland_protocoldir)/%.xml
+
+%-server-inert.h: $(wayland_protocoldir)/%.xml
\ No newline at end of file
Daniel Stone
2015-09-29 14:55:10 UTC
Permalink
Hi,
Post by Derek Foreman
Post by David FORT
As stated in the very good blog post[1] of Pekka Paalanen, server-side we can have
sometime troubles with object that the server has deleted but that the client
is still requesting. This patch complements the scanner to create some code
that will return inert objects, ie objects that will do nothing and will never
return anything else but an inert object. An example is a get_pointer() on
a wl_seat, with an unplugged mouse (and so no pointer). Instead of keeping alive
the old pointer, we could bind that inert object and wait that the client release
it (which should happen very quickly as a wl_global_remove should be in the wire).
Personally, I'd like to NAK this one :/
Adding a way to hand out already-dead objects kind of makes me nervous
(perhaps for no good reason).
I think we have to keep the old pointers alive in weston for libinput
backed pointers because we want to retain their position if they go
away/come back.
This could happen on a USB bus reset and not on an intentional
unplug/plug event.
So this doesn't actually simplify compositor code?
The capabilities event from the seat should tell you that your pointer
isn't valid, and that already works.
I guess it's not clear to me what problem this is actually solving.
I have to reluctantly agree with Derek. I think the idea is good, just
not the implementation; it only gets worse when you consider objects
created from those interfaces.

To wit:
- if someone gets an inert wl_seat, they're probably going to call
get_keyboard or get_pointer on it quite soon
- those requests _must_ generate valid objects, because there are
very legitimate race conditions where clients can call them, even
though server-side the object is now inert
- those interfaces can themselves generate other objects
- some of the behaviours there are interface-specific: e.g. with a
wl_seat, the global remove tells you that it's no longer valid, but
with a wl_pointer / wl_keyboard, you might have to poke the client
with an empty caps event
- other interfaces want to immediately send a release event as soon
as they're bound

I think the way to do this is ultimately going to require explicit
compositor behaviour that is aware of each object type. We can of
course make this easier by providing common helpers which are easy for
other implementers to copy (and also just to reduce open-coding), but
I've never really been convinced of the scanner approach.

(I know I've said this on IRC, but not sure I ever translated it to
mail - sorry.)

Cheers,
Daniel
Continue reading on narkive:
Loading...