libei: require ei_device_remove() for a never-added device

This simplifies the handling of devices that were never added a bit, including
handling the refs between seat and device. And for legitimate use-cases
there's no reason why a caller would create a device but never add it.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

src/libei-device.c

@@ -59,38 +59,13 @@ ei_device_set_state(struct ei_device *device,
 		  ei_device_state_to_string(state));
 }
 
-static void
-ei_device_add_to_seat(struct ei_device *device)
-{
-	struct ei_seat *seat = ei_device_get_seat(device);
-
-	list_remove(&device->link); /* remove from pending */
-	list_append(&seat->devices, &device->link);
-	/* Once the seat has the device proper, it owns a ref to the
-	 * device (and the device always owns a ref to the seat)
-	 */
-	ei_device_ref(device);
-
-	ei_device_set_state(device, EI_DEVICE_STATE_CONNECTING);
-}
-
-static void
-ei_device_remove_from_seat(struct ei_device *device)
-{
-	list_remove(&device->link);
-	list_init(&device->link);
-	/* seat only has a ref for devices already added */
-	if (device->state != EI_DEVICE_STATE_NEW)
-		ei_device_unref(device);
-}
-
 static void
 ei_device_destroy(struct ei_device *device)
 {
 	struct ei_seat *seat = ei_device_get_seat(device);
 
-	if (device->state == EI_DEVICE_STATE_NEW)
-		ei_device_remove_from_seat(device);
+	assert(device->state == EI_DEVICE_STATE_DEAD);
+	list_remove(&device->link);
 	ei_keymap_unref(device->keymap);
 	ei_seat_unref(seat);
 	free(device->name);
@@ -138,12 +113,15 @@ ei_device_new(struct ei_seat *seat)
 	device->state = EI_DEVICE_STATE_NEW;
 	device->name = xaprintf("unnamed device %d", device->id);
 
-	list_append(&seat->devices_pending, &device->link); /* not a ref! */
-
 	/* We have a ref to the seat to make sure our seat doesn't get
 	 * destroyed while a ref to the device is still alive.
+	 * And the seat has a ref to the device, dropped when the device
+	 * state changes to DEAD (ei_device_remove()).
 	 */
 	ei_seat_ref(seat);
+	ei_device_ref(device);
+
+	list_append(&seat->devices, &device->link);
 
 	return device;
 }
@@ -354,7 +332,7 @@ ei_device_add(struct ei_device *device)
 			       __func__, device->name);
 
 	if (ei_send_add_device(device) == 0)
-		ei_device_add_to_seat(device);
+		ei_device_set_state(device, EI_DEVICE_STATE_CONNECTING);
 }
 
 _public_ void
@@ -362,14 +340,17 @@ ei_device_remove(struct ei_device *device)
 {
 	switch (device->state) {
 	case EI_DEVICE_STATE_NEW:
+		ei_device_set_state(device, EI_DEVICE_STATE_DEAD);
+		ei_device_unref(device);
+		break;
 	case EI_DEVICE_STATE_DEAD:
 	case EI_DEVICE_STATE_REMOVED_FROM_CLIENT:
 		break;
 	case EI_DEVICE_STATE_REMOVED_FROM_SERVER:
 		/* device is properly dead now */
 		ei_device_set_state(device, EI_DEVICE_STATE_DEAD);
-		ei_device_remove_from_seat(device);
 		ei_send_remove_device(device);
+		ei_device_unref(device);
 		break;
 	case EI_DEVICE_STATE_CONNECTING:
 	case EI_DEVICE_STATE_SUSPENDED:
@@ -392,14 +373,14 @@ ei_device_removed_by_server(struct ei_device *device)
 		/* device is properly dead now */
 		ei_queue_device_removed_event(device);
 		ei_device_set_state(device, EI_DEVICE_STATE_DEAD);
-		ei_device_remove_from_seat(device);
+		ei_device_unref(device);
 		break;
 	case EI_DEVICE_STATE_CONNECTING:
 		/* Device never seen by server, but our client still thinks
 		 * it's there */
 		ei_queue_device_removed_event(device);
 		ei_device_set_state(device, EI_DEVICE_STATE_DEAD);
-		ei_device_remove_from_seat(device);
+		ei_device_unref(device);
 		break;
 	case EI_DEVICE_STATE_SUSPENDED:
 	case EI_DEVICE_STATE_RESUMED:
@@ -647,8 +628,12 @@ ei_touch_up(struct ei_touch *touch)
 #include "src/util-memfile.h"
 
 #define FAKE_SEAT(_seat) \
+	struct ei ei_##__LINE__ = { 0 }; \
 	struct ei_seat seat_##__LINE__ = { \
-		.object = { .refcount = 1, }, \
+		.object = { \
+			.parent = &ei_##__LINE__.object, \
+			.refcount = 1, \
+		}, \
 		.id = __LINE__ << 16, \
 		.capabilities = ~0, \
 		.name = "default", \
@@ -669,6 +654,9 @@ MUNIT_TEST(test_device_new)
 
 	struct ei_device *unrefd = ei_device_unref(d);
 	munit_assert_ptr_null(unrefd);
+
+	ei_device_remove(d);
+
 	return MUNIT_OK;
 }
 
@@ -683,6 +671,10 @@ MUNIT_TEST(test_device_ids)
 	munit_assert_int(d1->id, <, d3->id);
 	munit_assert_int(d2->id, <, d3->id);
 
+	ei_device_remove(d1);
+	ei_device_remove(d2);
+	ei_device_remove(d3);
+
 	return MUNIT_OK;
 }
 
