[SERVER] More fine grained locking for RPC; better error logging

1 files changed, 45 insertions, 16 deletions

diff --git a/src/server/rpc.c b/src/server/rpc.c
index 48fc009..3ea8a9a 100644
--- a/src/server/rpc.c
+++ b/src/server/rpc.c
@@ -17,25 +17,30 @@ static void clientsToJson(json_t *jsonClients);
 
 void rpc_sendStatsJson(int sock)
 {
+	json_t *jsonClients = json_array();
+	clientsToJson( jsonClients );
 	const uint64_t bytesReceived = uplink_getTotalBytesReceived();
 	const uint64_t bytesSent = net_getTotalBytesSent();
 	const int uptime = dnbd3_serverUptime();
-	json_t *jsonClients = json_array();
-
-	clientsToJson( jsonClients );
 
-	json_t *statisticsJson = json_pack( "{sIsI}", "bytesReceived", (json_int_t) bytesReceived, "bytesSent", (json_int_t) bytesSent );
+	json_t *statisticsJson = json_pack( "{sIsI}",
+			"bytesReceived", (json_int_t) bytesReceived,
+			"bytesSent", (json_int_t) bytesSent );
 	json_object_set_new( statisticsJson, "clients", jsonClients );
 	json_object_set_new( statisticsJson, "images", image_getListAsJson() );
 	json_object_set_new( statisticsJson, "uptime", json_integer( uptime ) );
 	char *jsonString = json_dumps( statisticsJson, 0 );
+	json_decref( statisticsJson );
 
 	char buffer[500];
-	snprintf(buffer, sizeof buffer , "HTTP/1.1 200 OK\r\nConnection: Close\r\nContent-Length: %d\r\nContent-Type: application/json\r\n\r\n",
+	snprintf(buffer, sizeof buffer , "HTTP/1.1 200 OK\r\n"
+			"Connection: Close\r\n"
+			"Content-Length: %d\r\n"
+			"Content-Type: application/json\r\n"
+			"\r\n",
 			(int) strlen( jsonString ) );
 	write( sock, buffer, strlen( buffer ) );
 	sock_sendAll( sock, jsonString, strlen( jsonString ), 10 );
-	json_decref( statisticsJson );
 	// Wait for flush
 	shutdown( sock, SHUT_WR );
 	while ( read( sock, buffer, sizeof buffer ) > 0 );
@@ -46,19 +51,43 @@ static void clientsToJson(json_t *jsonClients)
 {
 	json_t *clientStats;
 	int i;
-	char clientName[100];
+	int imgId;
+	uint64_t bytesSent;
+	char host[HOSTNAMELEN];
+	host[HOSTNAMELEN-1] = '\0';
+
 	spin_lock( &_clients_lock );
-	for (i = 0; i < _num_clients; ++i) {
-		if ( _clients[i] == NULL ) continue;
-		spin_lock( &_clients[i]->lock );
-		if ( _clients[i]->image != NULL ) {
-			if ( !host_to_string( &_clients[i]->host, clientName, sizeof(clientName) ) ) {
-				strcpy( clientName, "???" );
-			}
-			clientStats = json_pack( "{sssisI}", "address", clientName, "imageId", _clients[i]->image->id, "bytesSent", (json_int_t)_clients[i]->bytesSent );
+	for ( i = 0; i < _num_clients; ++i ) {
+		if ( _clients[i] == NULL ) {
+			continue;
+		}
+		// Do not lock on client.lock here:
+		// 1) .image can only be set once, will never change (just like .image.id)
+		// 2) .hostName never changes as well
+		// 3) .bytesSent and .tmpBytesSent are guarded by .statsLock
+		// 4) the client cannot be freed, as it's still in the list and we hold the list's lock
+		if ( _clients[i]->image == NULL ) {
+			imgId = -1;
+		} else {
+			strncpy( host, _clients[i]->hostName, HOSTNAMELEN - 1 );
+			imgId = _clients[i]->image->id;
+			spin_lock( &_clients[i]->statsLock );
+			bytesSent = _clients[i]->bytesSent;
+			net_updateGlobalSentStatsFromClient( _clients[i] ); // Do this since we read the totalBytesSent counter later
+			spin_unlock( &_clients[i]->statsLock );
+		}
+		spin_unlock( &_clients_lock );
+		// Unlock so we give other threads a chance to access the client list.
+		// We might not get an atomic snapshot of the currently connected clients,
+		// but that doesn't really make a difference anyways.
+		if ( imgId != -1 ) {
+			clientStats = json_pack( "{sssisI}",
+					"address", host,
+					"imageId", imgId,
+					"bytesSent", (json_int_t)bytesSent );
 			json_array_append_new( jsonClients, clientStats );
 		}
-		spin_unlock( &_clients[i]->lock );
+		spin_lock( &_clients_lock );
 	}
 	spin_unlock( &_clients_lock );
 }