lease-server: Allow multiple client connections

Allow multiple clients to issue lease requests
on a server at the same time. This is
necessary to be able to grant or deny leases,
not just on a first-come-first-served basis.

Future patches will add extra contitions,
such as command-line options and lease configuration
settings to determine when and how lease requests should
be granted.

This update changes the behaviour of the lease-server interface
so that it reports every client connection request, instead of
when a server has accepted a request, so update the test suite to
reflect this.

Bug-AGL: SPEC-3816

Change-Id: I48cc392dd62a8c06ea74178bc52c627032817203
Signed-off-by: Damian Hobson-Garcia <dhobsong@igel.co.jp>

5 files changed, 144 insertions, 136 deletions

diff --git a/drm-lease-manager/lease-manager.c b/drm-lease-manager/lease-manager.c
index 5cfc5de..016f318 100644
--- a/drm-lease-manager/lease-manager.c
+++ b/drm-lease-manager/lease-manager.c
@@ -354,8 +354,10 @@ int lm_lease_grant(struct lm *lm, struct lease_handle *handle)
 	assert(handle);
 
 	struct lease *lease = (struct lease *)handle;
-	if (lease->is_granted)
-		return lease->lease_fd;
+	if (lease->is_granted) {
+		/* Lease is already claimed */
+		return -1;
+	}
 
 	lease->lease_fd =
 	    drmModeCreateLease(lm->drm_fd, lease->object_ids,
diff --git a/drm-lease-manager/lease-server.c b/drm-lease-manager/lease-server.c
index e05e8e4..c57316e 100644
--- a/drm-lease-manager/lease-server.c
+++ b/drm-lease-manager/lease-server.c
@@ -32,9 +32,31 @@
 
 #define SOCK_LOCK_SUFFIX ".lock"
 
+/* ACTIVE_CLIENTS
+ * An 'active' client is one that either
+ *  - owns a lease, or
+ *  - is requesting ownership of a lease (which will
+ *    disconnect the current owner if granted)
+ *
+ * There can only be at most one of each kind of client at the same
+ * time. Any other client connections are queued in the
+ * listen() backlog, waiting to be accept()'ed.
+ */
+#define ACTIVE_CLIENTS 2
+
 struct ls_socket {
 	int fd;
+	bool is_server;
+	union {
+		struct ls_server *server;
+		struct ls_client *client;
+	};
+};
+
+struct ls_client {
+	struct ls_socket socket;
 	struct ls_server *serv;
+	bool is_connected;
 };
 
 struct ls_server {
@@ -43,9 +65,7 @@ struct ls_server {
 	int server_socket_lock;
 
 	struct ls_socket listen;
-	struct ls_socket client;
-
-	bool is_client_connected;
+	struct ls_client clients[ACTIVE_CLIENTS];
 };
 
 struct ls {
@@ -55,37 +75,42 @@ struct ls {
 	int nservers;
 };
 
-static bool client_connect(struct ls *ls, struct ls_server *serv)
+static struct ls_client *client_connect(struct ls *ls, struct ls_server *serv)
 {
 	int cfd = accept(serv->listen.fd, NULL, NULL);
 	if (cfd < 0) {
 		DEBUG_LOG("accept failed on %s: %s\n", serv->address.sun_path,
 			  strerror(errno));
-		return false;
+		return NULL;
 	}
 
-	if (serv->is_client_connected) {
-		WARN_LOG("Client already connected on %s\n",
-			 serv->address.sun_path);
+	struct ls_client *client = NULL;
+
+	for (int i = 0; i < ACTIVE_CLIENTS; i++) {
+		if (!serv->clients[i].is_connected) {
+			client = &serv->clients[i];
+			break;
+		}
+	}
+	if (!client) {
 		close(cfd);
-		return false;
+		return NULL;
 	}
 
-	serv->client.fd = cfd;
-	serv->client.serv = serv;
+	client->socket.fd = cfd;
 
 	struct epoll_event ev = {
 	    .events = POLLHUP,
-	    .data.ptr = &serv->client,
+	    .data.ptr = &client->socket,
 	};
 	if (epoll_ctl(ls->epoll_fd, EPOLL_CTL_ADD, cfd, &ev)) {
 		DEBUG_LOG("epoll_ctl add failed: %s\n", strerror(errno));
 		close(cfd);
-		return false;
+		return NULL;
 	}
 
-	serv->is_client_connected = true;
-	return true;
+	client->is_connected = true;
+	return client;
 }
 
 static int create_socket_lock(struct sockaddr_un *addr)
@@ -161,11 +186,18 @@ static bool server_setup(struct ls *ls, struct ls_server *serv,
 		return false;
 	}
 
-	serv->is_client_connected = false;
+	for (int i = 0; i < ACTIVE_CLIENTS; i++) {
+		struct ls_client *client = &serv->clients[i];
+		client->serv = serv;
+		client->socket.client = client;
+	}
+
 	serv->lease_handle = lease_handle;
 	serv->server_socket_lock = socket_lock;
+
 	serv->listen.fd = server_socket;
-	serv->listen.serv = serv;
+	serv->listen.server = serv;
+	serv->listen.is_server = true;
 
 	struct epoll_event ev = {
 	    .events = POLLIN,
@@ -193,7 +225,10 @@ static void server_shutdown(struct ls *ls, struct ls_server *serv)
 
 	epoll_ctl(ls->epoll_fd, EPOLL_CTL_DEL, serv->listen.fd, NULL);
 	close(serv->listen.fd);
-	ls_disconnect_client(ls, serv);
+
+	for (int i = 0; i < ACTIVE_CLIENTS; i++)
+		ls_disconnect_client(ls, &serv->clients[i]);
+
 	close(serv->server_socket_lock);
 }
 
@@ -261,32 +296,37 @@ bool ls_get_request(struct ls *ls, struct ls_req *req)
 		struct ls_socket *sock = ev.data.ptr;
 		assert(sock);
 
-		struct ls_server *server = sock->serv;
-		req->lease_handle = server->lease_handle;
-		req->server = server;
+		struct ls_server *server;
+		struct ls_client *client;
 
-		if (sock == &server->listen) {
+		if (sock->is_server) {
 			if (!(ev.events & POLLIN))
 				continue;
-			if (client_connect(ls, server))
+
+			server = sock->server;
+			client = client_connect(ls, server);
+			if (client)
 				request = LS_REQ_GET_LEASE;
-		} else if (sock == &server->client) {
+		} else {
 			if (!(ev.events & POLLHUP))
 				continue;
+
+			client = sock->client;
+			server = client->serv;
 			request = LS_REQ_RELEASE_LEASE;
-		} else {
-			ERROR_LOG("Internal error: Invalid socket context\n");
-			return false;
 		}
+
+		req->lease_handle = server->lease_handle;
+		req->client = client;
+		req->type = request;
 	}
-	req->type = request;
 	return true;
 }
 
-bool ls_send_fd(struct ls *ls, struct ls_server *serv, int fd)
+bool ls_send_fd(struct ls *ls, struct ls_client *client, int fd)
 {
 	assert(ls);
-	assert(serv);
+	assert(client);
 
 	if (fd < 0)
 		return false;
@@ -312,7 +352,9 @@ bool ls_send_fd(struct ls *ls, struct ls_server *serv, int fd)
 	cmsg->cmsg_len = CMSG_LEN(sizeof(int));
 	*((int *)CMSG_DATA(cmsg)) = fd;
 
-	if (sendmsg(serv->client.fd, &msg, 0) < 0) {
+	struct ls_server *serv = client->serv;
+
+	if (sendmsg(client->socket.fd, &msg, 0) < 0) {
 		DEBUG_LOG("sendmsg failed on %s: %s\n", serv->address.sun_path,
 			  strerror(errno));
 		return false;
@@ -322,14 +364,15 @@ bool ls_send_fd(struct ls *ls, struct ls_server *serv, int fd)
 	return true;
 }
 
-void ls_disconnect_client(struct ls *ls, struct ls_server *serv)
+void ls_disconnect_client(struct ls *ls, struct ls_client *client)
 {
 	assert(ls);
-	assert(serv);
-	if (!serv->is_client_connected)
+	assert(client);
+
+	if (!client->is_connected)
 		return;
 
-	epoll_ctl(ls->epoll_fd, EPOLL_CTL_DEL, serv->client.fd, NULL);
-	close(serv->client.fd);
-	serv->is_client_connected = false;
+	epoll_ctl(ls->epoll_fd, EPOLL_CTL_DEL, client->socket.fd, NULL);
+	close(client->socket.fd);
+	client->is_connected = false;
 }
diff --git a/drm-lease-manager/lease-server.h b/drm-lease-manager/lease-server.h
index c87b834..1aaad30 100644
--- a/drm-lease-manager/lease-server.h
+++ b/drm-lease-manager/lease-server.h
@@ -20,7 +20,7 @@
 #include "drm-lease.h"
 
 struct ls;
-struct ls_server;
+struct ls_client;
 enum ls_req_type {
 	LS_REQ_GET_LEASE,
 	LS_REQ_RELEASE_LEASE,
@@ -28,7 +28,7 @@ enum ls_req_type {
 
 struct ls_req {
 	struct lease_handle *lease_handle;
-	struct ls_server *server;
+	struct ls_client *client;
 	enum ls_req_type type;
 };
 
@@ -36,7 +36,7 @@ struct ls *ls_create(struct lease_handle **lease_handles, int count);
 void ls_destroy(struct ls *ls);
 
 bool ls_get_request(struct ls *ls, struct ls_req *req);
-bool ls_send_fd(struct ls *ls, struct ls_server *server, int fd);
+bool ls_send_fd(struct ls *ls, struct ls_client *client, int fd);
 
-void ls_disconnect_client(struct ls *ls, struct ls_server *server);
+void ls_disconnect_client(struct ls *ls, struct ls_client *client);
 #endif
diff --git a/drm-lease-manager/main.c b/drm-lease-manager/main.c
index c3a933f..491c80c 100644
--- a/drm-lease-manager/main.c
+++ b/drm-lease-manager/main.c
@@ -91,21 +91,21 @@ int main(int argc, char **argv)
 				ERROR_LOG(
 				    "Can't fulfill lease request: lease=%s\n",
 				    req.lease_handle->name);
-				ls_disconnect_client(ls, req.server);
+				ls_disconnect_client(ls, req.client);
 				break;
 			}
 
-			if (!ls_send_fd(ls, req.server, fd)) {
+			if (!ls_send_fd(ls, req.client, fd)) {
 				ERROR_LOG(
 				    "Client communication error: lease=%s\n",
 				    req.lease_handle->name);
-				ls_disconnect_client(ls, req.server);
+				ls_disconnect_client(ls, req.client);
 				lm_lease_revoke(lm, req.lease_handle);
 			}
 			break;
 		}
 		case LS_REQ_RELEASE_LEASE:
-			ls_disconnect_client(ls, req.server);
+			ls_disconnect_client(ls, req.client);
 			lm_lease_revoke(lm, req.lease_handle);
 			break;
 		default:
diff --git a/drm-lease-manager/test/lease-server-test.c b/drm-lease-manager/test/lease-server-test.c
index 2672820..6a4d26c 100644
--- a/drm-lease-manager/test/lease-server-test.c
+++ b/drm-lease-manager/test/lease-server-test.c
@@ -28,6 +28,8 @@
 
 #define SOCKETDIR "/tmp"
 
+#define ARRAY_LENGTH(x) sizeof(x) / sizeof(x[0])
+
 /************** Test fixutre functions *************************/
 struct test_config default_test_config;
 
@@ -128,59 +130,6 @@ static void get_and_check_request(struct ls *ls,
 	check_request(&req, expected_lease, expected_type);
 }
 
-/* Asynchronous version of the above.  Has the extra overhead of
- * spawning a new thread, so should be used sparingly. */
-struct async_req {
-	pthread_t tid;
-	struct ls *ls;
-
-	bool req_valid;
-	struct ls_req expected;
-	struct ls_req actual;
-};
-
-static void *get_request_thread(void *arg)
-{
-	struct async_req *async_req = arg;
-	async_req->req_valid =
-	    ls_get_request(async_req->ls, &async_req->actual);
-
-	return NULL;
-}
-
-static struct async_req *
-get_and_check_request_async(struct ls *ls, struct lease_handle *expected_lease,
-			    enum ls_req_type expected_type)
-
-{
-	struct async_req *req = malloc(sizeof(struct async_req));
-	ck_assert_ptr_ne(req, NULL);
-
-	*req = (struct async_req){
-	    .ls = ls,
-	    .expected =
-		{
-		    .lease_handle = expected_lease,
-		    .type = expected_type,
-		},
-	};
-
-	int ret = pthread_create(&req->tid, NULL, get_request_thread, req);
-	ck_assert_int_eq(ret, 0);
-
-	return req;
-}
-
-static void check_async_req_result(struct async_req *req)
-{
-
-	pthread_join(req->tid, NULL);
-	ck_assert_int_eq(req->req_valid, true);
-	check_request(&req->actual, req->expected.lease_handle,
-		      req->expected.type);
-	free(req);
-}
-
 /* issue_lease_request_and_release
  *
  * Test details: Generate a lease request and lease release command from
@@ -225,52 +174,66 @@ END_TEST
  *
  * Test details: Generate multiple lease requests to the same lease server from
  *               multiple clients at the same time
- * Expected results: One get lease and one release lease request are returned
- *                   from ls_get_request().
- *                   Requests from all but the first client are rejected
- *                   (sockets are closed).
+ * Expected results: One lease request per client is returned from
+ *                   ls_get_request().
+ *                   (Test will process each connection in series and either
+ *                    keep the current connection or switch a new one.)
+ *                   Only one client remains connected at the end of the test,
+ *                   and it returns a validlease release request.
  */
 START_TEST(issue_multiple_lease_requests)
 {
+	/* List of which client connections to accept.
+	 * If the nth element is `false` that client request will be
+	 * rejected, and should be reflected in the final configuration
+	 * state */
+	bool keep_current_client[] = {false, true, true, false, true};
+
 	struct lease_handle *leases[] = {
 	    &test_lease,
 	};
 	struct ls *ls = ls_create(leases, 1);
 
-	struct test_config accepted_config;
-	struct client_state *accepted_cstate;
+	const int clients = ARRAY_LENGTH(keep_current_client);
+	struct test_config configs[clients];
+	struct client_state *cstates[clients];
 
-	accepted_config = default_test_config;
-	accepted_cstate = test_client_start(&accepted_config);
-	get_and_check_request(ls, &test_lease, LS_REQ_GET_LEASE);
-
-	/*Try to make additional connections while the first is still
-	 *connected. */
-	const int nextra_clients = 2;
-	struct test_config extra_configs[nextra_clients];
-	struct client_state *extra_cstates[nextra_clients];
-
-	for (int i = 0; i < nextra_clients; i++) {
-		extra_configs[i] = default_test_config;
-		extra_cstates[i] = test_client_start(&extra_configs[i]);
+	struct ls_req req;
+	struct ls_client *current_client = NULL;
+
+	/* Start all clients and accept / reject connections */
+	for (int i = 0; i < clients; i++) {
+		configs[i] = default_test_config;
+		cstates[i] = test_client_start(&configs[i]);
+		ck_assert_int_eq(ls_get_request(ls, &req), true);
+		check_request(&req, &test_lease, LS_REQ_GET_LEASE);
+		if (current_client && keep_current_client[i]) {
+			ls_disconnect_client(ls, req.client);
+		} else {
+			if (current_client)
+				ls_disconnect_client(ls, current_client);
+			current_client = req.client;
+		}
 	}
 
-	// Start asyncronously checking for the accepted client to release.
-	struct async_req *async_release_req =
-	    get_and_check_request_async(ls, &test_lease, LS_REQ_RELEASE_LEASE);
-
-	for (int i = 0; i < nextra_clients; i++) {
-		test_client_stop(extra_cstates[i]);
+	/* Shut down all clients */
+	for (int i = 0; i < clients; i++)
+		test_client_stop(cstates[i]);
+
+	/* Check that a valid release is received from the last accepted client
+	 * connection */
+	ck_assert_int_eq(ls_get_request(ls, &req), true);
+	check_request(&req, &test_lease, LS_REQ_RELEASE_LEASE);
+	ck_assert_ptr_eq(current_client, req.client);
+
+	/* Check that no other client connections have completed */
+	int connections_completed = 0;
+	for (int i = 0; i < clients; i++) {
+		if (configs[i].connection_completed) {
+			connections_completed++;
+			ck_assert_int_eq(connections_completed, 1);
+		}
 	}
-
-	/* Release the first connection and check results */
-	test_client_stop(accepted_cstate);
-	check_async_req_result(async_release_req);
-
-	/* Only one connection should be granted access by the lease manager */
-	ck_assert_int_eq(accepted_config.connection_completed, true);
-	for (int i = 0; i < nextra_clients; i++)
-		ck_assert_int_eq(extra_configs[i].connection_completed, false);
 	ls_destroy(ls);
 }
 END_TEST
@@ -311,7 +274,7 @@ START_TEST(send_fd_to_client)
 
 	/* send an fd to the client*/
 	int test_fd = get_dummy_fd();
-	ck_assert_int_eq(ls_send_fd(ls, req.server, test_fd), true);
+	ck_assert_int_eq(ls_send_fd(ls, req.client, test_fd), true);
 
 	test_client_stop(cstate);
 	get_and_check_request(ls, &test_lease, LS_REQ_RELEASE_LEASE);
@@ -343,7 +306,7 @@ START_TEST(ls_send_fd_is_noop_when_fd_is_invalid)
 	int invalid_fd = get_dummy_fd();
 	close(invalid_fd);
 
-	ck_assert_int_eq(ls_send_fd(ls, req.server, invalid_fd), false);
+	ck_assert_int_eq(ls_send_fd(ls, req.client, invalid_fd), false);
 
 	test_client_stop(cstate);
 	get_and_check_request(ls, &test_lease, LS_REQ_RELEASE_LEASE);