diff --git a/Rakefile b/Rakefile index 2ea949d..34ccc32 100644 --- a/Rakefile +++ b/Rakefile @@ -8,9 +8,16 @@ DEBUG = ENV.has_key?('DEBUG') && ALL_SOURCES =FileList['src/*'] SOURCES = ALL_SOURCES.select { |c| c =~ /\.c$/ } OBJECTS = SOURCES.pathmap( "%{^src,build}X.o" ) +TEST_SOURCES = FileList['tests/*.c'] +TEST_OBJECTS = TEST_SOURCES.pathmap( "%{^tests,build/tests}X.o" ) LIBS = %w( pthread ) -CCFLAGS = %w( -Wall -Werror-implicit-function-declaration ) +CCFLAGS = %w( -Wall + -Wextra + -Werror-implicit-function-declaration + -Wstrict-prototypes + -Wno-missing-field-initializers + ) # Added -Wno-missing-field-initializers to shut GCC up over {0} struct initialisers LDFLAGS = [] LIBCHECK = "/usr/lib/libcheck.a" @@ -59,18 +66,24 @@ namespace "test" do end + +def gcc_compile( target, source ) + FileUtils.mkdir_p File.dirname( target ) + sh "gcc -Isrc -c #{CCFLAGS.join(' ')} -o #{target} #{source} " +end + def gcc_link(target, objects) FileUtils.mkdir_p File.dirname( target ) sh "gcc #{LDFLAGS.join(' ')} "+ LIBS.map { |l| "-l#{l}" }.join(" ")+ - " -I src" + + " -Isrc " + " -o #{target} "+ objects.join(" ") end def headers(c) - `gcc -MM #{c}`.gsub("\\\n", " ").split(" ")[2..-1] + `gcc -Isrc -MM #{c}`.gsub("\\\n", " ").split(" ")[2..-1] end rule 'build/flexnbd' => OBJECTS do |t| @@ -79,7 +92,7 @@ end file check("client") => -%w{tests/check_client.c +%w{build/tests/check_client.o build/self_pipe.o build/nbdtypes.o build/control.o @@ -94,7 +107,7 @@ file check("client") => end file check("acl") => -%w{tests/check_acl.c +%w{build/tests/check_acl.o build/parse.o build/acl.o build/util.o} do |t| @@ -102,7 +115,7 @@ file check("acl") => end file check("serve") => -%w{tests/check_serve.c +%w{build/tests/check_serve.o build/self_pipe.o build/nbdtypes.o build/control.o @@ -118,22 +131,24 @@ end (TEST_MODULES- %w{acl client serve}).each do |m| - deps = ["tests/check_#{m}.c", "build/ioutil.o", "build/util.o"] + tgt = "build/tests/check_#{m}.o" + deps = ["build/ioutil.o", "build/util.o"] maybe_obj_name = "build/#{m}.o" deps << maybe_obj_name if OBJECTS.include?( maybe_obj_name ) - file check( m ) => deps do |t| - gcc_link(t.name, deps + [LIBCHECK]) + file check( m ) => deps + [tgt] do |t| + gcc_link(t.name, deps + [tgt, LIBCHECK]) end end OBJECTS.zip( SOURCES ).each do |o,c| - file o => [c]+headers(c) do |t| - FileUtils.mkdir_p File.dirname( o ) - sh "gcc -Isrc -c #{CCFLAGS.join(' ')} -o #{o} #{c} " - end + file o => [c]+headers(c) do |t| gcc_compile( o, c ) end +end + +TEST_OBJECTS.zip( TEST_SOURCES ).each do |o,c| + file o => [c] + headers(c) do |t| gcc_compile( o, c ) end end desc "Remove all build targets, binaries and temporary files" diff --git a/src/bitset.h b/src/bitset.h index ee0f8fb..522d291 100644 --- a/src/bitset.h +++ b/src/bitset.h @@ -1,5 +1,7 @@ -#ifndef __BITSET_H -#define __BITSET_H +#ifndef BITSET_H +#define BITSET_H + +#include "util.h" #include #include diff --git a/src/client.c b/src/client.c index 6af27af..dc0bb16 100644 --- a/src/client.c +++ b/src/client.c @@ -57,7 +57,7 @@ void client_destroy( struct client *client ) * allocated, we can proceed as normal and make one call to writeloop. * */ -void write_not_zeroes(struct client* client, off64_t from, int len) +void write_not_zeroes(struct client* client, uint64_t from, int len) { NULLCHECK( client ); @@ -83,7 +83,7 @@ void write_not_zeroes(struct client* client, off64_t from, int len) if (0) /* useful but expensive */ { - int i; + uint64_t i; fprintf(stderr, "full map resolution=%d: ", map->resolution); for (i=0; iserve->size; i+=map->resolution) { int here = (from >= i && from < i+map->resolution); @@ -248,8 +248,7 @@ int client_request_needs_reply( struct client * client, struct nbd_request reque break; case REQUEST_WRITE: /* check it's not out of range */ - if (request.from < 0 || - request.from+request.len > client->serve->size) { + if ( request.from+request.len > client->serve->size) { debug("request read %ld+%d out of range", request.from, request.len @@ -372,7 +371,8 @@ void client_send_hello(struct client* client) client_write_init( client, client->serve->size ); } -void client_cleanup(struct client* client, int fatal) +void client_cleanup(struct client* client, + int fatal __attribute__ ((unused)) ) { info("client cleanup"); diff --git a/src/control.c b/src/control.c index a103983..d104eef 100644 --- a/src/control.c +++ b/src/control.c @@ -45,7 +45,7 @@ static const int mirror_longest_write = 8<<20; /** If, during a mirror pass, we have sent this number of bytes or fewer, we * go to freeze the I/O and finish it off. This is just a guess. */ -static const int mirror_last_pass_after_bytes_written = 100<<20; +static const unsigned int mirror_last_pass_after_bytes_written = 100<<20; /** The largest number of full passes we'll do - the last one will always * cause the I/O to freeze, however many bytes are left to copy. @@ -169,6 +169,7 @@ int control_mirror(struct control_params* client, int linesc, char** lines) int use_connect_from = 0; uint64_t max_bytes_per_second; int action_at_finish; + int raw_port; if (linesc < 2) { @@ -181,12 +182,12 @@ int control_mirror(struct control_params* client, int linesc, char** lines) return -1; } - connect_to.v4.sin_port = atoi(lines[1]); - if (connect_to.v4.sin_port < 0 || connect_to.v4.sin_port > 65535) { + raw_port = atoi(lines[1]); + if (raw_port < 0 || raw_port > 65535) { write_socket("1: bad IP port number"); return -1; } - connect_to.v4.sin_port = htobe16(connect_to.v4.sin_port); + connect_to.v4.sin_port = htobe16(raw_port); if (linesc > 2) { if (parse_ip_to_sockaddr(&connect_from.generic, lines[2]) == 0) { @@ -290,12 +291,17 @@ int control_acl(struct control_params* client, int linesc, char** lines) } /** FIXME: add some useful statistics */ -int control_status(struct control_params* client, int linesc, char** lines) +int control_status( + struct control_params* client __attribute__ ((unused)), + int linesc __attribute__ ((unused)), + char** lines __attribute__((unused)) + ) { return 0; } -void control_cleanup(struct control_params* client, int fatal) +void control_cleanup(struct control_params* client, + int fatal __attribute__ ((unused)) ) { if (client->socket) close(client->socket); @@ -348,7 +354,8 @@ void* control_serve(void* client_uncast) return NULL; } -void accept_control_connection(struct server* params, int client_fd, union mysockaddr* client_address) +void accept_control_connection(struct server* params, int client_fd, + union mysockaddr* client_address __attribute__ ((unused)) ) { pthread_t control_thread; struct control_params* control_params; diff --git a/src/flexnbd.c b/src/flexnbd.c index 9cfc18f..5992c0b 100644 --- a/src/flexnbd.c +++ b/src/flexnbd.c @@ -45,46 +45,6 @@ void exit_err( char *msg ) exit( 1 ); } -void params_serve( - struct server* out, - char* s_ip_address, - char* s_port, - char* s_file, - char *s_ctrl_sock, - int default_deny, - int acl_entries, - char** s_acl_entries ) -{ - out->tcp_backlog = 10; /* does this need to be settable? */ - - FATAL_IF_NULL(s_ip_address, "No IP address supplied"); - FATAL_IF_NULL(s_port, "No port number supplied"); - FATAL_IF_NULL(s_file, "No filename supplied"); - FATAL_IF_ZERO( - parse_ip_to_sockaddr(&out->bind_to.generic, s_ip_address), - "Couldn't parse server address '%s' (use 0 if " - "you want to bind to all IPs)", - s_ip_address - ); - - /* control_socket_name is optional. It just won't get created if - * we pass NULL. */ - out->control_socket_name = s_ctrl_sock; - - out->acl = acl_create( acl_entries, s_acl_entries, default_deny ); - if (out->acl && out->acl->len != acl_entries) - fatal("Bad ACL entry '%s'", s_acl_entries[out->acl->len]); - - out->bind_to.v4.sin_port = atoi(s_port); - if (out->bind_to.v4.sin_port < 0 || out->bind_to.v4.sin_port > 65535) - fatal("Port number must be >= 0 and <= 65535"); - out->bind_to.v4.sin_port = htobe16(out->bind_to.v4.sin_port); - - out->filename = s_file; - out->filename_incomplete = xmalloc(strlen(s_file)+11+1); - strcpy(out->filename_incomplete, s_file); - strcpy(out->filename_incomplete + strlen(s_file), ".INCOMPLETE"); -} /* TODO: Separate this function. * It should be: @@ -124,11 +84,7 @@ void params_readwrite( if (s_bind_address != NULL && parse_ip_to_sockaddr(&out->connect_from.generic, s_bind_address) == 0) fatal("Couldn't parse bind address '%s'", s_bind_address); - /* FIXME: duplicated from above */ - out->connect_to.v4.sin_port = atoi(s_port); - if (out->connect_to.v4.sin_port < 0 || out->connect_to.v4.sin_port > 65535) - fatal("Port number must be >= 0 and <= 65535"); - out->connect_to.v4.sin_port = htobe16(out->connect_to.v4.sin_port); + parse_port( s_port, &out->connect_to.v4 ); out->from = atol(s_from); diff --git a/src/ioutil.c b/src/ioutil.c index d82b4fc..e8f3af7 100644 --- a/src/ioutil.c +++ b/src/ioutil.c @@ -14,9 +14,9 @@ #include "util.h" #include "bitset.h" -struct bitset_mapping* build_allocation_map(int fd, off64_t size, int resolution) +struct bitset_mapping* build_allocation_map(int fd, uint64_t size, int resolution) { - int i; + unsigned int i; struct bitset_mapping* allocation_map = bitset_alloc(size, resolution); struct fiemap *fiemap_count, *fiemap; @@ -46,15 +46,15 @@ struct bitset_mapping* build_allocation_map(int fd, off64_t size, int resolution fiemap->fm_extent_count = fiemap->fm_mapped_extents; fiemap->fm_mapped_extents = 0; - if (ioctl(fd, FS_IOC_FIEMAP, fiemap) < 0) - return NULL; - - for (i=0;ifm_mapped_extents;i++) + if (ioctl(fd, FS_IOC_FIEMAP, fiemap) < 0) { return NULL; } + + for (i=0;ifm_mapped_extents;i++) { bitset_set_range( allocation_map, fiemap->fm_extents[i].fe_logical, fiemap->fm_extents[i].fe_length ); + } for (i=0; i<16; i++) { debug("map[%d] = %d%d%d%d%d%d%d%d", @@ -105,9 +105,8 @@ int writeloop(int filedes, const void *buffer, size_t size) { size_t written=0; while (written < size) { - size_t result = write(filedes, buffer+written, size-written); - if (result == -1) - return -1; + ssize_t result = write(filedes, buffer+written, size-written); + if (result == -1) { return -1; } written += result; } return 0; @@ -117,9 +116,10 @@ int readloop(int filedes, void *buffer, size_t size) { size_t readden=0; while (readden < size) { - size_t result = read(filedes, buffer+readden, size-readden); - if (result == 0 /* EOF */ || result == -1 /* error */) + ssize_t result = read(filedes, buffer+readden, size-readden); + if (result == 0 /* EOF */ || result == -1 /* error */) { return -1; + } readden += result; } return 0; @@ -129,11 +129,10 @@ int sendfileloop(int out_fd, int in_fd, off64_t *offset, size_t count) { size_t sent=0; while (sent < count) { - size_t result = sendfile64(out_fd, in_fd, offset, count-sent); + ssize_t result = sendfile64(out_fd, in_fd, offset, count-sent); debug("sendfile64(out_fd=%d, in_fd=%d, offset=%p, count-sent=%ld) = %ld", out_fd, in_fd, offset, count-sent, result); - if (result == -1) - return -1; + if (result == -1) { return -1; } sent += result; debug("sent=%ld, count=%ld", sent, count); } diff --git a/src/parse.c b/src/parse.c index e2541f6..70f34d4 100644 --- a/src/parse.c +++ b/src/parse.c @@ -90,3 +90,17 @@ int parse_acl(struct ip_and_mask (**out)[], int max, char **entries) return max; } + +void parse_port( char *s_port, struct sockaddr_in *out ) +{ + NULLCHECK( s_port ); + + int raw_port; + + raw_port = atoi( s_port ); + if ( raw_port < 0 || raw_port > 65535 ) { + fatal( "Port number must be >= 0 and <= 65535" ); + } + out->sin_port = htobe16( raw_port ); +} + diff --git a/src/parse.h b/src/parse.h index ce7c24a..280902c 100644 --- a/src/parse.h +++ b/src/parse.h @@ -19,6 +19,7 @@ struct ip_and_mask { int parse_ip_to_sockaddr(struct sockaddr* out, char* src); int parse_acl(struct ip_and_mask (**out)[0], int max, char **entries); +void parse_port( char *s_port, struct sockaddr_in *out ); #endif diff --git a/src/self_pipe.c b/src/self_pipe.c index c31763b..1cdb8ed 100644 --- a/src/self_pipe.c +++ b/src/self_pipe.c @@ -87,7 +87,8 @@ struct self_pipe * self_pipe_create(void) */ int self_pipe_signal( struct self_pipe * sig ) { - if ( write( sig->write_fd, "1", 1 ) != 1 ) { + int written = write( sig->write_fd, "X", 1 ); + if ( written != 1 ) { self_pipe_server_error( errno, ERR_MSG_WRITE ); return 0; } diff --git a/src/serve.c b/src/serve.c index 3b84f58..e5bee9c 100644 --- a/src/serve.c +++ b/src/serve.c @@ -48,16 +48,15 @@ struct server * server_create ( out->tcp_backlog = 10; /* does this need to be settable? */ - if (s_ip_address == NULL) - fatal("No IP address supplied"); - if (s_port == NULL) - fatal("No port number supplied"); - if (s_file == NULL) - fatal("No filename supplied"); - - if (parse_ip_to_sockaddr(&out->bind_to.generic, s_ip_address) == 0) - fatal("Couldn't parse server address '%s' (use 0 if " - "you want to bind to all IPs)", s_ip_address); + FATAL_IF_NULL(s_ip_address, "No IP address supplied"); + FATAL_IF_NULL(s_port, "No port number supplied"); + FATAL_IF_NULL(s_file, "No filename supplied"); + FATAL_IF_ZERO( + parse_ip_to_sockaddr(&out->bind_to.generic, s_ip_address), + "Couldn't parse server address '%s' (use 0 if " + "you want to bind to all IPs)", + s_ip_address + ); /* control_socket_name is optional. It just won't get created if * we pass NULL. */ @@ -67,10 +66,7 @@ struct server * server_create ( if (out->acl && out->acl->len != acl_entries) fatal("Bad ACL entry '%s'", s_acl_entries[out->acl->len]); - out->bind_to.v4.sin_port = atoi(s_port); - if (out->bind_to.v4.sin_port < 0 || out->bind_to.v4.sin_port > 65535) - fatal("Port number must be >= 0 and <= 65535"); - out->bind_to.v4.sin_port = htobe16(out->bind_to.v4.sin_port); + parse_port( s_port, &out->bind_to.v4 ); out->filename = s_file; out->filename_incomplete = xmalloc(strlen(s_file)+11+1); @@ -471,7 +467,7 @@ int server_accept( struct server * params ) { NULLCHECK( params ); info("accept loop starting"); - int activity_fd, client_fd; + int client_fd; union mysockaddr client_address; fd_set fds; socklen_t socklen=sizeof(client_address); @@ -554,7 +550,8 @@ void serve_signal_close( struct server * serve ) /** Closes sockets, frees memory and waits for all client threads to finish */ -void serve_cleanup(struct server* params, int fatal) +void serve_cleanup(struct server* params, + int fatal __attribute__ ((unused)) ) { NULLCHECK( params ); @@ -562,20 +559,17 @@ void serve_cleanup(struct server* params, int fatal) int i; - if (params->server_fd) - close(params->server_fd); - if (params->control_fd) - close(params->control_fd); - if (params->control_socket_name){ - ; - } - if (params->proxy_fd); - close(params->proxy_fd); + if (params->server_fd){ close(params->server_fd); } + if (params->control_fd){ close(params->control_fd); } + if (params->control_socket_name){ ; } + if (params->proxy_fd){ close(params->proxy_fd); } - if (params->close_signal) + if (params->close_signal) { self_pipe_destroy( params->close_signal ); - if (params->allocation_map) + } + if (params->allocation_map) { free(params->allocation_map); + } if (params->mirror) { pthread_t mirror_t = params->mirror->thread; diff --git a/src/serve.h b/src/serve.h index 0b09718..570c5f8 100644 --- a/src/serve.h +++ b/src/serve.h @@ -58,7 +58,7 @@ struct server { /** (static) file name of UNIX control socket (or NULL if none) */ char* control_socket_name; /** size of file */ - off64_t size; + uint64_t size; /** Claims around any I/O to this file */ pthread_mutex_t l_io; diff --git a/src/util.c b/src/util.c index 5b5e8e4..95e2c69 100644 --- a/src/util.c +++ b/src/util.c @@ -13,12 +13,12 @@ pthread_key_t cleanup_handler_key; int log_level = 1; -void error_init() +void error_init(void) { pthread_key_create(&cleanup_handler_key, free); } -void error_handler(int fatal) +void error_handler(int fatal __attribute__ ((unused)) ) { DECLARE_ERROR_CONTEXT(context); diff --git a/src/util.h b/src/util.h index 86897b2..253b894 100644 --- a/src/util.h +++ b/src/util.h @@ -15,7 +15,7 @@ typedef void (cleanup_handler)(void* /* context */, int /* is fatal? */); extern int log_level; /* set up the error globals */ -void error_init(); +void error_init(void); /* error_set_handler must be a macro not a function due to setjmp stack rules */ #include diff --git a/tests/check_acl.c b/tests/check_acl.c index 8857894..101a45d 100644 --- a/tests/check_acl.c +++ b/tests/check_acl.c @@ -176,7 +176,7 @@ START_TEST( test_default_accept_rejects ) END_TEST -Suite* acl_suite() +Suite* acl_suite(void) { Suite *s = suite_create("acl"); TCase *tc_create = tcase_create("create"); diff --git a/tests/check_bitset.c b/tests/check_bitset.c index c71051c..cf3418b 100644 --- a/tests/check_bitset.c +++ b/tests/check_bitset.c @@ -62,7 +62,7 @@ START_TEST(test_bit_ranges) for (i=0; i<64; i++) { bit_set_range(buffer, i*64, i); fail_unless( - longs[i] == (1L< + #define FAKE_SERVER ((struct server *)23) #define FAKE_SOCKET (42) @@ -46,6 +48,8 @@ START_TEST( test_opens_stop_signal ) END_TEST +int fd_is_closed(int); + START_TEST( test_closes_stop_signal ) { struct client *c = client_create( FAKE_SERVER, FAKE_SOCKET ); @@ -78,7 +82,7 @@ START_TEST( test_read_request_quits_on_stop_signal ) END_TEST -Suite *client_suite() +Suite *client_suite(void) { Suite *s = suite_create("client"); diff --git a/tests/check_nbdtypes.c b/tests/check_nbdtypes.c index d4f64bf..81a5418 100644 --- a/tests/check_nbdtypes.c +++ b/tests/check_nbdtypes.c @@ -182,7 +182,7 @@ START_TEST(test_reply_handle) END_TEST -Suite *nbdtypes_suite() +Suite *nbdtypes_suite(void) { Suite *s = suite_create( "nbdtypes" ); TCase *tc_init = tcase_create( "nbd_init" ); diff --git a/tests/check_self_pipe.c b/tests/check_self_pipe.c index fed86cd..c3026c9 100644 --- a/tests/check_self_pipe.c +++ b/tests/check_self_pipe.c @@ -1,6 +1,8 @@ #include #include #include +#include +#include #include #include @@ -157,19 +159,8 @@ START_TEST( test_destroy_closes_write_pipe ) END_TEST -START_TEST( test_signal_after_destroy_fails ) -{ - struct self_pipe* sig; - sig = self_pipe_create(); - self_pipe_destroy( sig ); - - fail_unless( self_pipe_signal( sig ) == 0, "Signaling a closed self_pipe didn't return 0." ); -} -END_TEST - - -Suite *self_pipe_suite() +Suite *self_pipe_suite(void) { Suite *s = suite_create("self_pipe"); diff --git a/tests/check_serve.c b/tests/check_serve.c index 2a4fbf2..0cd3011 100644 --- a/tests/check_serve.c +++ b/tests/check_serve.c @@ -16,7 +16,7 @@ char * dummy_file; -char *make_tmpfile() +char *make_tmpfile(void) { FILE *fp; char *fn_buf; @@ -84,11 +84,13 @@ int connect_client( char *addr, int actual_port ) { int client_fd; - struct addrinfo hint = {0}; + struct addrinfo hint; struct addrinfo *ailist, *aip; + + memset( &hint, '\0', sizeof( struct addrinfo ) ); hint.ai_socktype = SOCK_STREAM; - myfail_if( getaddrinfo( "127.0.0.7", NULL, &hint, &ailist ) != 0, "getaddrinfo failed." ); + myfail_if( getaddrinfo( addr, NULL, &hint, &ailist ) != 0, "getaddrinfo failed." ); int connected = 0; for( aip = ailist; aip; aip = aip->ai_next ) { @@ -106,6 +108,13 @@ int connect_client( char *addr, int actual_port ) return client_fd; } +/* These are "internal" functions we need for the following test. We + * shouldn't need them but there's no other way at the moment. */ +void serve_open_server_socket( struct server * ); +int server_port( struct server * ); +void server_accept( struct server * ); +int fd_is_closed( int ); +void server_close_clients( struct server * ); START_TEST( test_acl_update_closes_bad_client ) { @@ -197,7 +206,7 @@ START_TEST( test_acl_update_leaves_good_client ) END_TEST -Suite* serve_suite() +Suite* serve_suite(void) { Suite *s = suite_create("serve"); TCase *tc_acl_update = tcase_create("acl_update");