Merge, just renaming old error macros.

Rakefile

@@ -101,8 +101,23 @@ file check("acl") =>
   gcc_link t.name, t.prerequisites + [LIBCHECK]
 end
 
+file check("serve") =>
+%w{tests/check_serve.c
+  build/self_pipe.o
+  build/nbdtypes.o
+  build/control.o
+  build/readwrite.o
+  build/parse.o
+  build/client.o
+  build/serve.o 
+  build/acl.o
+  build/ioutil.o 
+  build/util.o} do |t|
+  gcc_link t.name, t.prerequisites + [LIBCHECK]
+end
 
-(TEST_MODULES- %w{acl client}).each do |m|
+
+(TEST_MODULES- %w{acl client serve}).each do |m|
   deps = ["tests/check_#{m}.c", "build/ioutil.o", "build/util.o"]
   maybe_obj_name = "build/#{m}.o"
 

src/client.c

@@ -171,8 +171,9 @@ int client_read_request( struct client * client , struct nbd_request *out_reques
 	FATAL_IF_NEGATIVE(select(FD_SETSIZE, &fds, NULL, NULL, NULL), 
 	  "select() failed");
 	
-	if ( self_pipe_fd_isset( client->stop_signal, &fds ) )
+	if ( self_pipe_fd_isset( client->stop_signal, &fds ) ){
 		return 0;
+	}
 
 	if (readloop(client->socket, &request_raw, sizeof(request_raw)) == -1) {
 		if (errno == 0) {

src/client.h

@@ -1,6 +1,7 @@
 #ifndef CLIENT_H
 #define CLIENT_H
 
+
 struct client {
 	int     socket;
 	

src/control.c

@@ -282,8 +282,7 @@ int control_acl(struct control_params* client, int linesc, char** lines)
 		acl_destroy( new_acl );
 	}
 	else {
-		client->serve->acl = new_acl;
+		server_replace_acl( client->serve, new_acl );
-		acl_destroy( old_acl );
 		write_socket("0: updated");
 	}
 	

src/flexnbd.c

@@ -60,7 +60,6 @@ void params_serve(
 	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 "
@@ -293,7 +292,7 @@ int mode_serve( int argc, char *argv[] )
 	int default_deny = 0; // not on by default
 	int err = 0;
 
-	struct server serve;
+	struct server * serve;
 
 	while (1) {
 		c = getopt_long(argc, argv, serve_short_options, serve_options, NULL);
@@ -313,9 +312,9 @@ int mode_serve( int argc, char *argv[] )
 	}
 	if ( err ) { exit_err( serve_help_text ); }
 
-	memset( &serve, 0, sizeof( serve ) );
+	serve = server_create( ip_addr, ip_port, file, sock, default_deny, argc - optind, argv + optind );
-	params_serve( &serve, ip_addr, ip_port, file, sock, default_deny, argc - optind, argv + optind );
+	do_serve( serve );
-	do_serve( &serve );
+	server_destroy( serve );
 
 	return 0;
 }

src/self_pipe.c

@@ -62,7 +62,7 @@ struct self_pipe * self_pipe_create(void)
 		return NULL;
 	}
 
-	if ( fcntl( fds[0], F_SETFD, O_NONBLOCK ) || fcntl( fds[1], F_SETFD, O_NONBLOCK ) ) {
+	if ( fcntl( fds[0], F_SETFL, O_NONBLOCK ) || fcntl( fds[1], F_SETFL, O_NONBLOCK ) ) {
 		fcntl_err = errno;
 		while( close( fds[0] ) == -1 && errno == EINTR );
 		while( close( fds[1] ) == -1 && errno == EINTR );
@@ -98,7 +98,8 @@ int self_pipe_signal( struct self_pipe * sig )
 
 /** 
  * Clear a received signal from the pipe.  Every signal sent must be
- * cleared by one (and only one) recipient when they return from select().
+ * cleared by one (and only one) recipient when they return from select()
+ * if the signal is to be used more than once.
  * Returns the number of bytes read, which will be 1 on success and 0 if
  * there was no signal.
  */
@@ -106,7 +107,7 @@ int self_pipe_signal_clear( struct self_pipe *sig )
 {
 	char buf[1];
 
-	return read( sig->read_fd, buf, 1 );
+	return 1 == read( sig->read_fd, buf, 1 );
 }
 
 

src/serve.c

@@ -34,6 +34,75 @@ static inline void* sockaddr_address_data(struct sockaddr* sockaddr)
 	return NULL;
 }
 
+struct server * server_create (
+	char* s_ip_address,
+	char* s_port,
+	char* s_file,
+	char *s_ctrl_sock,
+	int default_deny,
+	int acl_entries,
+	char** s_acl_entries )
+{
+	struct server * out;
+	out = xmalloc( sizeof( struct server ) );
+
+	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);
+
+	/* 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");
+
+	pthread_mutex_init(&out->l_io, NULL);
+	pthread_mutex_init(&out->l_acl, NULL);
+
+	out->close_signal = self_pipe_create();
+	out->acl_updated_signal = self_pipe_create();
+
+	NULLCHECK( out->close_signal );
+	NULLCHECK( out->acl_updated_signal );
+
+	return out;
+}
+
+void server_destroy( struct server * serve )
+{
+	self_pipe_destroy( serve->acl_updated_signal );
+	self_pipe_destroy( serve->close_signal );
+
+	pthread_mutex_destroy( &serve->l_acl );
+	pthread_mutex_destroy( &serve->l_io );
+
+	if ( serve->acl ) { acl_destroy( serve->acl ); }
+
+	free( serve );
+}
+
+
 void server_dirty(struct server *serve, off64_t from, int len)
 {
 	NULLCHECK( serve );
@@ -42,29 +111,51 @@ void server_dirty(struct server *serve, off64_t from, int len)
 		bitset_set_range(serve->mirror->dirty_map, from, len);
 }
 
-int server_lock_io( struct server * serve)
+#define SERVER_LOCK( s, f, msg ) \
+	{ NULLCHECK( s ); \
+	 FATAL_IF_NEGATIVE( pthread_mutex_lock( &s->f ), msg ); }
+#define SERVER_UNLOCK( s, f, msg ) \
+	{ NULLCHECK( s ); \
+	 FATAL_IF_NEGATIVE( pthread_mutex_unlock( &s->f ), msg ); }
+
+void server_lock_io( struct server * serve)
 {
-	NULLCHECK( serve );
+	SERVER_LOCK( serve, l_io, "Problem with I/O lock" );
-
-	FATAL_IF_NEGATIVE(
-		pthread_mutex_lock(&serve->l_io),
-		"Problem with I/O lock"
-	);
-	
-	return 1;
 }
 
-
 void server_unlock_io( struct server* serve )
 {
-	NULLCHECK( serve );
+	SERVER_UNLOCK( serve, l_io, "Problem with I/O unlock" );
-
-	FATAL_IF_NEGATIVE(
-		pthread_mutex_unlock(&serve->l_io),
-		"Problem with I/O unlock"
-	);
 }
 
+void server_lock_acl( struct server *serve )
+{
+	SERVER_LOCK( serve, l_acl, "Problem with ACL lock" );
+}
+
+void server_unlock_acl( struct server *serve )
+{
+	SERVER_UNLOCK( serve, l_acl, "Problem with ACL unlock" );
+}
+
+
+/** Return the actual port the server bound to.  This is used because we
+ * are allowed to pass "0" on the command-line.
+ */
+int server_port( struct server * server )
+{
+	NULLCHECK( server );
+	union mysockaddr addr;
+	socklen_t len = sizeof( addr.v4 );
+
+	if ( getsockname( server->server_fd, &addr.v4, &len ) < 0 ) {
+		fatal( "Failed to get the port number." );
+	}
+
+	return be16toh( addr.v4.sin_port );
+}
+
+
 /** Prepares a listening socket for the NBD server, binding etc. */
 void serve_open_server_socket(struct server* params)
 {
@@ -100,6 +191,8 @@ void serve_open_server_socket(struct server* params)
 	);
 }
 
+
+
 int tryjoin_client_thread( struct client_tbl_entry *entry, int (*joinfunc)(pthread_t, void **) )
 {
 
@@ -192,16 +285,26 @@ int cleanup_and_find_client_slot(struct server* params)
 }
 
 
+/** Check whether the address client_address is allowed or not according
+ * to the current acl.  If params->acl is NULL, the result will be 1,
+ * otherwise it will be the result of acl_includes().
+ */
 int server_acl_accepts( struct server *params, union mysockaddr * client_address )
 {
 	NULLCHECK( params );
 	NULLCHECK( client_address );
 
-	if (params->acl) {
+	struct acl * acl;
-		return acl_includes( params->acl, client_address );
+	int accepted;
-	}
 
-	return 1;
+	server_lock_acl( params );
+	{
+		acl = params->acl;
+		accepted = acl ? acl_includes( acl, client_address ) : 1;
+	}
+	server_unlock_acl( params );
+
+	return accepted;
 }
 
 
@@ -274,7 +377,7 @@ void accept_nbd_client(
 	memcpy(&params->nbd_client[slot].address, client_address, 
 	  sizeof(union mysockaddr));
 	
-	if (pthread_create(&params->nbd_client[slot].thread, NULL, client_serve, client_params) < 0) {
+	if (pthread_create(&params->nbd_client[slot].thread, NULL, client_serve, client_params) != 0) {
 		debug( "Thread creation problem." );
 		write(client_fd, "Thread creation problem", 23);
 		client_destroy( client_params );
@@ -286,6 +389,30 @@ void accept_nbd_client(
 }
 
 
+void server_audit_clients( struct server * serve)
+{
+	NULLCHECK( serve );
+
+	int i;
+	struct client_tbl_entry * entry;
+
+	/* There's an apparent race here.  If the acl updates while
+	 * we're traversing the nbd_clients array, the earlier entries
+	 * won't have been audited against the later acl.  This isn't a
+	 * problem though, because in order to update the acl
+	 * server_replace_acl must have been called, so the
+	 * server_accept loop will see a second acl_updated signal as
+	 * soon as it hits select, and a second audit will be run.
+	 */
+	for( i = 0; i < MAX_NBD_CLIENTS; i++ ) {
+		entry = &serve->nbd_client[i];
+		if ( 0 == entry->thread ) { continue; }
+		if ( server_acl_accepts( serve, &entry->address ) ) { continue; }
+		client_signal_stop( entry->client );
+	}
+}
+
+
 int server_is_closed(struct server* serve)
 {
 	NULLCHECK( serve );
@@ -315,45 +442,79 @@ void server_close_clients( struct server *params )
 }
 
 
+/** Replace the current acl with a new one.  The old one will be thrown
+ * away.
+ */
+void server_replace_acl( struct server *serve, struct acl * new_acl )
+{
+	NULLCHECK(serve);
+	NULLCHECK(new_acl);
+
+	/* We need to lock around updates to the acl in case we try to
+	 * destroy the old acl while checking against it.
+	 */
+	server_lock_acl( serve );
+	{
+		struct acl * old_acl = serve->acl;
+		serve->acl = new_acl;
+		/* We should always have an old_acl, but just in case... */
+		if ( old_acl ) { acl_destroy( old_acl ); }
+	}
+	server_unlock_acl( serve );
+
+	self_pipe_signal( serve->acl_updated_signal );
+}
+
+
 /** Accept either an NBD or control socket connection, dispatch appropriately */
-void serve_accept_loop(struct server* params) 
+int server_accept( struct server * params )
 {
 	NULLCHECK( params );
 	info("accept loop starting");
-	while (1) {
+	int              activity_fd, client_fd;
-		int              activity_fd, client_fd;
+	union mysockaddr client_address;
-		union mysockaddr client_address;
+	fd_set           fds;
-		fd_set           fds;
+	socklen_t        socklen=sizeof(client_address);
-		socklen_t        socklen=sizeof(client_address);
+
-		
+	FD_ZERO(&fds);
-		FD_ZERO(&fds);
+	FD_SET(params->server_fd, &fds);
-		FD_SET(params->server_fd, &fds);
+	self_pipe_fd_set( params->close_signal, &fds );
-		self_pipe_fd_set( params->close_signal, &fds );
+	self_pipe_fd_set( params->acl_updated_signal, &fds );
-		if (params->control_socket_name)
+	if (params->control_socket_name)
-			FD_SET(params->control_fd, &fds);
+		FD_SET(params->control_fd, &fds);
-		
+
-		FATAL_IF_NEGATIVE(select(FD_SETSIZE, &fds, 
+	FATAL_IF_NEGATIVE(select(FD_SETSIZE, &fds, 
-		  NULL, NULL, NULL), "select() failed");
+				NULL, NULL, NULL), "select() failed");
-		
+
-		if ( self_pipe_fd_isset( params->close_signal, &fds ) ){
+	if ( self_pipe_fd_isset( params->close_signal, &fds ) ){
-			server_close_clients( params );
+		server_close_clients( params );
-			return;
+		return 0;
-		}
-		
-		activity_fd = FD_ISSET(params->server_fd, &fds) ? params->server_fd: 
-		  params->control_fd;
-		client_fd = accept(activity_fd, &client_address.generic, &socklen);
-		
-		if (activity_fd == params->server_fd) {
-			info("Accepted nbd client socket");
-			accept_nbd_client(params, client_fd, &client_address);
-		}
-		if (activity_fd == params->control_fd) {
-			info("Accepted control client socket"); 
-			accept_control_connection(params, client_fd, &client_address);
-		}
-			
 	}
+
+	if ( self_pipe_fd_isset( params->acl_updated_signal, &fds ) ) {
+		server_audit_clients( params );
+	}
+
+	activity_fd = FD_ISSET(params->server_fd, &fds) ? params->server_fd: 
+		params->control_fd;
+	client_fd = accept(activity_fd, &client_address.generic, &socklen);
+
+	if (activity_fd == params->server_fd) {
+		debug("Accepted nbd client socket");
+		accept_nbd_client(params, client_fd, &client_address);
+	}
+	if (activity_fd == params->control_fd) {
+		debug("Accepted control client socket"); 
+		accept_control_connection(params, client_fd, &client_address);
+	}
+
+	return 1;
+}
+
+
+void serve_accept_loop(struct server* params) 
+{
+	while( server_accept( params ) );
 }
 
 /** Initialisation function that sets up the initial allocation map, i.e. so
@@ -370,7 +531,7 @@ void serve_init_allocation_map(struct server* params)
 	size = lseek64(fd, 0, SEEK_END);
 	params->size = size;
 	FATAL_IF_NEGATIVE(size, "Couldn't find size of %s", 
-	  params->filename);
+			params->filename);
 	params->allocation_map = 
 		build_allocation_map(fd, size, block_allocation_resolution);
 	close(fd);
@@ -399,17 +560,14 @@ void serve_cleanup(struct server* params, int fatal)
 		close(params->server_fd);
 	if (params->control_fd)
 		close(params->control_fd);
-	if (params->acl)
+	if (params->control_socket_name){ 
-		free(params->acl);
-	if (params->control_socket_name)
 		;
-	pthread_mutex_destroy(&params->l_io);
+	}
 	if (params->proxy_fd);
 		close(params->proxy_fd);
 
 	if (params->close_signal)
 		self_pipe_destroy( params->close_signal );
-
 	if (params->allocation_map)
 		free(params->allocation_map);
 	
@@ -435,13 +593,6 @@ void do_serve(struct server* params)
 	NULLCHECK( params );
 	
 	error_set_handler((cleanup_handler*) serve_cleanup, params);
-	pthread_mutex_init(&params->l_io, NULL);
-
-	params->close_signal = self_pipe_create();
-	if ( NULL == params->close_signal) { 
-		fatal( "close signal creation failed" ); 
-	}
-	
 	serve_open_server_socket(params);
 	serve_open_control_socket(params);
 	serve_init_allocation_map(params);

src/serve.h

@@ -3,14 +3,14 @@
 
 #define _GNU_SOURCE
 
-#ifndef _LARGEFILE64_SOURCE
+#define _LARGEFILE64_SOURCE
-# define _LARGEFILE64_SOURCE
+
-#endif
+#include <sys/types.h>
+#include <unistd.h>
 
 #include "parse.h"
 #include "acl.h"
 
-#include <sys/types.h>
 
 static const int block_allocation_resolution = 4096;//128<<10;
 
@@ -49,8 +49,6 @@ struct client_tbl_entry {
 struct server {
 	/** address/port to bind to */
 	union mysockaddr     bind_to;
-	/** access control list */
-	struct acl *         acl;
 	/** (static) file name to serve */
 	char*                filename;
 	/** file name of INCOMPLETE flag */
@@ -71,6 +69,14 @@ struct server {
 	/** to interrupt accept loop and clients, write() to close_signal[1] */
 	struct self_pipe *   close_signal;
 
+	/** access control list */
+	struct acl *         acl;
+	/** acl_updated_signal will be signalled after the acl struct
+	 * has been replaced
+	 */
+	struct self_pipe *   acl_updated_signal;
+	pthread_mutex_t      l_acl;
+
 	struct mirror_status* mirror;
 	int                  server_fd;
 	int                  control_fd;
@@ -80,11 +86,15 @@ struct server {
 	struct client_tbl_entry nbd_client[MAX_NBD_CLIENTS];
 };
 
+struct server * server_create( char* s_ip_address, char* s_port, char* s_file,
+	char *s_ctrl_sock, int default_deny, int acl_entries, char** s_acl_entries );
+void server_destroy( struct server * );
 int server_is_closed(struct server* serve);
 void server_dirty(struct server *serve, off64_t from, int len);
-int server_lock_io( struct server * serve);
+void server_lock_io( struct server * serve);
 void server_unlock_io( struct server* serve );
 void serve_signal_close( struct server *serve );
+void server_replace_acl( struct server *serve, struct acl * acl);
 
 
 struct mode_readwrite_params {

tests/check_self_pipe.c

@@ -69,6 +69,15 @@ START_TEST( test_signals )
 END_TEST
 
 
+START_TEST( test_clear_returns_immediately )
+{
+	struct self_pipe *sig;
+	sig =  self_pipe_create();
+	fail_unless( 0 == self_pipe_signal_clear( sig ), "Wrong clear result." );
+}
+END_TEST
+
+
 START_TEST( test_destroy_closes_read_pipe )
 {
 	struct self_pipe* sig;
@@ -170,6 +179,7 @@ Suite *self_pipe_suite()
 
 	tcase_add_test(tc_create, test_opens_pipe);
 	tcase_add_test(tc_signal, test_signals );
+	tcase_add_test(tc_signal, test_clear_returns_immediately );
 	tcase_add_test(tc_destroy, test_destroy_closes_read_pipe );
 	tcase_add_test(tc_destroy, test_destroy_closes_write_pipe );
 	/* We don't test that destroy free()'s the self_pipe pointer because

tests/check_serve.c

@@ -0,0 +1,228 @@
+#include "serve.h"
+#include "self_pipe.h"
+#include "client.h"
+
+#include <stdlib.h>
+#include <check.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netdb.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+
+char * dummy_file;
+
+char *make_tmpfile()
+{
+	FILE *fp;
+	char *fn_buf;
+	char leader[] = "/tmp/check_serve";
+
+	fn_buf = (char *)malloc( 1024 );
+	strncpy( fn_buf, leader, sizeof( leader ) - 1);
+	snprintf( &fn_buf[sizeof( leader ) - 1], 10, "%d", getpid() );
+	fp = fopen( fn_buf, "w" );
+	fwrite( fn_buf, 1024, 1, fp );
+	fclose( fp );
+
+	return fn_buf;
+}
+
+
+void setup( void )
+{
+	dummy_file = make_tmpfile();
+}
+
+void teardown( void )
+{
+	if( dummy_file ){ unlink( dummy_file ); }
+	free( dummy_file );
+	dummy_file = NULL;
+}
+
+/* Need these because libcheck is braindead and doesn't
+ * run teardown after a failing test
+ */
+#define myfail( msg ) do { teardown(); fail(msg); } while (0)
+#define myfail_if( tst, msg ) do { if( tst ) { myfail( msg ); } } while (0)
+#define myfail_unless( tst, msg ) myfail_if( !(tst), msg )
+
+
+START_TEST( test_replaces_acl )
+{
+	struct server * s = server_create( "127.0.0.1", "0", dummy_file, NULL, 0, 0, NULL );
+	struct acl * new_acl = acl_create( 0, NULL, 0 );
+
+	server_replace_acl( s, new_acl );
+
+	myfail_unless( s->acl == new_acl, "ACL wasn't replaced." );
+	server_destroy( s );
+}
+END_TEST
+
+
+START_TEST( test_signals_acl_updated )
+{
+	struct server * s = server_create( "127.0.0.1", "0", dummy_file, NULL, 0, 0, NULL );
+	struct acl * new_acl = acl_create( 0, NULL, 0 );
+
+	server_replace_acl( s, new_acl );
+
+	myfail_unless( 1 == self_pipe_signal_clear( s->acl_updated_signal ), 
+			"No signal sent." );
+	server_destroy( s );
+}
+END_TEST
+
+
+int connect_client( char *addr, int actual_port )
+{
+	int client_fd;
+
+	struct addrinfo hint = {0};
+	struct addrinfo *ailist, *aip;
+	hint.ai_socktype = SOCK_STREAM;
+
+	myfail_if( getaddrinfo( "127.0.0.7", NULL, &hint, &ailist ) != 0, "getaddrinfo failed." );
+	
+	int connected = 0;
+	for( aip = ailist; aip; aip = aip->ai_next ) {
+		((struct sockaddr_in *)aip->ai_addr)->sin_port = htons( actual_port );
+		client_fd = socket( aip->ai_family, aip->ai_socktype, aip->ai_protocol );
+		if( client_fd == -1) { continue; }
+		if( connect( client_fd, aip->ai_addr, aip->ai_addrlen) == 0 ) {
+			connected = 1;
+			break;
+		}
+		close( client_fd );
+	}
+
+	myfail_unless( connected, "Didn't connect." );
+	return client_fd;
+}
+
+
+START_TEST( test_acl_update_closes_bad_client )
+{
+	/* This is the wrong way round.  Rather than pulling the thread
+	 * and socket out of the server structure, we should be testing
+	 * a client socket.
+	 */
+	struct server * s = server_create( "127.0.0.7", "0", dummy_file, NULL, 0, 0, NULL );
+	struct acl * new_acl = acl_create( 0, NULL, 1 );
+	struct client * c;
+	struct client_tbl_entry * entry;
+
+	int actual_port; 
+	int client_fd;
+	int server_fd;
+
+
+	serve_open_server_socket( s );
+	actual_port = server_port( s );
+
+	client_fd = connect_client( "127.0.0.7", actual_port );
+	server_accept( s );
+	entry = &s->nbd_client[0];
+	c = entry->client;
+	/* At this point there should be an entry in the nbd_clients
+	 * table and a background thread to run the client loop
+	 */
+	myfail_if( entry->thread == 0, "No client thread was started." );
+	server_fd = c->socket;
+	myfail_if( fd_is_closed(server_fd), 
+			"Sanity check failed - client socket wasn't open." );
+
+	server_replace_acl( s, new_acl );
+
+	server_accept( s );
+
+	pthread_join( entry->thread );
+
+	myfail_unless( fd_is_closed(server_fd),  
+			"Client socket wasn't closed." );
+	close( client_fd );
+	server_close_clients( s );
+	server_destroy( s );
+}
+END_TEST
+
+
+START_TEST( test_acl_update_leaves_good_client )
+{
+	struct server * s = server_create( "127.0.0.7", "0", dummy_file, NULL, 0, 0, NULL );
+	/* There's an assumption here that the localhost *is* 127.0.0.1.
+	 * If it's not, this test will fail and we'll have to explicitly
+	 * pick a source address.
+	 */
+	char *lines[] = {"127.0.0.1"};
+	struct acl * new_acl = acl_create( 1, lines, 0);
+	struct client * c;
+	struct client_tbl_entry * entry;
+
+	int actual_port; 
+	int client_fd;
+	int server_fd;
+
+
+	serve_open_server_socket( s );
+	actual_port = server_port( s );
+
+	client_fd = connect_client( "127.0.0.7", actual_port );
+	server_accept( s );
+	entry = &s->nbd_client[0];
+	c = entry->client;
+	/* At this point there should be an entry in the nbd_clients
+	 * table and a background thread to run the client loop
+	 */
+	myfail_if( entry->thread == 0, "No client thread was started." );
+	server_fd = c->socket;
+	myfail_if( fd_is_closed(server_fd), 
+			"Sanity check failed - client socket wasn't open." );
+
+	server_replace_acl( s, new_acl );
+	server_accept( s );
+
+	myfail_if( self_pipe_signal_clear( c->stop_signal ), 
+			"Client was told to stop." );
+
+	close( client_fd );
+	server_close_clients( s );
+	server_destroy( s );
+}
+END_TEST
+
+
+Suite* serve_suite()
+{
+	Suite *s = suite_create("serve");
+	TCase *tc_acl_update = tcase_create("acl_update");
+	
+	tcase_add_checked_fixture( tc_acl_update, setup, teardown );
+	tcase_add_test(tc_acl_update, test_replaces_acl);
+	tcase_add_test(tc_acl_update, test_signals_acl_updated);
+	tcase_add_test(tc_acl_update, test_acl_update_closes_bad_client);
+
+	tcase_add_test(tc_acl_update, test_acl_update_leaves_good_client);
+
+	suite_add_tcase(s, tc_acl_update);
+
+	return s;
+}
+
+
+int main(void)
+{
+	set_debug(1);
+	int number_failed;
+	Suite *s = serve_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;
+}
+