Merge lp:~ev/whoopsie/libwhoopsie into lp:whoopsie

[Evan]

This branch factors out the identifier generation into a new library, libwhoopsie.

The motivation for this is to allow the preferences panel in gnome-control-center to have a button for showing crashes that have occurred on the system in the past. To do this it needs to construct a URL that has the system identifier used by whoopsie as a component.

The metrics database and the hardware database will also need to generate this exact same identifier (so we can query across all three data sets), and it remains to be determined whether these will use the whoopsie binary to shovel data into their respective backend services.

libwhoopsie provides the following functions:
void whoopsie_identifier_generate (char** res, GError** error);
void whoopsie_identifier_get_mac_address (char** res, GError** error);
void whoopsie_identifier_get_system_uuid (char** res, GError** error);
void whoopsie_identifier_sha512 (char* source, char* res, GError** error);
void whoopsie_hex_to_char (char* buf, const char *str, int len);

whoopsie_identifier_generate gets the system UUID, falling back to the MAC address, and SHA-512 hashes it.

Whoopsie is dynamically linked against libwhoopsie as part of this change. I also have a branch of activity-log-manager that depends on libwhoopsie and uses the whoopsie_identifier_generate function in the privileged DBus backend to construct the aforementioned URL (lp:~ev/activity-log-manager/show-previous-reports).

Questions:
- Is libwhoopsie an acceptable name, or do we want to further generalize this?
- Should we push whoopsie_identifier_get_*, whoopsie_identifier_sha512 and whoopsie_hex_to_char into a private header, to be used only for unit testing? Or is it acceptable to expose this functions as part of the public API?

I've also taken the opportunity to fix a few Lintian errors and have debhelper pass the more secure build options via dpkg-buildflags (which is does by default in dh 9). Sorry for the extra diff noise caused by this.

[James Hunt (jamesodhunt)]

Hi Evan,

Here are my review comments:

- shouldn't lib/bson/* be in a separate package? (bug 1006500)
- might be worth adding assert() calls for all function arguments?
- src/connectivity.c:setup_network_route_monitor():
   - no g_print() if g_network_monitor_get_default() fails.
- src/utils.c:already_handled_report():
  - should check uploaded_file before using it with stat() since if
    change_file_extension() fails, *either* call to free(uploaded_file) could cause a crash.
- src/whoopsie.c:
  - process_existing_files()
    - need to check return of g_dir_open() before using 'dir' in g_dir_read_name().
    - can g_build_filename() ever return NULL?
    - use of crash_file when it is known to be NULL (mark_handled(crash_file) and g_print(..., crash_file)).
    - crash_file is freed even if change_file_extension() failed.

[lp:~ev/whoopsie/libwhoopsie]

513. By Evan on 2012-05-31

Fixes from James Hunt's review: print an error message if g_network_monitor_get_default fails.

514. By Evan on 2012-05-31

Fixes from James Hunt's review: do not deference NULL pointers

515. By Evan on 2012-05-31

Fixes from James Hunt's review: future proof against g_build_filename returning NULL.

516. By Evan on 2012-05-31

Fixes from James Hunt's review: do not try to deference and print strings we know to be NULL.

[Evan (ev)]

Thanks for the thorough review!

On Wed, May 30, 2012 at 9:23 PM, James Hunt <email address hidden> wrote:
> - shouldn't lib/bson/* be in a separate package? (bug 1006500)

I'll take care of this in a separate merge, but thanks a bunch for
pointing it out.

> - might be worth adding assert() calls for all function arguments?

Fixed (r517).

> - src/connectivity.c:setup_network_route_monitor():
>   - no g_print() if g_network_monitor_get_default() fails.

Fixed (r513).

> - src/utils.c:already_handled_report():
>  - should check uploaded_file before using it with stat() since if
>    change_file_extension() fails, *either* call to free(uploaded_file) could cause a crash.

Fixed (r514).

> - src/whoopsie.c:
>  - process_existing_files()
>    - need to check return of g_dir_open() before using 'dir' in g_dir_read_name().

g_dir_read_name will return NULL if it receives a NULL argument.

>    - can g_build_filename() ever return NULL?

Fixed (r515).

>    - use of crash_file when it is known to be NULL (mark_handled(crash_file) and g_print(..., crash_file)).

Fixed (r516).

>    - crash_file is freed even if change_file_extension() failed.

Fixed by r516 as well.

[lp:~ev/whoopsie/libwhoopsie]

517. By Evan on 2012-05-31

Fixes from James Hunt's review: guard against invalid arguments to functions.

[Evan (ev)]

