Merge lp:~xnox/whoopsie/stable-id-through-boot into lp:whoopsie

[Dimitri John Ledkov]

[John Lenton (chipaca)]

A bug and a question, inline.

review: Needs Fixing

[Dimitri John Ledkov (xnox)]

Replied inline.

[lp:~xnox/whoopsie/stable-id-through-boot]

627. By Dimitri John Ledkov on 2014-08-05

Make whoopsie_identifier_generate() stable throughout the system boot.

That means, whilst whoopsied is running that api call will return the
current ID used by whoopsie itself, irrespective if the caller is root
or non-root, and additional MAC network interfaces or IMEI modems
appearing, and takes into consideration if custom system id was set.

628. By Dimitri John Ledkov on 2014-08-05

Drop unlink.

[Brian Murray (brian-murray)]

It seems to me that in whoopsie.c we could check to see if whoopsie_identifier is the same as the one in the existing WHOOPSIE_ID_PATH (if it exists) rather than writing it again. Otherwise, this looks good and I'll merge and upload it. Thanks!

review: Approve

[Dimitri John Ledkov (xnox)]

did I change the cache path from /run to somewhere persistent? if not, do
update it. I recall discussing /var/lib/ or some such.
On 17 Sep 2014 20:48, "Brian Murray" <email address hidden> wrote:

> Review: Approve
>
> It seems to me that in whoopsie.c we could check to see if
> whoopsie_identifier is the same as the one in the existing WHOOPSIE_ID_PATH
> (if it exists) rather than writing it again. Otherwise, this looks good
> and I'll merge and upload it. Thanks!
> --
>
> https://code.launchpad.net/~xnox/whoopsie/stable-id-through-boot/+merge/229593
> You are the owner of lp:~xnox/whoopsie/stable-id-through-boot.
>

=== modified file 'src/identifier.c'
--- src/identifier.c	2014-06-12 22:47:27 +0000
+++ src/identifier.c	2014-08-05 13:49:18 +0000
@@ -11,6 +11,7 @@
 #include "identifier.h"
 
 static char* ofono_name = "org.ofono";
+static char* cache_file = WHOOPSIE_ID_PATH;
 static gint fail_next_get_mac = 0;
 static gint fail_next_get_uuid = 0;
 
@@ -18,6 +19,10 @@
     ofono_name = name;
 }
 
+void whoopsie_identifier_set_cache_file (char *filename) {
+    cache_file = filename;
+}
+
 void
 whoopsie_hex_to_char (char* buf, const char *str, int len)
 {
@@ -64,12 +69,30 @@
 }
 
 void
+whoopsie_identifier_get_cached (char** res, GError** error)
+{
+    g_return_if_fail (res);
+
+    gsize length = 0;
+
+    if (!g_file_get_contents (cache_file, res, &length, NULL)) {
+        g_set_error (error, g_quark_from_static_string ("whoopsie-quark"), 0,
+                     "Could not load cached identifier file.");
+    } else if (length != HASHLEN) {
+        g_set_error (error, g_quark_from_static_string ("whoopsie-quark"), 0,
+                     "Got an unexpected length reading cached id.");
+        g_free (*res);
+        *res = NULL;
+    }
+    return;
+}
+
+void
 whoopsie_identifier_get_mac_address (char** res, GError** error)
 {
     glob_t globbuf;
     char data[200] = {0};
     gboolean success = FALSE;
-    int i = 0;
 
     g_return_if_fail (res);
 
@@ -250,6 +273,15 @@
 
     g_return_if_fail (res);
 
+    whoopsie_identifier_get_cached (res, error);
+    if ((!error || !(*error)) && *res)
+        return;
+
+    if (error && *error) {
+        g_error_free (*error);
+        *error = NULL;
+    }
+
     whoopsie_identifier_get_system_uuid (&identifier, error);
     if ((!error || !(*error)) && identifier)
         goto out;

=== modified file 'src/identifier.h'
--- src/identifier.h	2014-06-12 22:47:27 +0000
+++ src/identifier.h	2014-08-05 13:49:18 +0000
@@ -8,8 +8,12 @@
 
 /* for testing: */
 void whoopsie_identifier_set_ofono_name (char* name) __attribute__ ((__warning__ ("Only for testing")));
+void whoopsie_identifier_set_cache_file (char* filename) __attribute__ ((__warning__ ("Only for testing")));
 void whoopsie_identifier_fail_next_get_mac (gint how) __attribute__ ((__warning__ ("Only for testing")));
 void whoopsie_identifier_fail_next_get_uuid () __attribute__ ((__warning__ ("Only for testing")));
 #define WHOOPSIE_FAIL_GENERIC 1
 #define WHOOPSIE_FAIL_NO_IFACES 2
 #define WHOOPSIE_FAILED_BY_REQUEST 42
+
+/* The file path for publishing whoopsie_identifier */
+#define WHOOPSIE_ID_PATH "/run/whoopsie-id"

=== modified file 'src/tests/test_identifier.c'
--- src/tests/test_identifier.c	2014-06-12 22:47:27 +0000
+++ src/tests/test_identifier.c	2014-08-05 13:49:18 +0000
@@ -469,6 +469,55 @@
     brofono_stop ();
 }
 
