[PATCH v2 weston] input: Don't test keyboard/pointer/touch pointers
derekf at osg.samsung.com
Mon Feb 2 13:24:24 PST 2015
Thanks for looking at this rather cumbersome patch! :)
On 02/02/15 01:40 PM, Bill Spitzak wrote:
> Is there a reason it does not just clear the pointer when
> keyboard_device_count is changed to zero? That would seem like a smaller
Pulled Jonas Ådahl in on the CC for that question... I understand why
weston_pointer isn't freed when released, but is there a need for the
same behavior for weston_keyboard and weston_touch?
> Assuming there is a good reason there seem to be some errors. At first I
> thought there were just redundant checks for seat being null, which I
> think should be removed, but I then found that most of them were actual
> errors. For instance right at the end:
>> @@ -1270,13 +1274,14 @@ weston_wm_window_handle_moveresize(struct
>> weston_wm_window *window,
>> struct weston_wm *wm = window->wm;
>> struct weston_seat *seat = weston_wm_pick_seat_for_window(window);
>> + struct weston_pointer *pointer = weston_seat_get_pointer(seat);
>> int detail;
>> struct weston_shell_interface *shell_interface =
>> - if (seat == NULL || seat->pointer->button_count != 1
>> - || !seat->pointer->focus
>> - || seat->pointer->focus->surface != window->surface)
>> + if (seat == NULL || pointer->button_count != 1
>> + || !pointer->focus
>> + || pointer->focus->surface != window->surface)
>> detail = client_message->data.data32;
Yeah, this and lots of other code assume seat has a pointer. Some of
it's safe (things further up the call stack already determined a pointer
was present), some not so much. :(
I think in this case the seat was picked by
weston_wm_pick_seat_for_window(), and it has a pointer. (except I broke
part of the test in that in this rev of the patch... heh)
> That will crash if the pointer count is zero. The correct if statement,
> which also removes the redundant check for seat being null, is (I think):
> if (!pointer || pointer->button_count != 1
> || !pointer->focus
> || pointer->focus->surface != window->surface)
I'll include it in the next revision of the patch. Thanks.
> I then started looking backwards through the code and spotted these:
> setup_output_seat_constraint which is passing &seat->base to
> weston_seat_get_pointer, followed by a test of (seat && pointer). That
> will not work if seat is null, so either you must test that first or the
> test for seat is irrelevant and should be removed.
Ugh, I definitely broke that.
I'd be inclined to if (!seat) return; immediately after seat =
> force_kill_binding() appears to have deleted the setting of the
> focus_surface variable.
I don't see it... I think I just moved it to the top of the function?
> Several other functions look like they should take a
> pointer/keyboard/touch as an argument instead of a seat, as the only
> thing they do is extract the pointer and then act like it is never going
> to be null. Examples are click_to_activate_binding, rotate_binding,
Yeah, I'll look into that for a follow up, but those aren't made any
more or less buggy by this patch I don't think.
Some of the bindings require a mouse button press to activate so can't
possibly be called without a valid pointer, so there may be less bugs
here than anticipated.
More information about the wayland-devel