MEDIUM: server: automatically add server to proxy list in new_server()

while new_server() takes the parent proxy as argument and even assigns
srv->proxy to the parent proxy, it didn't actually inserted the server
to the parent proxy server list on success.

The result is that sometimes we add the server to the list after
new_server() is called, and sometimes we don't.

This is really error-prone and because of that hooks such as
REGISTER_POST_SERVER_CHECK() which as run for all servers listed in
all proxies may not be relied upon for servers which are not actually
inserted in their parent proxy server list. Plus it feels very strange
to have a server that points to a proxy, but then the proxy doesn't know
about it because it cannot find it in its server list.

To prevent errors and make proxy->srv list reliable, we move the insertion
logic directly under new_server(). This requires to know if we are called
during parsing or during runtime to either insert or append the server to
the parent proxy list. For that we use PR_FL_CHECKED flag from the parent
proxy (if the flag is set, then the proxy was checked so we are past the
init phase, thus we assume we are called during runtime)

This implies that during startup if new_server() has to be cancelled on
error paths we need to call srv_detach() (which is now exposed in server.h)
before srv_drop().

The consequence of this commit is that REGISTER_POST_SERVER_CHECK() should
not run reliably on all servers created using new_server() (without having
to manually loop on global servers_list)

diff --git a/include/haproxy/server.h b/include/haproxy/server.h
index ade27c9408ca07a160e368708faaabd2339ae0be..f085074fcd2d3c21b4e8af5857025a53d4cb28f4 100644 (file)
--- a/include/haproxy/server.h
+++ b/include/haproxy/server.h
@@ -319,6 +319,29 @@ static inline int srv_is_transparent(const struct server *srv)
               (srv->flags & SRV_F_MAPPORTS);
 }
 
+/* Detach server from proxy list. It is supported to call this
+ * even if the server is not yet in the list
+ * Must be called under thread isolation or when it is safe to assume
+ * that the parent proxy doesn't is not skimming through the server list
+ */
+static inline void srv_detach(struct server *srv)
+{
+       struct proxy *px = srv->proxy;
+
+       if (px->srv == srv)
+               px->srv = srv->next;
+       else {
+               struct server *prev;
+
+               for (prev = px->srv; prev && prev->next != srv; prev = prev->next)
+                       ;
+
+               BUG_ON(!prev);
+
+               prev->next = srv->next;
+       }
+}
+
 #endif /* _HAPROXY_SERVER_H */
 
 /*

diff --git a/src/cli.c b/src/cli.c
index 4ab8713fa5820c82ae1ea11646943a41b5a22e9a..6fdeff94c798fcee2c2150b5519951f655f036d9 100644 (file)
--- a/src/cli.c
+++ b/src/cli.c
@@ -3682,8 +3682,6 @@ int mworker_cli_attach_server(char **errmsg)
                else
                        memprintf(&msg, "old-%d", child->pid);
 
-               newsrv->next = mworker_proxy->srv;
-               mworker_proxy->srv = newsrv;
                newsrv->conf.file = strdup(msg);
                newsrv->id = strdup(msg);
                newsrv->conf.line = 0;

diff --git a/src/hlua.c b/src/hlua.c
index 9699936e8138d0dde9e614adbf1880f6ac102b00..410eaf0ef356ee9aed5cd52b56556ec30ffc3179 100644 (file)
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -14742,12 +14742,6 @@ static void hlua_deinit()
                        lua_close(hlua_states[thr]);
        }
 
-       srv_drop(socket_tcp);
-
-#ifdef USE_OPENSSL
-       srv_drop(socket_ssl);
-#endif
-
        free_proxy(socket_proxy);
 }
 

diff --git a/src/http_client.c b/src/http_client.c
index a1b976ca0efe4dc01219783cb0159dcbf42a19da..df38e2923a55fa5f7e547c5dd5bfb7c269695042 100644 (file)
--- a/src/http_client.c
+++ b/src/http_client.c
@@ -1295,6 +1295,7 @@ struct proxy *httpclient_create_proxy(const char *id)
                                goto err;
                        } else {
                                ha_free(&srv_ssl->ssl_ctx.ca_file);
+                               srv_detach(srv_ssl);
                                srv_drop(srv_ssl);
                                srv_ssl = NULL;
                        }
@@ -1313,26 +1314,10 @@ struct proxy *httpclient_create_proxy(const char *id)
                goto err;
        }
 
-       /* link the 2 servers in the proxy */
-       srv_raw->next = px->srv;
-       px->srv = srv_raw;
-
-#ifdef USE_OPENSSL
-       if (srv_ssl) {
-               srv_ssl->next = px->srv;
-               px->srv = srv_ssl;
-       }
-#endif
-
-
 err:
        if (err_code & ERR_CODE) {
                ha_alert("httpclient: cannot initialize: %s\n", errmsg);
                free(errmsg);
-               srv_drop(srv_raw);
-#ifdef USE_OPENSSL
-               srv_drop(srv_ssl);
-#endif
                free_proxy(px);
 
                return NULL;

diff --git a/src/mworker.c b/src/mworker.c
index 0d084b8fdd3a41c96a099cfad8805f51fc01b6ea..f491ae6d7956593cc832278927b02cc98d044cad 100644 (file)
--- a/src/mworker.c
+++ b/src/mworker.c
@@ -566,8 +566,10 @@ restart_wait:
                        }
 
                        /* Drop server */
