From 3b783b9985492b80542d089a9207ebc1bfc81074 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 22 Jun 2023 00:39:20 -0400 Subject: [PATCH 1/7] context: Disconnect removed signal handlers on first callback This signal should happen just once, but ignore any further signal on first callback. --- libfprint/fp-context.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libfprint/fp-context.c b/libfprint/fp-context.c index 70d4062a..77234c04 100644 --- a/libfprint/fp-context.c +++ b/libfprint/fp-context.c @@ -142,6 +142,7 @@ remove_device (FpContext *context, FpDevice *device) static void device_remove_on_notify_open_cb (FpContext *context, GParamSpec *pspec, FpDevice *device) { + g_signal_handlers_disconnect_by_func (device, device_remove_on_notify_open_cb, context); remove_device (context, device); } From 51dd46b3d990c73559e7b742d5dc64582328d45a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 22 Jun 2023 00:48:45 -0400 Subject: [PATCH 2/7] context: Simplify handling of removal signals We used to notify device removed signal in an idle to ensure that this happened before the context device-removed signal, however this can be achieved also using the after-callbacks without having to imply further idles that complicates the context destruction handling, also causing that a device that has been removed is returned by get_devices() for longer than expected. Simplify the codepath adjusting the tests. They are still checking that the order is preserved. --- libfprint/fp-context.c | 65 +++++++++-------------------------------- tests/test-fp-context.c | 52 +++++---------------------------- 2 files changed, 21 insertions(+), 96 deletions(-) diff --git a/libfprint/fp-context.c b/libfprint/fp-context.c index 77234c04..1561d700 100644 --- a/libfprint/fp-context.c +++ b/libfprint/fp-context.c @@ -89,54 +89,17 @@ is_driver_allowed (const gchar *driver) return g_strv_contains ((const gchar * const *) allowlisted_drivers, driver); } -typedef struct +static void +remove_device (FpContext *context, + FpDevice *device) { - FpContext *context; - FpDevice *device; - GSource *source; -} RemoveDeviceData; - -static gboolean -remove_device_idle_cb (RemoveDeviceData *data) -{ - FpContextPrivate *priv = fp_context_get_instance_private (data->context); + FpContextPrivate *priv = fp_context_get_instance_private (context); guint idx = 0; - g_return_val_if_fail (g_ptr_array_find (priv->devices, data->device, &idx), G_SOURCE_REMOVE); + g_return_if_fail (g_ptr_array_find (priv->devices, device, &idx)); - g_signal_emit (data->context, signals[DEVICE_REMOVED_SIGNAL], 0, data->device); + g_signal_emit (context, signals[DEVICE_REMOVED_SIGNAL], 0, device); g_ptr_array_remove_index_fast (priv->devices, idx); - - return G_SOURCE_REMOVE; -} - -static void -remove_device_data_free (RemoveDeviceData *data) -{ - FpContextPrivate *priv = fp_context_get_instance_private (data->context); - - priv->sources = g_slist_remove (priv->sources, data->source); - g_free (data); -} - -static void -remove_device (FpContext *context, FpDevice *device) -{ - g_autoptr(GSource) source = NULL; - FpContextPrivate *priv = fp_context_get_instance_private (context); - RemoveDeviceData *data; - - data = g_new (RemoveDeviceData, 1); - data->context = context; - data->device = device; - - source = data->source = g_idle_source_new (); - g_source_set_callback (source, - G_SOURCE_FUNC (remove_device_idle_cb), data, - (GDestroyNotify) remove_device_data_free); - g_source_attach (source, g_main_context_get_thread_default ()); - - priv->sources = g_slist_prepend (priv->sources, source); } static void @@ -149,17 +112,13 @@ device_remove_on_notify_open_cb (FpContext *context, GParamSpec *pspec, FpDevice static void device_removed_cb (FpContext *context, FpDevice *device) { - gboolean open = FALSE; - - g_object_get (device, "open", &open, NULL); - /* Wait for device close if the device is currently still open. */ - if (open) + if (fp_device_is_open (device)) { g_signal_connect_object (device, "notify::open", (GCallback) device_remove_on_notify_open_cb, context, - G_CONNECT_SWAPPED); + G_CONNECT_SWAPPED | G_CONNECT_AFTER); } else { @@ -195,7 +154,7 @@ async_device_init_done_cb (GObject *source_object, GAsyncResult *res, gpointer u g_signal_connect_object (device, "removed", (GCallback) device_removed_cb, context, - G_CONNECT_SWAPPED); + G_CONNECT_SWAPPED | G_CONNECT_AFTER); g_signal_emit (context, signals[DEVICE_ADDED_SIGNAL], 0, device); } @@ -275,8 +234,10 @@ usb_device_removed_cb (FpContext *self, GUsbDevice *device, GUsbContext *usb_ctx if (cls->type != FP_DEVICE_TYPE_USB) continue; - if (fpi_device_get_usb_device (dev) == device) - fpi_device_remove (dev); + if (fpi_device_get_usb_device (dev) != device) + continue; + + fpi_device_remove (dev); } } diff --git a/tests/test-fp-context.c b/tests/test-fp-context.c index c74548cd..da33b690 100644 --- a/tests/test-fp-context.c +++ b/tests/test-fp-context.c @@ -121,29 +121,21 @@ static void test_context_remove_device_closed (void) { g_autoptr(FptContext) tctx = fpt_context_new_with_virtual_device (FPT_VIRTUAL_DEVICE_IMAGE); - gboolean removed; + FpDevice *device = tctx->device; tctx->user_data = NULL; - g_signal_connect (tctx->device, "removed", (GCallback) device_removed_cb, tctx); g_signal_connect (tctx->fp_context, "device-removed", (GCallback) context_device_removed_cb, tctx); + g_signal_connect (tctx->device, "removed", (GCallback) device_removed_cb, tctx); /* Triggering remove on closed device. */ fpi_device_remove (tctx->device); - g_assert_nonnull (tctx->device); - g_object_get (tctx->device, "removed", &removed, NULL); - g_assert_true (removed); - g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); - - /* device-removed is dispatched from idle. */ - while (g_main_context_iteration (NULL, FALSE)) - { - } - /* The device is now destroyed and device-removed was called. */ g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); + g_assert_false (g_ptr_array_find (fp_context_get_devices (tctx->fp_context), device, NULL)); + fpt_teardown_virtual_device_environment (); } @@ -165,6 +157,7 @@ test_context_remove_device_closing (void) g_autoptr(FptContext) tctx = fpt_context_new_with_virtual_device (FPT_VIRTUAL_DEVICE_IMAGE); g_autoptr(GError) close_error = NULL; g_autoptr(GError) error = NULL; + FpDevice *device = tctx->device; gboolean removed; tctx->user_data = NULL; @@ -190,17 +183,11 @@ test_context_remove_device_closing (void) g_assert_error (close_error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); /* Now the removed callback has been called already. */ - g_assert_nonnull (tctx->device); - g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); - - /* While device-removed needs another idle iteration. */ - while (g_main_context_iteration (NULL, FALSE)) - { - } - g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); + g_assert_false (g_ptr_array_find (fp_context_get_devices (tctx->fp_context), device, NULL)); + fpt_teardown_virtual_device_environment (); } @@ -234,16 +221,9 @@ test_context_remove_device_open (void) } g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); - /* On close, the device will be removed from the context, - * but only a main loop iteration later. */ + /* On close, the device will be removed from the context */ fp_device_close_sync (tctx->device, NULL, &error); g_assert_error (error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); - g_assert_nonnull (tctx->device); - g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); - - while (g_main_context_iteration (NULL, FALSE)) - { - } g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); @@ -298,14 +278,6 @@ test_context_remove_device_opening (void) fp_device_close_sync (tctx->device, NULL, &close_error); g_assert_error (close_error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); - g_assert_nonnull (tctx->device); - g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); - - /* The device-removed signal needs an idle iteration. */ - while (g_main_context_iteration (NULL, FALSE)) - { - } - g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); @@ -369,14 +341,6 @@ test_context_remove_device_active (void) /* Now we close the device, state remains unchanged mostly. */ fp_device_close_sync (tctx->device, NULL, &error); g_assert_error (error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); - g_assert_nonnull (tctx->device); - g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); - - /* And "device-removed" is called */ - while (g_main_context_iteration (NULL, FALSE)) - { - } - g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); From a2a7f35205a31fbe45bbbc4abe9b56fa1521c146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 22 Jun 2023 06:53:14 +0200 Subject: [PATCH 3/7] context: Make it clear that the returned array of device is owned by the lib The "new" word could be not correct here, given we return the one owned by the context. --- libfprint/fp-context.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libfprint/fp-context.c b/libfprint/fp-context.c index 1561d700..40bc8aa3 100644 --- a/libfprint/fp-context.c +++ b/libfprint/fp-context.c @@ -543,7 +543,7 @@ fp_context_enumerate (FpContext *context) * * Get all devices. fp_context_enumerate() will be called as needed. * - * Returns: (transfer none) (element-type FpDevice): a new #GPtrArray of #FpDevice's. + * Returns: (transfer none) (element-type FpDevice): a #GPtrArray of #FpDevice's. */ GPtrArray * fp_context_get_devices (FpContext *context) From da9849613e1e211b0b575c10232e14427d0afc91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 22 Jun 2023 15:46:43 +0200 Subject: [PATCH 4/7] fixup! context: Simplify handling of removal signals --- libfprint/fp-context.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libfprint/fp-context.c b/libfprint/fp-context.c index 40bc8aa3..c5c228b7 100644 --- a/libfprint/fp-context.c +++ b/libfprint/fp-context.c @@ -48,8 +48,6 @@ typedef struct GUsbContext *usb_ctx; GCancellable *cancellable; - GSList *sources; - gint pending_devices; gboolean enumerated; @@ -252,8 +250,6 @@ fp_context_finalize (GObject *object) g_clear_pointer (&priv->drivers, g_array_unref); g_clear_pointer (&priv->devices, g_ptr_array_unref); - g_slist_free_full (g_steal_pointer (&priv->sources), (GDestroyNotify) g_source_destroy); - if (priv->usb_ctx) g_object_run_dispose (G_OBJECT (priv->usb_ctx)); g_clear_object (&priv->usb_ctx); From d66e9470d806fdcf67094c17688b101cff872548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 22 Jun 2023 22:43:27 +0200 Subject: [PATCH 5/7] context: Ensure that context won't include a removed device at signal time If a device has been removed from context, at that point we it should not be returned by fp_context_get_devices(). --- libfprint/fp-context.c | 4 +++- tests/test-fp-context.c | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/libfprint/fp-context.c b/libfprint/fp-context.c index c5c228b7..08bfd4c7 100644 --- a/libfprint/fp-context.c +++ b/libfprint/fp-context.c @@ -96,8 +96,10 @@ remove_device (FpContext *context, g_return_if_fail (g_ptr_array_find (priv->devices, device, &idx)); - g_signal_emit (context, signals[DEVICE_REMOVED_SIGNAL], 0, device); + g_object_ref (device); g_ptr_array_remove_index_fast (priv->devices, idx); + g_signal_emit (context, signals[DEVICE_REMOVED_SIGNAL], 0, device); + g_clear_object (&device); } static void diff --git a/tests/test-fp-context.c b/tests/test-fp-context.c index da33b690..15bb469f 100644 --- a/tests/test-fp-context.c +++ b/tests/test-fp-context.c @@ -115,6 +115,8 @@ context_device_removed_cb (FpContext *ctx, FpDevice *device, FptContext *tctx) /* "device-removed" on context is always after "removed" on device */ g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); tctx->user_data = GINT_TO_POINTER (CTX_DEVICE_REMOVED_CB); + + g_assert_false (g_ptr_array_find (fp_context_get_devices (tctx->fp_context), device, NULL)); } static void From 18661aae0e4e6079919be0df7281093fd51ea3df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 23 Jun 2023 02:08:47 +0200 Subject: [PATCH 6/7] test-fp-context: Add more cases for closing device during removal --- tests/test-fp-context.c | 83 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/tests/test-fp-context.c b/tests/test-fp-context.c index 15bb469f..64ad2290 100644 --- a/tests/test-fp-context.c +++ b/tests/test-fp-context.c @@ -286,6 +286,53 @@ test_context_remove_device_opening (void) fpt_teardown_virtual_device_environment (); } +static void +open_close_cb (GObject *device, GAsyncResult *res, gpointer user_data) +{ + g_autoptr(GError) error = NULL; + g_autoptr(FpPrint) print = NULL; + gboolean *data = user_data; + + g_assert_true (fp_device_open_finish (FP_DEVICE (device), res, &error)); + g_assert_null (print); + g_assert_null (error); + + g_assert_false (fp_device_close_sync (FP_DEVICE (device), NULL, &error)); + g_assert_error (error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); + + *data = TRUE; +} + +static void +test_context_remove_device_closes_on_open_callback (void) +{ + g_autoptr(FptContext) tctx = fpt_context_new_with_virtual_device (FPT_VIRTUAL_DEVICE_IMAGE); + gboolean open_done = FALSE; + gboolean removed; + + tctx->user_data = NULL; + g_signal_connect (tctx->device, "removed", (GCallback) device_removed_cb, tctx); + g_signal_connect (tctx->fp_context, "device-removed", (GCallback) context_device_removed_cb, tctx); + + fp_device_open (tctx->device, NULL, open_close_cb, &open_done); + g_assert_false (open_done); + + fpi_device_remove (tctx->device); + + /* Removed but not yet notified*/ + g_assert_nonnull (tctx->device); + g_object_get (tctx->device, "removed", &removed, NULL); + g_assert_true (removed); + g_assert_null (tctx->user_data); + + /* Running the mainloop now will cause the open to *succeed* despite removal! */ + while (!open_done) + g_main_context_iteration (NULL, TRUE); + + g_assert_null (tctx->device); + g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); +} + static void enroll_done_cb (GObject *device, GAsyncResult *res, gpointer user_data) { @@ -345,8 +392,40 @@ test_context_remove_device_active (void) g_assert_error (error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); +} - fpt_teardown_virtual_device_environment (); +static void +device_removed_close_cb (FpDevice *device, FptContext *tctx) +{ + g_autoptr(GError) error = NULL; + g_assert_nonnull (device); + g_assert_true (device == tctx->device); + + g_assert_null (tctx->user_data); + tctx->user_data = GINT_TO_POINTER (DEV_REMOVED_CB); + + g_assert_true (fp_device_is_open (tctx->device)); + fp_device_close_sync (tctx->device, NULL, &error); + g_assert_error (error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); + g_assert_false (fp_device_is_open (tctx->device)); + + g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, DEV_REMOVED_CB); +} + +static void +test_context_remove_device_close_on_removal (void) +{ + g_autoptr(FptContext) tctx = fpt_context_new_with_virtual_device (FPT_VIRTUAL_DEVICE_IMAGE); + g_autoptr(GError) error = NULL; + + fp_device_open_sync (tctx->device, NULL, &error); + g_assert_no_error (error); + + g_signal_connect (tctx->fp_context, "device-removed", (GCallback) context_device_removed_cb, tctx); + g_signal_connect (tctx->device, "removed", (GCallback) device_removed_close_cb, tctx); + + fpi_device_remove (tctx->device); + g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); } int @@ -363,6 +442,8 @@ main (int argc, char *argv[]) g_test_add_func ("/context/remove-device-open", test_context_remove_device_open); g_test_add_func ("/context/remove-device-opening", test_context_remove_device_opening); g_test_add_func ("/context/remove-device-active", test_context_remove_device_active); + g_test_add_func ("/context/remove-device-close-on-removal", test_context_remove_device_close_on_removal); + g_test_add_func ("/context/remove-device-closes-on-open-callback", test_context_remove_device_closes_on_open_callback); return g_test_run (); } From 7423deb0ef0b42271d1429e6fe2658835953138f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 23 Jun 2023 17:35:12 +0200 Subject: [PATCH 7/7] tests: Remove teardown env calls They're already part of the FptContext autoptr cleanup. --- tests/test-fp-context.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/test-fp-context.c b/tests/test-fp-context.c index 64ad2290..23695342 100644 --- a/tests/test-fp-context.c +++ b/tests/test-fp-context.c @@ -137,8 +137,6 @@ test_context_remove_device_closed (void) g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); g_assert_false (g_ptr_array_find (fp_context_get_devices (tctx->fp_context), device, NULL)); - - fpt_teardown_virtual_device_environment (); } static void @@ -189,8 +187,6 @@ test_context_remove_device_closing (void) g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); g_assert_false (g_ptr_array_find (fp_context_get_devices (tctx->fp_context), device, NULL)); - - fpt_teardown_virtual_device_environment (); } static void @@ -228,8 +224,6 @@ test_context_remove_device_open (void) g_assert_error (error, FP_DEVICE_ERROR, FP_DEVICE_ERROR_REMOVED); g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); - - fpt_teardown_virtual_device_environment (); } static void @@ -282,8 +276,6 @@ test_context_remove_device_opening (void) g_assert_null (tctx->device); g_assert_cmpint (GPOINTER_TO_INT (tctx->user_data), ==, CTX_DEVICE_REMOVED_CB); - - fpt_teardown_virtual_device_environment (); } static void