From 80f298f6cd9dae6408ed6abe71fea15dd60aacf7 Mon Sep 17 00:00:00 2001 From: Alex Young Date: Thu, 21 Jun 2012 18:01:56 +0100 Subject: [PATCH] Make non-fatal errors return properly --- Rakefile | 15 +++- src/util.c | 4 +- src/util.h | 25 ++++--- tests/check_util.c | 172 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 202 insertions(+), 14 deletions(-) create mode 100644 tests/check_util.c diff --git a/Rakefile b/Rakefile index 3680560..31cdd05 100644 --- a/Rakefile +++ b/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 @@ -155,19 +162,21 @@ file check("listen") => build/readwrite.o build/parse.o build/client.o - build/serve.o + build/serve.o build/acl.o - build/ioutil.o + build/ioutil.o build/util.o} do |t| gcc_link t.name, t.prerequisites + [LIBCHECK] 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| diff --git a/src/util.c b/src/util.c index 510d4f8..a98aa98 100644 --- a/src/util.c +++ b/src/util.c @@ -9,7 +9,7 @@ #include "util.h" -pthread_key_t cleanup_handler_key; +pthread_key_t cleanup_handler_key; int log_level = 2; @@ -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 ); } diff --git a/src/util.h b/src/util.h index aa233f6..0a42ccf 100644 --- a/src/util.h +++ b/src/util.h @@ -5,6 +5,7 @@ #include #include #include +#include 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() @@ -49,19 +50,25 @@ extern pthread_key_t cleanup_handler_key; switch (setjmp(context->jmp)) \ { \ case 0: /* setup time */ \ - if (old) { free(old); }\ - pthread_setspecific(cleanup_handler_key, context); \ + if (old) { free(old); }\ + if( EINVAL == pthread_setspecific(cleanup_handler_key, context) ) { \ + fprintf(stderr, "Tried to set an error handler" \ + " without calling error_init().\n");\ + abort();\ + }\ break; \ - case 1: /* fatal error, terminate thread */ \ + case 1: /* fatal error, terminate process */ \ 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 */ \ + /* 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((void*) 1);\ + /* pthread_exit() can't return, so we can't fall through + * */\ default: \ - abort(); \ + abort(); \ } \ } diff --git a/tests/check_util.c b/tests/check_util.c new file mode 100644 index 0000000..30563ed --- /dev/null +++ b/tests/check_util.c @@ -0,0 +1,172 @@ +#include "util.h" +#include "self_pipe.h" + +#include +#include +#include +#include + + + +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; +} +