+static void
+test_identifier_cache (void)
+{
+    const char* expected = "f7fbba6e0636f890e56fbbf3283e524c6fa3204ae298382"
+                           "d624741d0dc6638326e282c41be5e4254d8820772c5518a"
+                           "2c5a8c0c7f7eda19594a7eb539453e1ed7";
+    char* res = NULL;
+    GError* error = NULL;
+
+    gchar *test_path = NULL;
+    gchar *test_dir = NULL;
+    gchar test_tmpl[PATH_MAX] = "/tmp/whoopsie-test-XXXXXX";
+    test_dir = g_mkdtemp (test_tmpl);
+    g_assert (test_dir != NULL);
+
+    /* Check non-existent id file fails lookup */
+    test_path = g_strdup_printf ("%s/non-existant", test_dir);
+    whoopsie_identifier_set_cache_file (test_path);
+    whoopsie_identifier_get_cached (&res, &error);
+    g_assert_null (res);
+    g_assert_error (error, g_quark_from_static_string ("whoopsie-quark"), 0);
+    g_free (test_path);
+    g_error_free (error);
+    error = NULL;
+
+    /* Check good cache is loaded */
+    test_path = g_strdup_printf ("%s/good", test_dir);
+    g_assert (g_file_set_contents (test_path, expected, -1, NULL));
+    whoopsie_identifier_set_cache_file (test_path);    
+    whoopsie_identifier_get_cached (&res, &error);
+    g_assert_nonnull (res);
+    g_assert_no_error (error);
+    g_assert_cmpstr (res, ==, expected);
+    g_unlink (test_path);
+    g_free (test_path);
+
+    /* Check bad cache is not loaded */
+    test_path = g_strdup_printf ("%s/good", test_dir);
+    g_assert (g_file_set_contents (test_path, expected, HASHLEN-3, NULL));
+    whoopsie_identifier_set_cache_file (test_path);
+    whoopsie_identifier_get_cached (&res, &error);
+    g_assert_null (res);
+    g_assert_error (error, g_quark_from_static_string ("whoopsie-quark"), 0);
+    g_unlink (test_path);
+    g_free (test_path);
+
+    g_assert (g_rmdir (test_dir) == 0);
+}
+
 int
 main (int argc, char** argv)
 {
@@ -478,6 +527,9 @@
 #endif
     g_test_init (&argc, &argv, NULL);
 
+    /* Prevent reading currently running system id */
+    whoopsie_identifier_set_cache_file ("/tmp");
+
     g_test_add_func ("/whoopsie/get-system-uuid", test_get_system_uuid);
     g_test_add_func ("/whoopsie/get-uuid-fails-when-told", test_get_system_uuid_fail_when_told_to);
     g_test_add_func ("/whoopsie/hex-to-char", test_hex_to_char);
@@ -493,6 +545,7 @@
     g_test_add_func ("/whoopsie/imei-empty-reply", test_get_imei_empty_reply);
     g_test_add_func ("/whoopsie/imei-bad", test_get_imei_bad_imei);
     g_test_add_func ("/whoopsie/imei-missing", test_get_imei_no_imei);
+    g_test_add_func ("/whoopsie/get-cached", test_identifier_cache);
 
     /* Do not consider warnings to be fatal. */
     g_log_set_always_fatal (G_LOG_FATAL_MASK);

=== modified file 'src/whoopsie.c'
--- src/whoopsie.c	2014-07-29 20:49:35 +0000
+++ src/whoopsie.c	2014-08-05 13:49:18 +0000
@@ -964,6 +964,9 @@
                                                whoopsie_identifier);
     }
 
+    /* Publish whoopsie-id */
+    g_file_set_contents (WHOOPSIE_ID_PATH, whoopsie_identifier, -1, NULL);
+
     create_crash_directory ();
 
     drop_privileges (&err);
@@ -1011,6 +1014,6 @@
 
     g_free (crash_db_url);
     g_free (crash_db_submit_url);
-	return 0;
+    return 0;
 }
 #endif