From 71e0c29f28b82c2504259289c517888466573f75 Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Wed, 21 Apr 2021 20:08:48 +0200 Subject: [PATCH] device: Always use an internal cancellable for tasks This will allow libfprint to cancel operations internally in the future. If the internal cancellation method is used, then the private current_cancellation_reason variable must be set to the GError. This error will be returned when set. --- libfprint/fp-device-private.h | 3 ++ libfprint/fp-device.c | 58 +++++++++++++++++++++++------------ libfprint/fpi-device.c | 30 ++++++++++++++---- tests/test-device-fake.h | 2 ++ tests/test-fpi-device.c | 44 ++++++++++++++++++++------ 5 files changed, 103 insertions(+), 34 deletions(-) diff --git a/libfprint/fp-device-private.h b/libfprint/fp-device-private.h index f17541e..ed9fe86 100644 --- a/libfprint/fp-device-private.h +++ b/libfprint/fp-device-private.h @@ -67,8 +67,11 @@ typedef struct /* We always make sure that only one task is run at a time. */ FpiDeviceAction current_action; GTask *current_task; + GError *current_cancellation_reason; GAsyncReadyCallback current_user_cb; + GCancellable *current_cancellable; gulong current_cancellable_id; + gulong current_task_cancellable_id; GSource *current_idle_cancel_source; GSource *current_task_idle_return_source; diff --git a/libfprint/fp-device.c b/libfprint/fp-device.c index a99596e..99399c6 100644 --- a/libfprint/fp-device.c +++ b/libfprint/fp-device.c @@ -119,20 +119,40 @@ fp_device_cancelled_cb (GCancellable *cancellable, FpDevice *self) g_source_unref (priv->current_idle_cancel_source); } +/* Forward the external task cancellable to the internal one. */ static void -maybe_cancel_on_cancelled (FpDevice *device, - GCancellable *cancellable) +fp_device_task_cancelled_cb (GCancellable *cancellable, FpDevice *self) +{ + FpDevicePrivate *priv = fp_device_get_instance_private (self); + + g_cancellable_cancel (priv->current_cancellable); +} + +static void +setup_task_cancellable (FpDevice *device) { - FpDeviceClass *cls = FP_DEVICE_GET_CLASS (device); FpDevicePrivate *priv = fp_device_get_instance_private (device); + FpDeviceClass *cls = FP_DEVICE_GET_CLASS (device); - if (!cancellable || !cls->cancel) - return; + /* Create an internal cancellable and hook it up. */ + priv->current_cancellable = g_cancellable_new (); + if (cls->cancel) + { + priv->current_cancellable_id = g_cancellable_connect (priv->current_cancellable, + G_CALLBACK (fp_device_cancelled_cb), + device, + NULL); + } - priv->current_cancellable_id = g_cancellable_connect (cancellable, - G_CALLBACK (fp_device_cancelled_cb), - device, - NULL); + /* Task cancellable is the externally visible one, make our internal one + * a slave of the external one. */ + if (g_task_get_cancellable (priv->current_task)) + { + priv->current_task_cancellable_id = g_cancellable_connect (g_task_get_cancellable (priv->current_task), + G_CALLBACK (fp_device_task_cancelled_cb), + device, + NULL); + } } static void @@ -343,7 +363,7 @@ fp_device_async_initable_init_async (GAsyncInitable *initable, priv->current_action = FPI_DEVICE_ACTION_PROBE; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (self, cancellable); + setup_task_cancellable (self); FP_DEVICE_GET_CLASS (self)->probe (self); } @@ -800,7 +820,7 @@ fp_device_open (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_OPEN; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); fpi_device_report_finger_status (device, FP_FINGER_STATUS_NONE); FP_DEVICE_GET_CLASS (device)->open (device); @@ -865,7 +885,7 @@ fp_device_close (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_CLOSE; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); FP_DEVICE_GET_CLASS (device)->close (device); } @@ -963,7 +983,7 @@ fp_device_enroll (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_ENROLL; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); fpi_device_update_temp (device, TRUE); @@ -1058,7 +1078,7 @@ fp_device_verify (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_VERIFY; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); fpi_device_update_temp (device, TRUE); @@ -1179,7 +1199,7 @@ fp_device_identify (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_IDENTIFY; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); fpi_device_update_temp (device, TRUE); @@ -1298,7 +1318,7 @@ fp_device_capture (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_CAPTURE; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); fpi_device_update_temp (device, TRUE); @@ -1382,7 +1402,7 @@ fp_device_delete_print (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_DELETE; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); g_task_set_task_data (priv->current_task, g_object_ref (enrolled_print), @@ -1461,7 +1481,7 @@ fp_device_list_prints (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_LIST; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); cls->list (device); } @@ -1546,7 +1566,7 @@ fp_device_clear_storage (FpDevice *device, priv->current_action = FPI_DEVICE_ACTION_CLEAR_STORAGE; priv->current_task = g_steal_pointer (&task); - maybe_cancel_on_cancelled (device, cancellable); + setup_task_cancellable (device); cls->clear_storage (device); diff --git a/libfprint/fpi-device.c b/libfprint/fpi-device.c index c6e21da..914af05 100644 --- a/libfprint/fpi-device.c +++ b/libfprint/fpi-device.c @@ -490,14 +490,11 @@ gboolean fpi_device_action_is_cancelled (FpDevice *device) { FpDevicePrivate *priv = fp_device_get_instance_private (device); - GCancellable *cancellable; g_return_val_if_fail (FP_IS_DEVICE (device), TRUE); g_return_val_if_fail (priv->current_action != FPI_DEVICE_ACTION_NONE, TRUE); - cancellable = g_task_get_cancellable (priv->current_task); - - return g_cancellable_is_cancelled (cancellable); + return g_cancellable_is_cancelled (priv->current_cancellable); } /** @@ -675,7 +672,7 @@ fpi_device_get_cancellable (FpDevice *device) g_return_val_if_fail (FP_IS_DEVICE (device), NULL); g_return_val_if_fail (priv->current_action != FPI_DEVICE_ACTION_NONE, NULL); - return g_task_get_cancellable (priv->current_task); + return priv->current_cancellable; } static void @@ -813,10 +810,17 @@ clear_device_cancel_action (FpDevice *device) if (priv->current_cancellable_id) { - g_cancellable_disconnect (g_task_get_cancellable (priv->current_task), + g_cancellable_disconnect (priv->current_cancellable, priv->current_cancellable_id); priv->current_cancellable_id = 0; } + + if (priv->current_task_cancellable_id) + { + g_cancellable_disconnect (g_task_get_cancellable (priv->current_task), + priv->current_task_cancellable_id); + priv->current_task_cancellable_id = 0; + } } typedef enum _FpDeviceTaskReturnType { @@ -843,6 +847,7 @@ fp_device_task_return_in_idle_cb (gpointer user_data) FpiDeviceAction action; g_autoptr(GTask) task = NULL; + g_autoptr(GError) cancellation_reason = NULL; action_str = g_enum_to_string (FPI_TYPE_DEVICE_ACTION, priv->current_action); @@ -852,6 +857,8 @@ fp_device_task_return_in_idle_cb (gpointer user_data) action = priv->current_action; priv->current_action = FPI_DEVICE_ACTION_NONE; priv->current_task_idle_return_source = NULL; + g_clear_object (&priv->current_cancellable); + cancellation_reason = g_steal_pointer (&priv->current_cancellation_reason); fpi_device_update_temp (data->device, FALSE); @@ -870,6 +877,8 @@ fp_device_task_return_in_idle_cb (gpointer user_data) g_object_notify (G_OBJECT (data->device), "open"); } + /* TODO: Port/use the cancellation mechanism for device removal! */ + /* Return FP_DEVICE_ERROR_REMOVED if the device is removed, * with the exception of a successful open, which is an odd corner case. */ if (priv->is_removed && @@ -884,6 +893,15 @@ fp_device_task_return_in_idle_cb (gpointer user_data) return G_SOURCE_REMOVE; } + /* Return internal cancellation reason if we have one. + * Note that an external cancellation always returns G_IO_ERROR_CANCELLED */ + if (cancellation_reason) + { + g_task_return_error (task, g_steal_pointer (&cancellation_reason)); + + return G_SOURCE_REMOVE; + } + switch (data->type) { case FP_DEVICE_TASK_RETURN_INT: diff --git a/tests/test-device-fake.h b/tests/test-device-fake.h index fa8b9b9..d285694 100644 --- a/tests/test-device-fake.h +++ b/tests/test-device-fake.h @@ -32,6 +32,8 @@ struct _FpiDeviceFake gpointer last_called_function; gboolean return_action_error; + GCancellable *ext_cancellable; + GError *ret_error; FpPrint *ret_print; FpPrint *ret_match; diff --git a/tests/test-fpi-device.c b/tests/test-fpi-device.c index 2879253..e8f331e 100644 --- a/tests/test-fpi-device.c +++ b/tests/test-fpi-device.c @@ -2356,32 +2356,32 @@ test_driver_action_get_cancellable_open (void) } static void -test_driver_action_get_cancellable_open_fail_vfunc (FpDevice *device) +test_driver_action_get_cancellable_open_internal_vfunc (FpDevice *device) { FpiDeviceFake *fake_dev = FPI_DEVICE_FAKE (device); g_assert_cmpuint (fpi_device_get_current_action (device), ==, FPI_DEVICE_ACTION_OPEN); - fake_dev->last_called_function = test_driver_action_get_cancellable_open_fail_vfunc; + fake_dev->last_called_function = test_driver_action_get_cancellable_open_internal_vfunc; - g_assert_false (G_IS_CANCELLABLE (fpi_device_get_cancellable (device))); + g_assert_true (G_IS_CANCELLABLE (fpi_device_get_cancellable (device))); fpi_device_open_complete (device, NULL); } static void -test_driver_action_get_cancellable_open_fail (void) +test_driver_action_get_cancellable_open_internal (void) { g_autoptr(FpAutoResetClass) dev_class = auto_reset_device_class (); g_autoptr(FpAutoCloseDevice) device = NULL; FpiDeviceFake *fake_dev; - dev_class->open = test_driver_action_get_cancellable_open_fail_vfunc; + dev_class->open = test_driver_action_get_cancellable_open_internal_vfunc; device = g_object_new (FPI_TYPE_DEVICE_FAKE, NULL); fake_dev = FPI_DEVICE_FAKE (device); g_assert_true (fp_device_open_sync (device, NULL, NULL)); - g_assert (fake_dev->last_called_function == test_driver_action_get_cancellable_open_fail_vfunc); + g_assert (fake_dev->last_called_function == test_driver_action_get_cancellable_open_internal_vfunc); } static void @@ -2406,7 +2406,11 @@ test_driver_action_is_cancelled_open_vfunc (FpDevice *device) g_assert_true (G_IS_CANCELLABLE (fpi_device_get_cancellable (device))); g_assert_false (fpi_device_action_is_cancelled (device)); - g_cancellable_cancel (fpi_device_get_cancellable (device)); + if (fake_dev->ext_cancellable) + g_cancellable_cancel (fake_dev->ext_cancellable); + else + g_cancellable_cancel (fpi_device_get_cancellable (device)); + g_assert_true (fpi_device_action_is_cancelled (device)); fpi_device_open_complete (device, NULL); @@ -2425,13 +2429,34 @@ test_driver_action_is_cancelled_open (void) device = g_object_new (FPI_TYPE_DEVICE_FAKE, NULL); fake_dev = FPI_DEVICE_FAKE (device); - cancellable = g_cancellable_new (); + cancellable = fake_dev->ext_cancellable = g_cancellable_new (); g_assert_false (fp_device_open_sync (device, cancellable, &error)); g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED); g_assert (fake_dev->last_called_function == test_driver_action_is_cancelled_open_vfunc); } +static void +test_driver_action_internally_cancelled_open (void) +{ + g_autoptr(FpAutoResetClass) dev_class = auto_reset_device_class (); + g_autoptr(FpAutoCloseDevice) device = NULL; + g_autoptr(GCancellable) cancellable = NULL; + g_autoptr(GError) error = NULL; + FpiDeviceFake *fake_dev; + + dev_class->open = test_driver_action_is_cancelled_open_vfunc; + device = g_object_new (FPI_TYPE_DEVICE_FAKE, NULL); + fake_dev = FPI_DEVICE_FAKE (device); + + /* No error, just some internal cancellation but we let nothing happen externally. */ + cancellable = g_cancellable_new (); + g_assert_true (fp_device_open_sync (device, cancellable, &error)); + g_assert_null (error); + + g_assert (fake_dev->last_called_function == test_driver_action_is_cancelled_open_vfunc); +} + static void test_driver_action_is_cancelled_error (void) { @@ -2897,8 +2922,9 @@ main (int argc, char *argv[]) g_test_add_func ("/driver/get_current_action/open", test_driver_current_action_open); g_test_add_func ("/driver/get_cancellable/error", test_driver_action_get_cancellable_error); g_test_add_func ("/driver/get_cancellable/open", test_driver_action_get_cancellable_open); - g_test_add_func ("/driver/get_cancellable/open/fail", test_driver_action_get_cancellable_open_fail); + g_test_add_func ("/driver/get_cancellable/open/internal", test_driver_action_get_cancellable_open_internal); g_test_add_func ("/driver/action_is_cancelled/open", test_driver_action_is_cancelled_open); + g_test_add_func ("/driver/action_is_cancelled/open/internal", test_driver_action_internally_cancelled_open); g_test_add_func ("/driver/action_is_cancelled/error", test_driver_action_is_cancelled_error); g_test_add_func ("/driver/complete_action/all/error", test_driver_complete_actions_errors); g_test_add_func ("/driver/action_error/error", test_driver_action_error_error);