Merge lp:~mhr3/dee/discard-invalid-transactions into lp:dee

[Michal Hruby]

Discard invalid transactions

When we receive a commit on a model that is not valid (wrong seqnum etc), we only emit a warning and apply the transaction. This is wrong as it may leave the model in a weird state, the transaction needs to be discarded.

Discard invalid transactions.

[Mikkel Kamstrup Erlandsen (kamstrup)]

Code looks correct. Nice to get rid of the FIXMEs :-)

Testing wise it would be quite a lot of work for little gain. These code paths are quite heavily tested already, and getting into all the nooks and crannies of the ways this can fail is a big undertaking. Very worthwhile, but let's not block this branch on it.

review: Approve

=== modified file 'src/dee-shared-model.c'
--- src/dee-shared-model.c	2012-02-28 10:56:27 +0000
+++ src/dee-shared-model.c	2012-03-26 08:47:25 +0000
@@ -1183,6 +1183,7 @@
   guint32                pos;
   guchar                 change_type;
   gint                   i, j;
+  gboolean               transaction_error;
 
   g_return_if_fail (DEE_IS_SHARED_MODEL (self));
   g_return_if_fail (transaction != NULL);
@@ -1249,48 +1250,56 @@
   g_variant_get (tt, "(tt)", &seqnum_before, &seqnum_after);
   g_variant_unref (tt);
 
+  transaction_error = FALSE;
   /* If this is our first transaction we accept anything, if not the
    * incoming seqnums must align with our own records */
   current_seqnum = dee_serializable_model_get_seqnum (DEE_MODEL (self));
-  if (current_seqnum != 0 &&
-      current_seqnum != seqnum_before)
-  {
+
+  if (current_seqnum != 0 && current_seqnum != seqnum_before)
+    {
       g_warning ("Transaction from %s is in the %s. Expected seqnum %"G_GUINT64_FORMAT
                  ", but got %"G_GUINT64_FORMAT". Ignoring transaction.",
                  sender_name,
                  current_seqnum < seqnum_before ? "future" : "past",
                  current_seqnum, seqnum_before);
-      if (dee_shared_model_is_leader (self))
-        {
-          g_warning ("Invalidating %s", sender_name);
-          invalidate_peer (self, sender_name, NULL);
-        }
-
-      g_variant_unref (transaction);
-      g_variant_unref (aav);
-      g_variant_unref (au);
-      g_variant_unref (ay);
-      return;
-  }
+      transaction_error = TRUE;
+    }
 
   /* Check that the lengths of all the arrays match up */
   n_rows = g_variant_n_children (aav);
+
   if (n_rows != g_variant_n_children (au))
     {
       g_warning ("Commit from %s has illegal position vector",
                  sender_name);
-      // FIXME cleanup
+      transaction_error = TRUE;
     }
   if (n_rows != g_variant_n_children (ay))
     {
       g_warning ("Commit from %s has illegal change type vector",
                  sender_name);
-      // FIXME cleanup
+      transaction_error = TRUE;
     }
   if (n_rows > (seqnum_after - seqnum_before))
     {
       g_warning ("Commit from %s has illegal seqnum count.",
                  sender_name);
+      transaction_error = TRUE;
+    }
+
+  if (transaction_error)
+    {
+      if (dee_shared_model_is_leader (self))
+        {
+          g_warning ("Invalidating %s", sender_name);
+          invalidate_peer (self, sender_name, NULL);
+        }
+
+      g_variant_unref (transaction);
+      g_variant_unref (aav);
+      g_variant_unref (au);
+      g_variant_unref (ay);
+      return;
     }
   
   /* Allocate an array on the stack as a temporary row data buffer */