On Fri, 20 May 2016 15:02:02 +0200 Hardening <[email protected]> wrote:
> Le 20/05/2016 11:58, Sam Spilsbury a écrit : > > On Fri, May 20, 2016 at 5:33 PM, David Fort <[email protected]> wrote: > >> Releasing a seat is not safe, so let's just announce it without keyboard > >> and mouse until this is fixed. Without this patch we just can't reconnect > >> on > >> the RDP compositor as it crashes. > >> > >> v2: fixed the leak of the xkb_keymap > >> > >> Signed-off-by: David Fort <[email protected]> > >> --- > >> src/compositor-rdp.c | 33 ++++++++++++++++++++------------- > >> 1 file changed, 20 insertions(+), 13 deletions(-) > >> > >> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c > >> index 4fc7c74..52cf426 100644 > >> --- a/src/compositor-rdp.c > >> +++ b/src/compositor-rdp.c > >> @@ -109,7 +109,7 @@ enum peer_item_flags { > >> struct rdp_peers_item { > >> int flags; > >> freerdp_peer *peer; > >> - struct weston_seat seat; > >> + struct weston_seat *seat; > >> > >> struct wl_list link; > >> }; > >> @@ -640,9 +640,9 @@ rdp_peer_context_free(freerdp_peer* client, > >> RdpPeerContext* context) > >> } > >> > >> if (context->item.flags & RDP_PEER_ACTIVATED) { > >> - weston_seat_release_keyboard(&context->item.seat); > >> - weston_seat_release_pointer(&context->item.seat); > >> - weston_seat_release(&context->item.seat); > >> + weston_seat_release_keyboard(context->item.seat); > >> + weston_seat_release_pointer(context->item.seat); > >> + /*weston_seat_release(context->item.seat);*/ > >> } > > > > I think instead of just having commented out code, put the reasons why > > the seat cannot be released safely at the moment. Just having it > > commented out will confuse future maintainers. > > > > Well, future maintainers actually means me. The explanation is quite > long, I don't think a comment in the code is the right place for this. > And BTW it's not something that is RDP compositor specific, all the > weston compositors have it. That is even more reason to add a comment there. Please do so, gather review tags, and re-send. The future you is not the same as today's you. You don't have to make comment exhaustive, even though that would be good. As long as there is /* XXX: should weston_seat_release() here, but it will crash on reconnect */ or something like that would be enough. > >> > >> Stream_Free(context->encode_stream, TRUE); > >> @@ -911,9 +911,16 @@ xf_peer_activate(freerdp_peer* client) > >> else > >> snprintf(seat_name, sizeof(seat_name), "RDP peer @%s", > >> settings->ClientAddress); > >> > >> - weston_seat_init(&peersItem->seat, b->compositor, seat_name); > >> - weston_seat_init_keyboard(&peersItem->seat, keymap); > >> - weston_seat_init_pointer(&peersItem->seat); > >> + peersItem->seat = zalloc(sizeof(*peersItem->seat)); > >> + if (!peersItem->seat) { > >> + xkb_keymap_unref(keymap); > >> + weston_log("unable to create a weston_seat\n"); > >> + return FALSE; > >> + } > >> + > >> + weston_seat_init(peersItem->seat, b->compositor, seat_name); > >> + weston_seat_init_keyboard(peersItem->seat, keymap); > >> + weston_seat_init_pointer(peersItem->seat); > > > > Any reason to make this dynamically allocated memory? It seems to me > > like it is adding an additional point of failure for little added > > benefit. If it needs to be dynamically allocated, I think such a > > change should come in a separate patch, since it seems unrelated to > > this one. > > > > I have already answered this in a previous mail to Bryce. To do it > short, it's because the seat has to live longer than the RdpPeerContext. > So when the RDP peer disconnects, the RdpPeerContext will be destroyed > immediately, while the seat is supposed to live until all wayland client > have released it. > > > > Reviewed-by: Sam Spilsbury <[email protected]> > > Yeah, it indeed seems no other backend dynamically destroys and re-creates whole seats, they only do it with the specific device capabilities, so the idea of this patch is good to me. However, I think you really should store the weston_seat pointer somewhere where it does not get lost, so you can re-use it, and destroy it on compositor shutdown. Anyway, leaking seems better than crashing, so: Acked-by: Pekka Paalanen <[email protected]> Thanks, pq
pgpmxxkvxjQ86.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
