Merge lp:~ted/whoopsie/recoverable-problem into lp:whoopsie

[Ted Gould]

libwhoopsie function to create a recoverable error

Makes building a recoverable error in a program a bit easier. Putting it in libwhoopsie so many programs who are already using it can remove the duplicate code and just link into libwhoopsie. This is very non-Go-like, but we're okay with it.

[Evan (ev)]

Could I trouble you for a test case?

Otherwise this looks good.

review: Needs Fixing

[lp:~ted/whoopsie/recoverable-problem]

599. By Ted Gould on 2015-06-05

Merge trunk

600. By Ted Gould on 2015-06-05

Adding in a stub for recoverable testing

601. By Ted Gould on 2015-06-05

Drop the identifier because it's disabled

602. By Ted Gould on 2015-06-05

Allow overriding the path to the recoverable problem script for testing

603. By Ted Gould on 2015-06-05

Adding a simple mock

604. By Ted Gould on 2015-06-05

Adding in some content and cmdline tests

605. By Ted Gould on 2015-06-05

Make sure we drop pidstr

606. By Ted Gould on 2015-06-05

Make it so that we can disable reporting of the recoverable errors

[Ted Gould (ted)]

Updated to trunk and tests added. Sorry that took so long.

[lp:~ted/whoopsie/recoverable-problem]

607. By Ted Gould on 2015-06-05

Updated symbols file

[Evan (ev)]

This looks good. Thanks for the persistence, Ted!

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2013-08-06 15:41:19 +0000
+++ .bzrignore	2015-06-05 21:31:49 +0000
@@ -9,7 +9,18 @@
 debian/files
 debian/whoopsie.substvars
 src/tests/test_parse_report
+src/tests/test_parse_report_coverage
 src/tests/test_utils
+src/tests/test_utils_coverage
 src/tests/test_monitor
+src/tests/test_monitor_coverage
 src/tests/test_identifier
 src/tests/test_logging
+src/tests/test_logging_coverage
+src/tests/test_recoverable
+src/tests/test_recoverable_coverage
+src/tests/recoverable_mock.cmdline
+src/tests/recoverable_mock.contents
+*.gcov
+*.gcda
+*.gcno

=== modified file 'Makefile'
--- Makefile	2015-03-13 19:35:46 +0000
+++ Makefile	2015-06-05 21:31:49 +0000
@@ -1,8 +1,9 @@
 VERSION=$(shell dpkg-parsechangelog | grep ^Version: | cut -d' ' -f2)
 
-libwhoopsie_SOURCES=src/identifier.c
+libwhoopsie_SOURCES=src/identifier.c \
+                    src/recoverable-problem.c
 libwhoopsie_OBJECTS=$(libwhoopsie_SOURCES:.c=.o)
-libwhoopsie_CFLAGS=$(shell pkg-config --cflags glib-2.0) \
+libwhoopsie_CFLAGS=$(shell pkg-config --cflags glib-2.0 gio-2.0) \
 				   $(shell libgcrypt-config --cflags) \
 				   $(CFLAGS) $(CPPFLAGS)
 libwhoopsie_LIBS=$(shell pkg-config --libs gio-2.0 glib-2.0) \
@@ -10,7 +11,8 @@
 				 $(LDFLAGS)
 
 whoopsie_CFLAGS=$(shell pkg-config --cflags gio-2.0 glib-2.0 libcurl) \
-	   -g -Ilib -Wall -Werror -Os -DVERSION=\"$(VERSION)\" \
+	   $(shell libgcrypt-config --cflags) \
+	   -g -Ilib -Wall -Werror -Os -DVERSION=\"$(VERSION)\" -fPIC \
 	   $(CFLAGS) $(CPPFLAGS)
 whoopsie_LIBS=$(shell pkg-config --libs gio-2.0 glib-2.0 libcurl) -lcap \
 			  $(LDFLAGS)
@@ -31,9 +33,6 @@
 
 all: $(SOURCES) $(EXECUTABLE)
 
