Re: [PATCH 3/3] Add a release request on wl_seat
On Sun, Sep 27, 2015 at 10:51:40PM +0200, Hardening wrote: > Le 24/09/2015 01:59, Jonas Ådahl a écrit : > > On Wed, Sep 23, 2015 at 11:17:33AM -0500, Derek Foreman wrote: > >> On 25/02/15 08:03 AM, David FORT wrote: > >>> 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. > > [...] > > >>> + > >>> + > >>> + > >>> + > >>> + > >>> + > >> > >> 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. > > >> > >> 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) > >> > > [...] > >> > >> 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 > > -- > David FORT > website: http://www.hardening-consulting.com/ > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 3/3] Add a release request on wl_seat
Le 24/09/2015 01:59, Jonas Ådahl a écrit : > On Wed, Sep 23, 2015 at 11:17:33AM -0500, Derek Foreman wrote: >> On 25/02/15 08:03 AM, David FORT wrote: >>> 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. [...] >>> + >>> + >>> + >>> + >>> + >>> + >> >> 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 ? >> >> 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) >> [...] >> >> 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/ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 3/3] Add a release request on wl_seat
On Wed, Sep 23, 2015 at 11:17:33AM -0500, Derek Foreman wrote: > On 25/02/15 08:03 AM, David FORT wrote: > > 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 @@ > > > > > > > > + > > + > > + > > + > > + > > + > > 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. > > 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 > > 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 > > > > > > > > > > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 3/3] Add a release request on wl_seat
On 25/02/15 08:03 AM, David FORT wrote: > 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 @@ > > > > + > + > + > + > + > + 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 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? > > > > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
