Make non-fatal errors return properly

Rakefile

@@ -116,6 +116,13 @@ file check("acl") =>
   gcc_link t.name, t.prerequisites + [LIBCHECK]
 end
 
+file check( "util" ) =>
+%w{build/tests/check_util.o
+  build/util.o
+  build/self_pipe.o} do |t|
+  gcc_link t.name, t.prerequisites + [LIBCHECK]
+end
+
 file check("serve") =>
 %w{build/tests/check_serve.o
   build/self_pipe.o
@@ -163,11 +170,13 @@ file check("listen") =>
 end
 
 
-(TEST_MODULES- %w{acl client serve readwrite listen}).each do |m|
+(TEST_MODULES- %w{acl client serve readwrite listen util}).each do |m|
   tgt = "build/tests/check_#{m}.o"
   maybe_obj_name = "build/#{m}.o"
+  # Take it out in case we're testing util.o or ioutil.o
   deps = ["build/ioutil.o", "build/util.o"] - [maybe_obj_name]
 
+  # Add it back in if it's something we need to compile
   deps << maybe_obj_name if OBJECTS.include?( maybe_obj_name )
 
   file check( m ) => deps + [tgt] do |t|

src/util.c

@@ -29,7 +29,7 @@ void error_handler(int fatal __attribute__ ((unused)) )
 		pthread_exit((void*) 1);
 	}
 		
-	longjmp(context->jmp, 1);
+	longjmp(context->jmp, fatal ? 1 : 2 );
 }
 
 

src/util.h

@@ -5,6 +5,7 @@
 #include <pthread.h>
 #include <string.h>
 #include <errno.h>
+#include <stdlib.h>
 
 void* xrealloc(void* ptr, size_t size);
 void* xmalloc(size_t size);
@@ -28,7 +29,7 @@ struct error_handler_context {
 
 #define DECLARE_ERROR_CONTEXT(name) \
   struct error_handler_context *name = (struct error_handler_context*) \
-  pthread_getspecific(cleanup_handler_key)
+  pthread_getspecific(cleanup_handler_key);
 
 /* clean up with the given function & data when error_handler() is invoked,
  * non-fatal errors will also return here (if that's dangerous, use fatal()
@@ -50,16 +51,22 @@ extern pthread_key_t cleanup_handler_key;
 	{ \
 	case 0: /* setup time */ \
  		if (old) { free(old); }\
-		pthread_setspecific(cleanup_handler_key, context); \
+		if( EINVAL == pthread_setspecific(cleanup_handler_key, context) ) { \
-		break; \
+			fprintf(stderr, "Tried to set an error handler" \
-	case 1: /* fatal error, terminate thread */ \
+					" without calling error_init().\n");\
-		debug( "Fatal error in thread %p", pthread_self() ); \
-		context->handler(context->data, 1); \
-		/*pthread_exit((void*) 1);*/ \
 			abort();\
-	case 2: /* non-fatal error, return to context of error handler setup */ \
+		}\
+		break; \
+	case 1: /* fatal error, terminate process */ \
+		debug( "Fatal error in thread %p", pthread_self() ); \
+		abort(); \
+		/* abort() can't return, so we can't fall through */ \
+	case 2: /* non-fatal error, call cleanup and terminate thread */ \
+		debug( "Error in thread %p", pthread_self() ); \
 		context->handler(context->data, 0); \
 		pthread_exit((void*) 1);\
+		/* pthread_exit() can't return, so we can't fall through
+		 * */\
 	default: \
 		 abort(); \
 	} \

tests/check_util.c

