lease-server: Allow multiple client connections 10/26210/3
authorDamian Hobson-Garcia <dhobsong@igel.co.jp>
Tue, 9 Mar 2021 03:05:29 +0000 (12:05 +0900)
committerDamian Hobson-Garcia <dhobsong@igel.co.jp>
Tue, 6 Apr 2021 01:33:11 +0000 (10:33 +0900)
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>
drm-lease-manager/lease-manager.c
drm-lease-manager/lease-server.c
drm-lease-manager/lease-server.h
drm-lease-manager/main.c
drm-lease-manager/test/lease-server-test.c

index 5cfc5de..016f318 100644 (file)
@@ -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,
index e05e8e4..c57316e 100644 (file)
 
 #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;
 }
index c87b834..1aaad30 100644 (file)
@@ -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
index c3a933f..491c80c 100644 (file)
@@ -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:
index 2672820..6a4d26c 100644 (file)
@@ -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);