From eb408727706086f9d908a37edc84cc5e2ffdc36b Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Wed, 23 Sep 2020 11:39:58 +1000 Subject: [PATCH] Fix client-side removal of devices Because events may be in-transit when a client removes the device, we need to make this a full roundtrip to the server. Otherwise the client may assume a device is removed, releases all references and then gets the original device added event for that device. Better to have this as a round-trip instead. This requires the server to call eis_device_disconnect() on the removed notifications but we do so during eis_event_unref() anyway in case the server forgets. And it changes some of the API behaviors, so adjust the tests for that. Signed-off-by: Peter Hutterer --- src/libei-device.c | 7 ++- src/libei.c | 35 ++++++++---- src/libei.h | 7 ++- src/libeis-client.c | 16 +++++- src/libeis-event.c | 4 +- src/libeis.h | 3 +- test/test-ei.c | 131 ++++++++++++++++++++++++++++++++++++++++---- test/test-eis.c | 10 ++++ 8 files changed, 184 insertions(+), 29 deletions(-) diff --git a/src/libei-device.c b/src/libei-device.c index fb4c525..0a57ee5 100644 --- a/src/libei-device.c +++ b/src/libei-device.c @@ -218,8 +218,13 @@ ei_device_add(struct ei_device *device) _public_ void ei_device_remove(struct ei_device *device) { - if (device->state == EI_DEVICE_STATE_REMOVED) + switch (device->state) { + case EI_DEVICE_STATE_NEW: + case EI_DEVICE_STATE_REMOVED: return; + default: + break; + } ei_device_set_state(device, EI_DEVICE_STATE_REMOVED); ei_remove_device(device); diff --git a/src/libei.c b/src/libei.c index b9cd0f7..35acc47 100644 --- a/src/libei.c +++ b/src/libei.c @@ -291,6 +291,25 @@ connection_send_key(struct ei *ei, struct ei_device *device, return ei_proto_send_key(ei, device, k, is_press); } +static void +ei_drop_device(struct ei_device *device) +{ + switch (device->state) { + case EI_DEVICE_STATE_NEW: + break; + /* In any of these states the client has added the device, so we + * must send the REMOVED */ + case EI_DEVICE_STATE_CONNECTING: + case EI_DEVICE_STATE_SUSPENDED: + case EI_DEVICE_STATE_RESUMED: + case EI_DEVICE_STATE_REMOVED: + queue_removed_event(device); + break; + } + list_remove(&device->link); + ei_device_unref(device); +} + void ei_disconnect(struct ei *ei) { @@ -306,7 +325,11 @@ ei_disconnect(struct ei *ei) struct ei_device *d, *tmp; list_for_each_safe(d, tmp, &ei->devices, link) { + /* remove the device */ ei_device_remove(d); + /* And pretend to process the removed message from the + * server */ + ei_drop_device(d); } if (state != EI_STATE_NEW) { @@ -340,15 +363,9 @@ ei_remove_device(struct ei_device *device) { struct ei *ei = ei_device_get_context(device); int rc = connection_send_remove(ei, device); - - if (ei->state == EI_STATE_DISCONNECTING) - queue_removed_event(device); - - list_remove(&device->link); - ei_device_unref(device); - if (rc) ei_disconnect(ei); + return rc; } @@ -387,9 +404,7 @@ handle_msg_removed(struct ei *ei, uint32_t deviceid) list_for_each(d, &ei->devices, link) { if (d->id == deviceid) { - queue_removed_event(d); - list_remove(&d->link); - ei_device_unref(d); + ei_drop_device(d); break; } } diff --git a/src/libei.h b/src/libei.h index fe5c6ae..f3903c8 100644 --- a/src/libei.h +++ b/src/libei.h @@ -456,9 +456,10 @@ ei_device_add(struct ei_device *device); /** * Notify the server that the device should be removed. * - * The server will **not** send an @ref EI_EVENT_DEVICE_REMOVED event for - * this device, it should be considered removed by the client once this - * function completes. + * Due to the asynchronous nature of the client-server interaction, + * events for this device may still be in transit. The server will send an + * @ref EI_EVENT_DEVICE_REMOVED event for this device. After that event, + * device is considered removed by the server. * * This does not release any resources associated with this device, use * ei_device_unref() for any references held by the client. diff --git a/src/libeis-client.c b/src/libeis-client.c index 101271f..278a37c 100644 --- a/src/libeis-client.c +++ b/src/libeis-client.c @@ -200,6 +200,20 @@ client_new_device(struct eis_client *client, return 0; } +static int +client_remove_device(struct eis_client *client, uint32_t deviceid) +{ + struct eis_device *device; + + list_for_each(device, &client->devices, link) { + if (device->id == deviceid) { + eis_queue_removed_event(device); + break; + } + } + return 0; +} + static int client_pointer_rel(struct eis_client *client, uint32_t deviceid, double x, double y) @@ -370,7 +384,7 @@ client_connected_handle_msg(struct eis_client *client, msg->add_device.keymap_size); break; case MESSAGE_REMOVE_DEVICE: - /* FIXME: remove device */ + rc = client_remove_device(client, msg->remove_device.deviceid); break; case MESSAGE_POINTER_REL: rc = client_pointer_rel(client, msg->pointer_rel.deviceid, diff --git a/src/libeis-event.c b/src/libeis-event.c index 7386a21..cd48b2c 100644 --- a/src/libeis-event.c +++ b/src/libeis-event.c @@ -37,11 +37,13 @@ eis_event_destroy(struct eis_event *event) case EIS_EVENT_CLIENT_CONNECT: case EIS_EVENT_CLIENT_DISCONNECT: case EIS_EVENT_DEVICE_ADDED: - case EIS_EVENT_DEVICE_REMOVED: case EIS_EVENT_POINTER_BUTTON: case EIS_EVENT_POINTER_MOTION: case EIS_EVENT_KEYBOARD_KEY: break; + case EIS_EVENT_DEVICE_REMOVED: + eis_device_disconnect(event->device); + break; default: abort(); /* not yet implemented */ } diff --git a/src/libeis.h b/src/libeis.h index ba7e224..5b183be 100644 --- a/src/libeis.h +++ b/src/libeis.h @@ -68,7 +68,8 @@ enum eis_event_type { */ EIS_EVENT_DEVICE_ADDED, /** - * The device created by the client was removed. + * The device created by the client was removed. The server must + * call eis_device_disconnect() in response to this device. * * libeis guarantees this event is generated before * @ref EIS_EVENT_CLIENT_DISCONNECT. diff --git a/test/test-ei.c b/test/test-ei.c index d110c13..3a30d2b 100644 --- a/test/test-ei.c +++ b/test/test-ei.c @@ -107,8 +107,6 @@ MUNIT_TEST(test_ei_disconnect_after_add_before_received) eis_client_disconnect(client); } - /* But from the POV of the client we have added our devices, so we - * must receive a Removed event. */ with_client(peck) { ei_dispatch(ei); _cleanup_ei_event_ struct ei_event *removed = @@ -199,16 +197,17 @@ MUNIT_TEST(test_ei_disconnect_after_remove_before_received) ei_device_remove(device); } - /* Disconnect before the server processed the Removed message. This - * does **not** trigger a EI_EVENT_REMOVED, the device has already - * been removed by the client. - */ + /* No server dispatch here so the server isn't aware of the + * ei_device_remove() call. Disconnect the client, this + * automatically removes all devices */ with_server(peck) { eis_client_disconnect(client); } with_client(peck) { ei_dispatch(ei); + _cleanup_ei_event_ struct ei_event *removed = + peck_ei_next_event(ei, EI_EVENT_DEVICE_REMOVED); _cleanup_ei_event_ struct ei_event *disconnect = peck_ei_next_event(ei, EI_EVENT_DISCONNECT); } @@ -253,10 +252,7 @@ MUNIT_TEST(test_ei_disconnect_after_remove_after_received) ei_device_remove(device); } - /* Disconnect after the server processed the Removed message. This - * does **not** trigger a EI_EVENT_REMOVED, the device has already - * been removed by the client. - */ + /* Dispatch, server is aware of the ei_device_remove() */ peck_dispatch_eis(peck); with_server(peck) { eis_client_disconnect(client); @@ -264,6 +260,9 @@ MUNIT_TEST(test_ei_disconnect_after_remove_after_received) with_client(peck) { ei_dispatch(ei); + _cleanup_ei_event_ struct ei_event *removed = + peck_ei_next_event(ei, EI_EVENT_DEVICE_REMOVED); + _cleanup_ei_event_ struct ei_event *disconnect = peck_ei_next_event(ei, EI_EVENT_DISCONNECT); } @@ -317,12 +316,12 @@ MUNIT_TEST(test_ei_device_basics) return MUNIT_OK; } -MUNIT_TEST(test_ei_device_add) +MUNIT_TEST(test_ei_device_add_remove) { _cleanup_peck_ struct peck *peck = peck_new(); with_server(peck) { - peck_enable_eis_behavior(peck, PECK_EIS_BEHAVIOR_ACCEPT_ALL); + peck_enable_eis_behavior(peck, PECK_EIS_BEHAVIOR_ACCEPT_CLIENT); peck_dispatch_eis(peck); } @@ -341,18 +340,126 @@ MUNIT_TEST(test_ei_device_add) peck_dispatch_until_stable(peck); + _cleanup_eis_device_ struct eis_device *eis_device = NULL; + + with_server(peck) { + _cleanup_eis_event_ struct eis_event *event = + peck_eis_next_event(eis, EIS_EVENT_DEVICE_ADDED); + + eis_device = eis_device_ref(eis_event_get_device(event)); + eis_device_allow_capability(eis_device, EIS_DEVICE_CAP_POINTER); + eis_device_connect(eis_device); + } + with_client(peck) { + peck_dispatch_ei(peck); + _cleanup_ei_event_ struct ei_event *event = peck_ei_next_event(ei, EI_EVENT_DEVICE_ADDED); struct ei_device *added = ei_event_get_device(event); munit_assert_ptr_equal(device, added); munit_assert(ei_device_has_capability(added, EI_DEVICE_CAP_POINTER)); + + ei_device_remove(device); + } + + with_server(peck) { + peck_dispatch_eis(peck); + _cleanup_eis_event_ struct eis_event *event = + peck_eis_next_event(eis, EIS_EVENT_DEVICE_REMOVED); + + munit_assert_ptr_equal(eis_event_get_device(event), eis_device); + eis_device_disconnect(eis_device); + } + + with_client(peck) { + peck_dispatch_ei(peck); + + _cleanup_ei_event_ struct ei_event *event = + peck_ei_next_event(ei, EI_EVENT_DEVICE_REMOVED); + + struct ei_device *removed = ei_event_get_device(event); + munit_assert_ptr_equal(device, removed); + } + + + return MUNIT_OK; +} + +MUNIT_TEST(test_ei_device_remove_forget_disconnect) +{ + _cleanup_peck_ struct peck *peck = peck_new(); + + peck_enable_eis_behavior(peck, PECK_EIS_BEHAVIOR_ACCEPT_ALL); + peck_enable_ei_behavior(peck, PECK_EI_BEHAVIOR_AUTODEVICES); + peck_dispatch_until_stable(peck); + + _cleanup_ei_device_ struct ei_device *d1 = NULL; + _cleanup_ei_device_ struct ei_device *d2 = NULL; + + with_client(peck) { + d1 = ei_device_new(ei); + ei_device_configure_name(d1, "first"); + ei_device_configure_capability(d1, EI_DEVICE_CAP_POINTER); + ei_device_add(d1); + + d2 = ei_device_new(ei); + ei_device_configure_name(d2, "second"); + ei_device_configure_capability(d2, EI_DEVICE_CAP_KEYBOARD); + ei_device_add(d2); + } + + peck_dispatch_until_stable(peck); + peck_enable_eis_behavior(peck, PECK_EIS_BEHAVIOR_NONE); + + with_client(peck) { + /* remove in order d1, d2 */ + ei_device_remove(d1); + ei_device_remove(d2); + } + + peck_dispatch_until_stable(peck); + + struct eis_event *e1 = NULL; + with_server(peck) { + e1 = peck_eis_next_event(eis, EIS_EVENT_DEVICE_REMOVED); + _cleanup_eis_event_ struct eis_event *e2 = + peck_eis_next_event(eis, EIS_EVENT_DEVICE_REMOVED); + + /* Explicitly disconnect device d2 */ + eis_device_disconnect(eis_event_get_device(e2)); + /* But forget to eis_device_disconnect(d1), happens + * automatically when the event is unref'd */ + } + + peck_dispatch_ei(peck); + with_client(peck) { + _cleanup_ei_event_ struct ei_event *event = + peck_ei_next_event(ei, EI_EVENT_DEVICE_REMOVED); + + struct ei_device *removed = ei_event_get_device(event); + munit_assert_ptr_equal(d2, removed); + peck_assert_no_ei_events(ei); + } + + /* unref should trigger eis_device_disconnect() */ + eis_event_unref(e1); + + peck_dispatch_ei(peck); + with_client(peck) { + _cleanup_ei_event_ struct ei_event *event = + peck_ei_next_event(ei, EI_EVENT_DEVICE_REMOVED); + + struct ei_device *removed = ei_event_get_device(event); + munit_assert_ptr_equal(d1, removed); + peck_assert_no_ei_events(ei); } return MUNIT_OK; } + MUNIT_TEST(test_ei_device_add_drop_caps) { _cleanup_peck_ struct peck *peck = peck_new(); diff --git a/test/test-eis.c b/test/test-eis.c index 90bd089..1a74690 100644 --- a/test/test-eis.c +++ b/test/test-eis.c @@ -284,12 +284,14 @@ MUNIT_TEST(eistest_device_late_connect) peck_dispatch_until_stable(peck); with_client(peck) { + /* remove before server accepted */ ei_device_remove(ei_device); } peck_dispatch_until_stable(peck); with_server(peck) { + /* accept even though the remove event is already pending */ eis_device_allow_capability(eis_device, EIS_DEVICE_CAP_POINTER); eis_device_connect(eis_device); } @@ -297,10 +299,18 @@ MUNIT_TEST(eistest_device_late_connect) peck_dispatch_until_stable(peck); with_server(peck) { + _cleanup_eis_event_ struct eis_event *removed = + peck_eis_next_event(eis, EIS_EVENT_DEVICE_REMOVED); peck_assert_no_eis_events(eis); } + peck_dispatch_until_stable(peck); + with_client(peck) { + _cleanup_ei_event_ struct ei_event *added = + peck_ei_next_event(ei, EI_EVENT_DEVICE_ADDED); + _cleanup_ei_event_ struct ei_event *removed = + peck_ei_next_event(ei, EI_EVENT_DEVICE_REMOVED); peck_assert_no_ei_events(ei); }