-$(libwhoopsie_OBJECTS): $(libwhoopsie_SOURCES)
-	$(CC) -std=c99 -fPIC -c $(libwhoopsie_CFLAGS) -o $@ $^
-
 src/libwhoopsie.so.0.0: $(libwhoopsie_OBJECTS)
 	$(CC) -std=c99 -shared -Wl,-soname,libwhoopsie.so.0 -o $@ $^ $(libwhoopsie_LIBS)
 	ln -sf libwhoopsie.so.0.0 src/libwhoopsie.so.0
@@ -75,3 +74,4 @@
 	install -m644 lib/libwhoopsie.pc $(DESTDIR)/usr/lib/pkgconfig
 	install -d $(DESTDIR)/usr/include/libwhoopsie
 	install -m644 src/identifier.h $(DESTDIR)/usr/include/libwhoopsie
+	install -m644 src/recoverable-problem.h $(DESTDIR)/usr/include/libwhoopsie

=== added file 'debian/libwhoopsie0.symbols'
--- debian/libwhoopsie0.symbols	1970-01-01 00:00:00 +0000
+++ debian/libwhoopsie0.symbols	2015-06-05 21:31:49 +0000
@@ -0,0 +1,16 @@
+libwhoopsie.so.0 libwhoopsie0 #MINVER#
+ whoopsie_hex_to_char@Base 0.2.48
+ whoopsie_identifier_append_imei@Base 0.2.48
+ whoopsie_identifier_fail_next_get_mac@Base 0.2.48
+ whoopsie_identifier_fail_next_get_serial@Base 0.2.48
+ whoopsie_identifier_fail_next_get_uuid@Base 0.2.48
+ whoopsie_identifier_generate@Base 0.2.48
+ whoopsie_identifier_get_android_serialno@Base 0.2.48
+ whoopsie_identifier_get_cached@Base 0.2.48
+ whoopsie_identifier_get_mac_address@Base 0.2.48
+ whoopsie_identifier_get_system_uuid@Base 0.2.48
+ whoopsie_identifier_set_cache_file@Base 0.2.48
+ whoopsie_identifier_set_ofono_name@Base 0.2.48
+ whoopsie_identifier_sha512@Base 0.2.48
+ whoopsie_report_recoverable_problem@Base 0.2.48
+

=== modified file 'debian/rules'
--- debian/rules	2014-11-07 10:17:30 +0000
+++ debian/rules	2015-06-05 21:31:49 +0000
@@ -9,6 +9,8 @@
 # Uncomment this to turn on verbose mode.
 #export DH_VERBOSE=1
 
+export DPKG_GENSYMBOLS_CHECK_LEVEL = 4
+
 export DESTDIR=$(CURDIR)/debian/whoopsie
 
 %:

