[PATCH v2 weston] input: Don't test keyboard/pointer/touch pointers

[Derek Foreman]

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
> patch.

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 =
>>           &wm->server->compositor->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)
>>           return;
>>
>>       detail = client_message->data.data32[2];

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)
>         return;

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 =
udev_lala() there.

> 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,
> surface_rotate.
> 

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.