@@ -0,0 +1,172 @@
+#include "util.h"
+#include "self_pipe.h"
+
+#include <check.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+
+
+struct cleanup_bucket {
+	struct self_pipe *called_signal;
+};
+
+struct cleanup_bucket bkt;
+
+void bucket_init(void){
+	if ( bkt.called_signal ) {
+		self_pipe_destroy( bkt.called_signal );
+	}
+	bkt.called_signal = self_pipe_create();
+}
+
+void setup(void)
+{
+	bkt.called_signal = NULL;
+}
+
+int handler_called(void)
+{
+	return self_pipe_signal_clear( bkt.called_signal );
+}
+
+void dummy_cleanup( struct cleanup_bucket * foo, int fatal __attribute__((unused)) )
+{
+	if (NULL != foo){
+		self_pipe_signal( foo->called_signal );
+	}
+}
+
+
+void trigger_fatal(void)
+{ 
+	error_init();
+	error_set_handler( (cleanup_handler*) dummy_cleanup, &bkt );
+
+	log_level = 5;
+
+	fatal("Expected fatal error");
+}
+
+void trigger_error( void )
+{
+	error_init();
+	error_set_handler( (cleanup_handler *) dummy_cleanup, &bkt);
+	log_level = 4;
+	error("Expected error");
+}
+
+
+START_TEST( test_fatal_kills_process ) 
+{
+	pid_t pid;
+
+	pid = fork();
+
+	if ( pid == 0 ) {
+		trigger_fatal();
+		/* If we get here, just block so the test timeout fails
+		 * us */
+		sleep(10);
+	} 
+	else {
+		int kidstatus;
+		int result;
+		result = waitpid( pid, &kidstatus, 0 );
+		fail_if(  result < 0, "Wait failed." );
+		fail_unless( kidstatus == 6, "Kid was not aborted." );
+	}
+
+}
+END_TEST
+
+
+void * error_thread( void *nothing __attribute__((unused)) )
+{
+	trigger_error();
+	return NULL;
+}
+
+
+START_TEST( test_error_doesnt_kill_process )
+{
+	bucket_init();
+	pthread_attr_t attr;
+	pthread_t tid;
+
+	pthread_attr_init( &attr );
+
+	pthread_create( &tid, &attr, error_thread, NULL );
+	pthread_join( tid, NULL );
+}
+END_TEST
+
+
+START_TEST( test_error_calls_handler )
+{
+	bucket_init();
+
+	pthread_attr_t attr;
+	pthread_t tid;
+
+	pthread_attr_init( &attr );
+
+	pthread_create( &tid, &attr, error_thread, NULL );
+	pthread_join( tid, NULL );
+	fail_unless( handler_called(), "Handler wasn't called." );
+}
+END_TEST
+
+
+START_TEST( test_fatal_doesnt_call_handler )
+{
+	bucket_init();
+
+	pid_t kidpid;
+
+	kidpid = fork();
+	if ( kidpid == 0 ) {
+		trigger_fatal();
+	}
+	else {
+		int kidstatus;
+		int result = waitpid( kidpid, &kidstatus, 0 );
+		fail_if( result < 0, "Wait failed" );
+		fail_if( handler_called(), "Handler was called.");
+	}
+
+}
+END_TEST
+
+
+Suite* error_suite(void)
+{
+	Suite *s = suite_create("error");
+	TCase *tc_process = tcase_create("process");
+	TCase *tc_handler = tcase_create("handler");
+
+	tcase_add_checked_fixture( tc_process, setup, NULL );
+
+	tcase_add_test(tc_process, test_fatal_kills_process);
+	tcase_add_test(tc_process, test_error_doesnt_kill_process);
+	tcase_add_test(tc_handler, test_error_calls_handler );
+	tcase_add_test(tc_handler, test_fatal_doesnt_call_handler);
+
+	suite_add_tcase(s, tc_process);
+	suite_add_tcase(s, tc_handler);
+	
+	return s;
+}
+
+int main(void)
+{
+	int number_failed;
+	Suite *s = error_suite();
+	SRunner *sr = srunner_create(s);
+	srunner_run_all(sr, CK_NORMAL);
+	number_failed = srunner_ntests_failed(sr);
+	srunner_free(sr);
+	return (number_failed == 0) ? 0 : 1;
+}
+