Comments from Ted:
[16:00:19] <ev> I don't suppose I could have your eyes on https://code.launchpad.net/~ev/whoopsie/libwhoopsie/+merge/107562 ?
[16:01:37] <ted> Meeting right now, but I can look after.
[16:01:44] <ev> thanks!
[16:43:04] <ted> Uhm, so what are your goals :-)
[16:43:18] <ted> There's a lot of things that glib libraries do like versioned API's
[16:43:33] <ted> That are handy, but chances are libwhoopsie won't be "that big"
[16:44:07] <ted> Also, you're not using autotools, eh?
[16:44:41] <ted> Makes some things a bit tricky.
[16:46:06] <ev> I *hate* autotools
[16:46:19] <ted> Heh
[16:46:20] <ev> I don't care about esoteric hardware or operating systems
[16:46:23] <ev> I want readable code
[16:46:30] <ted> Here's my libdbustest Makefile.am
[16:46:30] <ted> http://bazaar.launchpad.net/~indicator-applet-developers/dbus-test-runner/trunk.12.10/view/head:/libdbustest/Makefile.am
[16:47:09] <ev> yeah, it looks clean, but dragons. If I change things and then it starts complaining about a missing brace.
[16:47:19] <ted> So then I end up with this at my pc file
[16:47:19] <ted> http://bazaar.launchpad.net/~indicator-applet-developers/dbus-test-runner/trunk.12.10/view/head:/libdbustest/dbustest.pc.in
[16:47:59] <ted> It's all clean, as long as you only use the right characters in the right place. What's wrong with doing things right? ;-)
[16:48:23] <ev> heh
[16:48:32] <ted> So, I don't know if you want the API versioned include directory
[16:48:46] <ted> If you do, I'd recommend moving the files to their own directory in the source tree as well.
[16:49:06] <ev> could you put that in the MP? :)
[16:49:20] <ted> Sure, but I guess my question more was: is that your goal? :-)
[16:49:21] <ev> I like having a record of these sorts of things
[16:49:39] <ev> having a versioned include directory?
[16:49:46] <ev> hmm
[16:49:57] <ev> I guess I can't imagine us ever wanting to allow parallel installation
[16:49:58] <ted> Yeah: /usr/include/libwhoopsie-1.0/libwhoopsie/myfile.h
[16:50:20] <ev> which is presumably the use case for such things
[16:50:36] <ted> Yeah, it becomes a pain for folks like me that aren't coredev
[16:50:47] <ted> We get yelled at more for making things FTBFS :-)
[16:51:13] <ev> heh
[16:52:15] <ev> what do you mean by it being a pain for non-core dev? I don't see how not having parallel installation makes things FTBFS
[16:52:58] <ted> Because you have something like, let's say libmetrics depend on API version 1. So when version 2 gets uploaded, it breaks. Until libmetrics v2 gets uploaded.
[16:54:15] <ev> ahh

[Evan (ev)]

Comments from Colin and Steve:
[16:03:35] <slangasek> ev: why use c99 instead of $(CC)?
[16:03:58] <ev> slangasek: $(CC) with the c99 option?
[16:04:04] <slangasek> if you need to
[16:04:06] <cjwatson> Yeah, you should always use $(CC) for x-compiling support
[16:04:07] <ev> I do
[16:04:19] <slangasek> then yeah, especially now that there's a lib, $(CC) would be a good idea
[16:04:20] <ev> need to use c99 features, that is
[16:04:26] <cjwatson> (not that whoopsie wouldn't need more work for that)
[16:04:44] <slangasek> do you actually have to invoke gcc with -std=c99 to make use of c99 features?
[16:04:46] <cjwatson> no multiarch support?
[16:05:01] <slangasek> I thought you only have to do that to *disallow* code that's *not* c99-compliant
[16:05:02] <ev> ah yes - I hadn't considered that. I was more thinking to hell with people trying to run it under clang
[16:05:14] <cjwatson> we don't care about that for the rest of the distro ;-)
[16:05:19] <slangasek> right :)
[16:05:42] <ev> slangasek: if I run it without, it cries about variables defined in loop constructs
[16:05:43] <ev> if memory serves
[16:06:10] <slangasek> anyway, I'll try to send some more feedback on that merge request... would be good if others could as well
[16:06:15] <ev> thanks!
[16:06:18] <slangasek> it's just the addition of a shared library in C, it won't bite ;)
[16:06:21] <slangasek> anything else?
[16:06:53] <cjwatson> if you haven't already, you should nm -D the shared library to make sure only the symbols you absolutely need are exported
[16:07:21] <cjwatson> and in answer to your earlier question, you should link whoopsie dynamically against libwhoopsie rather than (effectively) shipping two copies
[16:07:53] <ev> cjwatson: ahhh, noted, thanks! dynamically link> thanks bunches for clarifying that. I couldn't find a good answer elsewhere.
[16:08:09] <cjwatson> there's basically hardly ever a good reason to link statically

[lp:~ev/whoopsie/libwhoopsie]

518. By Evan on 2012-06-18

Use $(CC) -std=c99 instead of c99.

=== modified file '.bzrignore'
--- .bzrignore	2012-05-15 14:24:43 +0000
+++ .bzrignore	2012-06-18 08:24:18 +0000
@@ -1,6 +1,11 @@
 src/whoopsie
+src/libwhoopsie.so*
+debian/tmp
 debian/whoopsie
+debian/libwhoopsie0
+debian/libwhoopsie-dev
 debian/*.debhelper*
+debian/*.substvars
 debian/files
 debian/whoopsie.substvars
 src/tests/test_parse_report

=== modified file 'Makefile'
--- Makefile	2012-05-15 14:24:43 +0000
+++ Makefile	2012-06-18 08:24:18 +0000
@@ -1,11 +1,23 @@
 VERSION=$(shell dpkg-parsechangelog | grep ^Version: | cut -d' ' -f2)
-CFLAGS=$(shell pkg-config --cflags gio-2.0 glib-2.0 libcurl) $(shell libgcrypt-config --cflags) -g -Ilib -Wall -Werror -Os -DVERSION=\"$(VERSION)\"
-LIBS=$(shell pkg-config --libs gio-2.0 glib-2.0 libcurl) $(shell libgcrypt-config --libs) -lcap
+
+libwhoopsie_SOURCES=src/identifier.c
+libwhoopsie_OBJECTS=$(libwhoopsie_SOURCES:.c=.o)
+libwhoopsie_CFLAGS=$(shell pkg-config --cflags glib-2.0) \
+				   $(shell libgcrypt-config --cflags) \
+				   $(CFLAGS) $(CPPFLAGS)
+libwhoopsie_LIBS=$(shell pkg-config --libs glib-2.0) \
+				 $(shell libgcrypt-config --libs) \
+				 $(LDFLAGS)
+
+whoopsie_CFLAGS=$(shell pkg-config --cflags gio-2.0 glib-2.0 libcurl) \
+	   -g -Ilib -Wall -Werror -Os -DVERSION=\"$(VERSION)\" \
+	   $(CFLAGS) $(CPPFLAGS)
+whoopsie_LIBS=$(shell pkg-config --libs gio-2.0 glib-2.0 libcurl) -lcap \
+			  $(LDFLAGS)
 SOURCES=src/whoopsie.c \
 		src/utils.c \
 		src/connectivity.c \
 		src/monitor.c \
-		src/identifier.c \
 		lib/bson/bson.c \
 		lib/bson/encoding.c \
 		lib/bson/numbers.c
@@ -18,10 +30,18 @@
 
 all: $(SOURCES) $(EXECUTABLE)
 
-$(EXECUTABLE): $(OBJECTS)
-	c99 $(OBJECTS) $(LIBS) -o $@
+$(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
+	ln -sf libwhoopsie.so.0.0 src/libwhoopsie.so
+
+$(EXECUTABLE): $(OBJECTS) src/libwhoopsie.so.0.0
+	$(CC) -std=c99 $(OBJECTS) $(whoopsie_LIBS) -Lsrc -lwhoopsie -o $@
 clean:
-	rm -f $(EXECUTABLE) $(OBJECTS)
+	rm -f $(EXECUTABLE) $(OBJECTS) $(libwhoopsie_OBJECTS) src/libwhoopsie.so*
 	$(MAKE) -C src/tests clean
 check:
 	$(MAKE) -C src/tests
@@ -32,7 +52,7 @@
 coverage:
 	$(MAKE) -C src/tests coverage
 %.o: %.c
-	c99 -c $(CFLAGS) -o $@ $^
+	$(CC) -std=c99 -c $(whoopsie_CFLAGS) -o $@ $^
 
 install: all
 	install -d $(BIN)
@@ -43,3 +63,11 @@
 	install -m644 data/whoopsie $(DATA)/default
 	install -d $(DESTDIR)/usr/share/apport/package-hooks
 	install -m644 data/whoopsie.py $(DESTDIR)/usr/share/apport/package-hooks
+	install -d $(DESTDIR)/usr/lib
+	install -m644 src/libwhoopsie.so.0.0 $(DESTDIR)/usr/lib
+	cp -d src/libwhoopsie.so $(DESTDIR)/usr/lib
+	cp -d src/libwhoopsie.so.0 $(DESTDIR)/usr/lib
+	install -d $(DESTDIR)/usr/lib/pkgconfig
+	install -m644 lib/libwhoopsie.pc $(DESTDIR)/usr/lib/pkgconfig
+	install -d $(DESTDIR)/usr/include/libwhoopsie
+	install -m644 src/identifier.h $(DESTDIR)/usr/include/libwhoopsie

=== modified file 'debian/compat'
--- debian/compat	2011-12-01 17:04:38 +0000
+++ debian/compat	2012-06-18 08:24:18 +0000
@@ -1,1 +1,1 @@
-8
+9

=== modified file 'debian/control'
--- debian/control	2012-05-09 23:04:30 +0000
+++ debian/control	2012-06-18 08:24:18 +0000
@@ -2,7 +2,7 @@
 Section: utils
 Priority: optional
 Maintainer: Evan Dandrea <[e-mail]>
-Build-Depends: debhelper (>= 8.0.0),
+Build-Depends: debhelper (>= 9.0),
  libglib2.0-dev (>= 2.31.6),
  libcurl4-openssl-dev,
  libgcrypt11-dev,
@@ -10,15 +10,33 @@
  libgtk-3-dev,
  network-manager-dev (>= 0.9.4.0-0ubuntu1),
  python,
- pyflakes,
- dpkg-dev
-Standards-Version: 3.9.2
+ pyflakes
+Standards-Version: 3.9.3
 Homepage: http://wiki.ubuntu.com/ErrorTracker
 Vcs-Bzr: http://code.launchpad.net/whoopsie
 
 Package: whoopsie
 Architecture: any
-Depends: ${shlibs:Depends}, ${misc:Depends}, adduser
+Depends: ${shlibs:Depends},
+ ${misc:Depends},
+ adduser,
+ libwhoopsie0 (= ${binary:Version})
 Pre-Depends: ${misc:Pre-Depends}
-Description: Ubuntu crash database submission daemon
+Description: Ubuntu error tracker submission
  This program submits crash reports back to an Ubuntu server.
+
+Package: libwhoopsie0
+Section: libs
+Architecture: any
+Depends: ${shlibs:Depends}, ${misc:Depends}
+Description: Ubuntu error tracker submission - shared library
+ This library provides methods to generate an identifier for use with the
+ Ubuntu error tracker
+
+Package: libwhoopsie-dev
+Section: libdevel
+Architecture: any
+Depends: libwhoopsie0 (= ${binary:Version}), ${misc:Depends}
+Description: Ubuntu error tracker submission - library development files
+ This library provides methods to generate an identifier for use with the
+ Ubuntu error tracker

=== modified file 'debian/copyright'
--- debian/copyright	2012-06-14 19:52:38 +0000
+++ debian/copyright	2012-06-18 08:24:18 +0000
@@ -1,6 +1,6 @@
 Format: http://dep.debian.net/deps/dep5
-Upstream-Name: whoopsie-daisy
-Source: http://code.launchpad.net/whoopsie-daisy
+Upstream-Name: whoopsie
+Source: http://code.launchpad.net/whoopsie
 
 Files: src/*
 Copyright: 2011-2012 Canonical Ltd.
@@ -20,3 +20,21 @@
  .
  On Debian systems, the complete text of the GNU General
  Public License version 3 can be found in "/usr/share/common-licenses/GPL-3".
+<<<<<<< TREE
+=======
+
+Files: lib/bson/*
+Copyright: 2009, 2010 10gen Inc.
+License: Apache-2.0
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+ .
+ http://www.apache.org/licenses/LICENSE-2.0
+ .
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+>>>>>>> MERGE-SOURCE

=== added file 'debian/libwhoopsie-dev.install'
--- debian/libwhoopsie-dev.install	1970-01-01 00:00:00 +0000
+++ debian/libwhoopsie-dev.install	2012-06-18 08:24:18 +0000
@@ -0,0 +1,3 @@
+usr/include
+usr/lib/pkgconfig
+usr/lib/libwhoopsie.so

=== added file 'debian/libwhoopsie0.install'
--- debian/libwhoopsie0.install	1970-01-01 00:00:00 +0000
+++ debian/libwhoopsie0.install	2012-06-18 08:24:18 +0000
@@ -0,0 +1,1 @@
+usr/lib/*.so.*

=== added file 'debian/whoopsie.install'
--- debian/whoopsie.install	1970-01-01 00:00:00 +0000
+++ debian/whoopsie.install	2012-06-18 08:24:18 +0000
@@ -0,0 +1,3 @@
+etc
+usr/bin
+usr/share

=== modified file 'debian/whoopsie.maintscript' (properties changed: +x to -x)
=== renamed file 'debian/postinst' => 'debian/whoopsie.postinst'
=== renamed file 'debian/postrm' => 'debian/whoopsie.postrm'
=== added file 'lib/libwhoopsie.pc'
--- lib/libwhoopsie.pc	1970-01-01 00:00:00 +0000
+++ lib/libwhoopsie.pc	2012-06-18 08:24:18 +0000
@@ -0,0 +1,13 @@
+prefix=/usr
+exec_prefix=${prefix}
+libdir=${exec_prefix}/lib
+bindir=${exec_prefix}/bin
+includedir=${prefix}/include
+
+Requires: glib-2.0
+Libs: -lwhoopsie -lgcrypt
+
+Name: libwhoopsie
+Description: libwhoopsie.
+Version: 0.0.0
+

=== modified file 'src/connectivity.c'
--- src/connectivity.c	2012-03-30 08:59:52 +0000
+++ src/connectivity.c	2012-06-18 08:24:18 +0000
@@ -51,6 +51,9 @@
     guint value = 0;
     GError* err = NULL;
 
+    g_return_val_if_fail (connection, FALSE);
+    g_return_val_if_fail (path, FALSE);
+
     result = g_dbus_connection_call_sync (connection,
                                  NETWORK_MANAGER_SERVICE,
                                  path,
@@ -86,6 +89,10 @@
     GVariant* result = NULL;
     GVariant* properties = NULL;
     guint device_type;
+
+    g_return_val_if_fail (connection, FALSE);
+    g_return_val_if_fail (path, FALSE);
+
     result = g_dbus_connection_call_sync (connection,
                                   NETWORK_MANAGER_SERVICE,
                                   path,
@@ -123,6 +130,10 @@
     GVariant* properties = NULL;
     GVariantIter* iter = NULL;
     gchar* device_path;
+
+    g_return_val_if_fail (connection, FALSE);
+    g_return_val_if_fail (path, FALSE);
+
     result = g_dbus_connection_call_sync (connection,
                                           NETWORK_MANAGER_SERVICE,
                                           path,
@@ -171,8 +182,8 @@
     gboolean paid = TRUE;
     ConnectionAvailableCallback callback = user_data;
 
-    if (!parameters)
-        return;
+    g_return_if_fail (connection);
+    g_return_if_fail (parameters);
     
     g_variant_get_child (parameters, 0, "u", &connected_state);
     if (NM_STATE_CONNECTED_GLOBAL != connected_state) {
@@ -225,6 +236,8 @@
     GError* err = NULL;
     GSocketConnectable *addr = NULL;
 
+    g_return_if_fail (nm);
+
     addr = g_network_address_parse_uri (connectivity_data.url, 80, &err);
     if (!addr) {
         g_print ("Could not parse crash database URL: %s\n", err->message);
@@ -261,8 +274,10 @@
     }
 
     nm = g_network_monitor_get_default ();
-    if (!nm)
+    if (!nm) {
+        g_print ("Could not get the network monitor.\n");
         return;
+    }
 
     route_available = (g_network_monitor_get_network_available (nm) &&
                        g_network_monitor_can_reach (nm, addr, NULL, NULL));
@@ -281,6 +296,8 @@
     GVariant* current_state = NULL;
     guint value = 0;
     
+    g_return_val_if_fail (crash_url, FALSE);
+
     connectivity_data.url = crash_url;
     connectivity_data.callback = callback;
     /* Checking whether a NetworkManager connection is not enough.

=== modified file 'src/identifier.c'
--- src/identifier.c	2012-05-15 16:45:45 +0000
+++ src/identifier.c	2012-06-18 08:24:18 +0000
@@ -12,11 +12,14 @@
 #include "identifier.h"
 
 void
-hex_to_char (char* buf, const char *str, int len)
+whoopsie_hex_to_char (char* buf, const char *str, int len)
 {
     char* p = NULL;
     int i = 0;
 
+    g_return_if_fail (buf);
+    g_return_if_fail (str);
+
     p = buf;
     for (i = 0; i < len; i++) {
         snprintf(p, 3, "%02x", (unsigned char) str[i]);
@@ -26,7 +29,7 @@
 }
 
 void
-identifier_get_mac_address (char** res, GError** error)
+whoopsie_identifier_get_mac_address (char** res, GError** error)
 {
     struct ifreq ifr;
     struct ifconf ifc;
@@ -36,6 +39,8 @@
     struct ifreq* it;
     int i = 0;
 
+    g_return_if_fail (res);
+
     sock = socket(AF_INET, SOCK_DGRAM, IPPROTO_IP);
     if (sock == -1) {
         g_set_error (error, g_quark_from_static_string ("whoopsie-quark"), 0,
@@ -79,10 +84,12 @@
 }
 
 void
-identifier_get_system_uuid (char** res, GError** error)
+whoopsie_identifier_get_system_uuid (char** res, GError** error)
 {
     int fp;
 
+    g_return_if_fail (res);
+
     fp = open ("/sys/class/dmi/id/product_uuid", O_RDONLY);
     if (fp < 0) {
         g_set_error (error, g_quark_from_static_string ("whoopsie-quark"), 0,
@@ -99,12 +106,15 @@
 }
 
 void
-identifier_sha512 (char* source, char* res, GError** error)
+whoopsie_identifier_sha512 (char* source, char* res, GError** error)
 {
     int md_len;
     gcry_md_hd_t sha512 = NULL;
     unsigned char* id = NULL;
 
+    g_return_if_fail (source);
+    g_return_if_fail (res);
+
     if (!gcry_check_version (GCRYPT_VERSION)) {
         g_set_error (error, g_quark_from_static_string ("whoopsie-quark"), 0,
                      "libcrypt version mismatch.");
@@ -131,6 +141,35 @@
         gcry_md_close (sha512);
         return;
     }
-    hex_to_char (res, (const char*)id, md_len);
+    whoopsie_hex_to_char (res, (const char*)id, md_len);
     gcry_md_close (sha512);
 }
+
+void
+whoopsie_identifier_generate (char** res, GError** error)
+{
+    char* identifier = NULL;
+
+    g_return_if_fail (res);
+
+    whoopsie_identifier_get_system_uuid (&identifier, error);
+    if ((!error || !(*error)) && identifier)
+        goto out;
+
+    if (error && *error) {
+        g_error_free (*error);
+        *error = NULL;
+    }
+
+    whoopsie_identifier_get_mac_address (&identifier, error);
+    if ((!error || !(*error)) && identifier)
+        goto out;
+
+    return;
+
+out:
+    *res = malloc (HASHLEN + 1);
+    whoopsie_identifier_sha512 (identifier, *res, error);
+    free (identifier);
+}
+

=== modified file 'src/identifier.h'
--- src/identifier.h	2012-05-15 16:45:45 +0000
+++ src/identifier.h	2012-06-18 08:24:18 +0000
@@ -1,5 +1,6 @@
-void identifier_get_mac_address (char** res, GError** error);
-void identifier_get_system_uuid (char** res, GError** error);
-void identifier_sha512 (char* source, char* res, GError** error);
-void hex_to_char (char* buf, const char *str, int len);
+void whoopsie_identifier_generate (char** res, GError** error);
+void whoopsie_identifier_get_mac_address (char** res, GError** error);
+void whoopsie_identifier_get_system_uuid (char** res, GError** error);
+void whoopsie_identifier_sha512 (char* source, char* res, GError** error);
+void whoopsie_hex_to_char (char* buf, const char *str, int len);
 #define HASHLEN 128

=== modified file 'src/monitor.c'
--- src/monitor.c	2012-03-29 15:02:13 +0000
+++ src/monitor.c	2012-06-18 08:24:18 +0000
@@ -35,6 +35,8 @@
     char* upload_file = NULL;
     char* crash_file = NULL;
 
+    g_return_if_fail (upload);
+
     crash_file = change_file_extension (upload, ".crash");
     if (!crash_file) {
         g_print ("Unable to parse the upload file path.\n");
@@ -90,6 +92,8 @@
     GError* err = NULL;
     GFile* path = NULL;
 
+    g_return_if_fail (directory);
+
     mkdir (directory, 0755);
     path = g_file_new_for_path (directory);
     monitor = g_file_monitor_directory (path, G_FILE_MONITOR_NONE, NULL, &err);

=== modified file 'src/tests/Makefile'
--- src/tests/Makefile	2012-05-15 14:24:43 +0000
+++ src/tests/Makefile	2012-06-18 08:24:18 +0000
@@ -39,22 +39,22 @@
 	$(foreach p, $^, ./$p -k;)
 
 $(test_parse_report_EXECUTABLE): $(test_parse_report_OBJECTS)
-	c99 $^ $(LIBS) -o $@
+	$(CC) -std=c99 $^ $(LIBS) -o $@
 $(test_utils_EXECUTABLE): $(test_utils_OBJECTS)
-	c99 $^ $(LIBS) -o $@
+	$(CC) -std=c99 $^ $(LIBS) -o $@
 $(test_monitor_EXECUTABLE): $(test_monitor_OBJECTS)
-	c99 $^ $(LIBS) -o $@
+	$(CC) -std=c99 $^ $(LIBS) -o $@
 $(test_identifier_EXECUTABLE): $(test_identifier_OBJECTS)
-	c99 $^ $(LIBS) -o $@
+	$(CC) -std=c99 $^ $(LIBS) -o $@
 
 test_parse_report_coverage: $(test_parse_report_OBJECTS:.test.o=.coverage.o)
-	c99 $^ $(LIBS) -lgcov -o $@
+	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
 test_utils_coverage: $(test_utils_OBJECTS:.test.o=.coverage.o)
-	c99 $^ $(LIBS) -lgcov -o $@
+	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
 test_monitor_coverage: $(test_monitor_OBJECTS:.test.o=.coverage.o)
-	c99 $^ $(LIBS) -lgcov -o $@
+	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
 test_identifier_coverage: $(test_identifier_OBJECTS:.test.o=.coverage.o)
-	c99 $^ $(LIBS) -lgcov -o $@
+	$(CC) -std=c99 $^ $(LIBS) -lgcov -o $@
 coverage: test_parse_report_coverage test_utils_coverage test_monitor_coverage test_identifier_coverage
 	$(foreach p, $^, ./$p -k;)
 	$(foreach p, $(wildcard ../*.c), gcov $p -o $(p:.c=.coverage.o);)
@@ -78,6 +78,6 @@
 		-name '*.gcov' \) -delete
 
 %.coverage.o: %.c
-	c99 -c -o $@ $^ $(CFLAGS) --coverage -O0
+	$(CC) -std=c99 -c -o $@ $^ $(CFLAGS) --coverage -O0
 %.test.o: %.c
-	c99 -c -o $@ $^ $(CFLAGS)
+	$(CC) -std=c99 -c -o $@ $^ $(CFLAGS)

=== modified file 'src/tests/test_identifier.c'
--- src/tests/test_identifier.c	2012-05-15 16:45:45 +0000
+++ src/tests/test_identifier.c	2012-06-18 08:24:18 +0000
@@ -30,7 +30,7 @@
 test_hex_to_char (void)
 {
     char buf[9];
-    hex_to_char (buf, "\xFF\xFF\xFF\xFF", 4);
+    whoopsie_hex_to_char (buf, "\xFF\xFF\xFF\xFF", 4);
     g_assert_cmpstr (buf, ==, "ffffffff");
 
 }
@@ -43,7 +43,7 @@
     int fp;
     char buf[18];
 
-    identifier_get_mac_address (&res, &error);
+    whoopsie_identifier_get_mac_address (&res, &error);
     g_assert (res != NULL);
     fp = open ("/sys/class/net/eth0/address", O_RDONLY);
     if (fp < 0) {
@@ -70,7 +70,7 @@
 {
     /* DEADBEEF-1234-1234-1234-DEADBEEF1234 */
     char* res = NULL;
-    identifier_get_system_uuid (&res, NULL);
+    whoopsie_identifier_get_system_uuid (&res, NULL);
     if (getuid () != 0) {
         g_print ("Need root to run this complete test: ");
         return;
@@ -100,11 +100,27 @@
     const char* expected = "f7fbba6e0636f890e56fbbf3283e524c6fa3204ae298382"
                            "d624741d0dc6638326e282c41be5e4254d8820772c5518a"
                            "2c5a8c0c7f7eda19594a7eb539453e1ed7";
-    identifier_sha512 ("foo", res, NULL);
-    //if (strcmp (res, expected) != 0)
-    //    g_test_fail ();
-}
-
+    whoopsie_identifier_sha512 ("foo", res, NULL);
+    if (strcmp (res, expected) != 0)
+        g_test_fail ();
+}
+
+static void
+test_identifier_generate (void)
+{
+    char* result = NULL;
+    char* expected = NULL;
+    char hashed[HASHLEN + 1];
+
+    whoopsie_identifier_generate (&result, NULL);
+    if (getuid () != 0)
+        whoopsie_identifier_get_mac_address (&expected, NULL);
+    else
+        whoopsie_identifier_get_system_uuid (&expected, NULL);
+    whoopsie_identifier_sha512 (expected, hashed, NULL);
+    if (strcmp (result, hashed) != 0)
+        g_test_fail ();
+}
 int
 main (int argc, char** argv)
 {
@@ -117,6 +133,7 @@
     g_test_add_func ("/whoopsie/hex-to-char", test_hex_to_char);
     g_test_add_func ("/whoopsie/get-mac-address", test_get_mac_address);
     g_test_add_func ("/whoopsie/sha512", test_sha512);
+    g_test_add_func ("/whoopsie/identifier-generate", test_identifier_generate);
 
     /* Do not consider warnings to be fatal. */
     g_log_set_always_fatal (G_LOG_FATAL_MASK);

=== modified file 'src/utils.c'
--- src/utils.c	2012-03-29 21:19:59 +0000
+++ src/utils.c	2012-06-18 08:24:18 +0000
@@ -29,6 +29,8 @@
 void
 init_response_string (struct response_string* resp)
 {
+    g_return_if_fail (resp);
+
     resp->length = 0;
     resp->p = g_malloc (1);
     resp->p[0] = '\0';
@@ -37,7 +39,12 @@
 void
 grow_response_string (struct response_string* resp, char* str, size_t length)
 {
-    size_t new_length = resp->length + length;
+    size_t new_length;
+
+    g_return_if_fail (resp);
+    g_return_if_fail (str);
+
+    new_length = resp->length + length;
     resp->p = g_realloc (resp->p, new_length + 1);
     memcpy (resp->p + resp->length, str, length);
     resp->p[new_length] = '\0';
@@ -96,6 +103,8 @@
     struct stat upload_stat;
     struct stat crash_stat;
 
+    g_return_val_if_fail (crash_file, FALSE);
+
     if (stat (crash_file, &crash_stat) < 0)
         /* .crash doesn't exist; don't process. */
         return TRUE;
@@ -112,6 +121,10 @@
     free (upload_file);
     
     uploaded_file = change_file_extension (crash_file, ".uploaded");
+    if (!uploaded_file)
+        /* Bogus crash file; don't process. */
+        return TRUE;
+
     if (stat (uploaded_file, &uploaded_stat) < 0) {
         /* .uploaded doesn't exist. */
         free (uploaded_file);
@@ -131,6 +144,8 @@
     char* uploaded_file = NULL;
     int fd;
 
+    g_return_val_if_fail (crash_file, FALSE);
+
     uploaded_file = change_file_extension (crash_file, ".uploaded");
     if (!uploaded_file)
         return FALSE;

=== modified file 'src/whoopsie.c'
--- src/whoopsie.c	2012-05-15 16:45:45 +0000
+++ src/whoopsie.c	2012-06-18 08:24:18 +0000
@@ -60,8 +60,10 @@
 /* The URL of the crash database. */
 static char* crash_db_url = NULL;
 
-/*  The system UUID, taken from the DMI tables and SHA-512 hashed */
-static char sha512_system_uuid[HASHLEN + 1] = {0};
+/*  The database identifier. Either:
+ *  - The system UUID, taken from the DMI tables and SHA-512 hashed
+ *  - The MAC address of the first non-loopback device, SHA-512 hashed */
+static char* whoopsie_identifier = NULL;
 
 /* The URL for sending the initial crash report */
 static char* crash_db_submit_url = NULL;
@@ -141,7 +143,11 @@
 static gboolean
 is_acceptable_field (const char* field)
 {
-    const char** p = acceptable_fields;
+    const char** p;
+
+    g_return_val_if_fail (field, FALSE);
+
+    p = acceptable_fields;
     while (*p) {
         if (strcmp (*p, field) == 0)
             return TRUE;
@@ -195,6 +201,9 @@
 void
 split_string (char* head, char** tail)
 {
+    g_return_if_fail (head);
+    g_return_if_fail (tail);
+
     *tail = strchr (head, ' ');
     if (*tail) {
         **tail = '\0';
@@ -216,7 +225,7 @@
     *bson_message = NULL;
     *bson_message_len = 0;
 
-    assert (report != NULL);
+    g_return_val_if_fail (report, FALSE);
 
     bson_init (b);
     if (!bson_data (b))
@@ -246,6 +255,8 @@
     long response_code = 0;
     struct curl_slist* list = NULL;
 
+    g_return_val_if_fail (message_data, -1);
+
     if ((curl = curl_easy_init ()) == NULL) {
         printf ("Couldn't init curl.\n");
         return FALSE;
@@ -312,6 +323,8 @@
     int value_pos;
     gboolean ignore_field = FALSE;
 
+    g_return_val_if_fail (report_path, NULL);
+
     if (g_file_test (report_path, G_FILE_TEST_IS_SYMLINK) ||
         !g_file_test (report_path, G_FILE_TEST_IS_REGULAR)) {
         g_set_error (error, g_quark_from_static_string ("whoopsie-quark"), 0,
@@ -479,8 +492,13 @@
     char* crash_db_core_url = NULL;
     struct response_string s;
 
+
+    g_return_val_if_fail (uuid, FALSE);
+    g_return_val_if_fail (arch, FALSE);
+    g_return_val_if_fail (core_data, FALSE);
+
     if (asprintf (&crash_db_core_url, "%s/%s/submit-core/%s/%s",
-        crash_db_url, uuid, arch, sha512_system_uuid) < 0)
+        crash_db_url, uuid, arch, whoopsie_identifier) < 0)
         g_error ("Unable to allocate memory.");
 
     /* TODO use CURLOPT_READFUNCTION to transparently compress data with
@@ -523,6 +541,8 @@
     char* core = NULL;
     char* arch = NULL;
 
+    g_return_if_fail (report);
+
     /* Command could be CORE, which requests the core dump, BUG ######, if in a
      * development release, which points to the bug report, or UPDATE, if this
      * is fixed in an update. */
@@ -618,6 +638,9 @@
     while ((file = g_dir_read_name (dir)) != NULL) {
 
         upload_file = g_build_filename ("/var/crash", file, NULL);
+        if (!upload_file)
+            continue;
+
         ext = strrchr (upload_file, '.');
         if (ext && strcmp(++ext, "upload") != 0) {
             free (upload_file);
@@ -626,10 +649,11 @@
 
         crash_file = change_file_extension (upload_file, ".crash");
         if (!crash_file) {
-            /* FIXME this would be bad - we'd keep uploading it */
-            if (!mark_handled (crash_file))
-                g_print ("Unable to mark report as seen (%s)\n", crash_file);
-        } else if (already_handled_report (crash_file)) {
+            free (upload_file);
+            continue;
+        }
+
+        if (already_handled_report (crash_file)) {
             if (!mark_handled (crash_file))
                 g_print ("Unable to mark report as seen (%s)\n", crash_file);
         } else if (online_state && parse_and_upload_report (crash_file)) {
@@ -813,8 +837,6 @@
 main (int argc, char** argv)
 {
     GError* err = NULL;
-    char* system_uuid = NULL;
-    char* mac_address = NULL;
 
     setup_signals ();
     parse_arguments (&argc, &argv);
@@ -824,34 +846,16 @@
         exit (EXIT_FAILURE);
     }
 
-    identifier_get_system_uuid (&system_uuid, &err);
+    whoopsie_identifier_generate (&whoopsie_identifier, &err);
     if (err) {
         g_print ("%s\n", err->message);
         g_error_free (err);
         err = NULL;
-    }
-    if (system_uuid) {
-        identifier_sha512 (system_uuid, sha512_system_uuid, NULL);
-        free (system_uuid);
-    } else {
-        identifier_get_mac_address (&mac_address, &err);
-        if (err) {
-            g_print ("%s\n", err->message);
-            g_error_free (err);
-            err = NULL;
-        }
-        if (mac_address) {
-            identifier_sha512 (mac_address, sha512_system_uuid, NULL);
-            free (mac_address);
-        }
-    }
-
-    if (*sha512_system_uuid != '\0') {
-        if (asprintf (&crash_db_submit_url, "%s/%s",
-            crash_db_url, sha512_system_uuid) < 0)
-            g_error ("Unable to allocate memory.");
-    } else {
         crash_db_submit_url = strdup (crash_db_url);
+
+    } else if (asprintf (&crash_db_submit_url, "%s/%s", crash_db_url,
+               whoopsie_identifier) < 0) {
+        g_error ("Unable to allocate memory.");
     }
 
     drop_privileges (&err);