From b8064774697b98d16be25510d54cbfee3663bba8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 6 Jul 2026 11:11:28 -0700 Subject: [PATCH 01/22] Doc: based, libcrmcluster: Drop gpointer from include comments The responsible commits were written before we replaced all gpointers with void pointers. Signed-off-by: Reid Wahl --- daemons/based/based_corosync.c | 2 +- lib/cluster/corosync.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/daemons/based/based_corosync.c b/daemons/based/based_corosync.c index a6d4229b50a..cf8fc547d6c 100644 --- a/daemons/based/based_corosync.c +++ b/daemons/based/based_corosync.c @@ -15,7 +15,7 @@ #include // free #include // cpg_* -#include // gpointer +#include // g_clear_pointer #include // xmlNode #include // SUPPORT_COROSYNC diff --git a/lib/cluster/corosync.c b/lib/cluster/corosync.c index 071360662bf..5b1c9136745 100644 --- a/lib/cluster/corosync.c +++ b/lib/cluster/corosync.c @@ -23,7 +23,7 @@ #include // cmap_* #include // cs_*, CS_* #include // quorum_* -#include // gboolean, gpointer, g_*, G_PRIORITY_HIGH +#include // gboolean, g_*, G_PRIORITY_HIGH #include // xmlNode #include // QB_XS From 13dbe8b73ab6cde9eaadb2e8557ada2b16225d49 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 7 Jan 2026 15:55:42 -0800 Subject: [PATCH 02/22] Feature: libcrmcommon: Drop support for PCMK_ipc_type env variable There were four allowed values: shared-mem, socket, posix, and sysv. Libqb removed support for posix and sysv (message queues) in 2012 via commit 70a9623a. It simply returns an error if you try to set up those types of IPC. I spoke with the libqb maintainers last night, and they said that socket IPC has never worked correctly and that they are considering removing it. Sockets were mainly intended for systems where shared memory was not available. That is believed to be no longer applicable -- shared memory should be available on all the platforms that Pacemaker and libqb target for support. That leaves only shared memory as a working option. We are dropping this option rather than deprecating it. As discussed above, two of the values (posix and sysv) will fail immediately during IPC setup, and socket is very buggy. So user-facing behavior should not be affected. If the option is configured, it will simply be ignored. Signed-off-by: Reid Wahl --- etc/sysconfig/pacemaker.in | 14 -------------- include/crm/common/options_internal.h | 1 - lib/common/mainloop.c | 12 +----------- 3 files changed, 1 insertion(+), 26 deletions(-) diff --git a/etc/sysconfig/pacemaker.in b/etc/sysconfig/pacemaker.in index a7763f6d180..fe65ee30576 100644 --- a/etc/sysconfig/pacemaker.in +++ b/etc/sysconfig/pacemaker.in @@ -337,20 +337,6 @@ # Default: PCMK_dh_max_bits="0" (no maximum) -## Inter-process Communication - -# PCMK_ipc_type (Advanced Use Only) -# -# Force use of a particular IPC method. Allowed values: -# -# shared-mem -# socket -# posix -# sysv -# -# Default: PCMK_ipc_type="shared-mem" - - ## Cluster type # PCMK_cluster_type (Advanced Use Only) diff --git a/include/crm/common/options_internal.h b/include/crm/common/options_internal.h index 22b8e303a21..b1b2085db7f 100644 --- a/include/crm/common/options_internal.h +++ b/include/crm/common/options_internal.h @@ -152,7 +152,6 @@ bool pcmk__valid_fencing_watchdog_timeout(const char *value); #define PCMK__ENV_CRL_FILE "crl_file" #define PCMK__ENV_DEBUG "debug" #define PCMK__ENV_FAIL_FAST "fail_fast" -#define PCMK__ENV_IPC_TYPE "ipc_type" #define PCMK__ENV_KEY_FILE "key_file" #define PCMK__ENV_LOGFACILITY "logfacility" #define PCMK__ENV_LOGFILE "logfile" diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index 530175cfe16..b410bc0a5b8 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -595,17 +595,7 @@ struct qb_ipcs_poll_handlers gio_poll_funcs = { static enum qb_ipc_type pick_ipc_type(enum qb_ipc_type requested) { - const char *env = pcmk__env_option(PCMK__ENV_IPC_TYPE); - - if (env && strcmp("shared-mem", env) == 0) { - return QB_IPC_SHM; - } else if (env && strcmp("socket", env) == 0) { - return QB_IPC_SOCKET; - } else if (env && strcmp("posix", env) == 0) { - return QB_IPC_POSIX_MQ; - } else if (env && strcmp("sysv", env) == 0) { - return QB_IPC_SYSV_MQ; - } else if (requested == QB_IPC_NATIVE) { + if (requested == QB_IPC_NATIVE) { /* We prefer shared memory because the server never blocks on * send. If part of a message fits into the socket, libqb * needs to block until the remainder can be sent also. From 941f39782ba84b073a0f320c1af2ee342e4204d9 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 7 Jan 2026 16:06:02 -0800 Subject: [PATCH 03/22] Refactor: libcrmcommon: Use QB_IPC_SHM in pcmk__serve_DAEMON_ipc() QB_IPC_NATIVE tells libqb to use shared memory (QB_IPC_SHM) unless it's shared-memory IPC is specifically disabled at build time because it's unavailable on the system. In that case, libqb will use sockets (QB_IPC_SOCKET). I spoke with the libqb maintainers last night, and they said that socket IPC has never worked correctly and that they are considering removing it. They told me never to use libqb socket IPC. Libqb socket IPC was mainly intended for systems where shared memory was not available. That is believed to be no longer applicable -- shared memory should be available on all the platforms that Pacemaker and libqb target for support. The maintainers weakly suggested using QB_IPC_NATIVE (rather than QB_IPC_SHM). I suppose this was for future-proofing ("let libqb decide what to use"). Nonetheless, since we have a couple of use cases where we MUST avoid blocking, and QB_IPC_SHM is non-blocking, we'll just use QB_IPC_SHM everywhere until we have a reason not to. Besides, this makes things explicit. Signed-off-by: Reid Wahl --- lib/common/ipc_server.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/common/ipc_server.c b/lib/common/ipc_server.c index 212d43bef6e..501709185d5 100644 --- a/lib/common/ipc_server.c +++ b/lib/common/ipc_server.c @@ -1035,14 +1035,14 @@ void pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, struct qb_ipcs_service_handlers *ro_cb, struct qb_ipcs_service_handlers *rw_cb) { - *ipcs_ro = mainloop_add_ipc_server(PCMK__SERVER_BASED_RO, - QB_IPC_NATIVE, ro_cb); + *ipcs_ro = mainloop_add_ipc_server(PCMK__SERVER_BASED_RO, QB_IPC_SHM, + ro_cb); - *ipcs_rw = mainloop_add_ipc_server(PCMK__SERVER_BASED_RW, - QB_IPC_NATIVE, rw_cb); + *ipcs_rw = mainloop_add_ipc_server(PCMK__SERVER_BASED_RW, QB_IPC_SHM, + rw_cb); - *ipcs_shm = mainloop_add_ipc_server(PCMK__SERVER_BASED_SHM, - QB_IPC_SHM, rw_cb); + *ipcs_shm = mainloop_add_ipc_server(PCMK__SERVER_BASED_SHM, QB_IPC_SHM, + rw_cb); if (*ipcs_ro == NULL || *ipcs_rw == NULL || *ipcs_shm == NULL) { pcmk__crit("Failed to create %s IPC server; shutting down", @@ -1085,7 +1085,7 @@ pcmk__stop_based_ipc(qb_ipcs_service_t *ipcs_ro, qb_ipcs_service_t * pcmk__serve_controld_ipc(struct qb_ipcs_service_handlers *cb) { - return mainloop_add_ipc_server(CRM_SYSTEM_CRMD, QB_IPC_NATIVE, cb); + return mainloop_add_ipc_server(CRM_SYSTEM_CRMD, QB_IPC_SHM, cb); } /*! @@ -1102,7 +1102,7 @@ pcmk__serve_attrd_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { *ipcs = mainloop_add_ipc_server(pcmk__server_ipc_name(pcmk_ipc_attrd), - QB_IPC_NATIVE, cb); + QB_IPC_SHM, cb); if (*ipcs == NULL) { pcmk__crit("Failed to create %s IPC server; shutting down", @@ -1150,7 +1150,7 @@ pcmk__serve_fenced_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { *ipcs = mainloop_add_ipc_server_with_prio(pcmk__server_ipc_name(pcmk_ipc_fenced), - QB_IPC_NATIVE, cb, QB_LOOP_HIGH); + QB_IPC_SHM, cb, QB_LOOP_HIGH); if (*ipcs == NULL) { pcmk__crit("Failed to create %s IPC server; shutting down", @@ -1175,7 +1175,7 @@ pcmk__serve_pacemakerd_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { *ipcs = mainloop_add_ipc_server(pcmk__server_ipc_name(pcmk_ipc_pacemakerd), - QB_IPC_NATIVE, cb); + QB_IPC_SHM, cb); if (*ipcs == NULL) { pcmk__crit("Failed to create %s IPC server; shutting down", @@ -1206,7 +1206,7 @@ pcmk__serve_schedulerd_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { *ipcs = mainloop_add_ipc_server(pcmk__server_ipc_name(pcmk_ipc_schedulerd), - QB_IPC_NATIVE, cb); + QB_IPC_SHM, cb); if (*ipcs == NULL) { pcmk__crit("Failed to create %s IPC server; shutting down", From dda959fe1e668763e413bfc5ada59dd1a7c3a47d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 7 Jan 2026 16:12:41 -0800 Subject: [PATCH 04/22] Feature: libcrmcommon: mainloop_add_ipc_server{,_with_prio} ignore type ...argument. QB_IPC_NATIVE tells libqb to use shared memory (QB_IPC_SHM) unless shared-memory IPC is specifically disabled at build time because it's unavailable on the system. In that case, libqb will use sockets (QB_IPC_SOCKET). I spoke with the libqb maintainers last night, and they said that socket IPC has never worked correctly and that they are considering removing it. They told me never to use libqb socket IPC. Libqb socket IPC was mainly intended for systems where shared memory was not available. That is believed to be no longer applicable -- shared memory should be available on all the platforms that Pacemaker and libqb target for support. The maintainers weakly suggested using QB_IPC_NATIVE (rather than QB_IPC_SHM). I suppose this was for future-proofing ("let libqb decide what to use"). Nonetheless, since we have a couple of use cases where we MUST avoid blocking, and QB_IPC_SHM is non-blocking, we'll just use QB_IPC_SHM everywhere until we have a reason not to. Signed-off-by: Reid Wahl --- include/crm/common/mainloop.h | 2 +- lib/common/mainloop.c | 17 +---------------- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/include/crm/common/mainloop.h b/include/crm/common/mainloop.h index 680d251994e..17a0ea1cc12 100644 --- a/include/crm/common/mainloop.h +++ b/include/crm/common/mainloop.h @@ -118,7 +118,7 @@ qb_ipcs_service_t *mainloop_add_ipc_server(const char *name, enum qb_ipc_type ty * \brief Start server-side API end-point, hooked into the internal event loop * * \param[in] name name of the IPC end-point ("address" for the client) - * \param[in] type selects libqb's IPC back-end (or use #QB_IPC_NATIVE) + * \param[in] type Ignored * \param[in] callbacks defines libqb's IPC service-level handlers * \param[in] priority priority relative to other events handled in the * abstract handling loop, use #QB_LOOP_MED when unsure diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index b410bc0a5b8..a1525d9b8d3 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -592,21 +592,6 @@ struct qb_ipcs_poll_handlers gio_poll_funcs = { .dispatch_del = gio_poll_dispatch_del, }; -static enum qb_ipc_type -pick_ipc_type(enum qb_ipc_type requested) -{ - if (requested == QB_IPC_NATIVE) { - /* We prefer shared memory because the server never blocks on - * send. If part of a message fits into the socket, libqb - * needs to block until the remainder can be sent also. - * Otherwise the client will wait forever for the remaining - * bytes. - */ - return QB_IPC_SHM; - } - return requested; -} - qb_ipcs_service_t * mainloop_add_ipc_server(const char *name, enum qb_ipc_type type, struct qb_ipcs_service_handlers *callbacks) @@ -626,7 +611,7 @@ mainloop_add_ipc_server_with_prio(const char *name, enum qb_ipc_type type, gio_map = qb_array_create_2(64, sizeof(struct gio_to_qb_poll), 1); } - server = qb_ipcs_create(name, 0, pick_ipc_type(type), callbacks); + server = qb_ipcs_create(name, 0, QB_IPC_NATIVE, callbacks); if (server == NULL) { pcmk__err("Could not create %s IPC server: %s (%d)", name, From 9e6f028290cc688499ad08d572a449a5d51ca455 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 10 Jan 2026 17:37:56 -0800 Subject: [PATCH 05/22] API: libcib: Deprecate cib_command_nonblocking It behaves identically to cib_command. This became even more clear with the recent "Use QB_IPC_SHM in pcmk__serve_DAEMON_ipc()" commit. Signed-off-by: Reid Wahl --- daemons/controld/controld_cib.c | 6 ++---- include/crm/cib/cib_types.h | 2 ++ lib/cib/cib_native.c | 25 +++++++++++++------------ 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/daemons/controld/controld_cib.c b/daemons/controld/controld_cib.c index f88fc0492d1..cadda850a80 100644 --- a/daemons/controld/controld_cib.c +++ b/daemons/controld/controld_cib.c @@ -155,13 +155,11 @@ do_cib_control(long long action, enum crmd_fsa_cause cause, return; } - rc = cib_conn->cmds->signon(cib_conn, crm_system_name, - cib_command_nonblocking); + rc = cib_conn->cmds->signon(cib_conn, crm_system_name, cib_command); if (rc != pcmk_ok) { // A short wait that usually avoids stalling the FSA sleep(1); - rc = cib_conn->cmds->signon(cib_conn, crm_system_name, - cib_command_nonblocking); + rc = cib_conn->cmds->signon(cib_conn, crm_system_name, cib_command); } if (rc != pcmk_ok) { diff --git a/include/crm/cib/cib_types.h b/include/crm/cib/cib_types.h index 172439bbf29..41fd6094b87 100644 --- a/include/crm/cib/cib_types.h +++ b/include/crm/cib/cib_types.h @@ -53,6 +53,8 @@ enum cib_conn_type { cib_query, cib_no_connection, + + //! \deprecated Use \c cib_command instead cib_command_nonblocking, }; diff --git a/lib/cib/cib_native.c b/lib/cib/cib_native.c index f5e59415d9a..39524166492 100644 --- a/lib/cib/cib_native.c +++ b/lib/cib/cib_native.c @@ -297,20 +297,21 @@ cib_native_signon(cib_t *cib, const char *name, enum cib_conn_type type) cib->call_timeout = PCMK__IPC_TIMEOUT; - if (type == cib_command) { - cib->state = cib_connected_command; - channel = PCMK__SERVER_BASED_RW; - - } else if (type == cib_command_nonblocking) { - cib->state = cib_connected_command; - channel = PCMK__SERVER_BASED_SHM; + switch (type) { + case cib_command: + case cib_command_nonblocking: + // @COMPAT cib_command_nonblocking is deprecated since 3.0.2 + cib->state = cib_connected_command; + channel = PCMK__SERVER_BASED_RW; + break; - } else if (type == cib_query) { - cib->state = cib_connected_query; - channel = PCMK__SERVER_BASED_RO; + case cib_query: + cib->state = cib_connected_query; + channel = PCMK__SERVER_BASED_RO; + break; - } else { - return -ENOTCONN; + default: + return -ENOTCONN; } pcmk__trace("Connecting %s channel", channel); From d206be59f14921166864a43bb009200301ed7ea8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 10 Jan 2026 17:52:06 -0800 Subject: [PATCH 06/22] Refactor: various: Drop PCMK__SERVER_BASED_SHM And associated code. It behaves identically to PCMK__SERVER_BASED_RW. A key point to note is that only non-proxied cib_native clients connected to PCMK__SERVER_BASED_SHM (prior to deprecation of cib_command_nonblocking in a previous commit). cib_remote clients and proxied connections used only PCMK__SERVER_BASED_RO and PCMK__SERVER_BASED_RW. So backward compatibility should not be a concern -- this affects only local clients on a cluster node. Signed-off-by: Reid Wahl --- daemons/based/based_ipc.c | 6 +----- daemons/execd/remoted_proxy.c | 6 ++---- include/crm/common/ipc_internal.h | 4 +--- include/crm_internal.h | 3 +-- lib/common/ipc_server.c | 27 ++++++++------------------- lib/common/servers.c | 23 +++++++++++------------ 6 files changed, 24 insertions(+), 45 deletions(-) diff --git a/daemons/based/based_ipc.c b/daemons/based/based_ipc.c index 9f1af42b97b..9e32dfdbd5f 100644 --- a/daemons/based/based_ipc.c +++ b/daemons/based/based_ipc.c @@ -30,7 +30,6 @@ static qb_ipcs_service_t *ipcs_ro = NULL; static qb_ipcs_service_t *ipcs_rw = NULL; -static qb_ipcs_service_t *ipcs_shm = NULL; /*! * \internal @@ -305,7 +304,7 @@ static struct qb_ipcs_service_handlers ipc_rw_callbacks = { void based_ipc_init(void) { - pcmk__serve_based_ipc(&ipcs_ro, &ipcs_rw, &ipcs_shm, &ipc_ro_callbacks, + pcmk__serve_based_ipc(&ipcs_ro, &ipcs_rw, &ipc_ro_callbacks, &ipc_rw_callbacks); } @@ -322,9 +321,6 @@ based_ipc_cleanup(void) pcmk__drop_all_clients(ipcs_rw); g_clear_pointer(&ipcs_rw, qb_ipcs_destroy); - pcmk__drop_all_clients(ipcs_shm); - g_clear_pointer(&ipcs_shm, qb_ipcs_destroy); - /* Drop remote clients here because they're part of the IPC client table and * must be dropped before \c pcmk__client_cleanup() */ diff --git a/daemons/execd/remoted_proxy.c b/daemons/execd/remoted_proxy.c index d4432284291..c8f1857e1b6 100644 --- a/daemons/execd/remoted_proxy.c +++ b/daemons/execd/remoted_proxy.c @@ -30,7 +30,6 @@ static qb_ipcs_service_t *cib_ro = NULL; static qb_ipcs_service_t *cib_rw = NULL; -static qb_ipcs_service_t *cib_shm = NULL; static qb_ipcs_service_t *attrd_ipcs = NULL; static qb_ipcs_service_t *crmd_ipcs = NULL; @@ -520,7 +519,7 @@ ipc_proxy_init(void) { ipc_clients = pcmk__strkey_table(NULL, NULL); - pcmk__serve_based_ipc(&cib_ro, &cib_rw, &cib_shm, &cib_proxy_callbacks_ro, + pcmk__serve_based_ipc(&cib_ro, &cib_rw, &cib_proxy_callbacks_ro, &cib_proxy_callbacks_rw); pcmk__serve_attrd_ipc(&attrd_ipcs, &attrd_proxy_callbacks); pcmk__serve_fenced_ipc(&fencer_ipcs, &fencer_proxy_callbacks); @@ -541,7 +540,7 @@ ipc_proxy_cleanup(void) g_clear_pointer(&ipc_providers, g_list_free); g_clear_pointer(&ipc_clients, g_hash_table_destroy); - pcmk__stop_based_ipc(cib_ro, cib_rw, cib_shm); + pcmk__stop_based_ipc(cib_ro, cib_rw); g_clear_pointer(&attrd_ipcs, qb_ipcs_destroy); g_clear_pointer(&fencer_ipcs, qb_ipcs_destroy); @@ -550,5 +549,4 @@ ipc_proxy_cleanup(void) cib_ro = NULL; cib_rw = NULL; - cib_shm = NULL; } diff --git a/include/crm/common/ipc_internal.h b/include/crm/common/ipc_internal.h index eecc8bb612a..1d14eced14b 100644 --- a/include/crm/common/ipc_internal.h +++ b/include/crm/common/ipc_internal.h @@ -251,13 +251,11 @@ qb_ipcs_service_t *pcmk__serve_controld_ipc(struct qb_ipcs_service_handlers *cb) void pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, qb_ipcs_service_t **ipcs_rw, - qb_ipcs_service_t **ipcs_shm, struct qb_ipcs_service_handlers *ro_cb, struct qb_ipcs_service_handlers *rw_cb); void pcmk__stop_based_ipc(qb_ipcs_service_t *ipcs_ro, - qb_ipcs_service_t *ipcs_rw, - qb_ipcs_service_t *ipcs_shm); + qb_ipcs_service_t *ipcs_rw); static inline const char * pcmk__ipc_sys_name(const char *ipc_name, const char *fallback) diff --git a/include/crm_internal.h b/include/crm_internal.h index 277fe824830..407737bb7d9 100644 --- a/include/crm_internal.h +++ b/include/crm_internal.h @@ -1,5 +1,5 @@ /* - * Copyright 2006-2025 the Pacemaker project contributors + * Copyright 2006-2026 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -61,7 +61,6 @@ extern "C" { #define PCMK__SERVER_BASED_RO "cib_ro" #define PCMK__SERVER_BASED_RW "cib_rw" -#define PCMK__SERVER_BASED_SHM "cib_shm" /* * IPC commands that can be sent to Pacemaker daemons diff --git a/lib/common/ipc_server.c b/lib/common/ipc_server.c index 501709185d5..ec59b61894d 100644 --- a/lib/common/ipc_server.c +++ b/lib/common/ipc_server.c @@ -1021,19 +1021,15 @@ pcmk__ipc_send_ack_as(const char *function, int line, pcmk__client_t *c, * * \param[out] ipcs_ro New IPC server for read-only CIB manager API * \param[out] ipcs_rw New IPC server for read/write CIB manager API - * \param[out] ipcs_shm New IPC server for shared-memory CIB manager API * \param[in] ro_cb IPC callbacks for read-only API * \param[in] rw_cb IPC callbacks for read/write and shared-memory APIs * * \note This function exits fatally on error. - * \note There is no actual difference between the three IPC endpoints other - * than their names. */ -void pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, - qb_ipcs_service_t **ipcs_rw, - qb_ipcs_service_t **ipcs_shm, - struct qb_ipcs_service_handlers *ro_cb, - struct qb_ipcs_service_handlers *rw_cb) +void +pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, qb_ipcs_service_t **ipcs_rw, + struct qb_ipcs_service_handlers *ro_cb, + struct qb_ipcs_service_handlers *rw_cb) { *ipcs_ro = mainloop_add_ipc_server(PCMK__SERVER_BASED_RO, QB_IPC_SHM, ro_cb); @@ -1041,10 +1037,7 @@ void pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, *ipcs_rw = mainloop_add_ipc_server(PCMK__SERVER_BASED_RW, QB_IPC_SHM, rw_cb); - *ipcs_shm = mainloop_add_ipc_server(PCMK__SERVER_BASED_SHM, QB_IPC_SHM, - rw_cb); - - if (*ipcs_ro == NULL || *ipcs_rw == NULL || *ipcs_shm == NULL) { + if ((*ipcs_ro == NULL) || (*ipcs_rw == NULL)) { pcmk__crit("Failed to create %s IPC server; shutting down", pcmk__server_log_name(pcmk_ipc_based)); pcmk__crit("Verify pacemaker and pacemaker_remote are not both " @@ -1057,21 +1050,17 @@ void pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, * \internal * \brief Destroy IPC servers for the CIB manager API * - * \param[out] ipcs_ro IPC server for read-only the CIB manager API - * \param[out] ipcs_rw IPC server for read/write the CIB manager API - * \param[out] ipcs_shm IPC server for shared-memory the CIB manager API + * \param[in,out] ipcs_ro IPC server for read-only the CIB manager API + * \param[in,out] ipcs_rw IPC server for read/write the CIB manager API * * \note This is a convenience function for calling qb_ipcs_destroy() for each * argument. */ void -pcmk__stop_based_ipc(qb_ipcs_service_t *ipcs_ro, - qb_ipcs_service_t *ipcs_rw, - qb_ipcs_service_t *ipcs_shm) +pcmk__stop_based_ipc(qb_ipcs_service_t *ipcs_ro, qb_ipcs_service_t *ipcs_rw) { qb_ipcs_destroy(ipcs_ro); qb_ipcs_destroy(ipcs_rw); - qb_ipcs_destroy(ipcs_shm); } /*! diff --git a/lib/common/servers.c b/lib/common/servers.c index f2e6d79ef50..6a4fdc7405b 100644 --- a/lib/common/servers.c +++ b/lib/common/servers.c @@ -1,5 +1,5 @@ /* - * Copyright 2024 the Pacemaker project contributors + * Copyright 2024-2026 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -33,63 +33,62 @@ static struct { const char *log_name; // Readable server name for use in logs const char *system_names[2]; // crm_system_name values (subdaemon names) - const char *ipc_names[3]; // libqb IPC names used to contact server + const char *ipc_names[2]; // libqb IPC names used to contact server const char *message_types[3]; // IPC/cluster message types sent to server } server_info[] = { [pcmk_ipc_unknown] = { NULL, { NULL, NULL, }, - { NULL, NULL, NULL, }, + { NULL, NULL, }, { NULL, NULL, NULL, }, }, [pcmk_ipc_attrd] = { "attribute manager", { PCMK__SERVER_ATTRD, NULL, }, - { PCMK__VALUE_ATTRD, NULL, NULL, }, + { PCMK__VALUE_ATTRD, NULL, }, { PCMK__VALUE_ATTRD, NULL, NULL, }, }, [pcmk_ipc_based] = { "CIB manager", { PCMK__SERVER_BASED, NULL, }, - { PCMK__SERVER_BASED_RW, PCMK__SERVER_BASED_RO, - PCMK__SERVER_BASED_SHM, }, + { PCMK__SERVER_BASED_RW, PCMK__SERVER_BASED_RO, }, { CRM_SYSTEM_CIB, NULL, NULL, }, }, [pcmk_ipc_controld] = { "controller", { PCMK__SERVER_CONTROLD, NULL, }, - { PCMK__VALUE_CRMD, NULL, NULL, }, + { PCMK__VALUE_CRMD, NULL, }, { PCMK__VALUE_CRMD, CRM_SYSTEM_DC, CRM_SYSTEM_TENGINE, }, }, [pcmk_ipc_execd] = { "executor", { PCMK__SERVER_EXECD, PCMK__SERVER_REMOTED, }, - { PCMK__VALUE_LRMD, NULL, NULL, }, + { PCMK__VALUE_LRMD, NULL, }, { PCMK__VALUE_LRMD, NULL, NULL, }, }, [pcmk_ipc_fenced] = { "fencer", { PCMK__SERVER_FENCED, NULL, }, - { PCMK__VALUE_STONITH_NG, NULL, NULL, }, + { PCMK__VALUE_STONITH_NG, NULL, }, { PCMK__VALUE_STONITH_NG, NULL, NULL, }, }, [pcmk_ipc_pacemakerd] = { "launcher", { PCMK__SERVER_PACEMAKERD, NULL, }, - { CRM_SYSTEM_MCP, NULL, NULL, }, + { CRM_SYSTEM_MCP, NULL, }, { CRM_SYSTEM_MCP, NULL, NULL, }, }, [pcmk_ipc_schedulerd] = { "scheduler", { PCMK__SERVER_SCHEDULERD, NULL, }, - { CRM_SYSTEM_PENGINE, NULL, NULL, }, + { CRM_SYSTEM_PENGINE, NULL, }, { CRM_SYSTEM_PENGINE, NULL, NULL, }, }, }; @@ -193,7 +192,7 @@ pcmk__parse_server(const char *text) } } for (name = 0; - (name < 3) && (server_info[server].ipc_names[name] != NULL); + (name < 2) && (server_info[server].ipc_names[name] != NULL); ++name) { if (strcmp(text, server_info[server].ipc_names[name]) == 0) { return server; From b3c1bc29f43d20f854a86e57e28f1662f6f93a17 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 10 Jan 2026 17:56:13 -0800 Subject: [PATCH 07/22] Refactor: libcrmcommon: Drop pcmk__stop_based_ipc() It has one caller and saves one line at most. Signed-off-by: Reid Wahl --- daemons/execd/remoted_proxy.c | 9 +++------ include/crm/common/ipc_internal.h | 3 --- lib/common/ipc_server.c | 17 ----------------- 3 files changed, 3 insertions(+), 26 deletions(-) diff --git a/daemons/execd/remoted_proxy.c b/daemons/execd/remoted_proxy.c index c8f1857e1b6..7c64d4675d1 100644 --- a/daemons/execd/remoted_proxy.c +++ b/daemons/execd/remoted_proxy.c @@ -540,13 +540,10 @@ ipc_proxy_cleanup(void) g_clear_pointer(&ipc_providers, g_list_free); g_clear_pointer(&ipc_clients, g_hash_table_destroy); - pcmk__stop_based_ipc(cib_ro, cib_rw); - g_clear_pointer(&attrd_ipcs, qb_ipcs_destroy); + g_clear_pointer(&cib_ro, qb_ipcs_destroy); + g_clear_pointer(&cib_rw, qb_ipcs_destroy); + g_clear_pointer(&crmd_ipcs, qb_ipcs_destroy); g_clear_pointer(&fencer_ipcs, qb_ipcs_destroy); g_clear_pointer(&pacemakerd_ipcs, qb_ipcs_destroy); - g_clear_pointer(&crmd_ipcs, qb_ipcs_destroy); - - cib_ro = NULL; - cib_rw = NULL; } diff --git a/include/crm/common/ipc_internal.h b/include/crm/common/ipc_internal.h index 1d14eced14b..4fc29087420 100644 --- a/include/crm/common/ipc_internal.h +++ b/include/crm/common/ipc_internal.h @@ -254,9 +254,6 @@ void pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, struct qb_ipcs_service_handlers *ro_cb, struct qb_ipcs_service_handlers *rw_cb); -void pcmk__stop_based_ipc(qb_ipcs_service_t *ipcs_ro, - qb_ipcs_service_t *ipcs_rw); - static inline const char * pcmk__ipc_sys_name(const char *ipc_name, const char *fallback) { diff --git a/lib/common/ipc_server.c b/lib/common/ipc_server.c index ec59b61894d..85021f6ff23 100644 --- a/lib/common/ipc_server.c +++ b/lib/common/ipc_server.c @@ -1046,23 +1046,6 @@ pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, qb_ipcs_service_t **ipcs_rw, } } -/*! - * \internal - * \brief Destroy IPC servers for the CIB manager API - * - * \param[in,out] ipcs_ro IPC server for read-only the CIB manager API - * \param[in,out] ipcs_rw IPC server for read/write the CIB manager API - * - * \note This is a convenience function for calling qb_ipcs_destroy() for each - * argument. - */ -void -pcmk__stop_based_ipc(qb_ipcs_service_t *ipcs_ro, qb_ipcs_service_t *ipcs_rw) -{ - qb_ipcs_destroy(ipcs_ro); - qb_ipcs_destroy(ipcs_rw); -} - /*! * \internal * \brief Add an IPC server to the main loop for the controller API From f4c3b9d51e72982186540fd503e038b6b96db27f Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 10 Jan 2026 18:00:39 -0800 Subject: [PATCH 08/22] Refactor: libcrmcommon: Assert for arguments in pcmk__serve_DAEMON_ipc() Signed-off-by: Reid Wahl --- lib/common/ipc_server.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/common/ipc_server.c b/lib/common/ipc_server.c index 85021f6ff23..21fc876873f 100644 --- a/lib/common/ipc_server.c +++ b/lib/common/ipc_server.c @@ -1031,6 +1031,9 @@ pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, qb_ipcs_service_t **ipcs_rw, struct qb_ipcs_service_handlers *ro_cb, struct qb_ipcs_service_handlers *rw_cb) { + pcmk__assert((ipcs_ro != NULL) && (*ipcs_ro == NULL) && (ro_cb != NULL) + && (ipcs_rw != NULL) && (*ipcs_rw == NULL) && (rw_cb != NULL)); + *ipcs_ro = mainloop_add_ipc_server(PCMK__SERVER_BASED_RO, QB_IPC_SHM, ro_cb); @@ -1057,6 +1060,8 @@ pcmk__serve_based_ipc(qb_ipcs_service_t **ipcs_ro, qb_ipcs_service_t **ipcs_rw, qb_ipcs_service_t * pcmk__serve_controld_ipc(struct qb_ipcs_service_handlers *cb) { + pcmk__assert(cb != NULL); + return mainloop_add_ipc_server(CRM_SYSTEM_CRMD, QB_IPC_SHM, cb); } @@ -1073,6 +1078,8 @@ void pcmk__serve_attrd_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { + pcmk__assert((ipcs != NULL) && (*ipcs == NULL) && (cb != NULL)); + *ipcs = mainloop_add_ipc_server(pcmk__server_ipc_name(pcmk_ipc_attrd), QB_IPC_SHM, cb); @@ -1121,6 +1128,8 @@ void pcmk__serve_fenced_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { + pcmk__assert((ipcs != NULL) && (*ipcs == NULL) && (cb != NULL)); + *ipcs = mainloop_add_ipc_server_with_prio(pcmk__server_ipc_name(pcmk_ipc_fenced), QB_IPC_SHM, cb, QB_LOOP_HIGH); @@ -1146,6 +1155,8 @@ void pcmk__serve_pacemakerd_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { + pcmk__assert((ipcs != NULL) && (*ipcs == NULL) && (cb != NULL)); + *ipcs = mainloop_add_ipc_server(pcmk__server_ipc_name(pcmk_ipc_pacemakerd), QB_IPC_SHM, cb); @@ -1177,6 +1188,8 @@ void pcmk__serve_schedulerd_ipc(qb_ipcs_service_t **ipcs, struct qb_ipcs_service_handlers *cb) { + pcmk__assert((ipcs != NULL) && (*ipcs == NULL) && (cb != NULL)); + *ipcs = mainloop_add_ipc_server(pcmk__server_ipc_name(pcmk_ipc_schedulerd), QB_IPC_SHM, cb); From 891a7f5378b2a4b20efeb8045488151a0134203d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 7 Jan 2026 16:50:32 -0800 Subject: [PATCH 09/22] Refactor: based: New based_{callbacks,io}_cleanup() Signed-off-by: Reid Wahl --- daemons/based/based_callbacks.c | 143 ++++++++++++++++++-------------- daemons/based/based_callbacks.h | 3 + daemons/based/based_io.c | 14 +++- daemons/based/based_io.h | 2 + lib/common/mainloop.c | 2 + 5 files changed, 100 insertions(+), 64 deletions(-) diff --git a/daemons/based/based_callbacks.c b/daemons/based/based_callbacks.c index d2e59a67373..447164670ab 100644 --- a/daemons/based/based_callbacks.c +++ b/daemons/based/based_callbacks.c @@ -39,6 +39,85 @@ static long long ping_seq = 0; static char *ping_digest = NULL; static bool ping_modified_since = false; +/*! + * \internal + * \brief Request CIB digests from all peer nodes + * + * This is used as a callback that runs 5 seconds after we modify the CIB on the + * DC. It sends a ping request to all cluster nodes. They will respond by + * sending their current digests and version info, which we will validate in + * process_ping_reply(). If their digest doesn't match, we'll sync our own CIB + * to them. This helps ensure consistency across the cluster after a CIB update. + * + * \param[in] data Ignored + * + * \return \c G_SOURCE_REMOVE (to destroy the timeout) + * + * \note It's not clear why we wait 5 seconds rather than sending the ping + * request immediately after a performing a modifying op. Perhaps it's to + * avoid overwhelming other nodes with ping requests when there are a lot + * of modifying requests in a short period. The timer restarts after + * every successful modifying op, so we send ping requests **at most** + * every 5 seconds. Or perhaps it's a remnant of legacy mode (pre-1.1.12). + * In any case, the other nodes shouldn't need time to process the + * modifying op before responding to the ping request. The ping request is + * sent after the op is sent, so it should also be received after the op + * is received. + */ +static gboolean +digest_timer_cb(void *data) +{ + xmlNode *ping = NULL; + + if (!based_get_local_node_dc()) { + // Only the DC sends a ping + return G_SOURCE_REMOVE; + } + + if (++ping_seq < 0) { + ping_seq = 0; + } + + g_clear_pointer(&ping_digest, free); + ping_modified_since = false; + + ping = pcmk__xe_create(NULL, PCMK__XE_PING); + pcmk__xe_set(ping, PCMK__XA_T, PCMK__VALUE_CIB); + pcmk__xe_set(ping, PCMK__XA_CIB_OP, CRM_OP_PING); + pcmk__xe_set_ll(ping, PCMK__XA_CIB_PING_ID, ping_seq); + pcmk__xe_set(ping, PCMK_XA_CRM_FEATURE_SET, CRM_FEATURE_SET); + + pcmk__trace("Requesting peer digests (%lld)", ping_seq); + pcmk__cluster_send_message(NULL, pcmk_ipc_based, ping); + + pcmk__xml_free(ping); + return G_SOURCE_REMOVE; +} + +/*! + * \internal + * \brief Initialize data structures used for CIB manager callbacks + */ +void +based_callbacks_init(void) +{ + if (digest_timer == NULL) { + digest_timer = mainloop_timer_add("based_digest_timer", 5000, false, + digest_timer_cb, NULL); + } +} + +/*! + * \internal + * \brief Free data structures used for CIB manager callbacks + */ +void +based_callbacks_cleanup(void) +{ + g_clear_pointer(&digest_timer, mainloop_timer_del); + g_clear_pointer(&ping_digest, free); +} + /*! * \internal * \brief Create reply XML for a CIB request @@ -148,61 +227,6 @@ do_local_notify(const xmlNode *xml, const char *client_id, bool sync_reply, } } -/*! - * \internal - * \brief Request CIB digests from all peer nodes - * - * This is used as a callback that runs 5 seconds after we modify the CIB on the - * DC. It sends a ping request to all cluster nodes. They will respond by - * sending their current digests and version info, which we will validate in - * process_ping_reply(). If their digest doesn't match, we'll sync our own CIB - * to them. This helps ensure consistency across the cluster after a CIB update. - * - * \param[in] data Ignored - * - * \return \c G_SOURCE_REMOVE (to destroy the timeout) - * - * \note It's not clear why we wait 5 seconds rather than sending the ping - * request immediately after a performing a modifying op. Perhaps it's to - * avoid overwhelming other nodes with ping requests when there are a lot - * of modifying requests in a short period. The timer restarts after - * every successful modifying op, so we send ping requests **at most** - * every 5 seconds. Or perhaps it's a remnant of legacy mode (pre-1.1.12). - * In any case, the other nodes shouldn't need time to process the - * modifying op before responding to the ping request. The ping request is - * sent after the op is sent, so it should also be received after the op - * is received. - */ -static gboolean -digest_timer_cb(void *data) -{ - xmlNode *ping = NULL; - - if (!based_get_local_node_dc()) { - // Only the DC sends a ping - return G_SOURCE_REMOVE; - } - - if (++ping_seq < 0) { - ping_seq = 0; - } - - g_clear_pointer(&ping_digest, free); - ping_modified_since = false; - - ping = pcmk__xe_create(NULL, PCMK__XE_PING); - pcmk__xe_set(ping, PCMK__XA_T, PCMK__VALUE_CIB); - pcmk__xe_set(ping, PCMK__XA_CIB_OP, CRM_OP_PING); - pcmk__xe_set_ll(ping, PCMK__XA_CIB_PING_ID, ping_seq); - pcmk__xe_set(ping, PCMK_XA_CRM_FEATURE_SET, CRM_FEATURE_SET); - - pcmk__trace("Requesting peer digests (%lld)", ping_seq); - pcmk__cluster_send_message(NULL, pcmk_ipc_based, ping); - - pcmk__xml_free(ping); - return G_SOURCE_REMOVE; -} - /*! * \internal * \brief Process a reply to a \c CRM_OP_PING request @@ -542,11 +566,6 @@ based_perform_op_rw(xmlNode *request, const cib__operation_t *operation, ping_modified_since = true; } - if (digest_timer == NULL) { - digest_timer = mainloop_timer_add("based_digest_timer", 5000, false, - digest_timer_cb, NULL); - } - mainloop_timer_start(digest_timer); done: @@ -822,11 +841,11 @@ based_process_request(xmlNode *request, bool privileged, void based_terminate(crm_exit_t exit_status) { + based_callbacks_cleanup(); + based_io_cleanup(); based_ipc_cleanup(); based_remote_cleanup(); - g_clear_pointer(&digest_timer, mainloop_timer_del); - g_clear_pointer(&ping_digest, free); g_clear_pointer(&based_cib, pcmk__xml_free); // Exit immediately on error diff --git a/daemons/based/based_callbacks.h b/daemons/based/based_callbacks.h index 3e71e2f2984..2a1387fb275 100644 --- a/daemons/based/based_callbacks.h +++ b/daemons/based/based_callbacks.h @@ -17,6 +17,9 @@ #include // pcmk__client_t #include // crm_exit_t +void based_callbacks_init(void); +void based_callbacks_cleanup(void); + int based_process_request(xmlNode *request, bool privileged, const pcmk__client_t *client); void based_terminate(crm_exit_t exit_status); diff --git a/daemons/based/based_io.c b/daemons/based/based_io.c index 2f30e4d522c..5e6a12153ab 100644 --- a/daemons/based/based_io.c +++ b/daemons/based/based_io.c @@ -31,7 +31,7 @@ #include // createEmptyCib #include // pcmk__assert_asprintf, PCMK__XE_*, etc. #include // CRM_CHECK -#include // mainloop_add_signal +#include // mainloop_* #include // pcmk_legacy2rc, pcmk_rc_* #include // pcmk_common_cleanup #include // PCMK_XA_*, PCMK_XE_* @@ -176,7 +176,7 @@ based_enable_writes(int nsig) /*! * \internal - * \brief Initialize data structures for \c pacemaker-based I/O + * \brief Initialize data structures used for CIB manager I/O */ void based_io_init(void) @@ -198,6 +198,16 @@ based_io_init(void) write_trigger = mainloop_add_trigger(G_PRIORITY_LOW, write_cib_async, NULL); } +/*! + * \internal + * \brief Free data structures used for CIB manager I/O + */ +void +based_io_cleanup(void) +{ + g_clear_pointer(&write_trigger, mainloop_destroy_trigger); +} + /*! * \internal * \brief Rename a CIB or digest file after digest mismatch diff --git a/daemons/based/based_io.h b/daemons/based/based_io.h index f9bb5011a1a..706d476d026 100644 --- a/daemons/based/based_io.h +++ b/daemons/based/based_io.h @@ -15,6 +15,8 @@ #include // xmlNode void based_io_init(void); +void based_io_cleanup(void); + void based_enable_writes(int nsig); xmlNode *based_read_cib(void); int based_activate_cib(xmlNode *new_cib, bool to_disk, const char *op); diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index a1525d9b8d3..6f70d58376d 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -335,6 +335,8 @@ mainloop_destroy_signal_entry(int sig) * \note The true signal handler merely sets a mainloop trigger to call this * dispatch function via the mainloop. Therefore, the dispatch function * does not need to be async-safe. + * \note The added signal handler gets freed by \c mainloop_cleanup() if it is + * not freed manually using \c mainloop_destroy_signal(). */ gboolean mainloop_add_signal(int sig, void (*dispatch) (int sig)) From 2efa004100e229ebe8fe8e2d948a110e39125054 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 7 Jan 2026 17:01:03 -0800 Subject: [PATCH 10/22] Refactor: based: Move based_terminate() to pacemaker-based.c And make the mainloop variable static. No other code changes. Signed-off-by: Reid Wahl --- daemons/based/based_callbacks.c | 34 +----------------------------- daemons/based/based_callbacks.h | 2 -- daemons/based/pacemaker-based.c | 37 ++++++++++++++++++++++++++++++--- daemons/based/pacemaker-based.h | 8 ++++--- 4 files changed, 40 insertions(+), 41 deletions(-) diff --git a/daemons/based/based_callbacks.c b/daemons/based/based_callbacks.c index 447164670ab..71679d43275 100644 --- a/daemons/based/based_callbacks.c +++ b/daemons/based/based_callbacks.c @@ -28,7 +28,7 @@ #include // crm_ipc_*, pcmk_ipc_* #include // CRM_LOG_ASSERT, CRM_CHECK #include // mainloop_* -#include // CRM_EX_OK, crm_exit_t, pcmk_rc_* +#include // pcmk_rc_* #include // PCMK_XA_*, PCMK_XE_* #include // CRM_OP_* @@ -831,35 +831,3 @@ based_process_request(xmlNode *request, bool privileged, pcmk__xml_free(reply); return rc; } - -/*! - * \internal - * \brief Close remote sockets, free the global CIB and quit - * - * \param[in] exit_status Exit code - */ -void -based_terminate(crm_exit_t exit_status) -{ - based_callbacks_cleanup(); - based_io_cleanup(); - based_ipc_cleanup(); - based_remote_cleanup(); - - g_clear_pointer(&based_cib, pcmk__xml_free); - - // Exit immediately on error - if (exit_status != CRM_EX_OK) { - crm_exit(exit_status); - return; - } - - based_cluster_disconnect(); - - if ((mainloop != NULL) && g_main_loop_is_running(mainloop)) { - g_main_loop_quit(mainloop); - return; - } - - crm_exit(CRM_EX_OK); -} diff --git a/daemons/based/based_callbacks.h b/daemons/based/based_callbacks.h index 2a1387fb275..7363e7ab2d6 100644 --- a/daemons/based/based_callbacks.h +++ b/daemons/based/based_callbacks.h @@ -15,13 +15,11 @@ #include // xmlNode #include // pcmk__client_t -#include // crm_exit_t void based_callbacks_init(void); void based_callbacks_cleanup(void); int based_process_request(xmlNode *request, bool privileged, const pcmk__client_t *client); -void based_terminate(crm_exit_t exit_status); #endif // BASED_CALLBACKS__H diff --git a/daemons/based/pacemaker-based.c b/daemons/based/pacemaker-based.c index 65653ab517c..a6362d10df3 100644 --- a/daemons/based/pacemaker-based.c +++ b/daemons/based/pacemaker-based.c @@ -26,7 +26,7 @@ #include // crm_ipc_* #include // crm_log_* #include // mainloop_add_signal -#include // CRM_EX_*, pcmk_rc_* +#include // CRM_EX_*, crm_exit_t, pcmk_rc_* #include "pacemaker-based.h" @@ -48,14 +48,13 @@ xmlNode *based_cib = NULL; int cib_status = pcmk_rc_ok; - -GMainLoop *mainloop = NULL; gchar *cib_root = NULL; static bool local_node_dc = false; static bool shutting_down = false; static gboolean stand_alone = FALSE; static crm_exit_t exit_code = CRM_EX_OK; +static GMainLoop *mainloop = NULL; /*! * \internal @@ -221,6 +220,38 @@ build_arg_context(pcmk__common_args_t *args, GOptionGroup **group) return context; } +/*! + * \internal + * \brief Close remote sockets, free the global CIB and quit + * + * \param[in] exit_status Exit code + */ +void +based_terminate(crm_exit_t exit_status) +{ + based_callbacks_cleanup(); + based_io_cleanup(); + based_ipc_cleanup(); + based_remote_cleanup(); + + g_clear_pointer(&based_cib, pcmk__xml_free); + + // Exit immediately on error + if (exit_status != CRM_EX_OK) { + crm_exit(exit_status); + return; + } + + based_cluster_disconnect(); + + if ((mainloop != NULL) && g_main_loop_is_running(mainloop)) { + g_main_loop_quit(mainloop); + return; + } + + crm_exit(CRM_EX_OK); +} + static void based_shutdown(int nsig) { diff --git a/daemons/based/pacemaker-based.h b/daemons/based/pacemaker-based.h index 9922ee462e6..b4c53ded680 100644 --- a/daemons/based/pacemaker-based.h +++ b/daemons/based/pacemaker-based.h @@ -12,7 +12,9 @@ #include -#include // gchar, GMainLoop +#include // gchar + +#include // crm_exit_t #include "based_callbacks.h" #include "based_corosync.h" @@ -25,8 +27,6 @@ #include "based_transaction.h" extern xmlNode *based_cib; - -extern GMainLoop *mainloop; extern gchar *cib_root; extern int cib_status; @@ -36,4 +36,6 @@ void based_set_local_node_dc(bool value); bool based_shutting_down(void); bool based_stand_alone(void); +void based_terminate(crm_exit_t exit_status); + #endif // PACEMAKER_BASED__H From 5e6bb01140af97c8f15c05a4872486740d63d416 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 7 Jan 2026 17:57:28 -0800 Subject: [PATCH 11/22] Refactor: based: New based_cleanup() There is still more work to do to consolidate/reorganize the based exit/shutdown-related code. I'm keeping the pieces small to try to make it easier to reason about. based_terminate() seems too complicated. Note that based_ipc_cleanup() calls pcmk__client_cleanup(), which is why we drop the call from main. Signed-off-by: Reid Wahl --- daemons/based/pacemaker-based.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/daemons/based/pacemaker-based.c b/daemons/based/pacemaker-based.c index a6362d10df3..db769b972cf 100644 --- a/daemons/based/pacemaker-based.c +++ b/daemons/based/pacemaker-based.c @@ -222,12 +222,10 @@ build_arg_context(pcmk__common_args_t *args, GOptionGroup **group) /*! * \internal - * \brief Close remote sockets, free the global CIB and quit - * - * \param[in] exit_status Exit code + * \brief Clean up CIB manager data structures */ -void -based_terminate(crm_exit_t exit_status) +static void +based_cleanup(void) { based_callbacks_cleanup(); based_io_cleanup(); @@ -235,6 +233,19 @@ based_terminate(crm_exit_t exit_status) based_remote_cleanup(); g_clear_pointer(&based_cib, pcmk__xml_free); + g_clear_pointer(&cib_root, g_free); +} + +/*! + * \internal + * \brief Clean up data structures and exit + * + * \param[in] exit_status Exit code + */ +void +based_terminate(crm_exit_t exit_status) +{ + based_cleanup(); // Exit immediately on error if (exit_status != CRM_EX_OK) { @@ -399,9 +410,8 @@ main(int argc, char **argv) g_strfreev(processed_args); pcmk__free_arg_context(context); - pcmk__client_cleanup(); based_cluster_disconnect(); - g_free(cib_root); + based_cleanup(); pcmk__output_and_clear_error(&error, out); From b1e23cab331e7b24fa8d541b7567290ed9ba0440 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 7 Jan 2026 23:36:12 -0800 Subject: [PATCH 12/22] Refactor: based: Assume main loop is running in based_terminate() There are two call sites. * based_cpg_destroy: This is called only via the main loop. See pcmk_cluster_set_destroy_fn() and pcmk__cpg_connect(). * based_shutdown: We can reach this only through the main loop. It's set up as a main loop signal handler via mainloop_add_signal(). The true signal handler is mainloop_signal_handler(), which sets a main loop trigger to call based_shutdown(). That trigger can't do anything unless the main loop is running. Signed-off-by: Reid Wahl --- daemons/based/pacemaker-based.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/daemons/based/pacemaker-based.c b/daemons/based/pacemaker-based.c index db769b972cf..80bb36f91f9 100644 --- a/daemons/based/pacemaker-based.c +++ b/daemons/based/pacemaker-based.c @@ -255,12 +255,11 @@ based_terminate(crm_exit_t exit_status) based_cluster_disconnect(); - if ((mainloop != NULL) && g_main_loop_is_running(mainloop)) { - g_main_loop_quit(mainloop); - return; - } + // There should be no way to get here without the main loop running + CRM_CHECK((mainloop != NULL) && g_main_loop_is_running(mainloop), + crm_exit(exit_status)); - crm_exit(CRM_EX_OK); + g_main_loop_quit(mainloop); } static void From 8c90b73fe43505a59ba7ca83e49fcd50789ecfdb Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 8 Jan 2026 01:00:21 -0800 Subject: [PATCH 13/22] Doc: based: Add notes to based_terminate() Signed-off-by: Reid Wahl --- daemons/based/pacemaker-based.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/daemons/based/pacemaker-based.c b/daemons/based/pacemaker-based.c index 80bb36f91f9..d92f1e9621d 100644 --- a/daemons/based/pacemaker-based.c +++ b/daemons/based/pacemaker-based.c @@ -247,10 +247,17 @@ based_terminate(crm_exit_t exit_status) { based_cleanup(); - // Exit immediately on error if (exit_status != CRM_EX_OK) { + /* After calling g_main_loop_quit(), sources that have already been + * dispatched are still executed. On error, skip that and exit + * immediately after cleaning up data structures. + * + * @TODO Is this necessary? It would be nice to do the cleanup at the + * end of main(). If so, then one (complicated) option would be to keep + * track of all main loop sources and destroy them so that + * g_main_dispatch() ignores them. + */ crm_exit(exit_status); - return; } based_cluster_disconnect(); From 0674c2c60280400f50d6648b022d494c001ceb14 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 8 Jan 2026 01:08:42 -0800 Subject: [PATCH 14/22] Refactor: based: Set shutting_down to true in based_terminate() Mostly for sanity. Between the g_main_loop_quit() call and the main loop actually stopping, the main loop runs callbacks for any sources that have already been dispatched. In case some callback actually runs, it should be able to call based_shutting_down() and get a true return value. Signed-off-by: Reid Wahl --- daemons/based/pacemaker-based.c | 1 + 1 file changed, 1 insertion(+) diff --git a/daemons/based/pacemaker-based.c b/daemons/based/pacemaker-based.c index d92f1e9621d..fefc431a5bc 100644 --- a/daemons/based/pacemaker-based.c +++ b/daemons/based/pacemaker-based.c @@ -245,6 +245,7 @@ based_cleanup(void) void based_terminate(crm_exit_t exit_status) { + shutting_down = true; based_cleanup(); if (exit_status != CRM_EX_OK) { From d95a59720f42be88b12c69e687246c72b21110ec Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 9 Jan 2026 00:01:48 -0800 Subject: [PATCH 15/22] Low: based: Fix memory leak in remote listeners We were allocating an int to hold the server socket file descriptor, and we were never freeing it. (Its destroy callback, based_remote_listener_destroy(), did not free it.) Don't allocate an int, but instead call GINT_TO_POINTER() and GPOINTER_TO_INT() to cast it to and from a pointer. Signed-off-by: Reid Wahl --- daemons/based/based_remote.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/daemons/based/based_remote.c b/daemons/based/based_remote.c index 035ece5d3bd..a978a450fa2 100644 --- a/daemons/based/based_remote.c +++ b/daemons/based/based_remote.c @@ -547,13 +547,13 @@ based_remote_client_destroy(void *user_data) } static int -cib_remote_listen(void *data) +cib_remote_listen(void *user_data) { + int ssock = GPOINTER_TO_INT(user_data); int csock = -1; unsigned laddr; struct sockaddr_storage addr; char ipstr[INET6_ADDRSTRLEN]; - int ssock = *(int *)data; int rc; pcmk__client_t *new_client = NULL; @@ -630,7 +630,7 @@ static int init_remote_listener(int port) { int rc; - int *ssock = NULL; + int ssock = -1; struct sockaddr_in saddr; int optval; @@ -645,17 +645,15 @@ init_remote_listener(int port) #endif /* create server socket */ - ssock = pcmk__assert_alloc(1, sizeof(int)); - *ssock = socket(AF_INET, SOCK_STREAM, 0); - if (*ssock == -1) { + ssock = socket(AF_INET, SOCK_STREAM, 0); + if (ssock == -1) { pcmk__err("Listener socket creation failed: %s", pcmk_rc_str(errno)); - free(ssock); return -1; } /* reuse address */ optval = 1; - rc = setsockopt(*ssock, SOL_SOCKET, SO_REUSEADDR, &optval, sizeof(optval)); + rc = setsockopt(ssock, SOL_SOCKET, SO_REUSEADDR, &optval, sizeof(optval)); if (rc < 0) { pcmk__err("Local address reuse not allowed on listener socket: %s", pcmk_rc_str(errno)); @@ -666,23 +664,22 @@ init_remote_listener(int port) saddr.sin_family = AF_INET; saddr.sin_addr.s_addr = INADDR_ANY; saddr.sin_port = htons(port); - if (bind(*ssock, (struct sockaddr *)&saddr, sizeof(saddr)) == -1) { + if (bind(ssock, (struct sockaddr *)&saddr, sizeof(saddr)) == -1) { pcmk__err("Cannot bind to listener socket: %s", pcmk_rc_str(errno)); - close(*ssock); - free(ssock); + close(ssock); return -2; } - if (listen(*ssock, 10) == -1) { + if (listen(ssock, 10) == -1) { pcmk__err("Cannot listen on socket: %s", pcmk_rc_str(errno)); - close(*ssock); - free(ssock); + close(ssock); return -3; } - mainloop_add_fd("cib-remote", G_PRIORITY_DEFAULT, *ssock, ssock, &remote_listen_fd_callbacks); + mainloop_add_fd("based-remote-listener", G_PRIORITY_DEFAULT, ssock, + GINT_TO_POINTER(ssock), &remote_listen_fd_callbacks); pcmk__debug("Started listener on port %d", port); - return *ssock; + return ssock; } /*! From c58ff59191f94fded690e4dd21d01b732417d7e9 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 9 Jan 2026 00:22:39 -0800 Subject: [PATCH 16/22] Refactor: libcrmcommon: Expose struct mainloop_io_s internally We will use this soon. Signed-off-by: Reid Wahl --- include/crm/common/mainloop_internal.h | 14 ++++++++++++++ lib/common/mainloop.c | 14 -------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/crm/common/mainloop_internal.h b/include/crm/common/mainloop_internal.h index d16aab01fbb..70cf3da79bb 100644 --- a/include/crm/common/mainloop_internal.h +++ b/include/crm/common/mainloop_internal.h @@ -38,6 +38,20 @@ struct mainloop_child_s { pcmk__mainloop_child_exit_fn_t exit_fn; }; +struct mainloop_io_s { + char *name; + void *userdata; + + int fd; + unsigned int source; + crm_ipc_t *ipc; + GIOChannel *channel; + + int (*dispatch_fn_ipc)(const char *buffer, ssize_t length, void *user_data); + int (*dispatch_fn_io)(void *user_data); + void (*destroy_fn)(void *user_data); +}; + int pcmk__add_mainloop_ipc(crm_ipc_t *ipc, int priority, void *userdata, const struct ipc_client_callbacks *callbacks, mainloop_io_t **source); diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index 6f70d58376d..39e680e609e 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -647,20 +647,6 @@ mainloop_del_ipc_server(qb_ipcs_service_t * server) } } -struct mainloop_io_s { - char *name; - void *userdata; - - int fd; - unsigned int source; - crm_ipc_t *ipc; - GIOChannel *channel; - - int (*dispatch_fn_ipc)(const char *buffer, ssize_t length, void *userdata); - int (*dispatch_fn_io)(void *userdata); - void (*destroy_fn)(void *userdata); -}; - /*! * \internal * \brief I/O watch callback function (GIOFunc) From c1baa15c87a0750614de3cd85d89c817f7dde4cd Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 9 Jan 2026 00:15:32 -0800 Subject: [PATCH 17/22] Low: based: Destroy remote listeners in based_remote_cleanup() Previously we were only closing the file descriptors. Instead we destroy the mainloop_io_t objects returned by mainloop_add_fd(). In order to do that, we need to start storing those objects in the first place. Previously we were ignoring the return value of mainloop_add_fd() in init_remote_listener(). Signed-off-by: Reid Wahl --- daemons/based/based_remote.c | 59 ++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/daemons/based/based_remote.c b/daemons/based/based_remote.c index a978a450fa2..7695ea8947b 100644 --- a/daemons/based/based_remote.c +++ b/daemons/based/based_remote.c @@ -48,8 +48,8 @@ static pcmk__tls_t *tls = NULL; -static int remote_fd = -1; -static int remote_tls_fd = -1; +static mainloop_io_t *tcp_listener = NULL; +static mainloop_io_t *tls_listener = NULL; // @TODO This is rather short for someone to type their password #define REMOTE_AUTH_TIMEOUT 10000 @@ -550,6 +550,8 @@ static int cib_remote_listen(void *user_data) { int ssock = GPOINTER_TO_INT(user_data); + const bool is_tls = (tls_listener != NULL) && (ssock == tls_listener->fd); + int csock = -1; unsigned laddr; struct sockaddr_storage addr; @@ -587,7 +589,7 @@ cib_remote_listen(void *user_data) new_client = pcmk__new_unauth_client(NULL); new_client->remote = pcmk__assert_alloc(1, sizeof(pcmk__remote_t)); - if (ssock == remote_tls_fd) { + if (is_tls) { pcmk__set_client_flags(new_client, pcmk__client_tls); /* create gnutls session for the server socket */ @@ -610,8 +612,7 @@ cib_remote_listen(void *user_data) remote_auth_timeout_cb, new_client); pcmk__info("%s connection from %s pending authentication for client %s", - ((ssock == remote_tls_fd)? "Encrypted" : "Clear-text"), ipstr, - new_client->id); + (is_tls? "Encrypted" : "Clear-text"), ipstr, new_client->id); new_client->remote->source = mainloop_add_fd("cib-remote-client", G_PRIORITY_DEFAULT, csock, new_client, @@ -626,13 +627,14 @@ based_remote_listener_destroy(void *user_data) pcmk__info("No longer listening for remote connections"); } -static int +static mainloop_io_t * init_remote_listener(int port) { int rc; int ssock = -1; struct sockaddr_in saddr; int optval; + mainloop_io_t *listener = NULL; static struct mainloop_fd_callbacks remote_listen_fd_callbacks = { .dispatch = cib_remote_listen, @@ -647,8 +649,8 @@ init_remote_listener(int port) /* create server socket */ ssock = socket(AF_INET, SOCK_STREAM, 0); if (ssock == -1) { - pcmk__err("Listener socket creation failed: %s", pcmk_rc_str(errno)); - return -1; + pcmk__err("Listener socket creation failed: %s", strerror(errno)); + return NULL; } /* reuse address */ @@ -656,7 +658,7 @@ init_remote_listener(int port) rc = setsockopt(ssock, SOL_SOCKET, SO_REUSEADDR, &optval, sizeof(optval)); if (rc < 0) { pcmk__err("Local address reuse not allowed on listener socket: %s", - pcmk_rc_str(errno)); + strerror(errno)); } /* bind server socket */ @@ -665,21 +667,27 @@ init_remote_listener(int port) saddr.sin_addr.s_addr = INADDR_ANY; saddr.sin_port = htons(port); if (bind(ssock, (struct sockaddr *)&saddr, sizeof(saddr)) == -1) { - pcmk__err("Cannot bind to listener socket: %s", pcmk_rc_str(errno)); + pcmk__err("Cannot bind to listener socket: %s", strerror(errno)); close(ssock); - return -2; + return NULL; } if (listen(ssock, 10) == -1) { - pcmk__err("Cannot listen on socket: %s", pcmk_rc_str(errno)); + pcmk__err("Cannot listen on socket: %s", strerror(errno)); close(ssock); - return -3; + return NULL; } - mainloop_add_fd("based-remote-listener", G_PRIORITY_DEFAULT, ssock, - GINT_TO_POINTER(ssock), &remote_listen_fd_callbacks); - pcmk__debug("Started listener on port %d", port); + listener = mainloop_add_fd("based-remote-listener", G_PRIORITY_DEFAULT, + ssock, GINT_TO_POINTER(ssock), + &remote_listen_fd_callbacks); + if (listener == NULL) { + pcmk__err("Cannot add pacemaker-based remote listener (port %d) to " + "mainloop: %s", port, strerror(errno)); + return NULL; + } - return ssock; + pcmk__debug("Started pacemaker-based remote listener on port %d", port); + return listener; } /*! @@ -731,8 +739,6 @@ based_remote_init(void) if (rc != pcmk_rc_ok) { pcmk__err("Failed to initialize TLS: %s. Not starting TLS listener ", "on port %d", pcmk_rc_str(rc), port); - - remote_tls_fd = -1; goto try_clear_port; } @@ -742,7 +748,7 @@ based_remote_init(void) } pcmk__notice("Starting TLS listener on port %d", port); - remote_tls_fd = init_remote_listener(port); + tls_listener = init_remote_listener(port); try_clear_port: /* Regardless of whether or not we successfully enabled remote-tls-port, @@ -754,7 +760,7 @@ based_remote_init(void) pcmk__warn("Starting clear-text listener on port %d. This is insecure " "and will be removed in a future release. Use " PCMK_XA_REMOTE_TLS_PORT " instead.", port); - remote_fd = init_remote_listener(port); + tcp_listener = init_remote_listener(port); } } @@ -769,15 +775,8 @@ based_remote_init(void) void based_remote_cleanup(void) { - if (remote_fd >= 0) { - close(remote_fd); - remote_fd = -1; - } - - if (remote_tls_fd >= 0) { - close(remote_tls_fd); - remote_tls_fd = -1; - } + g_clear_pointer(&tcp_listener, mainloop_del_fd); + g_clear_pointer(&tls_listener, mainloop_del_fd); } /*! From 098641f08852326cc523c4a6743793ec439ad633 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 9 Jan 2026 21:19:23 -0800 Subject: [PATCH 18/22] Refactor: libcrmcommon: Free mainloop child list at exit Signed-off-by: Reid Wahl --- lib/common/mainloop.c | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index 39e680e609e..cf157fe3b49 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -29,7 +29,6 @@ struct trigger_s { gboolean trigger; void *user_data; unsigned int id; - }; struct mainloop_timer_s { @@ -41,6 +40,21 @@ struct mainloop_timer_s { void *userdata; }; +static GList *child_list = NULL; +static qb_array_t *gio_map = NULL; + +static void +child_free(mainloop_child_t *child) +{ + if (child->timerid != 0) { + pcmk__trace("Removing timer %d", child->timerid); + g_source_remove(child->timerid); + child->timerid = 0; + } + free(child->desc); + free(child); +} + static gboolean crm_trigger_prepare(GSource *source, int *timeout) { @@ -400,11 +414,18 @@ mainloop_destroy_signal(int sig) return TRUE; } -static qb_array_t *gio_map = NULL; - +/*! + * \internal + * \brief Free data structures used for the mainloop + * + * \todo This is incomplete. Free other data structures created in this file. + */ void mainloop_cleanup(void) { + g_list_free_full(child_list, (GDestroyNotify) child_free); + child_list = NULL; + g_clear_pointer(&gio_map, qb_array_free); for (int sig = 0; sig < NSIG; ++sig) { @@ -969,8 +990,6 @@ mainloop_del_fd(mainloop_io_t *client) g_source_remove(client->source); } -static GList *child_list = NULL; - pid_t mainloop_child_pid(mainloop_child_t * child) { @@ -1001,20 +1020,6 @@ mainloop_clear_child_userdata(mainloop_child_t * child) child->privatedata = NULL; } -/* good function name */ -static void -child_free(mainloop_child_t *child) -{ - if (child->timerid != 0) { - pcmk__trace("Removing timer %d", child->timerid); - g_source_remove(child->timerid); - child->timerid = 0; - } - free(child->desc); - free(child); -} - -/* terrible function name */ static int child_kill_helper(mainloop_child_t *child) { From 551ae76cfd61666e67b7ae6874f39aa98aa805d1 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 9 Jan 2026 22:09:34 -0800 Subject: [PATCH 19/22] Refactor: libcrmcommon: Drop callback_needed variable Return false early where possible instead. Signed-off-by: Reid Wahl --- lib/common/mainloop.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index cf157fe3b49..db103624165 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -1078,15 +1078,16 @@ child_waitpid(mainloop_child_t *child, int flags) int signo = 0; int status = 0; int exitcode = 0; - bool callback_needed = true; rc = waitpid(child->pid, &status, flags); + if (rc == 0) { // WNOHANG in flags, and child status is not available pcmk__trace("Child process %lld (%s) still active", (long long) child->pid, child->desc); - callback_needed = false; + return false; + } - } else if (rc != child->pid) { + if (rc != child->pid) { /* According to POSIX, possible conditions: * - child->pid was non-positive (process group or any child), * and rc is specific child @@ -1098,8 +1099,8 @@ child_waitpid(mainloop_child_t *child, int flags) */ signo = SIGCHLD; exitcode = 1; - pcmk__notice("Wait for child process %d (%s) interrupted: %s", - child->pid, child->desc, pcmk_rc_str(errno)); + pcmk__notice("Wait for child process %lld (%s) interrupted: %s", + (long long) child->pid, child->desc, strerror(errno)); } else if (WIFEXITED(status)) { exitcode = WEXITSTATUS(status); @@ -1115,19 +1116,21 @@ child_waitpid(mainloop_child_t *child, int flags) #ifdef WCOREDUMP // AIX, SunOS, maybe others } else if (WCOREDUMP(status)) { core = 1; - pcmk__err("Child process %d (%s) dumped core", child->pid, child->desc); + pcmk__err("Child process %lld (%s) dumped core", (long long) child->pid, + child->desc); #endif } else { // flags must contain WUNTRACED and/or WCONTINUED to reach this pcmk__trace("Child process %lld (%s) stopped or continued", (long long) child->pid, child->desc); - callback_needed = false; + return false; } - if (callback_needed && child->exit_fn) { + if (child->exit_fn != NULL) { child->exit_fn(child, core, signo, exitcode); } - return callback_needed; + + return true; } static void From 6a03bf583c9f928192dddf083aafa38eb29951d1 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 9 Jan 2026 22:20:52 -0800 Subject: [PATCH 20/22] Refactor: libcrmcommon: child_kill_helper() returns standard code Also take a const argument. This function may kill a process, but it doesn't modify the mainloop_child_t. Signed-off-by: Reid Wahl --- lib/common/mainloop.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index db103624165..fca4eee1293 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -1021,33 +1021,40 @@ mainloop_clear_child_userdata(mainloop_child_t * child) } static int -child_kill_helper(mainloop_child_t *child) +child_kill_helper(const mainloop_child_t *child) { - int rc; - if (child->flags & mainloop_leave_pid_group) { + int rc = 0; + + if (pcmk__is_set(child->flags, mainloop_leave_pid_group)) { pcmk__debug("Killing PID %lld only. Leaving its process group intact.", (long long) child->pid); rc = kill(child->pid, SIGKILL); + } else { pcmk__debug("Killing PID %lld's entire process group", (long long) child->pid); rc = kill(-child->pid, SIGKILL); } - if (rc < 0) { - if (errno != ESRCH) { - pcmk__err("kill(%d, KILL) failed: %s", child->pid, strerror(errno)); - } - return -errno; + if (rc == 0) { + return pcmk_rc_ok; } - return 0; + + rc = errno; + if (rc == ESRCH) { + return rc; + } + + pcmk__err("kill(%lld, KILL) failed: %s", (long long) child->pid, + strerror(rc)); + return rc; } static gboolean child_timeout_callback(void *p) { mainloop_child_t *child = p; - int rc = 0; + int rc = pcmk_rc_ok; child->timerid = 0; if (child->timeout) { @@ -1057,7 +1064,7 @@ child_timeout_callback(void *p) } rc = child_kill_helper(child); - if (rc == -ESRCH) { + if (rc == ESRCH) { /* Nothing left to do. pid doesn't exist */ return FALSE; } @@ -1171,7 +1178,8 @@ mainloop_child_kill(pid_t pid) mainloop_child_t *match = NULL; /* It is impossible to block SIGKILL, this allows us to * call waitpid without WNOHANG flag.*/ - int waitflags = 0, rc = 0; + int waitflags = 0; + int rc = pcmk_rc_ok; for (iter = child_list; iter != NULL && match == NULL; iter = iter->next) { child = iter->data; @@ -1185,7 +1193,7 @@ mainloop_child_kill(pid_t pid) } rc = child_kill_helper(match); - if(rc == -ESRCH) { + if (rc == ESRCH) { /* It's gone, but hasn't shown up in waitpid() yet. Wait until we get * SIGCHLD and let handler clean it up as normal (so we get the correct * return code/status). The blocking alternative would be to call @@ -1194,10 +1202,11 @@ mainloop_child_kill(pid_t pid) pcmk__trace("Waiting for signal that child process %lld completed", (long long) match->pid); return TRUE; + } - } else if(rc != 0) { - /* If KILL for some other reason set the WNOHANG flag since we - * can't be certain what happened. + if (rc != pcmk_rc_ok) { + /* If kill() failed for some other reason, set the WNOHANG flag, since + * we can't be certain what happened. */ waitflags = WNOHANG; } From f7019de72dd0510c2d7dc53ca55b99a7a33ba4d5 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 9 Jan 2026 22:01:22 -0800 Subject: [PATCH 21/22] Refactor: libcrmcommon: Track mainloop children in a hash table We index it by stringified PID. pid_t is specified as a signed integer type, but there's no guarantee that it's a 32-bit integer. So GINT_TO_POINTER() is technically unsafe (although it's likely fine in practice). The semantics of using a hash table seem like a better fit for looking up a child record by PID. Removing an entry is also simpler, and we can set the destroy function when we create the table. Now that I've looked at most of the mainloop child code, I REALLY want to start using GSubprocess if possible. Note: I started working on this so that the CIB manager could free a mainloop_child_t that it created with mainloop_child_add(), without calling mainloop_child_kill() and killing it. I now realize this is not necessary, since mainloop_cleanup() frees any children that we haven't already freed. However, a hash table still seems like a better data structure for storing these children. Signed-off-by: Reid Wahl --- lib/common/mainloop.c | 122 +++++++++++++++++++++++++++--------------- 1 file changed, 80 insertions(+), 42 deletions(-) diff --git a/lib/common/mainloop.c b/lib/common/mainloop.c index fca4eee1293..ca650284a95 100644 --- a/lib/common/mainloop.c +++ b/lib/common/mainloop.c @@ -40,7 +40,7 @@ struct mainloop_timer_s { void *userdata; }; -static GList *child_list = NULL; +static GHashTable *mainloop_children = NULL; static qb_array_t *gio_map = NULL; static void @@ -423,9 +423,7 @@ mainloop_destroy_signal(int sig) void mainloop_cleanup(void) { - g_list_free_full(child_list, (GDestroyNotify) child_free); - child_list = NULL; - + g_clear_pointer(&mainloop_children, g_hash_table_destroy); g_clear_pointer(&gio_map, qb_array_free); for (int sig = 0; sig < NSIG; ++sig) { @@ -1140,22 +1138,28 @@ child_waitpid(mainloop_child_t *child, int flags) return true; } +static gboolean +child_waitpid_no_hang(void *key, void *value, void *user_data) +{ + mainloop_child_t *child = value; + + if (!child_waitpid(child, WNOHANG)) { + return FALSE; + } + + pcmk__trace("Removing completed process %lld from child table", + (long long) child->pid); + return TRUE; +} + static void child_death_dispatch(int signal) { - for (GList *iter = child_list; iter; ) { - GList *saved = iter; - mainloop_child_t *child = iter->data; - - iter = iter->next; - if (child_waitpid(child, WNOHANG)) { - pcmk__trace("Removing completed process %lld from child list", - (long long) child->pid); - child_list = g_list_remove_link(child_list, saved); - g_list_free(saved); - child_free(child); - } + if (mainloop_children == NULL) { + return; } + + g_hash_table_foreach_remove(mainloop_children, child_waitpid_no_hang, NULL); } static gboolean @@ -1173,35 +1177,42 @@ child_signal_init(void *p) gboolean mainloop_child_kill(pid_t pid) { - GList *iter; - mainloop_child_t *child = NULL; - mainloop_child_t *match = NULL; - /* It is impossible to block SIGKILL, this allows us to - * call waitpid without WNOHANG flag.*/ + /* It is impossible to block SIGKILL. This allows us to call waitpid() + * without the WNOHANG flag. + */ int waitflags = 0; int rc = pcmk_rc_ok; + char *pid_s = NULL; + mainloop_child_t *child = NULL; + gboolean killed = FALSE; - for (iter = child_list; iter != NULL && match == NULL; iter = iter->next) { - child = iter->data; - if (pid == child->pid) { - match = child; - } + if (mainloop_children == NULL) { + // We're not tracking any children, so don't kill this PID + goto done; } - if (match == NULL) { - return FALSE; + pid_s = pcmk__assert_asprintf("%lld", (long long) pid); + child = g_hash_table_lookup(mainloop_children, pid_s); + + if (child == NULL) { + // We're not tracking a child with this PID, so don't kill it + goto done; } - rc = child_kill_helper(match); + rc = child_kill_helper(child); if (rc == ESRCH) { - /* It's gone, but hasn't shown up in waitpid() yet. Wait until we get - * SIGCHLD and let handler clean it up as normal (so we get the correct - * return code/status). The blocking alternative would be to call - * child_waitpid(match, 0). + /* kill() didn't find the PID. Assume this means the process is in some + * intermediate stage of exiting. Wait until we get SIGCHLD and let + * the handler clean it up as normal, so that we get the correct return + * code/status. The blocking alternative would be to call + * child_waitpid(child, 0). + * + * Treat this as success in the meantime. */ pcmk__trace("Waiting for signal that child process %lld completed", - (long long) match->pid); - return TRUE; + (long long) child->pid); + killed = TRUE; + goto done; } if (rc != pcmk_rc_ok) { @@ -1211,19 +1222,34 @@ mainloop_child_kill(pid_t pid) waitflags = WNOHANG; } - if (!child_waitpid(match, waitflags)) { - /* not much we can do if this occurs */ - return FALSE; + if (!child_waitpid(child, waitflags)) { + /* waitpid() didn't find the child. This shouldn't happen if kill() + * succeeded. In any case, there's no obviously good way to proceed. + * Treat this as a failure. + * + * @TODO Should we call the child's exit_fn with a failure here, and/or + * free the mainloop_child_t? We can hope that we successfully wait on + * it in response to a SIGCHLD that comes later. But we might just never + * do anything with this child again. + * + * @TODO Consider using GSubprocess for all subprocess tracking. + */ + goto done; } - child_list = g_list_remove(child_list, match); - child_free(match); - return TRUE; + g_hash_table_remove(mainloop_children, pid_s); + killed = TRUE; + +done: + free(pid_s); + return killed; } /* Create/Log a new tracked process * To track a process group, use -pid * + * Newly created child will be freed by mainloop_cleanup(), if not sooner. + * * @TODO Using a non-positive pid (i.e. any child, or process group) would * likely not be useful since we will free the child after the first * completed process. @@ -1235,6 +1261,7 @@ mainloop_child_add_with_flags(pid_t pid, int timeout, const char *desc, pcmk__mainloop_child_exit_fn_t exit_fn) { static bool need_init = TRUE; + bool is_new = false; mainloop_child_t *child = pcmk__assert_alloc(1, sizeof(mainloop_child_t)); child->pid = pid; @@ -1249,7 +1276,17 @@ mainloop_child_add_with_flags(pid_t pid, int timeout, const char *desc, child->timerid = pcmk__create_timer(timeout, child_timeout_callback, child); } - child_list = g_list_append(child_list, child); + if (mainloop_children == NULL) { + mainloop_children = pcmk__strkey_table(free, + (GDestroyNotify) child_free); + } + + is_new = g_hash_table_insert(mainloop_children, + pcmk__assert_asprintf("%lld", (long long) pid), + child); + + // It should be impossible to have this PID in mainloop_children already + CRM_LOG_ASSERT(is_new); if(need_init) { need_init = FALSE; @@ -1261,6 +1298,7 @@ mainloop_child_add_with_flags(pid_t pid, int timeout, const char *desc, } } +// Newly created child will be freed by mainloop_cleanup(), if not sooner void mainloop_child_add(pid_t pid, int timeout, const char *desc, void *privatedata, pcmk__mainloop_child_exit_fn_t exit_fn) From 552e2ce575b5c8d45595c2ed49aaa8a8d6b962aa Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 10 Jan 2026 16:48:30 -0800 Subject: [PATCH 22/22] Refactor: based: Add missing include to pacemaker-based.c Signed-off-by: Reid Wahl --- daemons/based/pacemaker-based.c | 1 + 1 file changed, 1 insertion(+) diff --git a/daemons/based/pacemaker-based.c b/daemons/based/pacemaker-based.c index fefc431a5bc..bb5f767225a 100644 --- a/daemons/based/pacemaker-based.c +++ b/daemons/based/pacemaker-based.c @@ -23,6 +23,7 @@ #include // CRM_CONFIG_DIR, CRM_DAEMON_USER #include // pcmk__node_update, etc. +#include // PCMK__EXITC_ERROR, pcmk__err, etc. #include // crm_ipc_* #include // crm_log_* #include // mainloop_add_signal