@@ -691,14 +683,17 @@ MUNIT_TEST(test_device_ref_unref)
 	FAKE_SEAT(seat);
 	struct ei_device *d = ei_device_new(seat);
 
-	munit_assert_int(d->object.refcount, ==, 1);
+	munit_assert_int(d->object.refcount, ==, 2);
 
 	struct ei_device *refd = ei_device_ref(d);
 	munit_assert_ptr_equal(d, refd);
-	munit_assert_int(d->object.refcount, ==, 2);
+	munit_assert_int(d->object.refcount, ==, 3);
 
 	struct ei_device *unrefd = ei_device_unref(d);
 	munit_assert_ptr_null(unrefd);
+	munit_assert_int(d->object.refcount, ==, 2);
+
+	ei_device_remove(d);
 	munit_assert_int(d->object.refcount, ==, 1);
 
 	unrefd = ei_device_unref(d);
@@ -732,6 +727,8 @@ MUNIT_TEST(test_device_cap)
 	munit_assert(!ei_device_configure_capability(d, EI_DEVICE_CAP_TOUCH + 1));
 	munit_assert(!ei_device_has_capability(d, EI_DEVICE_CAP_TOUCH + 1));
 
+	ei_device_remove(d);
+
 	return MUNIT_OK;
 }
 
@@ -743,6 +740,8 @@ MUNIT_TEST(test_device_get_seat)
 	munit_assert_ptr_equal(d->object.parent, seat);
 	munit_assert_ptr_equal(ei_device_get_seat(d), seat);
 
+	ei_device_remove(d);
+
 	return MUNIT_OK;
 }
 
@@ -775,6 +774,8 @@ MUNIT_TEST(test_device_pointer_ranges)
 	munit_assert_int(ei_device_pointer_get_width(d), ==, 640);
 	munit_assert_int(ei_device_pointer_get_height(d), ==, 480);
 
+	ei_device_remove(d);
+
 	return MUNIT_OK;
 }
 
@@ -807,6 +808,8 @@ MUNIT_TEST(test_device_touch_ranges)
 	munit_assert_int(ei_device_touch_get_width(d), ==, 640);
 	munit_assert_int(ei_device_touch_get_height(d), ==, 480);
 
+	ei_device_remove(d);
+
 	return MUNIT_OK;
 }
 

src/libei-seat.c

@@ -71,7 +71,6 @@ ei_seat_new(struct ei *ei, uint32_t id, const char *name, uint32_t capabilities)
 	seat->capabilities = capabilities;
 
 	list_init(&seat->devices);
-	list_init(&seat->devices_pending);
 	list_init(&seat->link);
 
 	return seat; /* ref owned by caller */
@@ -120,9 +119,5 @@ ei_seat_find_device(struct ei_seat *seat, uint32_t deviceid)
 			return device;
 	}
 
-	list_for_each(device, &seat->devices_pending, link) {
-		if (device->id == deviceid)
-			return device;
-	}
 	return NULL;
 }

src/libei.h

@@ -532,6 +532,10 @@ ei_device_unref(struct ei_device *device);
  * the device, then call ei_device_add() to request creation of the device
  * by the server.
  *
+ * @note A caller that does not want to add a created device to a seat
+ *	**must** call ei_device_remove() for this device to ensure the
+ *	resources are released.
+ *
  * The returned object must be released by the caller with ei_event_unref()
  */
 struct ei_device *

test/test-ei.c

@@ -596,6 +596,44 @@ MUNIT_TEST(test_ei_device_set_name)
 	return MUNIT_OK;
 }
 
+MUNIT_TEST(test_ei_device_never_added)
+{
+	_unref_(peck) *peck = peck_new();
+
+	peck_enable_eis_behavior(peck, PECK_EIS_BEHAVIOR_ACCEPT_ALL);
+	peck_dispatch_until_stable(peck);
+
+	/* unref after remove */
+	with_client(peck) {
+		struct ei_seat *seat = peck_ei_get_default_seat(peck);
+		_unref_(ei_device) *device = ei_device_new(seat);
+		ei_device_remove(device);
+	}
+
+	peck_dispatch_until_stable(peck);
+
+	/* unref before remove.
+	 *
+	 * This would be invalid client code since you can't expect to have
+	 * a ref after unref, but since we know the device is still here, we
+	 * can test for the lib being correct.
+	 */
+	with_client(peck) {
+		struct ei_seat *seat = peck_ei_get_default_seat(peck);
+		struct ei_device *device = ei_device_new(seat);
+		ei_device_unref(device);
+		ei_device_remove(device);
+	}
+
+	peck_dispatch_until_stable(peck);
+
+	with_server(peck) {
+		peck_assert_no_eis_events(eis);
+	}
+
+	return MUNIT_OK;
+}
+
 MUNIT_TEST(test_ei_device_add_remove)
 {
 	_unref_(peck) *peck = peck_new();