ssm: Add cleanup state feature

In some situations one may want to guarantee that the last steps of an
SSM are run even when the SSM is completed early or failed.

This can easily be done by making fpi_ssm_mark_completed jump to the
next cleanup stage when called (this also includes mark_failed). Due to
the mechanism, it is still possible to explicitly jump cleanup states by
using fpi_ssm_jump_to_state, including a jump to the final state in
order to skip all cleanup states.

libfprint/drivers/vfs5011.c

@@ -196,6 +196,7 @@ usb_exchange_async (FpiSsm                  *ssm,
   FpiSsm *subsm = fpi_ssm_new_full (FP_DEVICE (data->device),
                                     usbexchange_loop,
                                     data->stepcount,
+                                    data->stepcount,
                                     exchange_name);
 
   fpi_ssm_set_data (subsm, data, NULL);

libfprint/drivers/vfs7552.c

@@ -327,6 +327,7 @@ usb_exchange_async (FpiSsm                  *ssm,
   FpiSsm *subsm = fpi_ssm_new_full (fpi_ssm_get_device (ssm),
                                     usbexchange_loop,
                                     data->stepcount,
+                                    data->stepcount,
                                     exchange_name);
 
   fpi_ssm_set_data (subsm, data, NULL);

libfprint/fpi-ssm.c

@@ -35,9 +35,11 @@
  * is often entirely linear. You can however also jump to a specific state
  * or do an early return from the SSM by completing it.
  *
- * e.g. `S1` ↦ `S2` ↦ `S3` ↦ `S4`
+ * e.g. `S1` ↦ `S2` ↦ `S3` ↦ `S4` ↦ `C1` ↦ `C2` ↦ `final`
  *
- * Where `S1` is the start state.
+ * Where `S1` is the start state. The `C1` and later states are cleanup states
+ * that may be defined. The difference is that these states will never be
+ * skipped when marking the SSM as completed.
  *
  * Use fpi_ssm_new() to create a new state machine with a defined number of
  * states. Note that the state numbers start at zero, making them match the
@@ -76,6 +78,7 @@ struct _FpiSsm
   gpointer                ssm_data;
   GDestroyNotify          ssm_data_destroy;
   int                     nr_states;
+  int                     start_cleanup;
   int                     cur_state;
   gboolean                completed;
   GSource                *timeout;
@@ -94,8 +97,9 @@ struct _FpiSsm
  *
  * Allocate a new ssm, with @nr_states states. The @handler callback
  * will be called after each state transition.
- * This is a macro that calls fpi_ssm_new_full() using the stringified
+ * This is a macro that calls fpi_ssm_new_full() using @nr_states as the
- * version of @nr_states, so will work better with named parameters.
+ * cleanup states and using the stringified version of @nr_states. It should
+ * be used with an enum value.
  *
  * Returns: a new #FpiSsm state machine
  */
@@ -105,6 +109,7 @@ struct _FpiSsm
  * @dev: a #fp_dev fingerprint device
  * @handler: the callback function
  * @nr_states: the number of states
+ * @start_cleanup: the first cleanup state
  * @machine_name: the name of the state machine (for debug purposes)
  *
  * Allocate a new ssm, with @nr_states states. The @handler callback
@@ -116,17 +121,21 @@ FpiSsm *
 fpi_ssm_new_full (FpDevice             *dev,
                   FpiSsmHandlerCallback handler,
                   int                   nr_states,
+                  int                   start_cleanup,
                   const char           *machine_name)
 {
   FpiSsm *machine;
 
   BUG_ON (dev == NULL);
   BUG_ON (nr_states < 1);
+  BUG_ON (start_cleanup < 1);
+  BUG_ON (start_cleanup > nr_states);
   BUG_ON (handler == NULL);
 
   machine = g_new0 (FpiSsm, 1);
   machine->handler = handler;
   machine->nr_states = nr_states;
+  machine->start_cleanup = start_cleanup;
   machine->dev = dev;
   machine->name = g_strdup (machine_name);
   machine->completed = TRUE;
@@ -371,10 +380,14 @@ fpi_ssm_start_subsm (FpiSsm *parent, FpiSsm *child)
  *
  * Mark a ssm as completed successfully. The callback set when creating
  * the state machine with fpi_ssm_new() will be called synchronously.
+ *
+ * Note that any later cleanup state will still be executed.
  */
 void
 fpi_ssm_mark_completed (FpiSsm *machine)
 {
+  int next_state;
+
   g_return_if_fail (machine != NULL);
 
   BUG_ON (machine->completed);
@@ -382,6 +395,19 @@ fpi_ssm_mark_completed (FpiSsm *machine)
 
   fpi_ssm_clear_delayed_action (machine);
 
+  /* complete in a cleanup state just moves forward one step */
+  if (machine->cur_state < machine->start_cleanup)
+    next_state = machine->start_cleanup;
+  else
+    next_state = machine->cur_state + 1;
+
+  if (next_state < machine->nr_states)
+    {
+      machine->cur_state = next_state;
+      __ssm_call_handler (machine);
+      return;
+    }
+
   machine->completed = TRUE;
 
   if (machine->error)
@@ -451,7 +477,9 @@ fpi_ssm_mark_failed (FpiSsm *machine, GError *error)
 {
   g_return_if_fail (machine != NULL);
   g_assert (error);
-  if (machine->error)
+
+  /* During cleanup it is OK to call fpi_ssm_mark_failed a second time */
+  if (machine->error && machine->cur_state < machine->start_cleanup)
     {
       fp_warn ("[%s] SSM %s already has an error set, ignoring new error %s",
                fp_device_get_driver (machine->dev), machine->name, error->message);
@@ -459,10 +487,15 @@ fpi_ssm_mark_failed (FpiSsm *machine, GError *error)
       return;
     }
 
-  fp_dbg ("[%s] SSM %s failed in state %d with error: %s",
+  fp_dbg ("[%s] SSM %s failed in state %d%s with error: %s",
           fp_device_get_driver (machine->dev), machine->name,
-          machine->cur_state, error->message);
+          machine->cur_state,
+          machine->cur_state >= machine->start_cleanup ? " (cleanup)" : "",
+          error->message);
+  if (!machine->error)
     machine->error = g_steal_pointer (&error);
+  else
+    g_error_free (error);
   fpi_ssm_mark_completed (machine);
 }
 
@@ -560,12 +593,15 @@ fpi_ssm_jump_to_state (FpiSsm *machine, int state)
   g_return_if_fail (machine != NULL);
 
   BUG_ON (machine->completed);
-  BUG_ON (state < 0 || state >= machine->nr_states);
+  BUG_ON (state < 0 || state > machine->nr_states);
   BUG_ON (machine->timeout != NULL);
 
   fpi_ssm_clear_delayed_action (machine);
 
   machine->cur_state = state;
+  if (machine->cur_state == machine->nr_states)
+    fpi_ssm_mark_completed (machine);
+  else
     __ssm_call_handler (machine);
 }
 
@@ -607,7 +643,7 @@ fpi_ssm_jump_to_state_delayed (FpiSsm       *machine,
   g_autofree char *source_name = NULL;
 
   g_return_if_fail (machine != NULL);
-  BUG_ON (state < 0 || state >= machine->nr_states);
+  BUG_ON (state < 0 || state > machine->nr_states);
 
   data = g_new0 (FpiSsmJumpToStateDelayedData, 1);
   data->machine = machine;

libfprint/fpi-ssm.h

@@ -60,10 +60,11 @@ typedef void (*FpiSsmHandlerCallback)(FpiSsm   *ssm,
 
 /* for library and drivers */
 #define fpi_ssm_new(dev, handler, nr_states) \
-  fpi_ssm_new_full (dev, handler, nr_states, #nr_states)
+  fpi_ssm_new_full (dev, handler, nr_states, nr_states, #nr_states)
 FpiSsm *fpi_ssm_new_full (FpDevice             *dev,
                           FpiSsmHandlerCallback handler,
                           int                   nr_states,
+                          int                   start_cleanup,
                           const char           *machine_name);
 void fpi_ssm_free (FpiSsm *machine);
 void fpi_ssm_start (FpiSsm                 *ssm,

tests/test-fpi-ssm.c

@@ -129,12 +129,12 @@ test_ssm_completed_callback (FpiSsm   *ssm,
 }
 
 static FpiSsm *
-ssm_test_new_full (int nr_states, const char *name)
+ssm_test_new_full (int nr_states, int cleanup_state, const char *name)
 {
   FpiSsm *ssm;
   FpiSsmTestData *data;
 
-  ssm = fpi_ssm_new_full (fake_device, test_ssm_handler, nr_states, name);
+  ssm = fpi_ssm_new_full (fake_device, test_ssm_handler, nr_states, cleanup_state, name);
   data = fpi_ssm_test_data_new ();
   data->expected_last_state = nr_states;
   fpi_ssm_set_data (ssm, data, (GDestroyNotify) fpi_ssm_test_data_unref_by_ssm);
@@ -145,7 +145,7 @@ ssm_test_new_full (int nr_states, const char *name)
 static FpiSsm *
 ssm_test_new (void)
 {
-  return ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SSM");
+  return ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SSM");
 }
 
 static gboolean
@@ -188,7 +188,8 @@ test_ssm_new_full (void)
   FpiSsm *ssm;
 
   ssm = fpi_ssm_new_full (fake_device, test_ssm_handler,
-                          FPI_TEST_SSM_STATE_NUM, "Test SSM Name");
+                          FPI_TEST_SSM_STATE_NUM, FPI_TEST_SSM_STATE_NUM,
+                          "Test SSM Name");
 
   g_assert_null (fpi_ssm_get_data (ssm));
   g_assert_no_error (fpi_ssm_get_error (ssm));
@@ -215,6 +216,8 @@ test_ssm_new_wrong_states (void)
 
   g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL,
                          "*BUG:*nr_states*");
+  g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL,
+                         "*BUG:*start_cleanup*");
   ssm = fpi_ssm_new (fake_device, test_ssm_handler, -1);
   g_test_assert_expected_messages ();
 }
@@ -271,7 +274,7 @@ test_ssm_start_single (void)
   g_autoptr(FpiSsmTestData) data = NULL;
   FpiSsm *ssm;
 
-  ssm = ssm_test_new_full (1, "FPI_TEST_SSM_SINGLE_STATE");
+  ssm = ssm_test_new_full (1, 1, "FPI_TEST_SSM_SINGLE_STATE");
   data = fpi_ssm_test_data_ref (fpi_ssm_get_data (ssm));
 
   fpi_ssm_start (ssm, test_ssm_completed_callback);
@@ -1170,7 +1173,7 @@ test_ssm_subssm_start (void)
 {
   g_autoptr(FpiSsm) ssm = ssm_test_new ();
   g_autoptr(FpiSsm) subssm =
-    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
+    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
   FpiSsmTestData *data = fpi_ssm_get_data (ssm);
 
   g_autoptr(FpiSsmTestData) subdata =
@@ -1222,7 +1225,7 @@ test_ssm_subssm_mark_failed (void)
 {
   g_autoptr(FpiSsm) ssm = ssm_test_new ();
   g_autoptr(FpiSsm) subssm =
-    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
+    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
   g_autoptr(FpiSsmTestData) data =
     fpi_ssm_test_data_ref (fpi_ssm_get_data (ssm));
   g_autoptr(FpiSsmTestData) subdata =
@@ -1261,7 +1264,7 @@ test_ssm_subssm_start_with_started (void)
 {
   g_autoptr(FpiSsm) ssm = ssm_test_new ();
   g_autoptr(FpiSsm) subssm =
-    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
+    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
   FpiSsmTestData *data = fpi_ssm_get_data (ssm);
 
   g_autoptr(FpiSsmTestData) subdata =
@@ -1305,7 +1308,7 @@ test_ssm_subssm_start_with_delayed (void)
 {
   g_autoptr(FpiSsm) ssm = ssm_test_new ();
   g_autoptr(FpiSsm) subssm =
-    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
+    ssm_test_new_full (FPI_TEST_SSM_STATE_NUM, FPI_TEST_SSM_STATE_NUM, "FPI_TEST_SUB_SSM");
   FpiSsmTestData *data = fpi_ssm_get_data (ssm);
 
   g_autoptr(FpiSsmTestData) subdata =
@@ -1348,6 +1351,76 @@ test_ssm_subssm_start_with_delayed (void)
   g_assert_no_error (data->error);
 }
 
+static void
+test_ssm_cleanup_complete (void)
+{
+  FpiSsm *ssm = ssm_test_new_full (4, FPI_TEST_SSM_STATE_2, "FPI_TEST_SSM");
+
+  g_autoptr(FpiSsmTestData) data = fpi_ssm_test_data_ref (fpi_ssm_get_data (ssm));
+
+  fpi_ssm_start (ssm, test_ssm_completed_callback);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 1);
+
+  data->expected_last_state = FPI_TEST_SSM_STATE_3;
+
+  /* Completing jumps to the cleanup state */
+  fpi_ssm_mark_completed (ssm);
+  g_assert_cmpint (data->handler_state, ==, FPI_TEST_SSM_STATE_2);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 2);
+  g_assert_false (data->completed);
+  g_assert_false (data->ssm_destroyed);
+
+  /* Completing again jumps to the next cleanup state */
+  fpi_ssm_mark_completed (ssm);
+  g_assert_cmpint (data->handler_state, ==, FPI_TEST_SSM_STATE_3);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 3);
+  g_assert_false (data->completed);
+  g_assert_false (data->ssm_destroyed);
+
+  /* Completing again finalizes everything */
+  fpi_ssm_mark_completed (ssm);
+  g_assert_cmpint (data->handler_state, ==, FPI_TEST_SSM_STATE_3);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 4);
+  g_assert_true (data->completed);
+  g_assert_no_error (data->error);
+  g_assert_true (data->ssm_destroyed);
+}
+
+static void
+test_ssm_cleanup_fail (void)
+{
+  FpiSsm *ssm = ssm_test_new_full (4, FPI_TEST_SSM_STATE_2, "FPI_TEST_SSM");
+
+  g_autoptr(FpiSsmTestData) data = fpi_ssm_test_data_ref (fpi_ssm_get_data (ssm));
+
+  fpi_ssm_start (ssm, test_ssm_completed_callback);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 1);
+
+  data->expected_last_state = FPI_TEST_SSM_STATE_3;
+
+  /* Failing jumps to the cleanup state */
+  fpi_ssm_mark_failed (ssm, g_error_new (G_IO_ERROR, G_IO_ERROR_CANCELLED, "non-cleanup"));
+  g_assert_cmpint (data->handler_state, ==, FPI_TEST_SSM_STATE_2);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 2);
+  g_assert_false (data->completed);
+  g_assert_false (data->ssm_destroyed);
+
+  /* Failing again jumps to the next cleanup state */
+  fpi_ssm_mark_failed (ssm, g_error_new (G_IO_ERROR, G_IO_ERROR_FAILED, "cleanup 1"));
+  g_assert_cmpint (data->handler_state, ==, FPI_TEST_SSM_STATE_3);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 3);
+  g_assert_false (data->completed);
+  g_assert_false (data->ssm_destroyed);
+
+  /* Failing again finalizes everything */
+  fpi_ssm_mark_failed (ssm, g_error_new (G_IO_ERROR, G_IO_ERROR_FAILED, "cleanup 2"));
+  g_assert_cmpint (data->handler_state, ==, FPI_TEST_SSM_STATE_3);
+  g_assert_cmpuint (g_slist_length (data->handlers_chain), ==, 4);
+  g_assert_true (data->completed);
+  g_assert_error (data->error, G_IO_ERROR, G_IO_ERROR_CANCELLED);
+  g_assert_true (data->ssm_destroyed);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -1402,6 +1475,8 @@ main (int argc, char *argv[])
   g_test_add_func ("/ssm/subssm/start/with_started", test_ssm_subssm_start_with_started);
   g_test_add_func ("/ssm/subssm/start/with_delayed", test_ssm_subssm_start_with_delayed);
   g_test_add_func ("/ssm/subssm/mark_failed", test_ssm_subssm_mark_failed);
+  g_test_add_func ("/ssm/cleanup/complete", test_ssm_cleanup_complete);
+  g_test_add_func ("/ssm/cleanup/fail", test_ssm_cleanup_fail);
 
   return g_test_run ();
 }