=== added file 'src/recoverable-problem.c'
--- src/recoverable-problem.c	1970-01-01 00:00:00 +0000
+++ src/recoverable-problem.c	2015-06-05 21:31:49 +0000
@@ -0,0 +1,177 @@
+/*
+ * Copyright 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Ted Gould <[e-mail]>
+ */
+
+#include "recoverable-problem.h"
+#include <glib/gstdio.h>
+#include <string.h>
+#include <errno.h>
+
+/* Helpers to ensure we write nicely */
+static void 
+write_string (int          fd,
+              const gchar *string)
+{
+	int res; 
+	do
+		res = write (fd, string, strlen (string));
+	while (G_UNLIKELY (res == -1 && errno == EINTR));
+}
+
+/* Make NULLs fast and fun! */
+static void 
+write_null (int fd)
+{
+	int res; 
+	do
+		res = write (fd, "", 1);
+	while (G_UNLIKELY (res == -1 && errno == EINTR));
+}
+
+/* Child watcher */
+static gboolean
+apport_child_watch (GPid pid, gint status, gpointer user_data)
+{
+	g_main_loop_quit((GMainLoop *)user_data);
+	return FALSE;
+}
+
+static gboolean
+apport_child_timeout (gpointer user_data)
+{
+	g_warning("Recoverable Error Reporter Timeout");
+	g_main_loop_quit((GMainLoop *)user_data);
+	return FALSE;
+}
+
+/* Code to report an error */
+void
+whoopsie_report_recoverable_problem (const gchar * signature, GPid report_pid, gboolean wait, const gchar * additional_properties[])
+{
+	GError * error = NULL;
+	gint error_stdin = 0;
+	GPid pid = 0;
+	gchar * pid_str = NULL;
+	const gchar * recov_env = NULL;
+	gchar ** argv = NULL;
+	gchar * argv_nopid[2] = {
+		"/usr/share/apport/recoverable_problem",
+		NULL
+	};
+	gchar * argv_pid[4] = {
+		"/usr/share/apport/recoverable_problem",
+		"-p",
+		NULL, /* put pid_str when allocated here */
+		NULL
+	};
+
+	/* Mostly for test suites, we don't want the recoverable errors from those */
+	if (g_getenv("WHOOPSIE_RECOVERABLE_PROBLEM_DISABLE")) {
+		g_debug("Reporting of recoverable problems disabled");
+		return;
+	}
+
+	argv = (gchar **)argv_nopid;
+
+	if (report_pid != 0) {
+		pid_str = g_strdup_printf("%d", report_pid);
+		argv_pid[2] = pid_str;
+		argv = (gchar**)argv_pid;
+	}
+
+	if ((recov_env = g_getenv("WHOOPSIE_RECOVERABLE_PROBLEM_PATH")) != NULL) {
+		argv[0] = (gchar *)recov_env;
+	}
+
+	GSpawnFlags flags = G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
+	if (wait) {
+		flags |= G_SPAWN_DO_NOT_REAP_CHILD;
+	}
+
+	g_spawn_async_with_pipes(NULL, /* cwd */
+		argv,
+		NULL, /* envp */
+		flags,
+		NULL, NULL, /* child setup func */
+		&pid,
+		&error_stdin,
+		NULL, /* stdout */
+		NULL, /* stderr */
+		&error);
+
+	if (error != NULL) {
+		g_warning("Unable to report a recoverable error: %s", error->message);
+		g_error_free(error);
+		g_free(pid_str);
+		return;
+	}
+
+	gboolean first = TRUE;
+
+	if (error_stdin != 0 && signature != NULL) {
+		write_string(error_stdin, "DuplicateSignature");
+		write_null(error_stdin);
+		write_string(error_stdin, signature);
+
+		first = FALSE;
+	}
+
+	if (error_stdin != 0 && additional_properties != NULL) {
+		gint i;
+		for (i = 0; additional_properties[i] != NULL; i++) {
+			if (!first) {
+				write_null(error_stdin);
+			} else {
+				first = FALSE;
+			}
+
+			write_string(error_stdin, additional_properties[i]);
+		}
+	}
+
+	if (error_stdin != 0) {
+		close(error_stdin);
+	}
+
+	if (wait && pid != 0) {
+		GSource * child_source, * timeout_source;
+		GMainContext * context = g_main_context_new();
+		GMainLoop * loop = g_main_loop_new(context, FALSE);
+
+		child_source = g_child_watch_source_new(pid);
+		g_source_attach(child_source, context);
+		g_source_set_callback(child_source, (GSourceFunc)apport_child_watch, loop, NULL);
+
+		timeout_source = g_timeout_source_new_seconds(5);
+		g_source_attach(timeout_source, context);
+		g_source_set_callback(timeout_source, apport_child_timeout, loop, NULL);
+
+		g_main_loop_run(loop);
+
+		g_source_destroy(timeout_source);
+		g_source_destroy(child_source);
+		g_main_loop_unref(loop);
+		g_main_context_unref(context);
+
+		g_spawn_close_pid(pid);
+	}
+
+	g_free(pid_str);
+
+	return;
+}

