Re: [gpsd-dev] Policy on assert(3) use in GPSD

[Gary E. Miller]

Yo Eric!

On Fri, 30 Jan 2015 09:50:42 -0500 (EST)
address@hidden (Eric S. Raymond) wrote:

> I think the asserts Gary added to gpsd_acquire_reporting_lock() and
> gpsd_release_reporting_lock() were mistaken, and not just because he
> got the test backwards.  That slip could happen to anybody, was easily
> caught, and doesn't bother me at all. The real problem is more subtle.

Let me quote the man page for you:

       The mutex object referenced by mutex shall be locked by a call to
       pthread_mutex_lock() that returns zero or [EOWNERDEAD].   If  the
       mutex  is  already  locked  by another thread, the calling thread
       shall block until the mutex  becomes  available.

Thus a non-zero return is a FATAL error.  Something horrible hass gone
wrong and there is no chance of recovery.

> 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.

If you can figure a way to recover from a deadlock then let's add it.

But if we are seeing a deadlock then game over anyway.

> 1. Use asserts sparingly.

Yes, assert()s should only be on things that nevver happen.  

> 2. 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.

No, removing debug code is always bad.  Things fail in production that
do not fail in test.  Without the debug code there is no way to know.
 
> 3. 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.

Yes, that is what I was doing.
 
> 4. Outside the above two cases, do not assert when you can log an
> internal error and recover.

Since no recovery possible, this rule does not apply to my assert()s.
 
> 5. Never use assert() to check resource exhaustion conditions or
> other dynamic errors. (This was Gary's mistake.) 

Gotta start somewhere.  If the locking was written correctly the
assert() should never have been hit.  Which to my mind is the only
proper time to use an assert().

> I will add a version of this policy to the Hacker's Guide.

With correction, of course.

RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
        address@hidden  Tel:+1(541)382-8588