[gnome-keyring] gnome2-store: Add additional tests for locking

[Stefan Walter <stefw src gnome org>]

commit 052fa8ebf4296b31df8c0424c4e85f9213eee159
Author: Stef Walter <stefw collabora co uk>
Date:   Tue Nov 1 14:28:44 2011 +0100

    gnome2-store: Add additional tests for locking
    
    https://bugzilla.gnome.org/show_bug.cgi?id=657234

 pkcs11/gnome2-store/gkm-gnome2-storage.c        |   72 +++++------
 pkcs11/gnome2-store/tests/test-gnome2-storage.c |  159 +++++++++++++++++++++--
 2 files changed, 176 insertions(+), 55 deletions(-)
---
diff --git a/pkcs11/gnome2-store/gkm-gnome2-storage.c b/pkcs11/gnome2-store/gkm-gnome2-storage.c
index cd705c7..e0f7720 100644
--- a/pkcs11/gnome2-store/gkm-gnome2-storage.c
+++ b/pkcs11/gnome2-store/gkm-gnome2-storage.c
@@ -415,7 +415,6 @@ static gboolean
 begin_modification_state (GkmGnome2Storage *self, GkmTransaction *transaction)
 {
 	GkmDataResult res;
-	struct stat sb;
 	CK_RV rv = CKR_OK;
 
 	/* Already in write state for this transaction? */
@@ -428,33 +427,30 @@ begin_modification_state (GkmGnome2Storage *self, GkmTransaction *transaction)
 		return FALSE;
 
 	/* See if file needs updating */
-	if (fstat (self->read_fd, &sb) >= 0 && sb.st_mtime != self->last_mtime) {
-
-		res = gkm_gnome2_file_read_fd (self->file, self->read_fd, self->login);
-		switch (res) {
-		case GKM_DATA_FAILURE:
-			g_message ("failure updating user store file: %s", self->filename);
-			rv = CKR_FUNCTION_FAILED;
-			break;
-		case GKM_DATA_LOCKED:
-			rv = CKR_USER_NOT_LOGGED_IN;
-			break;
-		case GKM_DATA_UNRECOGNIZED:
-			g_message ("unrecognized or invalid user store file: %s", self->filename);
-			rv = CKR_FUNCTION_FAILED;
-			break;
-		case GKM_DATA_SUCCESS:
-			rv = CKR_OK;
-			break;
-		default:
-			g_assert_not_reached ();
-			break;
-		}
+	res = gkm_gnome2_file_read_fd (self->file, self->read_fd, self->login);
+	switch (res) {
+	case GKM_DATA_FAILURE:
+		g_message ("failure updating user store file: %s", self->filename);
+		rv = CKR_FUNCTION_FAILED;
+		break;
+	case GKM_DATA_LOCKED:
+		rv = CKR_USER_NOT_LOGGED_IN;
+		break;
+	case GKM_DATA_UNRECOGNIZED:
+		g_message ("unrecognized or invalid user store file: %s", self->filename);
+		rv = CKR_FUNCTION_FAILED;
+		break;
+	case GKM_DATA_SUCCESS:
+		rv = CKR_OK;
+		break;
+	default:
+		g_assert_not_reached ();
+		break;
+	}
 
-		if (rv != CKR_OK) {
-			gkm_transaction_fail (transaction, rv);
-			return FALSE;
-		}
+	if (rv != CKR_OK) {
+		gkm_transaction_fail (transaction, rv);
+		return FALSE;
 	}
 
 	/* Write out the data once completed with modifications */
@@ -813,16 +809,16 @@ gkm_gnome2_storage_real_read_value (GkmStore *base, GkmObject *object, CK_ATTRIB
 	g_return_val_if_fail (GKM_IS_OBJECT (object), CKR_GENERAL_ERROR);
 	g_return_val_if_fail (attr, CKR_GENERAL_ERROR);
 
-	identifier = g_hash_table_lookup (self->object_to_identifier, object);
-	if (!identifier)
-		return CKR_ATTRIBUTE_TYPE_INVALID;
-
 	if (self->last_mtime == 0) {
 		rv = gkm_gnome2_storage_refresh (self);
 		if (rv != CKR_OK)
 			return rv;
 	}
 
+	identifier = g_hash_table_lookup (self->object_to_identifier, object);
+	if (!identifier)
+		return CKR_ATTRIBUTE_TYPE_INVALID;
+
 	res = gkm_gnome2_file_read_value (self->file, identifier, attr->type, &value, &n_value);
 	switch (res) {
 	case GKM_DATA_FAILURE:
@@ -855,23 +851,15 @@ gkm_gnome2_storage_real_write_value (GkmStore *base, GkmTransaction *transaction
 	g_return_if_fail (!gkm_transaction_get_failed (transaction));
 	g_return_if_fail (attr);
 
+	if (!begin_modification_state (self, transaction))
+		return;
+
 	identifier = g_hash_table_lookup (self->object_to_identifier, object);
 	if (!identifier) {
 		gkm_transaction_fail (transaction, CKR_ATTRIBUTE_READ_ONLY);
 		return;
 	}
 
-	if (!begin_modification_state (self, transaction))
-		return;
-
-	if (self->last_mtime == 0) {
-		rv = gkm_gnome2_storage_refresh (self);
-		if (rv != CKR_OK) {
-			gkm_transaction_fail (transaction, rv);
-			return;
-		}
-	}
-
 	res = gkm_gnome2_file_write_value (self->file, identifier, attr->type, attr->pValue, attr->ulValueLen);
 	switch (res) {
 	case GKM_DATA_FAILURE:
diff --git a/pkcs11/gnome2-store/tests/test-gnome2-storage.c b/pkcs11/gnome2-store/tests/test-gnome2-storage.c
index 5cb4488..18de3c2 100644
--- a/pkcs11/gnome2-store/tests/test-gnome2-storage.c
+++ b/pkcs11/gnome2-store/tests/test-gnome2-storage.c
@@ -53,6 +53,8 @@ typedef struct {
 	GkmObject *old_object;
 } Test;
 
+#define MSEC(x) ((x) * 1000)
+
 static void
 copy_scratch_file (Test *test,
                    const gchar *name)
@@ -269,8 +271,110 @@ test_write_value (Test *test,
 }
 
 static void
-test_lock_contention (Test *test,
-                      gconstpointer unused)
+test_locking_transaction (Test *test,
+                          gconstpointer unused)
+{
+	guint iterations = 30;
+	guint i;
+	pid_t pid;
+
+	/* Fork before setting up the model, as it may start threads */
+	pid = fork ();
+	g_assert (pid >= 0);
+
+	/*
+	 * This is the child. It initializes, writes a value, waits 100 ms,
+	 * writes a second value, and then writes another value.
+	 */
+	if (pid == 0) {
+		CK_ATTRIBUTE attr;
+		GkmTransaction *transaction;
+		gchar *string;
+
+		setup_module (test, unused);
+
+		for (i = 0; i < iterations; i++) {
+			g_printerr ("c");
+
+			transaction = gkm_transaction_new ();
+
+			string = g_strdup_printf ("%d", i);
+
+			attr.type = CKA_LABEL;
+			attr.pValue = string;
+			attr.ulValueLen = strlen (string);
+
+			gkm_store_write_value (GKM_STORE (test->storage), transaction,
+			                       test->old_object, &attr);
+			gkm_assert_cmprv (gkm_transaction_get_result (transaction), ==, CKR_OK);
+
+			g_usleep (100 * 1000);
+
+			attr.type = CKA_URL;
+			attr.pValue = string;
+			attr.ulValueLen = strlen (string);
+
+			gkm_store_write_value (GKM_STORE (test->storage), transaction,
+			                       test->old_object, &attr);
+			gkm_assert_cmprv (gkm_transaction_get_result (transaction), ==, CKR_OK);
+
+			g_free (string);
+
+			gkm_transaction_complete_and_unref (transaction);
+
+			g_usleep (10 * 1000);
+		}
+
+		teardown_module (test, unused);
+		_exit (0);
+		g_assert_not_reached ();
+
+	/*
+	 * This is the parent. it initializes, waits 100 ms, writes a value that
+	 * should override the one from the child, because the file is locked
+	 * when it tries to write, so it waits for the child to finish. The other
+	 * attribute from the child (the label) should come through.
+	 */
+	} else {
+		gchar *string1;
+		gchar *string2;
+		pid_t wpid;
+		int status;
+		CK_RV rv;
+
+		g_assert (pid != -1);
+
+		setup_module (test, unused);
+
+		for (i = 0; i < iterations; i++) {
+			g_printerr ("p");
+
+			string1 = gkm_store_read_string (GKM_STORE (test->storage), test->old_object, CKA_URL);
+
+			g_usleep (g_random_int_range (1, 200) * 1000);
+
+			string2 = gkm_store_read_string (GKM_STORE (test->storage), test->old_object, CKA_LABEL);
+
+			g_assert_cmpstr (string1, ==, string2);
+			g_free (string1);
+			g_free (string2);
+
+			rv = gkm_gnome2_storage_refresh (test->storage);
+			gkm_assert_cmprv (rv, ==, CKR_OK);
+		}
+
+		/* wait for the child to finish */
+		wpid = waitpid (pid, &status, 0);
+		g_assert_cmpint (wpid, ==, pid);
+		g_assert_cmpint (status, ==, 0);
+
+		teardown_module (test, unused);
+	}
+}
+
+static void
+test_lock_writes (Test *test,
+                  gconstpointer unused)
 {
 	pid_t pid;
 
@@ -283,8 +387,8 @@ test_lock_contention (Test *test,
 	 * writes a second value, and then writes another value.
 	 */
 	if (pid == 0) {
-		CK_ATTRIBUTE label = { CKA_LABEL, "Hello", 5 };
-		CK_ATTRIBUTE url = { CKA_URL, "http://example.com";, 18 };
+		CK_ATTRIBUTE label = { CKA_LABEL, "Hello from child", 16 };
+		CK_ATTRIBUTE url = { CKA_URL, "http://child.example.com";, 24 };
 		GkmTransaction *transaction;
 
 		setup_module (test, unused);
@@ -295,7 +399,7 @@ test_lock_contention (Test *test,
 		                       test->old_object, &label);
 		gkm_assert_cmprv (gkm_transaction_get_result (transaction), ==, CKR_OK);
 
-		g_usleep (300 * 1000);
+		g_usleep (MSEC (100));
 
 		gkm_store_write_value (GKM_STORE (test->storage), transaction,
 		                       test->old_object, &url);
@@ -319,38 +423,60 @@ test_lock_contention (Test *test,
 		gchar *string;
 		pid_t wpid;
 		int status;
+		CK_RV rv;
 
 		g_assert (pid != -1);
 
-		g_usleep (100 * 1000);
-
 		setup_module (test, unused);
 
+		/* Refresh the store, and check values are not set */
+		string = gkm_store_read_string (GKM_STORE (test->storage), test->old_object, CKA_URL);
+		g_assert (string == NULL);
+
+		string = gkm_store_read_string (GKM_STORE (test->storage), test->old_object, CKA_LABEL);
+		g_assert (string == NULL);
+
+		g_usleep (MSEC (1000));
+
 		transaction = gkm_transaction_new ();
 
 		gkm_store_write_value (GKM_STORE (test->storage), transaction,
 		                       test->old_object, &url);
 		gkm_assert_cmprv (gkm_transaction_get_result (transaction), ==, CKR_OK);
 
-		gkm_transaction_complete_and_unref (transaction);
-
 		/* wait for the child to finish */
 		wpid = waitpid (pid, &status, 0);
 		g_assert_cmpint (wpid, ==, pid);
 		g_assert_cmpint (status, ==, 0);
 
+		gkm_transaction_complete_and_unref (transaction);
+
+		g_usleep (MSEC (1000));
+
+		rv = gkm_gnome2_storage_refresh (test->storage);
+		gkm_assert_cmprv (rv, ==, CKR_OK);
+
 		string = gkm_store_read_string (GKM_STORE (test->storage), test->old_object, CKA_URL);
 		g_assert_cmpstr (string, ==, "http://parent.example.com";);
 		g_free (string);
 
 		string = gkm_store_read_string (GKM_STORE (test->storage), test->old_object, CKA_LABEL);
-		g_assert_cmpstr (string, ==, "Hello");
+		g_assert_cmpstr (string, ==, "Hello from child");
 		g_free (string);
 
 		teardown_module (test, unused);
 	}
 }
 
+static void
+null_log_handler (const gchar *log_domain,
+                  GLogLevelFlags log_level,
+                  const gchar *message,
+                  gpointer user_data)
+{
+
+}
+
 int
 main (int argc, char **argv)
 {
@@ -359,6 +485,10 @@ main (int argc, char **argv)
 
 	egg_libgcrypt_initialize ();
 
+	/* Suppress these messages in tests */
+	g_log_set_handler (G_LOG_DOMAIN, G_LOG_LEVEL_MESSAGE,
+	                   null_log_handler, NULL);
+
 	g_test_add ("/gnome2-store/storage/create", Test, NULL,
 	            setup_all, test_create, teardown_all);
 	g_test_add ("/gnome2-store/storage/create_and_fail", Test, NULL,
@@ -366,9 +496,12 @@ main (int argc, char **argv)
 	g_test_add ("/gnome2-store/storage/write_value", Test, NULL,
 	            setup_all, test_write_value, teardown_all);
 
-	if (g_test_thorough ())
-		g_test_add ("/gnome2-store/storage/lock_contention", Test, NULL,
-		            setup_directory, test_lock_contention, teardown_directory);
+	if (!g_test_quick ()) {
+		g_test_add ("/gnome2-store/storage/locking_transaction", Test, NULL,
+		            setup_directory, test_locking_transaction, teardown_directory);
+		g_test_add ("/gnome2-store/storage/lock_writes", Test, NULL,
+		            setup_directory, test_lock_writes, teardown_directory);
+	}
 
 	return g_test_run ();
 }