=== added file 'src/recoverable-problem.h'
--- src/recoverable-problem.h	1970-01-01 00:00:00 +0000
+++ src/recoverable-problem.h	2015-06-05 21:31:49 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Ted Gould <[e-mail]>
+ */
+
+#include <glib.h>
+#include <gio/gio.h>
+
+#ifndef WHOOPSIE_RECOVERABLE_PROBLEM__
+#define WHOOPSIE_RECOVERABLE_PROBLEM__ 1
+
+G_BEGIN_DECLS
+
+/**
+ * whoopsie_report_recoverable_problem:
+ * @signature: The signature bucket for this problem
+ * @report_pid: PID to report the error on or zero for this process
+ * @wait: Wait until the problem is reported (important if this
+ *        and there's a chance we'd exit
+ * @additional_properties: Other properties that should be included
+ *        in the report. Should have a multiple of 2 number of entries.
+ *
+ * Reports a recoverable problem so that it can be tracked on
+ * daisy. Generally these are issues that can be dealt with, but having
+ * tracking of them would be helpful.
+ */
+void    whoopsie_report_recoverable_problem    (const gchar *   signature,
+                                                GPid            report_pid,
+                                                gboolean        wait,
+                                                const gchar *   additional_properties[]);
+
+G_END_DECLS
+
+#endif /* WHOOPSIE_RECOVERABLE_PROBLEM__ */

=== modified file 'src/tests/Makefile'
--- src/tests/Makefile	2014-10-09 22:13:17 +0000
+++ src/tests/Makefile	2015-06-05 21:31:49 +0000
@@ -8,6 +8,7 @@
 test_parse_report_SOURCES=test_parse_report.c \
 							../whoopsie.c \
 							../logging.c \
+							../recoverable-problem.c \
 							../utils.c \
 							../identifier.c \
 							../../lib/bson/bson.c \
@@ -38,11 +39,16 @@
 test_logging_EXECUTABLE=test_logging
 test_logging_OBJECTS=$(test_logging_SOURCES:.c=.test.o)
 
+test_recoverable_SOURCES=test_recoverable.c \
+					../recoverable-problem.c
+test_recoverable_EXECUTABLE=test_recoverable
+test_recoverable_OBJECTS=$(test_recoverable_SOURCES:.c=.test.o)
+
 .PHONY: all clean check
 
 all: check
 
-check: $(test_parse_report_EXECUTABLE) $(test_utils_EXECUTABLE) $(test_monitor_EXECUTABLE) $(test_logging_EXECUTABLE)
+check: $(test_parse_report_EXECUTABLE) $(test_utils_EXECUTABLE) $(test_monitor_EXECUTABLE) $(test_logging_EXECUTABLE) $(test_recoverable_EXECUTABLE)
 	@r=0; for t in $^; do \
 		./$$t -k || r=1; \
 	done; \
@@ -58,6 +64,8 @@
 	#$(CC) -std=c99 $^ $(LIBS) -o $@
 $(test_logging_EXECUTABLE): $(test_logging_OBJECTS)
 	$(CC) -std=c99 $^ $(LIBS) -o $@
+$(test_recoverable_EXECUTABLE): $(test_recoverable_OBJECTS)
+	$(CC) -std=c99 $^ $(LIBS) -o $@
 
 test_parse_report_coverage: $(test_parse_report_OBJECTS:.test.o=.coverage.o)
 	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
@@ -65,11 +73,13 @@
 	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
 test_monitor_coverage: $(test_monitor_OBJECTS:.test.o=.coverage.o)
 	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
-test_identifier_coverage: $(test_identifier_OBJECTS:.test.o=.coverage.o)
-	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
+#test_identifier_coverage: $(test_identifier_OBJECTS:.test.o=.coverage.o)
+#	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
 test_logging_coverage: $(test_logging_OBJECTS:.test.o=.coverage.o)
 	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
-coverage: test_parse_report_coverage test_utils_coverage test_monitor_coverage test_identifier_coverage test_logging_coverage
+test_recoverable_coverage: $(test_recoverable_OBJECTS:.test.o=.coverage.o)
+	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
+coverage: test_parse_report_coverage test_utils_coverage test_monitor_coverage test_logging_coverage test_recoverable_coverage
 	@r=0; for t in $^; do \
 		./$$t -k || r=1; \
 	done; \
@@ -87,10 +97,13 @@
 		$(test_identifier_EXECUTABLE) \
 		$(test_logging_EXECUTABLE) \
 		$(test_logging_OBJECTS) \
+		$(test_recoverable_EXECUTABLE) \
+		$(test_recoverable_OBJECTS) \
 		test_parse_report_coverage \
 		test_utils_coverage \
 		test_identifier_coverage \
 		test_logging_coverage \
