From d0439dab88545fd6a8646cdb0a51a9fa6432ff32 Mon Sep 17 00:00:00 2001 From: Patrick J Cherry Date: Fri, 16 Feb 2018 12:58:03 +0000 Subject: [PATCH 1/4] Call the thread cleanup code when requesting `status` This ensures the correct number of connected clients is returned when the status command is issued. Previously the thread pool would only be cleaned up on a new connection. --- src/server/serve.c | 2 ++ tests/acceptance/test_serve_mode.rb | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/src/server/serve.c b/src/server/serve.c index 3a86c3a..b7b8e44 100644 --- a/src/server/serve.c +++ b/src/server/serve.c @@ -332,6 +332,8 @@ int server_count_clients( struct server *params ) { NULLCHECK( params ); int i, count = 0; + + cleanup_client_threads( params->nbd_client, params->max_nbd_clients ); for ( i = 0 ; i < params->max_nbd_clients ; i++ ) { if ( params->nbd_client[i].thread != 0 ) { diff --git a/tests/acceptance/test_serve_mode.rb b/tests/acceptance/test_serve_mode.rb index 6c3e3f6..5bc3eb5 100644 --- a/tests/acceptance/test_serve_mode.rb +++ b/tests/acceptance/test_serve_mode.rb @@ -207,4 +207,14 @@ class TestServeMode < Test::Unit::TestCase 'TCP keepalive count not set to 3') end end + + def test_status_returns_correct_client_count + require 'pp' + connect_to_server do |client| + status = @env.status1 + assert_equal('1', status['num_clients']) + end + status = @env.status1 + assert_equal('0', status['num_clients']) + end end From 1407407ff493287fb9fef8dbbf97dfe47cca0fce Mon Sep 17 00:00:00 2001 From: Patrick J Cherry Date: Fri, 16 Feb 2018 13:00:31 +0000 Subject: [PATCH 2/4] Updated changelog --- debian/changelog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/debian/changelog b/debian/changelog index 8d6029f..63afd7c 100644 --- a/debian/changelog +++ b/debian/changelog @@ -20,6 +20,8 @@ flexnbd (0.2.0) UNRELEASED; urgency=medium calculating if a request exceeds the max block size (!45) * Added tests for setting TCP_NODELAY on upstream-reconnections in the proxy, and refactored the other LD_PRELOAD tests (!43) + * Clean up dead threads before calculating the number of connected clients + on the status command (!46) -- James Carter Thu, 11 Jan 2018 10:05:35 +0000 From 27a94a807e16876de7be8efc049af2c922e2af34 Mon Sep 17 00:00:00 2001 From: Patrick J Cherry Date: Fri, 16 Feb 2018 13:46:31 +0000 Subject: [PATCH 3/4] Remove the test_gets_num_clients test from the C unit tests This test was causing problems by using dummy pointers to simulate connections. When calling the cleanup code, these pointers were thought to be real, and the code attemtped to clean up threads referenced by those pointers, causing a segfault. I've reimplemented the test in the ruby acceptance suite. --- tests/acceptance/test_serve_mode.rb | 17 ++++++++++------- tests/unit/check_status.c | 19 ------------------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/tests/acceptance/test_serve_mode.rb b/tests/acceptance/test_serve_mode.rb index 5bc3eb5..ddeb836 100644 --- a/tests/acceptance/test_serve_mode.rb +++ b/tests/acceptance/test_serve_mode.rb @@ -209,12 +209,15 @@ class TestServeMode < Test::Unit::TestCase end def test_status_returns_correct_client_count - require 'pp' - connect_to_server do |client| - status = @env.status1 - assert_equal('1', status['num_clients']) - end - status = @env.status1 - assert_equal('0', status['num_clients']) + @env.writefile1('0') + @env.serve1 + assert_equal('0', @env.status1['num_clients']) + client = FlexNBD::FakeSource.new(@env.ip, @env.port1, 'Connecting to server failed') + assert_equal('1', @env.status1['num_clients']) + client2 = FlexNBD::FakeSource.new(@env.ip, @env.port1, 'Connecting to server failed') + assert_equal('2', @env.status1['num_clients']) + client2.close + client.close + assert_equal('0', @env.status1['num_clients']) end end diff --git a/tests/unit/check_status.c b/tests/unit/check_status.c index 1daf556..1a7962b 100644 --- a/tests/unit/check_status.c +++ b/tests/unit/check_status.c @@ -105,25 +105,6 @@ START_TEST( test_gets_clients_allowed ) } END_TEST -START_TEST( test_gets_num_clients ) -{ - struct server * server = mock_server(); - struct status * status = status_create( server ); - - fail_if( status->num_clients != 0, "num_clients was wrong" ); - status_destroy( status ); - - server->nbd_client[0].thread = 1; - server->nbd_client[1].thread = 1; - status = status_create( server ); - - fail_unless( status->num_clients == 2, "num_clients was wrong" ); - status_destroy( status ); - destroy_mock_server( server ); - -} -END_TEST - START_TEST( test_gets_pid ) { struct server * server = mock_server(); From 623007bfffa00aa5bf068834c550806ae9ee317f Mon Sep 17 00:00:00 2001 From: Patrick J Cherry Date: Mon, 19 Feb 2018 10:22:01 +0000 Subject: [PATCH 4/4] Remove last reference to removed test_gets_num_clients --- tests/unit/check_status.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/check_status.c b/tests/unit/check_status.c index 1a7962b..ea89d3b 100644 --- a/tests/unit/check_status.c +++ b/tests/unit/check_status.c @@ -338,7 +338,6 @@ Suite *status_suite(void) tcase_add_test(tc_create, test_gets_has_control); tcase_add_test(tc_create, test_gets_is_mirroring); tcase_add_test(tc_create, test_gets_clients_allowed); - tcase_add_test(tc_create, test_gets_num_clients); tcase_add_test(tc_create, test_gets_pid); tcase_add_test(tc_create, test_gets_size); tcase_add_test(tc_create, test_gets_migration_statistics);