-                       if (child->srv)
+                       if (child->srv) {
+                               srv_detach(child->srv);
                                srv_drop(child->srv);
+                       }
 
                        /* Delete fd from poller fdtab, which will close it */
                        fd_delete(child->ipc_fd[0]);
@@ -772,6 +774,7 @@ void mworker_cleanup_proc()
                                close(child->ipc_fd[1]);
                        if (child->srv) {
                                /* only exists if we created a master CLI listener */
+                               srv_detach(child->srv);
                                srv_drop(child->srv);
                        }
                        LIST_DELETE(&child->list);

diff --git a/src/server.c b/src/server.c
index 54c7518e185b4a78fe6d7fa5219eb45965231ee4..1cf324f79e4cd88d22d3e6acad5bf1f6fc8d511d 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -3015,8 +3015,12 @@ void srv_settings_cpy(struct server *srv, const struct server *src, int srv_tmpl
        }
 }
 
-/* allocate a server and attach it to the global servers_list. Returns
- * the server on success, otherwise NULL.
+/* allocate a server, attachs it to the global servers_list
+ * and adds it to <proxy> server list. Before deleting the server with
+ * srv_drop(), srv_detach() must be called to remove it from the parent
+ * proxy list
+ *
+ * Returns the server on success, otherwise NULL.
  */
 struct server *new_server(struct proxy *proxy)
 {
@@ -3062,6 +3066,24 @@ struct server *new_server(struct proxy *proxy)
        HA_RWLOCK_INIT(&srv->ssl_ctx.lock);
 #endif
 
+       // add server to proxy list:
+       /* TODO use a double-linked list for px->srv */
+       if (!(proxy->flags & PR_FL_CHECKED) || !proxy->srv) {
+               /* they are linked backwards first during parsing
+                * This will be restablished after parsing.
+                */
+               srv->next = proxy->srv;
+               proxy->srv = srv;
+       }
+       else {
+               struct server *sv = proxy->srv;
+
+               // runtime, add the server at the end of the list
+               while (sv && sv->next)
+                       sv = sv->next;
+               sv->next = srv;
+       }
+
        /* please don't put default server settings here, they are set in
         * proxy_preset_defaults().
         */
@@ -3166,26 +3188,6 @@ struct server *srv_drop(struct server *srv)
        return next;
 }
 
-/* Detach server from proxy list. It is supported to call this
- * even if the server is not yet in the list
- */
-static void _srv_detach(struct server *srv)
-{
-       struct proxy *be = srv->proxy;
-
-       if (be->srv == srv) {
-               be->srv = srv->next;
-       }
-       else {
-               struct server *prev;
-
-               for (prev = be->srv; prev && prev->next != srv; prev = prev->next)
-                       ;
-               if (prev)
-                       prev->next = srv->next;
-       }
-}
-
 /* Remove a server <srv> from a tracking list if <srv> is tracking another
  * server. No special care is taken if <srv> is tracked itself by another one :
  * this situation should be avoided by the caller.
@@ -3319,10 +3321,6 @@ static int _srv_parse_tmpl_init(struct server *srv, struct proxy *px)
                /* Set this new server ID. */
                _srv_parse_set_id_from_prefix(newsrv, srv->tmpl_info.prefix, i);
 
