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. _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