+		test_recoverable_coverage \
 		coverage
 	find ../.. \( -name '*.coverage.o' -o \
 		-name '*.gcda' -o \

=== added file 'src/tests/recoverable_mock.sh'
--- src/tests/recoverable_mock.sh	1970-01-01 00:00:00 +0000
+++ src/tests/recoverable_mock.sh	2015-06-05 21:31:49 +0000
@@ -0,0 +1,4 @@
+#!/bin/bash
+
+echo -n $@ > recoverable_mock.cmdline
+cat /dev/stdin > recoverable_mock.contents

=== added file 'src/tests/test_recoverable.c'
--- src/tests/test_recoverable.c	1970-01-01 00:00:00 +0000
+++ src/tests/test_recoverable.c	2015-06-05 21:31:49 +0000
@@ -0,0 +1,99 @@
+/* whoopsie
+ * 
+ * Copyright © 2015 Canonical Ltd.
+ * Author: Ted Gould <[e-mail]>
+ * 
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <string.h>
+#include <glib.h>
+#include "../recoverable-problem.h"
+
+#define TEST_SIGNATURE "DuplicateSignature\0test-signature"
+#define TEST_DATALIST "DuplicateSignature\0test-signature\0Foo\0Bar\0LongDataList\0A\n B\n C"
+
+void
+test_recoverable (void)
+{
+    gchar * contents = NULL;
+    gsize length = 0;
+
+    /* Basic First Run */
+    whoopsie_report_recoverable_problem("test-signature", 0, TRUE, NULL);
+
+    g_file_get_contents("recoverable_mock.contents", &contents, &length, NULL);
+    if (length != sizeof(TEST_SIGNATURE) - 1) { /* Don't count trailing NULL */
+        g_error("Size of contents is %d instead of %d", (int)length, (int)sizeof(TEST_SIGNATURE) - 1);
+        return;
+    }
+    if (memcmp(contents, TEST_SIGNATURE, length) != 0) {
+        g_error("Contents is '%s' instead of '%s'", contents, TEST_SIGNATURE);
+        return;
+    }
+
+    g_free(contents);
+
+    /* Let's add some properties */
+    const gchar * properties[5] = {
+        "Foo", "Bar",
+        "LongDataList", "A\n B\n C",
+        NULL
+    };
+
+    whoopsie_report_recoverable_problem("test-signature", 0, TRUE, properties);
+
+    g_file_get_contents("recoverable_mock.contents", &contents, &length, NULL);
+    if (length != sizeof(TEST_DATALIST) - 1) { /* Don't count trailing NULL */
+        g_error("Size of contents is %d instead of %d", (int)length, (int)sizeof(TEST_DATALIST) - 1);
+        return;
+    }
+    if (memcmp(contents, TEST_DATALIST, length) != 0) {
+        g_error("Contents is '%s' instead of '%s'", contents, TEST_DATALIST);
+        return;
+    }
+
+    g_free(contents);
+    
+    /* Set a PID */
+    whoopsie_report_recoverable_problem("test-signature", 1234, TRUE, NULL);
+
+    g_file_get_contents("recoverable_mock.cmdline", &contents, &length, NULL);
+    if (g_strcmp0(contents, "-p 1234") != 0) {
+        g_error("Contents is '%s' instead of '%s'", contents, "-p 1234");
+        return;
+    }
+
+    g_free(contents);
+
+    /* Don't wait */
+    /* We can't reliably test to see if we can't wait, so this is
+       here to ensure mostly that we don't crash */
+    whoopsie_report_recoverable_problem("test-signature", 0, FALSE, NULL);
+}
+
+int
+main (int argc, char** argv)
+{
+#if GLIB_MAJOR_VERSION <= 2 && GLIB_MINOR_VERSION < 35
+    /* Deprecated in glib 2.35/2.36. */
+    g_type_init ();
+#endif
+    g_test_init (&argc, &argv, NULL);
+
+    g_setenv("WHOOPSIE_RECOVERABLE_PROBLEM_PATH", "./recoverable_mock.sh", TRUE);
+
+    g_test_add_func ("/whoopsie/recoverbale-error",
+                     test_recoverable);
+    return g_test_run ();
+}