-               /* Linked backwards first. This will be restablished after parsing. */
-               newsrv->next = px->srv;
-               px->srv = newsrv;
-
                newsrv->conf.name.key = newsrv->id;
                ebis_insert(&curproxy->conf.used_server_name, &newsrv->conf.name);
        }
@@ -3820,12 +3818,6 @@ int parse_server(const char *file, int linenum, char **args,
        err_code = _srv_parse_init(&newsrv, args, &cur_arg, curproxy,
                                   parse_flags);
 
-       /* the servers are linked backwards first */
-       if (newsrv && !(parse_flags & SRV_PARSE_DEFAULT_SERVER)) {
-               newsrv->next = curproxy->srv;
-               curproxy->srv = newsrv;
-       }
-
        if (err_code & ERR_CODE)
                goto out;
 
@@ -5956,6 +5948,15 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
         */
        thread_isolate();
 
+       /*
+        * If a server with the same name is found, reject the new one.
+        */
+       if (server_find_by_name(be, sv_name)) {
+               thread_release();
+               cli_err(appctx, "Already exists a server with the same name in backend.\n");
+               return 1;
+       }
+
        args[1] = sv_name;
        errcode = _srv_parse_init(&srv, args, &argc, be, parse_flags);
        if (errcode)
@@ -6054,37 +6055,6 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
        if (errcode)
                goto out;
 
-       /* Attach the server to the end of the proxy linked list. Note that this
-        * operation is not thread-safe so this is executed under thread
-        * isolation.
-        *
-        * If a server with the same name is found, reject the new one.
-        */
-
-       /* TODO use a double-linked list for px->srv */
-       if (be->srv) {
-               struct server *next = be->srv;
-
-               while (1) {
-                       /* check for duplicate server */
-                       if (strcmp(srv->id, next->id) == 0) {
-                               ha_alert("Already exists a server with the same name in backend.\n");
-                               goto out;
-                       }
-
-                       if (!next->next)
-                               break;
-
-                       next = next->next;
-               }
-
-               next->next = srv;
-       }
-       else {
-               srv->next = be->srv;
-               be->srv = srv;
-       }
-
        /* generate the server id if not manually specified */
        if (!srv->puid) {
                next_id = get_next_id(&be->conf.used_server_id, 1);
@@ -6162,7 +6132,7 @@ out:
                }
 
                /* remove the server from the proxy linked list */
-               _srv_detach(srv);
+               srv_detach(srv);
        }
 
        thread_release();
@@ -6369,7 +6339,7 @@ static int cli_parse_delete_server(char **args, char *payload, struct appctx *ap
         * The proxy servers list is currently not protected by a lock, so this
         * requires thread_isolate/release.
         */
-       _srv_detach(srv);
+       srv_detach(srv);
 
        /* remove srv from addr_node tree */
        eb32_delete(&srv->conf.id);

diff --git a/src/sink.c b/src/sink.c
index a801f697d8fb6ba67fbc64be1b553cbd7ed9f91f..1a6f5bfa2a508a9aa9827e5e82634dd3f8ba6fa4 100644 (file)
--- a/src/sink.c
+++ b/src/sink.c
@@ -1271,17 +1271,13 @@ struct sink *sink_new_from_logger(struct logger *logger)
        if (srv_init_per_thr(srv) == -1)
                goto error;
 
-       /* link srv with sink forward proxy: the servers are linked
-        * backwards first into proxy
-        */
-       srv->next = sink->forward_px->srv;
-       sink->forward_px->srv = srv;
-
        if (sink_finalize(sink) & ERR_CODE)
                goto error_final;
 
        return sink;
  error:
+       if (srv)
+               srv_detach(srv);
        srv_drop(srv);
 
  error_final: