Assertions cleanup and new policy. All regression tests pass.

4 files changed, 48 insertions, 11 deletions

diff --git a/gpsd.c b/gpsd.c
index 40cf803a..21b8855b 100644
--- a/gpsd.c
+++ b/gpsd.c
@@ -1452,7 +1452,11 @@ static void all_reports(struct gps_device_t *device, gps_mask_t changed)
 
     /* handle laggy response to a firmware version query */
     if ((changed & (DEVICEID_SET | DRIVER_IS)) != 0) {
-	assert(device->device_type != NULL);
+	if (device->device_type == NULL)
+	    gpsd_report(&context.errout, LOG_ERROR,
+			"internal error - device type of %s not set when expected\n",
+			device->gpsdata.dev.path);
+	else
 	{
 	    char id2[GPS_JSON_RESPONSE_MAX];
 	    json_device_dump(device, id2, sizeof(id2));
diff --git a/gpsd_json.c b/gpsd_json.c
index ee4fb708..d53e6ce3 100644
--- a/gpsd_json.c
+++ b/gpsd_json.c
@@ -131,7 +131,7 @@ void json_tpv_dump(const struct gps_device_t *session,
     timestamp_t rtime = timestamp();
 #endif /* TIMING_ENABLE */
 
-    assert(replylen > 2);
+    assert(replylen > sizeof(char *));
     (void)strlcpy(reply, "{\"class\":\"TPV\",", replylen);
     if (gpsdata->dev.path[0] != '\0')
 	str_appendf(reply, replylen, "\"device\":\"%s\",", gpsdata->dev.path);
@@ -213,7 +213,7 @@ void json_noise_dump(const struct gps_data_t *gpsdata,
 {
     char tbuf[JSON_DATE_MAX+1];
 
-    assert(replylen > 2);
+    assert(replylen > sizeof(char *));
     (void)strlcpy(reply, "{\"class\":\"GST\",", replylen);
     if (gpsdata->dev.path[0] != '\0')
 	str_appendf(reply, replylen, "\"device\":\"%s\",", gpsdata->dev.path);
@@ -244,7 +244,7 @@ void json_sky_dump(const struct gps_data_t *datap,
 {
     int i, reported = 0;
 
-    assert(replylen > 2);
+    assert(replylen > sizeof(char *));
     (void)strlcpy(reply, "{\"class\":\"SKY\",", replylen);
     if (datap->dev.path[0] != '\0')
 	str_appendf(reply, replylen, "\"device\":\"%s\",", datap->dev.path);
@@ -3294,7 +3294,7 @@ void json_att_dump(const struct gps_data_t *gpsdata,
 		   /*@out@*/ char *reply, size_t replylen)
 /* dump the contents of an attitude_t structure as JSON */
 {
-    assert(replylen > 2);
+    assert(replylen > sizeof(char *));
     (void)strlcpy(reply, "{\"class\":\"ATT\",", replylen);
     str_appendf(reply, replylen, "\"device\":\"%s\",", gpsdata->dev.path);
     if (isnan(gpsdata->attitude.heading) == 0) {
diff --git a/libgpsd_core.c b/libgpsd_core.c
index f0191534..bb3ea56c 100644
--- a/libgpsd_core.c
+++ b/libgpsd_core.c
@@ -62,20 +62,16 @@ static pthread_mutex_t report_mutex;
 
 void gpsd_acquire_reporting_lock(void)
 {
-    int err;
     /*@ -unrecog  (splint has no pthread declarations as yet) @*/
-    err = pthread_mutex_lock(&report_mutex);
+    pthread_mutex_lock(&report_mutex);
     /*@ +unrecog @*/
-    assert( 0 == err);
 }
 
 void gpsd_release_reporting_lock(void)
 {
-    int err;
     /*@ -unrecog (splint has no pthread declarations as yet) @*/
-    err = pthread_mutex_unlock(&report_mutex);
+    pthread_mutex_unlock(&report_mutex);
     /*@ +unrecog @*/
-    assert( 0 == err);
 }
 #endif /* PPS_ENABLE */
 
diff --git a/www/hacking.html.in b/www/hacking.html.in
index 37b13b5e..7af663aa 100644
--- a/www/hacking.html.in
+++ b/www/hacking.html.in
@@ -461,6 +461,43 @@ not all integer types have the same length across architectures. A long may
 be 4 bytes on a 32-bit machine and 8 bytes on a 64-bit. If you mean to skip
 4 bytes in a packet, then say so (or use sizeof(int32_t)).</p>
 
+<h2 id="malloc">Use assert(3) sparingly and carefully!</h2>
+
+<p>gpsd - I'm speaking of the daemon itself, not the clients and test
+tools - runs in a lot of contexts where it provides a life-critical
+service. We can't allow it to faint every time it gets a case of the
+vapors.  Instead, the right thing is to log a problem and soldier on.
+If the fault condition is in the logging machinery itself, the right
+thing is to just soldier on.</p>
+
+<p>Accordingly, there are very few asserts in the core code. Whenever
+possible we should avoid adding more.</p>
+
+<p>Here's the policy for the daemon and its service libraries.  This
+doesn't apply to client code and tools, which can be more relaxed
+because the cost of having them go tits-up is lower and recovery
+is easier.<p>
+
+<ol>
+<li><p>Use asserts sparingly.</p></li>
+
+<li><p>One valid use is to pass hints to splint and other analyzers. This
+sort of assert should be guarded with #ifdef S_SPLINT_S or #ifdef
+__COVERITY_ so we get the benefit of asserting the invariant without
+the risk of the assertion failing in production.</p></li>
+
+<li><p>Another use is to document should-never-happen invariants of the
+code.  Write this sort only if you are confident it will never fire
+in a production release; it's there to catch when changes in a
+*development* revision break an invariant.</p></li>
+
+<li><p>Outside the above two cases, do not assert when you can log an internal
+error and recover.</p></li>
+
+<li><p>Never use assert() to check resource exhaustion conditions or
+other dynamic errors.</p></li>
+</ol>
+
 <h2 id="nohash">Avoid git hashes</h2>
 
 <p>Style point: avoid using git hashes in change comments. They'll