From 5fb0cd4cca6fdb20867714d3d833fc2c51fb8620 Mon Sep 17 00:00:00 2001 From: Alex Young Date: Fri, 8 Jun 2012 10:11:06 +0100 Subject: [PATCH 1/4] Fix O_NONBLOCK setting on self_pipes --- src/self_pipe.c | 7 ++++--- tests/check_self_pipe.c | 10 ++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/self_pipe.c b/src/self_pipe.c index 71861fe..dddcb45 100644 --- a/src/self_pipe.c +++ b/src/self_pipe.c @@ -63,7 +63,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 ); @@ -99,7 +99,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. */ @@ -107,7 +108,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 ); } diff --git a/tests/check_self_pipe.c b/tests/check_self_pipe.c index f1d04a0..fed86cd 100644 --- a/tests/check_self_pipe.c +++ b/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 From f7e1a098b1183a335182ea599c5aaf1ceebd479e Mon Sep 17 00:00:00 2001 From: Alex Young Date: Fri, 8 Jun 2012 10:32:33 +0100 Subject: [PATCH 2/4] Move updating the acl object into serve.c * * * Replacing the server acl sends an acl_updated signal --- Rakefile | 17 +++++++++++- src/control.c | 3 +-- src/serve.c | 21 ++++++++++++--- src/serve.h | 13 ++++++--- tests/check_serve.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 tests/check_serve.c diff --git a/Rakefile b/Rakefile index 590db27..ba5c813 100644 --- a/Rakefile +++ b/Rakefile @@ -96,8 +96,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" diff --git a/src/control.c b/src/control.c index 67e7c30..8c36212 100644 --- a/src/control.c +++ b/src/control.c @@ -273,8 +273,7 @@ int control_acl(struct control_params* client, int linesc, char** lines) acl_destroy( new_acl ); } else { - client->serve->acl = new_acl; - acl_destroy( old_acl ); + server_replace_acl( client->serve, new_acl ); write_socket("0: updated"); } diff --git a/src/serve.c b/src/serve.c index b6e5000..384780b 100644 --- a/src/serve.c +++ b/src/serve.c @@ -314,6 +314,20 @@ void server_close_clients( struct server *params ) } +void server_replace_acl( struct server *serve, struct acl * new_acl ) +{ + NULLCHECK(serve); + NULLCHECK(new_acl); + + struct acl * old_acl = serve->acl; + + serve->acl = new_acl; + + if ( old_acl ) { acl_destroy( old_acl ); } + self_pipe_signal( serve->acl_updated_signal ); +} + + /** Accept either an NBD or control socket connection, dispatch appropriately */ void serve_accept_loop(struct server* params) { @@ -402,6 +416,7 @@ void serve_cleanup(struct server* params) close(params->proxy_fd); self_pipe_destroy( params->close_signal ); + self_pipe_destroy( params->acl_updated_signal ); free(params->allocation_map); @@ -426,9 +441,9 @@ void do_serve(struct server* params) pthread_mutex_init(¶ms->l_io, NULL); params->close_signal = self_pipe_create(); - if ( NULL == params->close_signal) { - SERVER_ERROR( "close signal creation failed" ); - } + NULLCHECK( params->close_signal ); + params->acl_updated_signal = self_pipe_create(); + NULLCHECK( params->acl_updated_signal ); serve_open_server_socket(params); serve_open_control_socket(params); diff --git a/src/serve.h b/src/serve.h index f96222b..3d0d4c4 100644 --- a/src/serve.h +++ b/src/serve.h @@ -3,14 +3,14 @@ #define _GNU_SOURCE -#ifndef _LARGEFILE64_SOURCE -# define _LARGEFILE64_SOURCE -#endif +#define _LARGEFILE64_SOURCE + +#include +#include #include "parse.h" #include "acl.h" -#include static const int block_allocation_resolution = 4096;//128<<10; @@ -69,6 +69,11 @@ struct server { /** to interrupt accept loop and clients, write() to close_signal[1] */ struct self_pipe * close_signal; + /** acl_updated_signal will be signalled after the acl struct + * has been replaced + */ + struct self_pipe * acl_updated_signal; + struct mirror_status* mirror; int server_fd; int control_fd; diff --git a/tests/check_serve.c b/tests/check_serve.c new file mode 100644 index 0000000..b14c93d --- /dev/null +++ b/tests/check_serve.c @@ -0,0 +1,65 @@ +#include "serve.h" +#include "self_pipe.h" + +#include +#include + + +START_TEST( test_replaces_acl ) +{ + struct server s; + s.acl_updated_signal = self_pipe_create(); + struct acl * acl = acl_create( 0, NULL, 0 ); + + server_replace_acl( &s, acl ); + + fail_unless( s.acl == acl, "ACL wasn't replaced." ); + self_pipe_destroy( s.acl_updated_signal ); +} +END_TEST + + +START_TEST( test_signals_acl_updated ) +{ + struct server s; + struct acl * new_acl = acl_create( 0, NULL, 0 ); + + s.acl_updated_signal = self_pipe_create(); + s.acl = acl_create( 0, NULL, 0); + + + server_replace_acl( &s, new_acl ); + + fail_unless( 1 == self_pipe_signal_clear( s.acl_updated_signal ), + "No signal sent." ); + self_pipe_destroy( s.acl_updated_signal ); +} +END_TEST + + +Suite* serve_suite() +{ + Suite *s = suite_create("serve"); + TCase *tc_acl_update = tcase_create("acl_update"); + + tcase_add_test(tc_acl_update, test_replaces_acl); + tcase_add_test(tc_acl_update, test_signals_acl_updated); + + 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; +} + From 35ca93b42cb1796d32ea070b2128d2214658606a Mon Sep 17 00:00:00 2001 From: Alex Young Date: Fri, 8 Jun 2012 11:02:40 +0100 Subject: [PATCH 3/4] Lock around acl updates --- src/serve.c | 71 ++++++++++++++++++++++++++++++--------------- src/serve.h | 7 +++-- tests/check_serve.c | 7 +++-- 3 files changed, 56 insertions(+), 29 deletions(-) diff --git a/src/serve.c b/src/serve.c index 384780b..96a74ff 100644 --- a/src/serve.c +++ b/src/serve.c @@ -42,29 +42,34 @@ 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 ); \ + SERVER_ERROR_ON_FAILURE( pthread_mutex_lock( &s->f ), msg ); } +#define SERVER_UNLOCK( s, f, msg ) \ + { NULLCHECK( s ); \ + SERVER_ERROR_ON_FAILURE( pthread_mutex_unlock( &s->f ), msg ); } + +void server_lock_io( struct server * serve) { - NULLCHECK( serve ); - - SERVER_ERROR_ON_FAILURE( - pthread_mutex_lock(&serve->l_io), - "Problem with I/O lock" - ); - - return 1; + SERVER_LOCK( serve, l_io, "Problem with I/O lock" ); } - void server_unlock_io( struct server* serve ) { - NULLCHECK( serve ); - - SERVER_ERROR_ON_FAILURE( - pthread_mutex_unlock(&serve->l_io), - "Problem with I/O unlock" - ); + SERVER_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" ); +} + + /** Prepares a listening socket for the NBD server, binding etc. */ void serve_open_server_socket(struct server* params) { @@ -194,16 +199,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) { - return acl_includes( params->acl, client_address ); - } + struct acl * acl; + 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; } @@ -319,11 +334,18 @@ void server_replace_acl( struct server *serve, struct acl * new_acl ) NULLCHECK(serve); NULLCHECK(new_acl); - struct acl * old_acl = serve->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 ); - serve->acl = new_acl; - - if ( old_acl ) { acl_destroy( old_acl ); } self_pipe_signal( serve->acl_updated_signal ); } @@ -439,6 +461,7 @@ void do_serve(struct server* params) NULLCHECK( params ); pthread_mutex_init(¶ms->l_io, NULL); + pthread_mutex_init(¶ms->l_acl, NULL); params->close_signal = self_pipe_create(); NULLCHECK( params->close_signal ); diff --git a/src/serve.h b/src/serve.h index 3d0d4c4..35118c7 100644 --- a/src/serve.h +++ b/src/serve.h @@ -47,8 +47,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 */ @@ -69,10 +67,13 @@ 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; @@ -85,7 +86,7 @@ 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 ); diff --git a/tests/check_serve.c b/tests/check_serve.c index b14c93d..823bcfb 100644 --- a/tests/check_serve.c +++ b/tests/check_serve.c @@ -7,8 +7,10 @@ START_TEST( test_replaces_acl ) { - struct server s; + struct server s = {0}; s.acl_updated_signal = self_pipe_create(); + pthread_mutex_init( &s.l_acl, NULL ); + struct acl * acl = acl_create( 0, NULL, 0 ); server_replace_acl( &s, acl ); @@ -21,10 +23,11 @@ END_TEST START_TEST( test_signals_acl_updated ) { - struct server s; + struct server s = {0}; struct acl * new_acl = acl_create( 0, NULL, 0 ); s.acl_updated_signal = self_pipe_create(); + pthread_mutex_init( &s.l_acl, NULL ); s.acl = acl_create( 0, NULL, 0); From b7096ef9081b8cd390f54e72a0c98914f8ea04c8 Mon Sep 17 00:00:00 2001 From: Alex Young Date: Fri, 8 Jun 2012 18:03:41 +0100 Subject: [PATCH 4/4] Audit client connections on acl update --- src/client.c | 3 +- src/client.h | 1 + src/flexnbd.c | 50 +--------- src/serve.c | 216 +++++++++++++++++++++++++++++++++----------- src/serve.h | 4 + tests/check_serve.c | 194 +++++++++++++++++++++++++++++++++++---- 6 files changed, 352 insertions(+), 116 deletions(-) diff --git a/src/client.c b/src/client.c index 092689f..ad68f6e 100644 --- a/src/client.c +++ b/src/client.c @@ -171,8 +171,9 @@ int client_read_request( struct client * client , struct nbd_request *out_reques CLIENT_ERROR_ON_FAILURE(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) { diff --git a/src/client.h b/src/client.h index 90a6bee..eccdcc1 100644 --- a/src/client.h +++ b/src/client.h @@ -1,6 +1,7 @@ #ifndef CLIENT_H #define CLIENT_H + struct client { int socket; diff --git a/src/flexnbd.c b/src/flexnbd.c index b334594..d6e41e7 100644 --- a/src/flexnbd.c +++ b/src/flexnbd.c @@ -45,48 +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? */ - - if (s_ip_address == NULL) - SERVER_ERROR("No IP address supplied"); - if (s_port == NULL) - SERVER_ERROR("No port number supplied"); - if (s_file == NULL) - SERVER_ERROR("No filename supplied"); - - if (parse_ip_to_sockaddr(&out->bind_to.generic, s_ip_address) == 0) - SERVER_ERROR("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) - SERVER_ERROR("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) - SERVER_ERROR("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: * params_read( struct mode_readwrite_params* out, @@ -295,7 +253,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); @@ -315,9 +273,9 @@ int mode_serve( int argc, char *argv[] ) } if ( err ) { exit_err( serve_help_text ); } - memset( &serve, 0, sizeof( serve ) ); - params_serve( &serve, ip_addr, ip_port, file, sock, default_deny, argc - optind, argv + optind ); - do_serve( &serve ); + serve = server_create( ip_addr, ip_port, file, sock, default_deny, argc - optind, argv + optind ); + do_serve( serve ); + server_destroy( serve ); return 0; } diff --git a/src/serve.c b/src/serve.c index 96a74ff..83f19c5 100644 --- a/src/serve.c +++ b/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) + SERVER_ERROR("No IP address supplied"); + if (s_port == NULL) + SERVER_ERROR("No port number supplied"); + if (s_file == NULL) + SERVER_ERROR("No filename supplied"); + + if (parse_ip_to_sockaddr(&out->bind_to.generic, s_ip_address) == 0) + SERVER_ERROR("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) + SERVER_ERROR("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) + SERVER_ERROR("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 ); @@ -70,6 +139,23 @@ void server_unlock_acl( struct server *serve ) } +/** 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 ) { + SERVER_ERROR( "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) { @@ -105,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 **) ) { @@ -290,7 +378,7 @@ void accept_nbd_client( memcpy(¶ms->nbd_client[slot].address, client_address, sizeof(union mysockaddr)); - if (pthread_create(¶ms->nbd_client[slot].thread, NULL, client_serve, client_params) < 0) { + if (pthread_create(¶ms->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 ); @@ -302,6 +390,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 ); @@ -329,6 +441,9 @@ 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); @@ -351,43 +466,54 @@ void server_replace_acl( struct server *serve, struct acl * new_acl ) /** 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 ); - while (1) { - int activity_fd, client_fd; - union mysockaddr client_address; - fd_set fds; - socklen_t socklen=sizeof(client_address); - - FD_ZERO(&fds); - FD_SET(params->server_fd, &fds); - self_pipe_fd_set( params->close_signal, &fds ); - if (params->control_socket_name) - FD_SET(params->control_fd, &fds); - - SERVER_ERROR_ON_FAILURE(select(FD_SETSIZE, &fds, - NULL, NULL, NULL), "select() failed"); - - if ( self_pipe_fd_isset( params->close_signal, &fds ) ){ - server_close_clients( params ); - return; - } - - 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); - } - + + int activity_fd, client_fd; + union mysockaddr client_address; + fd_set fds; + socklen_t socklen=sizeof(client_address); + + FD_ZERO(&fds); + FD_SET(params->server_fd, &fds); + self_pipe_fd_set( params->close_signal, &fds ); + self_pipe_fd_set( params->acl_updated_signal, &fds ); + if (params->control_socket_name) + FD_SET(params->control_fd, &fds); + + SERVER_ERROR_ON_FAILURE(select(FD_SETSIZE, &fds, + NULL, NULL, NULL), "select() failed"); + + if ( self_pipe_fd_isset( params->close_signal, &fds ) ){ + server_close_clients( params ); + return 0; } + + 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 @@ -404,7 +530,7 @@ void serve_init_allocation_map(struct server* params) size = lseek64(fd, 0, SEEK_END); params->size = size; SERVER_ERROR_ON_FAILURE(size, "Couldn't find size of %s", - params->filename); + params->filename); params->allocation_map = build_allocation_map(fd, size, block_allocation_resolution); close(fd); @@ -428,18 +554,12 @@ void serve_cleanup(struct server* params) close(params->server_fd); close(params->control_fd); - if (params->acl) - free(params->acl); - //free(params->filename); - if (params->control_socket_name) + if (params->control_socket_name){ //free(params->control_socket_name); - pthread_mutex_destroy(¶ms->l_io); + } if (params->proxy_fd); close(params->proxy_fd); - self_pipe_destroy( params->close_signal ); - self_pipe_destroy( params->acl_updated_signal ); - free(params->allocation_map); if (params->mirror) @@ -460,14 +580,6 @@ void do_serve(struct server* params) { NULLCHECK( params ); - pthread_mutex_init(¶ms->l_io, NULL); - pthread_mutex_init(¶ms->l_acl, NULL); - - params->close_signal = self_pipe_create(); - NULLCHECK( params->close_signal ); - params->acl_updated_signal = self_pipe_create(); - NULLCHECK( params->acl_updated_signal ); - serve_open_server_socket(params); serve_open_control_socket(params); serve_init_allocation_map(params); diff --git a/src/serve.h b/src/serve.h index 35118c7..210a8cb 100644 --- a/src/serve.h +++ b/src/serve.h @@ -84,11 +84,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); 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 { diff --git a/tests/check_serve.c b/tests/check_serve.c index 823bcfb..8877d20 100644 --- a/tests/check_serve.c +++ b/tests/check_serve.c @@ -1,41 +1,197 @@ #include "serve.h" #include "self_pipe.h" +#include "client.h" +#include #include #include +#include +#include +#include +#include +#include + + +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 = {0}; - s.acl_updated_signal = self_pipe_create(); - pthread_mutex_init( &s.l_acl, NULL ); + 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 ); - struct acl * acl = acl_create( 0, NULL, 0 ); + server_replace_acl( s, new_acl ); - server_replace_acl( &s, acl ); - - fail_unless( s.acl == acl, "ACL wasn't replaced." ); - self_pipe_destroy( s.acl_updated_signal ); + myfail_unless( s->acl == new_acl, "ACL wasn't replaced." ); + server_destroy( s ); } END_TEST START_TEST( test_signals_acl_updated ) { - struct server s = {0}; + 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 ); - s.acl_updated_signal = self_pipe_create(); - pthread_mutex_init( &s.l_acl, NULL ); - s.acl = acl_create( 0, NULL, 0); + server_replace_acl( s, new_acl ); - - server_replace_acl( &s, new_acl ); - - fail_unless( 1 == self_pipe_signal_clear( s.acl_updated_signal ), + myfail_unless( 1 == self_pipe_signal_clear( s->acl_updated_signal ), "No signal sent." ); - self_pipe_destroy( s.acl_updated_signal ); + 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 @@ -45,8 +201,12 @@ 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);