Make the compiler stricter and tidy up code to make the subsequent errors and warnings go away

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"

src/bitset.h

@@ -1,5 +1,7 @@
-#ifndef __BITSET_H
-#define __BITSET_H
+#ifndef BITSET_H
+#define BITSET_H
+
+#include "util.h"
 
 #include <inttypes.h>
 #include <string.h>

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; i<client->serve->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");
 	

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;

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);
 

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;
+	if (ioctl(fd, FS_IOC_FIEMAP, fiemap) < 0) { return NULL; }
 
-	for (i=0;i<fiemap->fm_mapped_extents;i++)
+	for (i=0;i<fiemap->fm_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);
 	}

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 );
+}
+

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
 

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;
 	}

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;

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;

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);
 	

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

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");

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<<i)-1,
+			longs[i] == (1UL<<i)-1,
 			"longs[%ld] = %lx SHOULD BE %lx",
 			i, longs[i], (1L<<i)-1
 		);
@@ -140,7 +140,7 @@ START_TEST(test_bitset)
 }
 END_TEST
 
-Suite* bitset_suite()
+Suite* bitset_suite(void)
 {
 	Suite *s = suite_create("bitset");
 	TCase *tc_core = tcase_create("bitset");

tests/check_client.c

@@ -5,6 +5,8 @@
 
 #include "client.h"
 
+#include <unistd.h>
+
 #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");
 

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" );

tests/check_self_pipe.c

@@ -1,6 +1,8 @@
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <unistd.h>
+#include <pthread.h>
 
 #include <check.h>
 #include <mcheck.h>
@@ -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");
 

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");