Re: [PATCH] rdp: don't release the seat until it is safe v2
On Fri, 20 May 2016 15:02:02 +0200
Hardening wrote:
> Le 20/05/2016 11:58, Sam Spilsbury a écrit :
> > On Fri, May 20, 2016 at 5:33 PM, David Fort 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
> >> ---
> >> 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
> >
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
Thanks,
pq
pgpmxxkvxjQ86.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] rdp: don't release the seat until it is safe v2
Le 20/05/2016 11:58, Sam Spilsbury a écrit :
> On Fri, May 20, 2016 at 5:33 PM, David Fort 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
>> ---
>> 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.
>>
>> 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
>
--
David FORT
website: http://www.hardening-consulting.com/
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] rdp: don't release the seat until it is safe v2
On Fri, May 20, 2016 at 5:33 PM, David Fort 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
> ---
> 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.
>
> 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.
Reviewed-by: Sam Spilsbury
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] rdp: don't release the seat until it is safe v2
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
---
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);*/
}
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);
peersItem->flags |= RDP_PEER_ACTIVATED;
@@ -952,7 +959,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x,
UINT16 y)
if (flags & PTR_FLAGS_MOVE) {
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
- notify_motion_absolute(&peerContext->item.seat,
weston_compositor_get_time(),
+ notify_motion_absolute(peerContext->item.seat,
weston_compositor_get_time(),
x, y);
need_frame = true;
}
@@ -966,7 +973,7 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x,
UINT16 y)
button = BTN_MIDDLE;
if (button) {
- notify_button(&peerContext->item.seat,
weston_compositor_get_time(), button,
+ notify_button(peerContext->item.seat,
weston_compositor_get_time(), button,
(flags & PTR_FLAGS_DOWN) ?
WL_POINTER_BUTTON_STATE_PRESSED : WL_POINTER_BUTTON_STATE_RELEASED
);
need_frame = true;
@@ -991,13 +998,13 @@ xf_mouseEvent(rdpInput *input, UINT16 flags, UINT16 x,
UINT16 y)
weston_event.discrete = (int)value;
weston_event.has_discrete = true;
- notify_axis(&peerContext->item.seat,
weston_compositor_get_time(),
+ notify_axis(peerContext->item.seat,
weston_compositor_get_time(),
&weston_event);
need_frame = true;
}
if (need_frame)
- notify_pointer_frame(&peerContext->item.seat);
+ notify_pointer_frame(peerContext->item.seat);
FREERDP_CB_RETURN(TRUE);
}
@@ -1010,7 +1017,7 @@ xf_extendedMouseEvent(rdpInput *input, UINT16 flags,
UINT16 x, UINT16 y)
output = peerContext->rdpBackend->output;
if (x < output->base.width && y < output->base.height) {
- notify_motion_absolute(&peerContext->item.seat,
weston_compositor_get_time(),
+ notify_motion_absolute(peerContext->item.seat,
weston_compositor_get_time(),
x, y);
}
@@ -1073,7 +1080,7 @@ xf_input_keyboard_event(rdpInput *input, UINT16 flags,
UINT16 code)
/*weston_log("code=%x ext=%d vk_code=%x scan_code=%x\n", code,
(flags & KBD_FLAGS_EXTENDED) ? 1 : 0,
vk_code, scan_code);*/
- notify_key(&peerContext->item.seat,
weston_compositor_get_time(),
+ notify_key(peerContext->item.seat, weston_compositor_get_time(),
scan_code - 8, keyState,
STATE_UPDATE_AUTOMATIC);
}
--
2.7.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesk
