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)

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 */
 
 /*

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;

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);
 }
 

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;

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);

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);

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: