From: Aurelien DARRAGON Date: Fri, 9 May 2025 17:24:55 +0000 (+0200) Subject: MEDIUM: server: automatically add server to proxy list in new_server() X-Git-Tag: v3.3-dev1~25 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=889ef6f67b39a29a68107513d8816b34b2a51915;p=thirdparty%2Fhaproxy.git 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 ade27c940..f085074fc 100644 --- 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 4ab8713fa..6fdeff94c 100644 --- 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 9699936e8..410eaf0ef 100644 --- 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 a1b976ca0..df38e2923 100644 --- 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 0d084b8fd..f491ae6d7 100644 --- 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 54c7518e1..1cf324f79 100644 --- 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 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 from a tracking list if is tracking another * server. No special care is taken if 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 a801f697d..1a6f5bfa2 100644 --- 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: