use names for migration states when logging (#250)

Plus migration states array fixes and unit tests.

Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>

3 files changed, 126 insertions, 27 deletions

diff --git a/lib/migration.c b/lib/migration.c
index e991dbb..a0f0745 100644
--- a/lib/migration.c
+++ b/lib/migration.c
@@ -38,6 +38,8 @@
 #include "migration.h"
 #include "private.h"
 
+/* FIXME no need to use __u32 etc., use uint32_t etc */
+
 /*
  * FSM to simplify saving device state.
  */
@@ -64,26 +66,59 @@ struct migration {
     } iter;
 };
 
+struct migr_state_data {
+    __u32 state;
+    const char *name;
+};
+
+#define VFIO_DEVICE_STATE_ERROR (VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RESUMING)
+
 /* valid migration state transitions */
-static const __u32 migr_states[VFIO_DEVICE_STATE_MASK] = {
-    [VFIO_DEVICE_STATE_STOP] = 1 << VFIO_DEVICE_STATE_STOP,
-    [VFIO_DEVICE_STATE_RUNNING] = /* running */
-        (1 << VFIO_DEVICE_STATE_STOP) |
-        (1 << VFIO_DEVICE_STATE_RUNNING) |
-        (1 << VFIO_DEVICE_STATE_SAVING) |
-        (1 << (VFIO_DEVICE_STATE_RUNNING | VFIO_DEVICE_STATE_SAVING)) |
-        (1 << VFIO_DEVICE_STATE_RESUMING),
-    [VFIO_DEVICE_STATE_SAVING] = /* stop-and-copy */
-        (1 << VFIO_DEVICE_STATE_STOP) |
-        (1 << VFIO_DEVICE_STATE_SAVING),
-    [VFIO_DEVICE_STATE_RUNNING | VFIO_DEVICE_STATE_SAVING] = /* pre-copy */
-        (1 << VFIO_DEVICE_STATE_SAVING) |
-        (1 << VFIO_DEVICE_STATE_RUNNING | VFIO_DEVICE_STATE_SAVING),
-    [VFIO_DEVICE_STATE_RESUMING] = /* resuming */
-        (1 << VFIO_DEVICE_STATE_RUNNING) |
-        (1 << VFIO_DEVICE_STATE_RESUMING)
+static const struct migr_state_data migr_states[(VFIO_DEVICE_STATE_MASK + 1)] = {
+    [VFIO_DEVICE_STATE_STOP] = {
+        .state = 1 << VFIO_DEVICE_STATE_STOP,
+        .name = "stopped"
+    },
+    [VFIO_DEVICE_STATE_RUNNING] = {
+        .state =
+            (1 << VFIO_DEVICE_STATE_STOP) |
+            (1 << VFIO_DEVICE_STATE_RUNNING) |
+            (1 << VFIO_DEVICE_STATE_SAVING) |
+            (1 << (VFIO_DEVICE_STATE_RUNNING | VFIO_DEVICE_STATE_SAVING)) |
+            (1 << VFIO_DEVICE_STATE_RESUMING) |
+            (1 << VFIO_DEVICE_STATE_ERROR),
+        .name = "running"
+    },
+    [VFIO_DEVICE_STATE_SAVING] = {
+        .state =
+            (1 << VFIO_DEVICE_STATE_STOP) |
+            (1 << VFIO_DEVICE_STATE_SAVING) |
+            (1 << VFIO_DEVICE_STATE_ERROR),
+        .name = "stop-and-copy"
+    },
+    [VFIO_DEVICE_STATE_RUNNING | VFIO_DEVICE_STATE_SAVING] = {
+        .state =
+            (1 << VFIO_DEVICE_STATE_STOP) |
+            (1 << VFIO_DEVICE_STATE_SAVING) |
+            (1 << VFIO_DEVICE_STATE_RUNNING | VFIO_DEVICE_STATE_SAVING) |
+            (1 << VFIO_DEVICE_STATE_ERROR),
+        .name = "pre-copy"
+    },
+    [VFIO_DEVICE_STATE_RESUMING] = {
+        .state =
+            (1 << VFIO_DEVICE_STATE_RUNNING) |
+            (1 << VFIO_DEVICE_STATE_RESUMING) |
+            (1 << VFIO_DEVICE_STATE_ERROR),
+        .name = "resuming"
+    }
 };
 
+bool
+vfio_migr_state_transition_is_valid(__u32 from, __u32 to)
+{
+    return migr_states[from].state & (1 << to);
+}
+
 struct migration *
 init_migration(const vfu_migration_t * const vfu_migr, int *err)
 {
@@ -125,12 +160,6 @@ init_migration(const vfu_migration_t * const vfu_migr, int *err)
     return migr;
 }
 
-static bool
-vfio_migr_state_transition_is_valid(__u32 from, __u32 to)
-{
-    return migr_states[from] & (1 << to);
-}
-
 static void
 migr_state_transition(struct migration *migr, enum migr_iter_state state)
 {
@@ -160,9 +189,9 @@ handle_device_state(vfu_ctx_t *vfu_ctx, struct migration *migr,
 
     if (!vfio_migr_state_transition_is_valid(migr->info.device_state,
                                               *device_state)) {
-        /* TODO print descriptive device state names instead of raw value */
-        vfu_log(vfu_ctx, LOG_ERR, "bad transition from state %d to state %d",
-               migr->info.device_state, *device_state);
+        vfu_log(vfu_ctx, LOG_ERR, "bad transition from state %s to state %s",
+               migr_states[migr->info.device_state].name,
+               migr_states[*device_state].name);
         return -EINVAL;
     }
 
diff --git a/lib/migration.h b/lib/migration.h
index f126c13..0265d9b 100644
--- a/lib/migration.h
+++ b/lib/migration.h
@@ -65,6 +65,9 @@ migration_get_pgsize(struct migration *migr);
 int
 migration_set_pgsize(struct migration *migr, size_t pgsize);
 
+bool
+vfio_migr_state_transition_is_valid(__u32 from, __u32 to);
+
 #endif /* LIB_VFIO_USER_MIGRATION_H */
 
 /* ex: set tabstop=4 shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/test/unit-tests.c b/test/unit-tests.c
index 9a8243b..1949b89 100644
--- a/test/unit-tests.c
+++ b/test/unit-tests.c
@@ -1102,6 +1102,72 @@ test_vfu_setup_device_dma_cb(void **state __attribute__((unused)))
     assert_non_null(vfu_ctx.dma);
 }
 
+static void
+test_migration_state_transitions(void **state __attribute__ ((unused)))
+{
+    bool (*f)(__u32, __u32) = vfio_migr_state_transition_is_valid;
+    __u32 i, j;
+
+    /* from stopped (000b): all transitions are invalid */
+    assert_true(f(0, 0));
+    for (i = 1; i < 8; i++) {
+        assert_false(f(0, i));
+    }
+
+    /* from running (001b) */
+    assert_true(f(1, 0));
+    assert_true(f(1, 1));
+    assert_true(f(1, 2));
+    assert_true(f(1, 3));
+    assert_true(f(1, 4));
+    assert_false(f(1, 5));
+    assert_true(f(1, 6));
+    assert_false(f(1, 5));
+
+    /* from stop-and-copy (010b) */
+    assert_true(f(2, 0));
+    assert_false(f(2, 1));
+    assert_true(f(2, 2));
+    assert_false(f(2, 3));
+    assert_false(f(2, 4));
+    assert_false(f(2, 5));
+    assert_true(f(2, 6));
+    assert_false(f(2, 7));
+
+    /* from pre-copy (011b) */
+    assert_true(f(3, 0));
+    assert_true(f(3, 1));
+    assert_true(f(3, 2));
+    assert_false(f(3, 3));
+    assert_false(f(3, 4));
+    assert_false(f(3, 5));
+    assert_true(f(3, 6));
+    assert_false(f(3, 7));
+
+    /* from resuming (100b) */
+    assert_false(f(4, 0));
+    assert_true(f(4, 1));
+    assert_false(f(4, 2));
+    assert_false(f(4, 3));
+    assert_true(f(4, 4));
+    assert_false(f(4, 5));
+    assert_true(f(4, 6));
+    assert_false(f(4, 7));
+
+    /*
+     * Transitioning to any other state from the remaining 3 states
+     * (101b - invalid, 110b - error, 111b - invalid)  is invalid.
+     * Transitioning from the error state to the stopped state is possible but
+     * that requires a device reset, so we don't consider it a valid state
+     * transition.
+     */
+    for (i = 5; i < 8; i++) {
+        for (j = 0; j < 8; j++) {
+            assert_false(f(i, j));
+        }
+    }
+}
+
 /*
  * FIXME we shouldn't have to specify a setup function explicitly for each unit
  * test, cmocka should provide that. E.g. cmocka_run_group_tests enables us to
@@ -1138,7 +1204,8 @@ int main(void)
         cmocka_unit_test_setup(test_dma_map_return_value, setup),
         cmocka_unit_test_setup(test_dma_map_sg, setup),
         cmocka_unit_test_setup(test_dma_addr_to_sg, setup),
-        cmocka_unit_test_setup(test_vfu_setup_device_dma_cb, setup)
+        cmocka_unit_test_setup(test_vfu_setup_device_dma_cb, setup),
+        cmocka_unit_test_setup(test_migration_state_transitions, setup)
     };
 
     return cmocka_run_group_tests(tests, NULL, NULL);