BUG/MEDIUM: server: ensure thread-safety of server runtime creation

cli_parse_add_server can be executed in parallel by several CLI
instances and so must be thread-safe. The critical points of the
function are :
- server duplicate detection
- insertion of the server in the proxy list

The mode of operation has been reversed. The server is first
instantiated and parsed. The duplicate check has been moved at the end
just before the insertion in the proxy list, under the thread isolation.
Thus, the thread safety is guaranteed and server allocation is kept
outside of locks/thread isolation.

diff --git a/src/server.c b/src/server.c
index 78620e019a4e113c7ed97599e94509bb3d8ffa53..ac53221ceab46da3d8303d9bc4eac08bfa180e62 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -4347,7 +4347,7 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
        if (!*sv_name)
                return cli_err(appctx, "Require 'backend/server'.");
 
-       get_backend_server(be_name, sv_name, &be, &srv);
+       be = proxy_be_by_name(be_name);
        if (!be)
                return cli_err(appctx, "No such backend.");
 
@@ -4356,9 +4356,6 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
                return 1;
        }
 
-       if (srv)
-               return cli_err(appctx, "Already exists a server with the same name in backend.");
-
        args[1] = sv_name;
        errcode = _srv_parse_init(&srv, args, &argc, be, parse_flags, &errmsg);
        if (errcode) {
@@ -4423,17 +4420,33 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
                goto out;
        }
 
-       /* attach the server to the end of proxy linked list
+       /* Attach the server to the end of the proxy linked list. The proxy
+        * servers list is currently not protected by a lock, so this requires
+        * thread_isolate/release.
         *
-        * The proxy servers list is currently not protected by a lock, so this
-        * requires thread_isolate/release.
+        * If a server with the same name is found, reject the new one. This
+        * operation requires thread-safety and thus cannot be executed at the
+        * beginning without having server allocation under locks/isolation.
         */
        thread_isolate();
+
        /* TODO use a double-linked list for px->srv */
        if (be->srv) {
-               struct server *next;
-               for (next = be->srv; next->next; next = next->next)
-                       ;
+               struct server *next = be->srv;
+
+               while (1) {
+                       /* check for duplicate server */
+                       if (!strcmp(srv->id, next->id)) {
+                               thread_release();
+                               cli_err(appctx, "Already exists a server with the same name in backend.");
+                               goto out;
+                       }
+
+                       if (!next->next)
+                               break;
+
+                       next = next->next;
+               }
 
                next->next = srv;
        }
@@ -4441,6 +4454,7 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
                srv->next = be->srv;
                be->srv = srv;
        }
+
        thread_release();
 
        cli_msg(appctx, LOG_INFO, "New server registered.");