From b26591d4489e21cdfc85099650be4fe114ebd011 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 02:20:54 -0700 Subject: [PATCH 01/24] Refactor: fencer: Best practices in update_cib_stonith_devices() * Remove some nesting. * Use const where possible. * Improve variable names and scope. * Use a macro for repeated string literal. * Use sizeof() - 1 instead of strlen() for string literal. * Remove unnecessary escaping of single quote within double quotes. * Copy only what is needed of the xpath string, if anything. * Log assertion on malformed patchset, since we created it in cib_perform_op(). * Add a comment to clarify why we're (apparently) doing all this in the first place. Keep the watchdog_device_update() comment as-is because I haven't looked into it. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 72 +++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 887486d04e7..a10777a4494 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -232,13 +232,16 @@ cib_devices_update(void) } } +#define PRIMITIVE_ID_XP_FRAGMENT "/" PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "='" + static void update_cib_stonith_devices(const char *event, xmlNode * msg) { int format = 1; - xmlNode *wrapper = pcmk__xe_first_child(msg, PCMK__XE_CIB_UPDATE_RESULT, - NULL, NULL); - xmlNode *patchset = pcmk__xe_first_child(wrapper, NULL, NULL, NULL); + const xmlNode *wrapper = pcmk__xe_first_child(msg, + PCMK__XE_CIB_UPDATE_RESULT, + NULL, NULL); + const xmlNode *patchset = pcmk__xe_first_child(wrapper, NULL, NULL, NULL); char *reason = NULL; CRM_CHECK(patchset != NULL, return); @@ -249,12 +252,12 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) return; } - for (xmlNode *change = pcmk__xe_first_child(patchset, NULL, NULL, NULL); + for (const xmlNode *change = pcmk__xe_first_child(patchset, NULL, NULL, + NULL); change != NULL; change = pcmk__xe_next(change, NULL)) { const char *op = crm_element_value(change, PCMK_XA_OPERATION); const char *xpath = crm_element_value(change, PCMK_XA_PATH); - const char *shortpath = NULL; if (pcmk__str_eq(op, PCMK_VALUE_MOVE, pcmk__str_null_matches) || (strstr(xpath, "/" PCMK_XE_STATUS) != NULL)) { @@ -264,8 +267,8 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) if (pcmk__str_eq(op, PCMK_VALUE_DELETE, pcmk__str_none) && (strstr(xpath, "/" PCMK_XE_PRIMITIVE) != NULL)) { const char *rsc_id = NULL; - char *search = NULL; - char *mutable = NULL; + const char *end_quote = NULL; + char *copy = NULL; if ((strstr(xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) || (strstr(xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { @@ -274,28 +277,43 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) "resource"); break; } - mutable = pcmk__str_copy(xpath); - rsc_id = strstr(mutable, PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "=\'"); - if (rsc_id != NULL) { - rsc_id += strlen(PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "=\'"); - search = strchr(rsc_id, '\''); + + rsc_id = strstr(xpath, PRIMITIVE_ID_XP_FRAGMENT); + if (rsc_id == NULL) { + continue; } - if (search != NULL) { - *search = 0; - stonith_device_remove(rsc_id, true); - /* watchdog_device_update called afterwards - to fall back to implicit definition if needed */ - } else { - crm_warn("Ignoring malformed CIB update (resource deletion)"); + + rsc_id += sizeof(PRIMITIVE_ID_XP_FRAGMENT) - 1; + end_quote = strchr(rsc_id, '\''); + + CRM_LOG_ASSERT(end_quote != NULL); + if (end_quote == NULL) { + crm_err("Bug: Malformed item in Pacemaker-generated patchset"); + continue; } - free(mutable); - - } else if (strstr(xpath, "/" PCMK_XE_RESOURCES) - || strstr(xpath, "/" PCMK_XE_CONSTRAINTS) - || strstr(xpath, "/" PCMK_XE_RSC_DEFAULTS)) { - shortpath = strrchr(xpath, '/'); - pcmk__assert(shortpath != NULL); - reason = crm_strdup_printf("%s %s", op, shortpath+1); + + /* A primitive resource was removed. If it was a fencing resource, + * it's faster to remove it directly than to run the scheduler and + * update all device registrations. + */ + copy = strndup(rsc_id, end_quote - rsc_id); + pcmk__assert(copy != NULL); + stonith_device_remove(copy, true); + + /* watchdog_device_update called afterwards + to fall back to implicit definition if needed */ + + free(copy); + continue; + } + + if (strstr(xpath, "/" PCMK_XE_RESOURCES) + || strstr(xpath, "/" PCMK_XE_CONSTRAINTS) + || strstr(xpath, "/" PCMK_XE_RSC_DEFAULTS)) { + + const char *shortpath = strrchr(xpath, '/'); + + reason = crm_strdup_printf("%s %s", op, shortpath + 1); break; } } From 242908a87fc76fbce8d0663b6dfd71a8046014e0 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 12:35:03 -0700 Subject: [PATCH 02/24] Refactor: fencer: Match primitive XPath only with ID An ID is required when schema validation is enabled anyway, and this allows us to be sure rsc_id will be non-NULL within the block. It also allows us to shorten some strstr() searches by starting later in the string, not that this will matter in practice. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index a10777a4494..cb673dfa6d6 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -258,32 +258,30 @@ update_cib_stonith_devices(const char *event, xmlNode * msg) const char *op = crm_element_value(change, PCMK_XA_OPERATION); const char *xpath = crm_element_value(change, PCMK_XA_PATH); + const char *primitive_xpath = NULL; if (pcmk__str_eq(op, PCMK_VALUE_MOVE, pcmk__str_null_matches) || (strstr(xpath, "/" PCMK_XE_STATUS) != NULL)) { continue; } - if (pcmk__str_eq(op, PCMK_VALUE_DELETE, pcmk__str_none) - && (strstr(xpath, "/" PCMK_XE_PRIMITIVE) != NULL)) { + primitive_xpath = strstr(xpath, PRIMITIVE_ID_XP_FRAGMENT); + if ((primitive_xpath != NULL) + && pcmk__str_eq(op, PCMK_VALUE_DELETE, pcmk__str_none)) { + const char *rsc_id = NULL; const char *end_quote = NULL; char *copy = NULL; - if ((strstr(xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) - || (strstr(xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { + if ((strstr(primitive_xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) + || (strstr(primitive_xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { reason = pcmk__str_copy("(meta) attribute deleted from " "resource"); break; } - rsc_id = strstr(xpath, PRIMITIVE_ID_XP_FRAGMENT); - if (rsc_id == NULL) { - continue; - } - - rsc_id += sizeof(PRIMITIVE_ID_XP_FRAGMENT) - 1; + rsc_id = primitive_xpath + sizeof(PRIMITIVE_ID_XP_FRAGMENT) - 1; end_quote = strchr(rsc_id, '\''); CRM_LOG_ASSERT(end_quote != NULL); From 746d319d00012699cd86af73cac4febbedb0a406 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 12:58:13 -0700 Subject: [PATCH 03/24] Refactor: fencer: Take patchset as arg in update_cib_stonith_devices() The sole caller has already done the checking, we don't need event, and we don't need msg except as a way to get the patchset. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index cb673dfa6d6..d933c996c61 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -235,23 +235,10 @@ cib_devices_update(void) #define PRIMITIVE_ID_XP_FRAGMENT "/" PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "='" static void -update_cib_stonith_devices(const char *event, xmlNode * msg) +update_cib_stonith_devices(const xmlNode *patchset) { - int format = 1; - const xmlNode *wrapper = pcmk__xe_first_child(msg, - PCMK__XE_CIB_UPDATE_RESULT, - NULL, NULL); - const xmlNode *patchset = pcmk__xe_first_child(wrapper, NULL, NULL, NULL); char *reason = NULL; - CRM_CHECK(patchset != NULL, return); - crm_element_value_int(patchset, PCMK_XA_FORMAT, &format); - - if (format != 2) { - crm_warn("Unknown patch format: %d", format); - return; - } - for (const xmlNode *change = pcmk__xe_first_child(patchset, NULL, NULL, NULL); change != NULL; change = pcmk__xe_next(change, NULL)) { @@ -481,6 +468,7 @@ update_fencing_topology(const char *event, xmlNode *msg) static void update_cib_cache_cb(const char *event, xmlNode * msg) { + xmlNode *patchset = NULL; long long timeout_ms_saved = stonith_watchdog_timeout_ms; bool need_full_refresh = false; @@ -499,7 +487,6 @@ update_cib_cache_cb(const char *event, xmlNode * msg) if (local_cib != NULL) { int rc = pcmk_ok; xmlNode *wrapper = NULL; - xmlNode *patchset = NULL; crm_element_value_int(msg, PCMK__XA_CIB_RC, &rc); if (rc != pcmk_ok) { @@ -514,6 +501,11 @@ update_cib_cache_cb(const char *event, xmlNode * msg) switch (rc) { case pcmk_ok: case -pcmk_err_old_data: + /* @TODO Full refresh (with or without query) in case of + * -pcmk_err_old_data? It seems wrong to call + * stonith_device_remove() based on primitive deletion in an + * old diff. + */ break; case -pcmk_err_diff_resync: case -pcmk_err_diff_failed: @@ -548,7 +540,7 @@ update_cib_cache_cb(const char *event, xmlNode * msg) } else { // Partial refresh update_fencing_topology(event, msg); - update_cib_stonith_devices(event, msg); + update_cib_stonith_devices(patchset); } watchdog_device_update(); From 8a12fef10d5013dd0a536ca84a48a2c173a3f95d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 5 Apr 2025 16:09:54 -0700 Subject: [PATCH 04/24] Refactor: fencer: Some best practices in stonith_device_register() * Rename to fenced_device_register(). * Return standard Pacemaker return code. * Rename rv to rc. * Take const and bool arguments. * Improve if-test formatting. * Make watchdog handling block more readable, using a done label to free the device if needed and to avoid the do-while loop (which was there to accommodate a break statement). * Check pointers against NULL explicitly. * Limit variable scope. * Limit comment line lengths. * Improve messages. * Drop redundant lookup (device_has_duplicate() already did this lookup). Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 7 +- daemons/fenced/fenced_commands.c | 127 ++++++++++++++++-------------- daemons/fenced/fenced_scheduler.c | 2 +- daemons/fenced/pacemaker-fenced.h | 4 +- 4 files changed, 73 insertions(+), 67 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index d933c996c61..f9c0925419a 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -328,15 +328,14 @@ watchdog_device_update(void) STONITH_WATCHDOG_ID, st_namespace_internal, STONITH_WATCHDOG_AGENT, - NULL, /* stonith_device_register will add our + NULL, /* fenced_device_register() will add our own name as PCMK_STONITH_HOST_LIST param so we can skip that here */ NULL); - rc = stonith_device_register(xml, TRUE); + rc = fenced_device_register(xml, true); pcmk__xml_free(xml); - if (rc != pcmk_ok) { - rc = pcmk_legacy2rc(rc); + if (rc != pcmk_rc_ok) { exit_code = CRM_EX_FATAL; crm_crit("Cannot register watchdog pseudo fence agent: %s", pcmk_rc_str(rc)); diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 51666992bff..ea748393daf 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -10,6 +10,7 @@ #include #include +#include // bool #include #include #include @@ -1017,7 +1018,7 @@ target_list_type(stonith_device_t * dev) } static stonith_device_t * -build_device_from_xml(xmlNode *dev) +build_device_from_xml(const xmlNode *dev) { const char *value; stonith_device_t *device = NULL; @@ -1325,91 +1326,92 @@ device_has_duplicate(const stonith_device_t *device) } int -stonith_device_register(xmlNode *dev, gboolean from_cib) +fenced_device_register(const xmlNode *dev, bool from_cib) { + const char *local_node_name = fenced_get_local_node(); stonith_device_t *dup = NULL; stonith_device_t *device = build_device_from_xml(dev); - guint ndevices = 0; - int rv = pcmk_ok; + int rc = pcmk_rc_ok; - CRM_CHECK(device != NULL, return -ENOMEM); + CRM_CHECK(device != NULL, return ENOMEM); /* do we have a watchdog-device? */ - if (pcmk__str_eq(device->id, STONITH_WATCHDOG_ID, pcmk__str_none) || - pcmk__str_any_of(device->agent, STONITH_WATCHDOG_AGENT, - STONITH_WATCHDOG_AGENT_INTERNAL, NULL)) do { + if (pcmk__str_eq(device->id, STONITH_WATCHDOG_ID, pcmk__str_none) + || pcmk__str_any_of(device->agent, STONITH_WATCHDOG_AGENT, + STONITH_WATCHDOG_AGENT_INTERNAL, NULL)) { + if (stonith_watchdog_timeout_ms <= 0) { crm_err("Ignoring watchdog fence device without " - PCMK_OPT_STONITH_WATCHDOG_TIMEOUT " set."); - rv = -ENODEV; - /* fall through to cleanup & return */ - } else if (!pcmk__str_any_of(device->agent, STONITH_WATCHDOG_AGENT, - STONITH_WATCHDOG_AGENT_INTERNAL, NULL)) { - crm_err("Ignoring watchdog fence device with unknown " - "agent '%s' unequal '" STONITH_WATCHDOG_AGENT "'.", - device->agent?device->agent:""); - rv = -ENODEV; - /* fall through to cleanup & return */ - } else if (!pcmk__str_eq(device->id, STONITH_WATCHDOG_ID, - pcmk__str_none)) { - crm_err("Ignoring watchdog fence device " - "named %s !='"STONITH_WATCHDOG_ID"'.", - device->id?device->id:""); - rv = -ENODEV; - /* fall through to cleanup & return */ - } else { - const char *local_node_name = fenced_get_local_node(); + PCMK_OPT_STONITH_WATCHDOG_TIMEOUT " set"); + rc = ENODEV; + goto done; + } + if (!pcmk__str_any_of(device->agent, STONITH_WATCHDOG_AGENT, + STONITH_WATCHDOG_AGENT_INTERNAL, NULL)) { + crm_err("Ignoring watchdog fence device with unknown agent '%s' " + "rather than '" STONITH_WATCHDOG_AGENT "'", + pcmk__s(device->agent, "")); + rc = ENODEV; + goto done; + } + if (!pcmk__str_eq(device->id, STONITH_WATCHDOG_ID, pcmk__str_none)) { + crm_err("Ignoring watchdog fence device named '%s' rather than " + "'" STONITH_WATCHDOG_ID "'", + pcmk__s(device->id, "")); + rc = ENODEV; + goto done; + } - if (pcmk__str_eq(device->agent, STONITH_WATCHDOG_AGENT, - pcmk__str_none)) { - /* this either has an empty list or the targets - configured for watchdog-fencing - */ - g_list_free_full(stonith_watchdog_targets, free); - stonith_watchdog_targets = device->targets; - device->targets = NULL; - } - if (node_does_watchdog_fencing(local_node_name)) { - g_list_free_full(device->targets, free); - device->targets = stonith__parse_targets(local_node_name); - pcmk__insert_dup(device->params, - PCMK_STONITH_HOST_LIST, local_node_name); - /* proceed as with any other stonith-device */ - break; - } + if (pcmk__str_eq(device->agent, STONITH_WATCHDOG_AGENT, + pcmk__str_none)) { + /* This has either an empty list or the targets configured for + * watchdog fencing + */ + g_list_free_full(stonith_watchdog_targets, free); + stonith_watchdog_targets = device->targets; + device->targets = NULL; + } - crm_debug("Skip registration of watchdog fence device on node not in host-list."); - /* cleanup and fall through to more cleanup and return */ + if (!node_does_watchdog_fencing(local_node_name)) { + crm_debug("Skip registration of watchdog fence device on node not " + "in host list"); device->targets = NULL; stonith_device_remove(device->id, from_cib); + goto done; } - free_device(device); - return rv; - } while (0); + + // Proceed as with any other fencing device + g_list_free_full(device->targets, free); + device->targets = stonith__parse_targets(local_node_name); + pcmk__insert_dup(device->params, PCMK_STONITH_HOST_LIST, + local_node_name); + } dup = device_has_duplicate(device); - if (dup) { - ndevices = g_hash_table_size(device_list); + if (dup != NULL) { + guint ndevices = g_hash_table_size(device_list); + crm_debug("Device '%s' already in device list (%d active device%s)", device->id, ndevices, pcmk__plural_s(ndevices)); free_device(device); device = dup; - dup = g_hash_table_lookup(device_list, device->id); - dup->dirty = FALSE; + device->dirty = FALSE; } else { + guint ndevices = 0; stonith_device_t *old = g_hash_table_lookup(device_list, device->id); - if (from_cib && old && old->api_registered) { - /* If the cib is writing over an entry that is shared with a stonith client, - * copy any pending ops that currently exist on the old entry to the new one. - * Otherwise the pending ops will be reported as failures + if (from_cib && (old != NULL) && old->api_registered) { + /* If the CIB is writing over an entry that is shared with a stonith + * client, copy any pending ops that currently exist on the old + * entry to the new one. Otherwise the pending ops will be reported + * as failures. */ crm_info("Overwriting existing entry for %s from CIB", device->id); device->pending_ops = old->pending_ops; device->api_registered = TRUE; old->pending_ops = NULL; - if (device->pending_ops) { + if (device->pending_ops != NULL) { mainloop_set_trigger(device->work); } } @@ -1426,7 +1428,11 @@ stonith_device_register(xmlNode *dev, gboolean from_cib) device->api_registered = TRUE; } - return pcmk_ok; +done: + if (rc != pcmk_rc_ok) { + free_device(device); + } + return rc; } void @@ -3403,8 +3409,9 @@ handle_device_add_request(pcmk__request_t *request) "//" PCMK__XE_ST_DEVICE_ID, LOG_ERR); if (is_privileged(request->ipc_client, op)) { - int rc = stonith_device_register(dev, FALSE); + int rc = fenced_device_register(dev, false); + rc = pcmk_rc2legacy(rc); pcmk__set_result(&request->result, ((rc == pcmk_ok)? CRM_EX_OK : CRM_EX_ERROR), stonith__legacy2status(rc), diff --git a/daemons/fenced/fenced_scheduler.c b/daemons/fenced/fenced_scheduler.c index 46d74320fcc..a67fef5c3ab 100644 --- a/daemons/fenced/fenced_scheduler.c +++ b/daemons/fenced/fenced_scheduler.c @@ -226,7 +226,7 @@ register_if_fencing_device(gpointer data, gpointer user_data) xml = create_device_registration_xml(rsc_id, st_namespace_any, agent, params, rsc_provides); stonith_key_value_freeall(params, 1, 1); - pcmk__assert(stonith_device_register(xml, TRUE) == pcmk_ok); + pcmk__assert(fenced_device_register(xml, true) == pcmk_rc_ok); pcmk__xml_free(xml); } diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index 04aadedc39e..b3c35a08e6c 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -1,5 +1,5 @@ /* - * Copyright 2009-2024 the Pacemaker project contributors + * Copyright 2009-2025 the Pacemaker project contributors * * This source code is licensed under the GNU General Public License version 2 * or later (GPLv2+) WITHOUT ANY WARRANTY. @@ -234,7 +234,7 @@ uint64_t get_stonith_flag(const char *name); void stonith_command(pcmk__client_t *client, uint32_t id, uint32_t flags, xmlNode *op_request, const char *remote_peer); -int stonith_device_register(xmlNode *msg, gboolean from_cib); +int fenced_device_register(const xmlNode *dev, bool from_cib); void stonith_device_remove(const char *id, bool from_cib); From d626bb1e2ff882dd40291215e8d0fa3a21a385cf Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 30 Mar 2025 02:31:22 -0700 Subject: [PATCH 05/24] Low: fencer: Don't remove device if child is deleted If the child of a primitive element representing a fencing device was deleted, and we didn't catch it earlier as instance_attributes or meta_attributes, then previously we would remove the fencing device. We should remove the device only if the primitive itself was deleted. By inspection, it looks like this would unregister a fencing device if one of its operations (op elements) were deleted. If we find that only a child of the primitive was deleted, fall through to the next if block -- that is, "continue" only if the primitive itself was deleted and we call stonith_device_remove(). Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index f9c0925419a..7b3ec17369b 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -258,7 +258,6 @@ update_cib_stonith_devices(const xmlNode *patchset) const char *rsc_id = NULL; const char *end_quote = NULL; - char *copy = NULL; if ((strstr(primitive_xpath, PCMK_XE_INSTANCE_ATTRIBUTES) != NULL) || (strstr(primitive_xpath, PCMK_XE_META_ATTRIBUTES) != NULL)) { @@ -277,19 +276,22 @@ update_cib_stonith_devices(const xmlNode *patchset) continue; } - /* A primitive resource was removed. If it was a fencing resource, - * it's faster to remove it directly than to run the scheduler and - * update all device registrations. - */ - copy = strndup(rsc_id, end_quote - rsc_id); - pcmk__assert(copy != NULL); - stonith_device_remove(copy, true); + if (strchr(end_quote, '/') == NULL) { + /* The primitive element itself was deleted. If this was a + * fencing resource, it's faster to remove it directly than to + * run the scheduler and update all device registrations. + */ + char *copy = strndup(rsc_id, end_quote - rsc_id); - /* watchdog_device_update called afterwards - to fall back to implicit definition if needed */ + pcmk__assert(copy != NULL); + stonith_device_remove(copy, true); - free(copy); - continue; + /* watchdog_device_update called afterwards + to fall back to implicit definition if needed */ + + free(copy); + continue; + } } if (strstr(xpath, "/" PCMK_XE_RESOURCES) From ae101e362f22d75e204bcfbe0d2c50b967224a09 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 5 Apr 2025 02:05:30 -0700 Subject: [PATCH 06/24] Refactor: fencer: Slight improvements to stonith_device_remove() Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index ea748393daf..8fea9aaff7c 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -1441,10 +1441,10 @@ stonith_device_remove(const char *id, bool from_cib) stonith_device_t *device = g_hash_table_lookup(device_list, id); guint ndevices = 0; - if (!device) { + if (device == NULL) { ndevices = g_hash_table_size(device_list); - crm_info("Device '%s' not found (%d active device%s)", - id, ndevices, pcmk__plural_s(ndevices)); + crm_info("Device '%s' not found (%u active device%s)", id, ndevices, + pcmk__plural_s(ndevices)); return; } @@ -1458,14 +1458,14 @@ stonith_device_remove(const char *id, bool from_cib) if (!device->cib_registered && !device->api_registered) { g_hash_table_remove(device_list, id); ndevices = g_hash_table_size(device_list); - crm_info("Removed '%s' from device list (%d active device%s)", + crm_info("Removed '%s' from device list (%u active device%s)", id, ndevices, pcmk__plural_s(ndevices)); } else { - crm_trace("Not removing '%s' from device list (%d active) because " - "still registered via:%s%s", + // Exactly one is true at this point + crm_trace("Not removing '%s' from device list (%u active) because " + "still registered via %s", id, g_hash_table_size(device_list), - (device->cib_registered? " cib" : ""), - (device->api_registered? " api" : "")); + (device->cib_registered? "CIB" : "API")); } } From 39de5626b4c4c0bb8cf9889fd779d39c2f1ae659 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 15:49:36 -0700 Subject: [PATCH 07/24] Refactor: fencer: Drop stonith_device_t:has_attr_map It's unused. Also drop name stonith_device_s for struct that gets typedef'd to stonith_device_t. Signed-off-by: Reid Wahl --- daemons/fenced/pacemaker-fenced.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index b3c35a08e6c..be0e2597eab 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -26,7 +26,7 @@ */ gboolean stonith_check_fence_tolerance(int tolerance, const char *target, const char *action); -typedef struct stonith_device_s { +typedef struct { char *id; char *agent; char *namespace; @@ -35,7 +35,6 @@ typedef struct stonith_device_s { GString *on_target_actions; GList *targets; time_t targets_age; - gboolean has_attr_map; // Whether target's nodeid should be passed as a parameter to the agent gboolean include_nodeid; From 41b2c5972e7bc895f1dc9591d71fc32a99c5d901 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 17:17:35 -0700 Subject: [PATCH 08/24] Test: cts-fencing: Drop test for nodeid parameter inclusion No known fence agent besides fence_dummy advertises the nodeid parameter in its metadata, so is_nodeid_required() should never return true unless some custom fence agent advertises nodeid and expects Pacemaker to pass it automatically. I checked all the agents in both fence-agents and cluster-glue. fence_dummy advertises nodeid purely for cts-fencing's use. So this test in cts-fencing is for a scenario that is not encountered in practice, and it can be dropped. The nodeid parameter in fence_dummy can be dropped accordingly. If anyone complains about this, we can deprecate this functionality for a while and drop it at a major or minor release. However, it seems likely this has been unused for a long time. It was added by 08c78ad (and again by c2265a12, oddly) in 2012. The commit message gives no information about what motivated the change, nor does the linked pull request give any real insight. The PR mentions "stonith scsi" support, but the fence_scsi agent at the time did not support a nodeid parameter. Signed-off-by: Reid Wahl --- cts/cts-fencing.in | 29 ----------------------------- cts/support/fence_dummy.in | 12 +----------- lib/fencing/st_actions.c | 1 - 3 files changed, 1 insertion(+), 41 deletions(-) diff --git a/cts/cts-fencing.in b/cts/cts-fencing.in index 034304a974c..30fb39f2989 100644 --- a/cts/cts-fencing.in +++ b/cts/cts-fencing.in @@ -570,34 +570,6 @@ class FenceTests(Tests): test.add_log_pattern("Delaying 'off' action targeting node3 using true3", negative=True) - def build_nodeid_tests(self): - """Register tests that use a corosync node id.""" - our_uname = localname() - - # verify nodeid is supplied when nodeid is in the metadata parameters - test = self.new_test("supply_nodeid", - "Verify nodeid is given when fence agent has nodeid as parameter") - - test.add_cmd("stonith_admin", - args=f'--output-as=xml -R true1 -a fence_dummy -o mode=pass -o "pcmk_host_list={our_uname}"') - test.add_cmd("stonith_admin", args=f"--output-as=xml -F {our_uname} -t 3") - test.add_log_pattern(f"as nodeid with fence action 'off' targeting {our_uname}") - - # verify nodeid is _NOT_ supplied when nodeid is not in the metadata parameters - test = self.new_test("do_not_supply_nodeid", - "Verify nodeid is _NOT_ given when fence agent does not have nodeid as parameter") - - # use a host name that won't be in corosync.conf - test.add_cmd("stonith_admin", - args='--output-as=xml -R true1 -a fence_dummy_no_nodeid ' - f'-o mode=pass -o pcmk_host_list="regr-test {our_uname}"') - test.add_cmd("stonith_admin", args="--output-as=xml -F regr-test -t 3") - test.add_log_pattern("as nodeid with fence action 'off' targeting regr-test", - negative=True) - test.add_cmd("stonith_admin", args=f"--output-as=xml -F {our_uname} -t 3") - test.add_log_pattern("as nodeid with fence action 'off' targeting {our_uname}", - negative=True) - def build_unfence_tests(self): """Register tests that verify unfencing.""" our_uname = localname() @@ -917,7 +889,6 @@ def main(): tests.build_fence_no_merge_tests() tests.build_unfence_tests() tests.build_unfence_on_target_tests() - tests.build_nodeid_tests() tests.build_remap_tests() tests.build_query_tests() tests.build_metadata_tests() diff --git a/cts/support/fence_dummy.in b/cts/support/fence_dummy.in index 42c9a54eb9c..842fe47504f 100644 --- a/cts/support/fence_dummy.in +++ b/cts/support/fence_dummy.in @@ -1,7 +1,7 @@ #!@PYTHON@ """Dummy fence agent for testing.""" -__copyright__ = "Copyright 2012-2024 the Pacemaker project contributors" +__copyright__ = "Copyright 2012-2025 the Pacemaker project contributors" __license__ = "GNU General Public License version 2 or later (GPLv2+) WITHOUT ANY WARRANTY" import io @@ -159,14 +159,6 @@ ALL_OPT = { "shortdesc": "Ignored", "order": 4 }, - "nodeid": { - "getopt": "i:", - "longopt": "nodeid", - "help": "-i, --nodeid Corosync id of fence target (ignored)", - "required": "0", - "shortdesc": "Ignored", - "order": 4 - }, "uuid": { "getopt": "U:", "longopt": "uuid", @@ -446,8 +438,6 @@ def main(): no_reboot = True elif sys.argv[0].endswith("_no_on"): no_on = True - elif sys.argv[0].endswith("_no_nodeid"): - del ALL_OPT["nodeid"] device_opt = ALL_OPT.keys() diff --git a/lib/fencing/st_actions.c b/lib/fencing/st_actions.c index d085ff8ab30..e1535b5529c 100644 --- a/lib/fencing/st_actions.c +++ b/lib/fencing/st_actions.c @@ -163,7 +163,6 @@ make_args(const char *agent, const char *action, const char *target, if (target_nodeid != 0) { char *nodeid = crm_strdup_printf("%" PRIu32, target_nodeid); - // cts-fencing looks for this log message crm_info("Passing '%s' as nodeid with fence action '%s' targeting %s", nodeid, action, pcmk__s(target, "no node")); g_hash_table_insert(arg_list, strdup("nodeid"), nodeid); From 42eb44698d2b9577fb8c1fd53dc96c7213d144e8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 17:01:16 -0700 Subject: [PATCH 09/24] API: fencer: Don't automatically pass nodeid as parameter ...for fence devices whose agent metadata advertises support for a nodeid parameter. No known fence agent advertises the nodeid parameter in its metadata, so is_nodeid_required() should never return true unless some custom fence agent advertises nodeid and expects Pacemaker to pass it automatically. If anyone complains about this, we can deprecate this functionality for a while and drop it at a major or minor release. However, it seems likely this has been unused for a long time. It was added by 08c78ad (and again by c2265a12, oddly) in 2012. The commit message gives no information about what motivated the change, nor does the linked pull request give any real insight. The PR mentions "stonith scsi" support, but the fence_scsi agent at the time did not support a nodeid parameter. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 41 +++---------------------------- daemons/fenced/pacemaker-fenced.h | 3 --- 2 files changed, 3 insertions(+), 41 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 8fea9aaff7c..ed5302deca8 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -99,7 +99,6 @@ typedef struct async_command_s { char *remote_op_id; char *target; - uint32_t target_nodeid; char *action; char *device; @@ -596,9 +595,9 @@ stonith_device_execute(stonith_device_t * device) host_arg = "plug"; } - action = stonith__action_create(device->agent, action_str, cmd->target, - cmd->target_nodeid, cmd->timeout, - device->params, device->aliases, host_arg); + action = stonith__action_create(device->agent, action_str, cmd->target, 0, + cmd->timeout, device->params, + device->aliases, host_arg); /* for async exec, exec_rc is negative for early error exit otherwise handling of success/errors is done via callbacks */ @@ -655,14 +654,6 @@ schedule_stonith_command(async_command_t * cmd, stonith_device_t * device) free(cmd->device); } - if (device->include_nodeid && (cmd->target != NULL)) { - pcmk__node_status_t *node = - pcmk__get_node(0, cmd->target, NULL, - pcmk__node_search_cluster_member); - - cmd->target_nodeid = node->cluster_layer_id; - } - cmd->device = pcmk__str_copy(device->id); cmd->timeout = get_action_timeout(device, cmd->action, cmd->default_timeout); @@ -908,27 +899,6 @@ get_agent_metadata(const char *agent, xmlNode ** metadata) return pcmk_rc_ok; } -static gboolean -is_nodeid_required(xmlNode * xml) -{ - xmlXPathObject *xpath = NULL; - - if (!xml) { - return FALSE; - } - - xpath = pcmk__xpath_search(xml->doc, - "//" PCMK_XE_PARAMETER - "[@" PCMK_XA_NAME "='nodeid']"); - if (pcmk__xpath_num_results(xpath) == 0) { - xmlXPathFreeObject(xpath); - return FALSE; - } - - xmlXPathFreeObject(xpath); - return TRUE; -} - static void read_action_metadata(stonith_device_t *device) { @@ -1072,11 +1042,6 @@ build_device_from_xml(const xmlNode *dev) break; } - value = g_hash_table_lookup(device->params, "nodeid"); - if (!value) { - device->include_nodeid = is_nodeid_required(device->agent_metadata); - } - value = crm_element_value(dev, PCMK__XA_RSC_PROVIDES); if (pcmk__str_eq(value, PCMK_VALUE_UNFENCING, pcmk__str_casei)) { device->automatic_unfencing = TRUE; diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index be0e2597eab..7ce14477bbf 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -36,9 +36,6 @@ typedef struct { GList *targets; time_t targets_age; - // Whether target's nodeid should be passed as a parameter to the agent - gboolean include_nodeid; - /* whether the cluster should automatically unfence nodes with the device */ gboolean automatic_unfencing; guint priority; From 7770e92ec854491196ad09a03e4ae166d8569829 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 17:16:27 -0700 Subject: [PATCH 10/24] Refactor: libstonithd: Drop stonith__action_create() target_nodeid arg Everything passes 0 currently. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 2 +- include/crm/fencing/internal.h | 3 +-- lib/fencing/st_actions.c | 24 ++++++------------------ lib/fencing/st_client.c | 4 ++-- lib/fencing/st_rhcs.c | 6 +++--- 5 files changed, 13 insertions(+), 26 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index ed5302deca8..2d791e58743 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -595,7 +595,7 @@ stonith_device_execute(stonith_device_t * device) host_arg = "plug"; } - action = stonith__action_create(device->agent, action_str, cmd->target, 0, + action = stonith__action_create(device->agent, action_str, cmd->target, cmd->timeout, device->params, device->aliases, host_arg); diff --git a/include/crm/fencing/internal.h b/include/crm/fencing/internal.h index d46dab7ab9a..46159936457 100644 --- a/include/crm/fencing/internal.h +++ b/include/crm/fencing/internal.h @@ -1,5 +1,5 @@ /* - * Copyright 2011-2024 the Pacemaker project contributors + * Copyright 2011-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -58,7 +58,6 @@ typedef struct stonith_action_s stonith_action_t; stonith_action_t *stonith__action_create(const char *agent, const char *action_name, const char *target, - uint32_t target_nodeid, int timeout_sec, GHashTable *device_args, GHashTable *port_map, diff --git a/lib/fencing/st_actions.c b/lib/fencing/st_actions.c index e1535b5529c..5da7acf026d 100644 --- a/lib/fencing/st_actions.c +++ b/lib/fencing/st_actions.c @@ -113,7 +113,6 @@ append_config_arg(gpointer key, gpointer value, gpointer user_data) * \param[in] agent Fencing agent name * \param[in] action Name of fencing action * \param[in] target Name of target node for fencing action - * \param[in] target_nodeid Node ID of target node for fencing action * \param[in] device_args Fence device parameters * \param[in] port_map Target node-to-port mapping for fence device * \param[in] host_arg Argument name for passing target @@ -122,8 +121,7 @@ append_config_arg(gpointer key, gpointer value, gpointer user_data) */ static GHashTable * make_args(const char *agent, const char *action, const char *target, - uint32_t target_nodeid, GHashTable *device_args, - GHashTable *port_map, const char *host_arg) + GHashTable *device_args, GHashTable *port_map, const char *host_arg) { GHashTable *arg_list = NULL; const char *value = NULL; @@ -159,15 +157,6 @@ make_args(const char *agent, const char *action, const char *target, */ pcmk__insert_dup(arg_list, "nodename", target); - // If the target's node ID was specified, pass it, too - if (target_nodeid != 0) { - char *nodeid = crm_strdup_printf("%" PRIu32, target_nodeid); - - crm_info("Passing '%s' as nodeid with fence action '%s' targeting %s", - nodeid, action, pcmk__s(target, "no node")); - g_hash_table_insert(arg_list, strdup("nodeid"), nodeid); - } - // Check whether target should be specified as some other argument param = g_hash_table_lookup(device_args, PCMK_STONITH_HOST_ARGUMENT); if (param == NULL) { @@ -252,7 +241,6 @@ stonith__action_result(stonith_action_t *action) * \param[in] agent Fence agent to use * \param[in] action_name Fencing action to be executed * \param[in] target Name of target of fencing action (if known) - * \param[in] target_nodeid Node ID of target of fencing action (if known) * \param[in] timeout_sec Timeout to be used when executing action * \param[in] device_args Parameters to pass to fence agent * \param[in] port_map Mapping of target names to device ports @@ -262,14 +250,14 @@ stonith__action_result(stonith_action_t *action) */ stonith_action_t * stonith__action_create(const char *agent, const char *action_name, - const char *target, uint32_t target_nodeid, - int timeout_sec, GHashTable *device_args, - GHashTable *port_map, const char *host_arg) + const char *target, int timeout_sec, + GHashTable *device_args, GHashTable *port_map, + const char *host_arg) { stonith_action_t *action = pcmk__assert_alloc(1, sizeof(stonith_action_t)); - action->args = make_args(agent, action_name, target, target_nodeid, - device_args, port_map, host_arg); + action->args = make_args(agent, action_name, target, device_args, port_map, + host_arg); crm_debug("Preparing '%s' action targeting %s using agent %s", action_name, pcmk__s(target, "no node"), agent); action->agent = strdup(agent); diff --git a/lib/fencing/st_client.c b/lib/fencing/st_client.c index a2c541c366b..52d348d27d2 100644 --- a/lib/fencing/st_client.c +++ b/lib/fencing/st_client.c @@ -2504,8 +2504,8 @@ stonith__metadata_async(const char *agent, int timeout_sec, int rc = pcmk_ok; action = stonith__action_create(agent, PCMK_ACTION_METADATA, - NULL, 0, timeout_sec, NULL, - NULL, NULL); + NULL, timeout_sec, NULL, NULL, + NULL); rc = stonith__execute_async(action, user_data, callback, NULL); if (rc != pcmk_ok) { diff --git a/lib/fencing/st_rhcs.c b/lib/fencing/st_rhcs.c index 6d0c6f94476..d091ea1153a 100644 --- a/lib/fencing/st_rhcs.c +++ b/lib/fencing/st_rhcs.c @@ -129,8 +129,8 @@ stonith__rhcs_get_metadata(const char *agent, int timeout_sec, xmlXPathObject *xpathObj = NULL; stonith_action_t *action = stonith__action_create(agent, PCMK_ACTION_METADATA, - NULL, 0, timeout_sec, - NULL, NULL, NULL); + NULL, timeout_sec, NULL, + NULL, NULL); int rc = stonith__execute(action); pcmk__action_result_t *result = stonith__action_result(action); @@ -306,7 +306,7 @@ stonith__rhcs_validate(stonith_t *st, int call_options, const char *target, host_arg = NULL; } - action = stonith__action_create(agent, PCMK_ACTION_VALIDATE_ALL, target, 0, + action = stonith__action_create(agent, PCMK_ACTION_VALIDATE_ALL, target, remaining_timeout, params, NULL, host_arg); rc = stonith__execute(action); From 5906b6cf0849cc93ebbae6b7857590ff243b92c7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 17:31:17 -0700 Subject: [PATCH 11/24] Refactor: fencer: Drop stonith_device_t:priority We've had a TODO to "hook up priority" for over 15 years: c6bac1b3. At this point, it's safe to skip this and come back to it if needed. Fencing levels are always an option in the meantime. This seems like a nice-to-have at best, with fencing levels providing an explicit way to accomplish it. (I can't think of anything priority could do that fencing levels couldn't.) Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 17 ----------------- daemons/fenced/pacemaker-fenced.h | 1 - doc/sphinx/Pacemaker_Explained/fencing.rst | 11 ----------- 3 files changed, 29 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 2d791e58743..5bd1fbb513e 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -1058,7 +1058,6 @@ build_device_from_xml(const xmlNode *dev) } device->work = mainloop_add_trigger(G_PRIORITY_HIGH, stonith_device_dispatch, device); - /* TODO: Hook up priority */ return device; } @@ -2762,20 +2761,6 @@ st_child_done(int pid, const pcmk__action_result_t *result, void *user_data) } } -static gint -sort_device_priority(gconstpointer a, gconstpointer b) -{ - const stonith_device_t *dev_a = a; - const stonith_device_t *dev_b = b; - - if (dev_a->priority > dev_b->priority) { - return -1; - } else if (dev_a->priority < dev_b->priority) { - return 1; - } - return 0; -} - static void stonith_fence_get_devices_cb(GList * devices, void *user_data) { @@ -2787,8 +2772,6 @@ stonith_fence_get_devices_cb(GList * devices, void *user_data) ndevices, pcmk__plural_s(ndevices), cmd->target); if (devices != NULL) { - /* Order based on priority */ - devices = g_list_sort(devices, sort_device_priority); device = g_hash_table_lookup(device_list, devices->data); } diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index 7ce14477bbf..db9ea2ab8cf 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -38,7 +38,6 @@ typedef struct { /* whether the cluster should automatically unfence nodes with the device */ gboolean automatic_unfencing; - guint priority; uint32_t flags; // Group of enum st_device_flags diff --git a/doc/sphinx/Pacemaker_Explained/fencing.rst b/doc/sphinx/Pacemaker_Explained/fencing.rst index 915f69fd0b9..dce479e3c61 100644 --- a/doc/sphinx/Pacemaker_Explained/fencing.rst +++ b/doc/sphinx/Pacemaker_Explained/fencing.rst @@ -179,17 +179,6 @@ fencing resource (*not* meta-attributes, even though they are interpreted by Pacemaker rather than the fence agent). These are also listed in the man page for ``pacemaker-fenced``. -.. Not_Yet_Implemented: - - +----------------------+---------+--------------------+----------------------------------------+ - | priority | integer | 0 | .. index:: | - | | | | single: priority | - | | | | | - | | | | The priority of the fence device. | - | | | | Devices are tried in order of highest | - | | | | priority to lowest. | - +----------------------+---------+--------------------+----------------------------------------+ - .. list-table:: **Additional Properties of Fencing Resources** :class: longtable :widths: 2 1 2 4 From f9110a3ed0f0d6f289efd487d1343a7fec91d223 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 17:51:50 -0700 Subject: [PATCH 12/24] Refactor: fencer: Rename stonith_device_t to fenced_device_t Proper prefix for fencer daemon. "stonith_" belongs to libstonithd. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 2 +- daemons/fenced/fenced_commands.c | 118 +++++++++++++++--------------- daemons/fenced/pacemaker-fenced.h | 2 +- 3 files changed, 61 insertions(+), 61 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 7b3ec17369b..93160ae9844 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -202,7 +202,7 @@ static void cib_devices_update(void) { GHashTableIter iter; - stonith_device_t *device = NULL; + fenced_device_t *device = NULL; crm_info("Updating devices to version %s.%s.%s", crm_element_value(local_cib, PCMK_XA_ADMIN_EPOCH), diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 5bd1fbb513e..ef29d1466e4 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -77,7 +77,7 @@ static void search_devices_record_result(struct device_search_s *search, const c gboolean can_fence); static int get_agent_metadata(const char *agent, xmlNode **metadata); -static void read_action_metadata(stonith_device_t *device); +static void read_action_metadata(fenced_device_t *device); static enum fenced_target_by unpack_level_kind(const xmlNode *level); typedef struct async_command_s { @@ -114,22 +114,22 @@ typedef struct async_command_s { * we sent to the process to get it to terminate */ int last_timeout_signo; - stonith_device_t *active_on; - stonith_device_t *activating_on; + fenced_device_t *active_on; + fenced_device_t *activating_on; } async_command_t; static xmlNode *construct_async_reply(const async_command_t *cmd, const pcmk__action_result_t *result); static gboolean -is_action_required(const char *action, const stonith_device_t *device) +is_action_required(const char *action, const fenced_device_t *device) { return (device != NULL) && device->automatic_unfencing && pcmk__str_eq(action, PCMK_ACTION_ON, pcmk__str_none); } static int -get_action_delay_max(const stonith_device_t *device, const char *action) +get_action_delay_max(const fenced_device_t *device, const char *action) { const char *value = NULL; guint delay_max = 0U; @@ -148,7 +148,7 @@ get_action_delay_max(const stonith_device_t *device, const char *action) } static int -get_action_delay_base(const stonith_device_t *device, const char *action, +get_action_delay_base(const fenced_device_t *device, const char *action, const char *target) { char *hash_value = NULL; @@ -214,7 +214,7 @@ get_action_delay_base(const stonith_device_t *device, const char *action, * the device is registered, whether by CIB change or API call. */ static int -get_action_timeout(const stonith_device_t *device, const char *action, +get_action_timeout(const fenced_device_t *device, const char *action, int default_timeout) { if (action && device && device->params) { @@ -250,7 +250,7 @@ get_action_timeout(const stonith_device_t *device, const char *action, * * \return Currently executing device for \p cmd if any, otherwise NULL */ -static stonith_device_t * +static fenced_device_t * cmd_device(const async_command_t *cmd) { if ((cmd == NULL) || (cmd->device == NULL) || (device_list == NULL)) { @@ -273,7 +273,7 @@ fenced_device_reboot_action(const char *device_id) const char *action = NULL; if ((device_list != NULL) && (device_id != NULL)) { - stonith_device_t *device = g_hash_table_lookup(device_list, device_id); + fenced_device_t *device = g_hash_table_lookup(device_list, device_id); if ((device != NULL) && (device->params != NULL)) { action = g_hash_table_lookup(device->params, "pcmk_reboot_action"); @@ -294,7 +294,7 @@ bool fenced_device_supports_on(const char *device_id) { if ((device_list != NULL) && (device_id != NULL)) { - stonith_device_t *device = g_hash_table_lookup(device_list, device_id); + fenced_device_t *device = g_hash_table_lookup(device_list, device_id); if (device != NULL) { return pcmk_is_set(device->flags, st_device_supports_on); @@ -393,7 +393,7 @@ create_async_command(xmlNode *msg) } static int -get_action_limit(stonith_device_t * device) +get_action_limit(fenced_device_t *device) { const char *value = NULL; int action_limit = 1; @@ -408,7 +408,7 @@ get_action_limit(stonith_device_t * device) } static int -get_active_cmds(stonith_device_t * device) +get_active_cmds(fenced_device_t *device) { int counter = 0; GList *gIter = NULL; @@ -433,11 +433,14 @@ static void fork_cb(int pid, void *user_data) { async_command_t *cmd = (async_command_t *) user_data; - stonith_device_t * device = - /* in case of a retry we've done the move from - activating_on to active_on already + fenced_device_t *device = cmd->activating_on; + + if (device == NULL) { + /* In case of a retry, we've done the move from activating_on to + * active_on already */ - cmd->activating_on?cmd->activating_on:cmd->active_on; + device = cmd->active_on; + } pcmk__assert(device != NULL); crm_debug("Operation '%s' [%d]%s%s using %s now running with %ds timeout", @@ -450,7 +453,7 @@ fork_cb(int pid, void *user_data) static int get_agent_metadata_cb(gpointer data) { - stonith_device_t *device = data; + fenced_device_t *device = data; guint period_ms; switch (get_agent_metadata(device->agent, &device->agent_metadata)) { @@ -495,7 +498,7 @@ report_internal_result(async_command_t *cmd, int exit_status, } static gboolean -stonith_device_execute(stonith_device_t * device) +stonith_device_execute(fenced_device_t *device) { int exec_rc = 0; const char *action_str = NULL; @@ -630,7 +633,7 @@ static gboolean start_delay_helper(gpointer data) { async_command_t *cmd = data; - stonith_device_t *device = cmd_device(cmd); + fenced_device_t *device = cmd_device(cmd); cmd->delay_id = 0; if (device) { @@ -641,7 +644,7 @@ start_delay_helper(gpointer data) } static void -schedule_stonith_command(async_command_t * cmd, stonith_device_t * device) +schedule_stonith_command(async_command_t *cmd, fenced_device_t *device) { int delay_max = 0; int delay_base = 0; @@ -716,7 +719,7 @@ static void free_device(gpointer data) { GList *gIter = NULL; - stonith_device_t *device = data; + fenced_device_t *device = data; g_hash_table_destroy(device->params); g_hash_table_destroy(device->aliases); @@ -900,7 +903,7 @@ get_agent_metadata(const char *agent, xmlNode ** metadata) } static void -read_action_metadata(stonith_device_t *device) +read_action_metadata(fenced_device_t *device) { xmlXPathObject *xpath = NULL; int max = 0; @@ -963,7 +966,7 @@ read_action_metadata(stonith_device_t *device) } static const char * -target_list_type(stonith_device_t * dev) +target_list_type(fenced_device_t *dev) { const char *check_type = NULL; @@ -987,16 +990,16 @@ target_list_type(stonith_device_t * dev) return check_type; } -static stonith_device_t * +static fenced_device_t * build_device_from_xml(const xmlNode *dev) { const char *value; - stonith_device_t *device = NULL; + fenced_device_t *device = NULL; char *agent = crm_element_value_copy(dev, PCMK_XA_AGENT); CRM_CHECK(agent != NULL, return device); - device = pcmk__assert_alloc(1, sizeof(stonith_device_t)); + device = pcmk__assert_alloc(1, sizeof(fenced_device_t)); device->id = crm_element_value_copy(dev, PCMK_XA_ID); device->agent = agent; @@ -1063,11 +1066,8 @@ build_device_from_xml(const xmlNode *dev) } static void -schedule_internal_command(const char *origin, - stonith_device_t * device, - const char *action, - const char *target, - int timeout, +schedule_internal_command(const char *origin, fenced_device_t *device, + const char *action, const char *target, int timeout, void *internal_user_data, void (*done_cb) (int pid, const pcmk__action_result_t *result, @@ -1106,7 +1106,7 @@ status_search_cb(int pid, const pcmk__action_result_t *result, void *user_data) { async_command_t *cmd = user_data; struct device_search_s *search = cmd->internal_user_data; - stonith_device_t *dev = cmd_device(cmd); + fenced_device_t *dev = cmd_device(cmd); gboolean can = FALSE; free_async_command(cmd); @@ -1156,7 +1156,7 @@ dynamic_list_search_cb(int pid, const pcmk__action_result_t *result, { async_command_t *cmd = user_data; struct device_search_s *search = cmd->internal_user_data; - stonith_device_t *dev = cmd_device(cmd); + fenced_device_t *dev = cmd_device(cmd); gboolean can_fence = FALSE; free_async_command(cmd); @@ -1265,10 +1265,10 @@ device_params_diff(GHashTable *first, GHashTable *second) { * \internal * \brief Checks to see if an identical device already exists in the device_list */ -static stonith_device_t * -device_has_duplicate(const stonith_device_t *device) +static fenced_device_t * +device_has_duplicate(const fenced_device_t *device) { - stonith_device_t *dup = g_hash_table_lookup(device_list, device->id); + fenced_device_t *dup = g_hash_table_lookup(device_list, device->id); if (!dup) { crm_trace("No match for %s", device->id); @@ -1293,8 +1293,8 @@ int fenced_device_register(const xmlNode *dev, bool from_cib) { const char *local_node_name = fenced_get_local_node(); - stonith_device_t *dup = NULL; - stonith_device_t *device = build_device_from_xml(dev); + fenced_device_t *dup = NULL; + fenced_device_t *device = build_device_from_xml(dev); int rc = pcmk_rc_ok; CRM_CHECK(device != NULL, return ENOMEM); @@ -1363,7 +1363,7 @@ fenced_device_register(const xmlNode *dev, bool from_cib) } else { guint ndevices = 0; - stonith_device_t *old = g_hash_table_lookup(device_list, device->id); + fenced_device_t *old = g_hash_table_lookup(device_list, device->id); if (from_cib && (old != NULL) && old->api_registered) { /* If the CIB is writing over an entry that is shared with a stonith @@ -1402,7 +1402,7 @@ fenced_device_register(const xmlNode *dev, bool from_cib) void stonith_device_remove(const char *id, bool from_cib) { - stonith_device_t *device = g_hash_table_lookup(device_list, id); + fenced_device_t *device = g_hash_table_lookup(device_list, id); guint ndevices = 0; if (device == NULL) { @@ -1875,7 +1875,7 @@ execute_agent_action(xmlNode *msg, pcmk__action_result_t *result) const char *id = crm_element_value(dev, PCMK__XA_ST_DEVICE_ID); const char *action = crm_element_value(op, PCMK__XA_ST_DEVICE_ACTION); async_command_t *cmd = NULL; - stonith_device_t *device = NULL; + fenced_device_t *device = NULL; if ((id == NULL) || (action == NULL)) { crm_info("Malformed API action request: device %s, action %s", @@ -1941,7 +1941,7 @@ search_devices_record_result(struct device_search_s *search, const char *device, search->replies_received++; if (can_fence && device) { if (search->support_action_only != st_device_supports_none) { - stonith_device_t *dev = g_hash_table_lookup(device_list, device); + fenced_device_t *dev = g_hash_table_lookup(device_list, device); if (dev && !pcmk_is_set(dev->flags, search->support_action_only)) { return; } @@ -1978,7 +1978,7 @@ search_devices_record_result(struct device_search_s *search, const char *device, * \return TRUE if local host is allowed to execute action, FALSE otherwise */ static gboolean -localhost_is_eligible(const stonith_device_t *device, const char *action, +localhost_is_eligible(const fenced_device_t *device, const char *action, const char *target, gboolean allow_self) { gboolean localhost_is_target = pcmk__str_eq(target, fenced_get_local_node(), @@ -2015,7 +2015,7 @@ localhost_is_eligible(const stonith_device_t *device, const char *action, * might be remapped to, otherwise false */ static bool -localhost_is_eligible_with_remap(const stonith_device_t *device, +localhost_is_eligible_with_remap(const fenced_device_t *device, const char *action, const char *target, gboolean allow_self) { @@ -2051,13 +2051,13 @@ localhost_is_eligible_with_remap(const stonith_device_t *device, * otherwise \c false */ static inline bool -can_use_target_cache(const stonith_device_t *dev) +can_use_target_cache(const fenced_device_t *dev) { return (dev->targets != NULL) && (time(NULL) < (dev->targets_age + 60)); } static void -can_fence_host_with_device(stonith_device_t *dev, +can_fence_host_with_device(fenced_device_t *dev, struct device_search_s *search) { gboolean can = FALSE; @@ -2171,7 +2171,7 @@ can_fence_host_with_device(stonith_device_t *dev, static void search_devices(gpointer key, gpointer value, gpointer user_data) { - stonith_device_t *dev = value; + fenced_device_t *dev = value; struct device_search_s *search = user_data; can_fence_host_with_device(dev, search); @@ -2234,7 +2234,7 @@ struct st_query_data { */ static void add_action_specific_attributes(xmlNode *xml, const char *action, - const stonith_device_t *device, + const fenced_device_t *device, const char *target) { int action_specific_timeout; @@ -2294,7 +2294,7 @@ add_action_specific_attributes(xmlNode *xml, const char *action, * \param[in] allow_self Whether self-fencing is allowed */ static void -add_disallowed(xmlNode *xml, const char *action, const stonith_device_t *device, +add_disallowed(xmlNode *xml, const char *action, const fenced_device_t *device, const char *target, gboolean allow_self) { if (!localhost_is_eligible(device, action, target, allow_self)) { @@ -2316,7 +2316,7 @@ add_disallowed(xmlNode *xml, const char *action, const stonith_device_t *device, */ static void add_action_reply(xmlNode *xml, const char *action, - const stonith_device_t *device, const char *target, + const fenced_device_t *device, const char *target, gboolean allow_self) { xmlNode *child = pcmk__xe_create(xml, PCMK__XE_ST_DEVICE_ACTION); @@ -2378,7 +2378,7 @@ stonith_query_capable_device_cb(GList * devices, void *user_data) crm_xml_add(list, PCMK__XA_ST_TARGET, query->target); for (lpc = devices; lpc != NULL; lpc = lpc->next) { - stonith_device_t *device = g_hash_table_lookup(device_list, lpc->data); + fenced_device_t *device = g_hash_table_lookup(device_list, lpc->data); const char *action = query->action; xmlNode *dev = NULL; @@ -2610,7 +2610,7 @@ send_async_reply(const async_command_t *cmd, const pcmk__action_result_t *result static void cancel_stonith_command(async_command_t * cmd) { - stonith_device_t *device = cmd_device(cmd); + fenced_device_t *device = cmd_device(cmd); if (device) { crm_trace("Cancel scheduled '%s' action using %s", @@ -2692,12 +2692,12 @@ reply_to_duplicates(async_command_t *cmd, const pcmk__action_result_t *result, * * \return Next device required for action if any, otherwise NULL */ -static stonith_device_t * +static fenced_device_t * next_required_device(async_command_t *cmd) { for (GList *iter = cmd->next_device_iter; iter != NULL; iter = iter->next) { - stonith_device_t *next_device = g_hash_table_lookup(device_list, - iter->data); + fenced_device_t *next_device = g_hash_table_lookup(device_list, + iter->data); if (is_action_required(cmd->action, next_device)) { /* This is only called for successful actions, so it's OK to skip @@ -2715,8 +2715,8 @@ st_child_done(int pid, const pcmk__action_result_t *result, void *user_data) { async_command_t *cmd = user_data; - stonith_device_t *device = NULL; - stonith_device_t *next_device = NULL; + fenced_device_t *device = NULL; + fenced_device_t *next_device = NULL; CRM_CHECK(cmd != NULL, return); @@ -2765,7 +2765,7 @@ static void stonith_fence_get_devices_cb(GList * devices, void *user_data) { async_command_t *cmd = user_data; - stonith_device_t *device = NULL; + fenced_device_t *device = NULL; guint ndevices = g_list_length(devices); crm_info("Found %d matching device%s for target '%s'", @@ -2804,7 +2804,7 @@ static void fence_locally(xmlNode *msg, pcmk__action_result_t *result) { const char *device_id = NULL; - stonith_device_t *device = NULL; + fenced_device_t *device = NULL; async_command_t *cmd = NULL; xmlNode *dev = NULL; diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index db9ea2ab8cf..83c086ee6b5 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -55,7 +55,7 @@ typedef struct { gboolean cib_registered; gboolean api_registered; gboolean dirty; -} stonith_device_t; +} fenced_device_t; /* These values are used to index certain arrays by "phase". Usually an * operation has only one "phase", so phase is always zero. However, some From 8c2581f35229b44eced6b47a65aa41b0d44cf786 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 18:04:27 -0700 Subject: [PATCH 13/24] Refactor: fencer: Drop async_command_t:pid Seems to be unused since de0e5650 in 2012. Also drop name of struct that we immediately typedef. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index ef29d1466e4..ea466c7b689 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -80,10 +80,8 @@ static int get_agent_metadata(const char *agent, xmlNode **metadata); static void read_action_metadata(fenced_device_t *device); static enum fenced_target_by unpack_level_kind(const xmlNode *level); -typedef struct async_command_s { - +typedef struct { int id; - int pid; int fd_stdout; uint32_t options; int default_timeout; /* seconds */ From e5d19793c764e986ece58bbcbc928b4c77eb13ca Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 18:05:58 -0700 Subject: [PATCH 14/24] Refactor: fencer: Drop async_command_t:fd_stdout Unused Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 1 - 1 file changed, 1 deletion(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index ea466c7b689..6128abe8712 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -82,7 +82,6 @@ static enum fenced_target_by unpack_level_kind(const xmlNode *level); typedef struct { int id; - int fd_stdout; uint32_t options; int default_timeout; /* seconds */ int timeout; /* seconds */ From f12f5a5f49a8073bf5ee804bfc7908ed5af92e17 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 18:08:54 -0700 Subject: [PATCH 15/24] Refactor: fencer: Drop async_command_t:last_timeout_signo Unused Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 6128abe8712..0e876110515 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -107,9 +107,6 @@ typedef struct { void *user_data); guint timer_sigterm; guint timer_sigkill; - /*! If the operation timed out, this is the last signal - * we sent to the process to get it to terminate */ - int last_timeout_signo; fenced_device_t *active_on; fenced_device_t *activating_on; From e4b8d7e6290eb7109dfaa3360c681cd8a1848710 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 18:09:21 -0700 Subject: [PATCH 16/24] Refactor: fencer: Drop async_command_t:timer_sigterm and timer_sigkill Unused Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 0e876110515..53961586a8b 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -105,8 +105,6 @@ typedef struct { void *internal_user_data; void (*done_cb) (int pid, const pcmk__action_result_t *result, void *user_data); - guint timer_sigterm; - guint timer_sigkill; fenced_device_t *active_on; fenced_device_t *activating_on; From dcd26d287ba57330577e3f665fb28dc788dfb61f Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 18:29:02 -0700 Subject: [PATCH 17/24] Doc: fencer: Clarify async_command_t:device_list I was confused for a bit because it's not directly used for anything. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 53961586a8b..252f3d68e34 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -99,8 +99,11 @@ typedef struct { char *action; char *device; + //! Head of device list (used only for freeing list with command object) GList *device_list; - GList *next_device_iter; // device_list entry for next device to execute + + //! Next item to process in \c device_list + GList *next_device_iter; void *internal_user_data; void (*done_cb) (int pid, const pcmk__action_result_t *result, @@ -2778,7 +2781,11 @@ stonith_fence_get_devices_cb(GList * devices, void *user_data) free_async_command(cmd); g_list_free_full(devices, free); - } else { // Device found, schedule it for fencing + } else { + /* Device found. Schedule a fencing command for it. + * + * Assign devices to device_list so that it will be freed with cmd. + */ cmd->device_list = devices; cmd->next_device_iter = devices->next; schedule_stonith_command(cmd, device); From 2f311059a12ac8ad0473123e41670d77a3d296e8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 22:00:03 -0700 Subject: [PATCH 18/24] Refactor: fencer: Functionize some of cib_devices_update() This is a step toward making device_list a file-scope variable. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 58 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 93160ae9844..40b883b745f 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -194,6 +194,47 @@ update_stonith_watchdog_timeout_ms(xmlNode *cib) stonith_watchdog_timeout_ms = timeout_ms; } +/*! + * \internal + * \brief Mark a fence device dirty if its \c cib_registered flag is \c TRUE + * + * \param[in] key Ignored + * \param[in,out] value Fence device (fenced_device_t *) + * \param[in] user_data Ignored + * + * \note This function is suitable for use with \c g_hash_table_foreach(). + */ +static void +mark_dirty_if_cib_registered(gpointer key, gpointer value, gpointer user_data) +{ + fenced_device_t *device = value; + + if (device->cib_registered) { + device->dirty = TRUE; + } +} + +/*! + * \internal + * \brief Return the value of a fence device's \c dirty flag + * + * \param[in] key Ignored + * \param[in] value Fence device (fenced_device_t *) + * \param[in] user_data Ignored + * + * \return \c dirty flag of \p value + * + * \note This function is suitable for use with + * \c g_hash_table_foreach_remove(). + */ +static gboolean +device_is_dirty(gpointer key, gpointer value, gpointer user_data) +{ + fenced_device_t *device = value; + + return device->dirty; +} + /*! * \internal * \brief Update all STONITH device definitions based on current CIB @@ -201,20 +242,12 @@ update_stonith_watchdog_timeout_ms(xmlNode *cib) static void cib_devices_update(void) { - GHashTableIter iter; - fenced_device_t *device = NULL; - crm_info("Updating devices to version %s.%s.%s", crm_element_value(local_cib, PCMK_XA_ADMIN_EPOCH), crm_element_value(local_cib, PCMK_XA_EPOCH), crm_element_value(local_cib, PCMK_XA_NUM_UPDATES)); - g_hash_table_iter_init(&iter, device_list); - while (g_hash_table_iter_next(&iter, NULL, (void **)&device)) { - if (device->cib_registered) { - device->dirty = TRUE; - } - } + g_hash_table_foreach(device_list, mark_dirty_if_cib_registered, NULL); /* have list repopulated if cib has a watchdog-fencing-resource TODO: keep a cached list for queries happening while we are refreshing @@ -224,12 +257,7 @@ cib_devices_update(void) fenced_scheduler_run(local_cib); - g_hash_table_iter_init(&iter, device_list); - while (g_hash_table_iter_next(&iter, NULL, (void **)&device)) { - if (device->dirty) { - g_hash_table_iter_remove(&iter); - } - } + g_hash_table_foreach_remove(device_list, device_is_dirty, NULL); } #define PRIMITIVE_ID_XP_FRAGMENT "/" PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "='" From 4666ca9bb1e3cda0d5eceba825eb560ec6b5316c Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 22:10:56 -0700 Subject: [PATCH 19/24] Refactor: fencer: New fenced_foreach_device() Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 2 +- daemons/fenced/fenced_commands.c | 17 ++++++++++++++++- daemons/fenced/pacemaker-fenced.h | 2 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 40b883b745f..90a79319087 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -247,7 +247,7 @@ cib_devices_update(void) crm_element_value(local_cib, PCMK_XA_EPOCH), crm_element_value(local_cib, PCMK_XA_NUM_UPDATES)); - g_hash_table_foreach(device_list, mark_dirty_if_cib_registered, NULL); + fenced_foreach_device(mark_dirty_if_cib_registered, NULL); /* have list repopulated if cib has a watchdog-fencing-resource TODO: keep a cached list for queries happening while we are refreshing diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 252f3d68e34..1c5c4eec544 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -116,6 +116,21 @@ typedef struct { static xmlNode *construct_async_reply(const async_command_t *cmd, const pcmk__action_result_t *result); +/*! + * \internal + * \brief Call a function for each known fence device + * + * \param[in] fn Function to call for each device + * \param[in,out] user_data User data + */ +void +fenced_foreach_device(GHFunc fn, gpointer user_data) +{ + if (device_list != NULL) { + g_hash_table_foreach(device_list, fn, user_data); + } +} + static gboolean is_action_required(const char *action, const fenced_device_t *device) { @@ -2206,7 +2221,7 @@ get_capable_devices(const char *host, const char *action, int timeout, ndevices, pcmk__plural_s(ndevices), (search->action? search->action : "unknown action"), (search->host? search->host : "any node")); - g_hash_table_foreach(device_list, search_devices, search); + fenced_foreach_device(search_devices, search); } struct st_query_data { diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index 83c086ee6b5..d9725b77bcc 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -217,6 +217,8 @@ void stonith_shutdown(int nsig); void init_device_list(void); void free_device_list(void); +void fenced_foreach_device(GHFunc fn, gpointer user_data); + void init_topology_list(void); void free_topology_list(void); void free_stonith_remote_op_list(void); From fe37a5fed78f5d2bffb7dc104d0a757104958bfe Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 22:22:25 -0700 Subject: [PATCH 20/24] Refactor: fencer: New fenced_foreach_device_remove() Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 2 +- daemons/fenced/fenced_commands.c | 15 +++++++++++++++ daemons/fenced/pacemaker-fenced.h | 1 + 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 90a79319087..9a07614612a 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -257,7 +257,7 @@ cib_devices_update(void) fenced_scheduler_run(local_cib); - g_hash_table_foreach_remove(device_list, device_is_dirty, NULL); + fenced_foreach_device_remove(device_is_dirty); } #define PRIMITIVE_ID_XP_FRAGMENT "/" PCMK_XE_PRIMITIVE "[@" PCMK_XA_ID "='" diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 1c5c4eec544..390133aa483 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -131,6 +131,21 @@ fenced_foreach_device(GHFunc fn, gpointer user_data) } } +/*! + * \internal + * \brief Remove each known fence device matching a given predicate + * + * \param[in] fn Function that returns \c TRUE to remove a fence device or + * \c FALSE to keep it + */ +void +fenced_foreach_device_remove(GHRFunc fn) +{ + if (device_list != NULL) { + g_hash_table_foreach_remove(device_list, fn, NULL); + } +} + static gboolean is_action_required(const char *action, const fenced_device_t *device) { diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index d9725b77bcc..6c48daf1779 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -218,6 +218,7 @@ void stonith_shutdown(int nsig); void init_device_list(void); void free_device_list(void); void fenced_foreach_device(GHFunc fn, gpointer user_data); +void fenced_foreach_device_remove(GHRFunc fn); void init_topology_list(void); void free_topology_list(void); From 43303cad276fa4194c373e239bfb334d573e30bd Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 22:20:58 -0700 Subject: [PATCH 21/24] Refactor: fencer: New fenced_has_watchdog_device() Signed-off-by: Reid Wahl --- daemons/fenced/fenced_cib.c | 6 +++--- daemons/fenced/fenced_commands.c | 14 ++++++++++++++ daemons/fenced/pacemaker-fenced.h | 1 + 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/daemons/fenced/fenced_cib.c b/daemons/fenced/fenced_cib.c index 9a07614612a..90c225569eb 100644 --- a/daemons/fenced/fenced_cib.c +++ b/daemons/fenced/fenced_cib.c @@ -346,8 +346,8 @@ static void watchdog_device_update(void) { if (stonith_watchdog_timeout_ms > 0) { - if (!g_hash_table_lookup(device_list, STONITH_WATCHDOG_ID) && - !stonith_watchdog_targets) { + if (!fenced_has_watchdog_device() + && (stonith_watchdog_targets == NULL)) { /* getting here watchdog-fencing enabled, no device there yet and reason isn't stonith_watchdog_targets preventing that */ @@ -373,7 +373,7 @@ watchdog_device_update(void) } } - } else if (g_hash_table_lookup(device_list, STONITH_WATCHDOG_ID) != NULL) { + } else if (fenced_has_watchdog_device()) { /* be silent if no device - todo parameter to stonith_device_remove */ stonith_device_remove(STONITH_WATCHDOG_ID, true); } diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 390133aa483..dd7fa6226dd 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -116,6 +116,20 @@ typedef struct { static xmlNode *construct_async_reply(const async_command_t *cmd, const pcmk__action_result_t *result); +/*! + * \internal + * \brief Check whether the fencer's device table contains a watchdog device + * + * \retval \c true If the device table contains a watchdog device + * \retval \c false Otherwise + */ +bool +fenced_has_watchdog_device(void) +{ + return (device_list != NULL) + && (g_hash_table_lookup(device_list, STONITH_WATCHDOG_ID) != NULL); +} + /*! * \internal * \brief Call a function for each known fence device diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index 6c48daf1779..b1d6b8c1552 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -217,6 +217,7 @@ void stonith_shutdown(int nsig); void init_device_list(void); void free_device_list(void); +bool fenced_has_watchdog_device(void); void fenced_foreach_device(GHFunc fn, gpointer user_data); void fenced_foreach_device_remove(GHRFunc fn); From 042aec77e9d85ce0d57b39e67ee5a4926fd61897 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 22:30:14 -0700 Subject: [PATCH 22/24] Refactor: fencer: Make device_list file-scope Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 75 ++++++++++++++++--------------- daemons/fenced/pacemaker-fenced.h | 1 - 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index dd7fa6226dd..c0a3d79890c 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -38,7 +38,8 @@ #include -GHashTable *device_list = NULL; +static GHashTable *device_table = NULL; + GHashTable *topology = NULL; static GList *cmd_list = NULL; @@ -126,8 +127,8 @@ static xmlNode *construct_async_reply(const async_command_t *cmd, bool fenced_has_watchdog_device(void) { - return (device_list != NULL) - && (g_hash_table_lookup(device_list, STONITH_WATCHDOG_ID) != NULL); + return (device_table != NULL) + && (g_hash_table_lookup(device_table, STONITH_WATCHDOG_ID) != NULL); } /*! @@ -140,8 +141,8 @@ fenced_has_watchdog_device(void) void fenced_foreach_device(GHFunc fn, gpointer user_data) { - if (device_list != NULL) { - g_hash_table_foreach(device_list, fn, user_data); + if (device_table != NULL) { + g_hash_table_foreach(device_table, fn, user_data); } } @@ -155,8 +156,8 @@ fenced_foreach_device(GHFunc fn, gpointer user_data) void fenced_foreach_device_remove(GHRFunc fn) { - if (device_list != NULL) { - g_hash_table_foreach_remove(device_list, fn, NULL); + if (device_table != NULL) { + g_hash_table_foreach_remove(device_table, fn, NULL); } } @@ -292,10 +293,10 @@ get_action_timeout(const fenced_device_t *device, const char *action, static fenced_device_t * cmd_device(const async_command_t *cmd) { - if ((cmd == NULL) || (cmd->device == NULL) || (device_list == NULL)) { + if ((cmd == NULL) || (cmd->device == NULL) || (device_table == NULL)) { return NULL; } - return g_hash_table_lookup(device_list, cmd->device); + return g_hash_table_lookup(device_table, cmd->device); } /*! @@ -311,8 +312,8 @@ fenced_device_reboot_action(const char *device_id) { const char *action = NULL; - if ((device_list != NULL) && (device_id != NULL)) { - fenced_device_t *device = g_hash_table_lookup(device_list, device_id); + if ((device_table != NULL) && (device_id != NULL)) { + fenced_device_t *device = g_hash_table_lookup(device_table, device_id); if ((device != NULL) && (device->params != NULL)) { action = g_hash_table_lookup(device->params, "pcmk_reboot_action"); @@ -332,8 +333,8 @@ fenced_device_reboot_action(const char *device_id) bool fenced_device_supports_on(const char *device_id) { - if ((device_list != NULL) && (device_id != NULL)) { - fenced_device_t *device = g_hash_table_lookup(device_list, device_id); + if ((device_table != NULL) && (device_id != NULL)) { + fenced_device_t *device = g_hash_table_lookup(device_table, device_id); if (device != NULL) { return pcmk_is_set(device->flags, st_device_supports_on); @@ -793,17 +794,17 @@ free_device(gpointer data) void free_device_list(void) { - if (device_list != NULL) { - g_hash_table_destroy(device_list); - device_list = NULL; + if (device_table != NULL) { + g_hash_table_destroy(device_table); + device_table = NULL; } } void init_device_list(void) { - if (device_list == NULL) { - device_list = pcmk__strkey_table(NULL, free_device); + if (device_table == NULL) { + device_table = pcmk__strkey_table(NULL, free_device); } } @@ -1302,12 +1303,12 @@ device_params_diff(GHashTable *first, GHashTable *second) { /*! * \internal - * \brief Checks to see if an identical device already exists in the device_list + * \brief Checks to see if an identical device already exists in the table */ static fenced_device_t * device_has_duplicate(const fenced_device_t *device) { - fenced_device_t *dup = g_hash_table_lookup(device_list, device->id); + fenced_device_t *dup = g_hash_table_lookup(device_table, device->id); if (!dup) { crm_trace("No match for %s", device->id); @@ -1392,7 +1393,7 @@ fenced_device_register(const xmlNode *dev, bool from_cib) dup = device_has_duplicate(device); if (dup != NULL) { - guint ndevices = g_hash_table_size(device_list); + guint ndevices = g_hash_table_size(device_table); crm_debug("Device '%s' already in device list (%d active device%s)", device->id, ndevices, pcmk__plural_s(ndevices)); @@ -1402,7 +1403,7 @@ fenced_device_register(const xmlNode *dev, bool from_cib) } else { guint ndevices = 0; - fenced_device_t *old = g_hash_table_lookup(device_list, device->id); + fenced_device_t *old = g_hash_table_lookup(device_table, device->id); if (from_cib && (old != NULL) && old->api_registered) { /* If the CIB is writing over an entry that is shared with a stonith @@ -1418,9 +1419,9 @@ fenced_device_register(const xmlNode *dev, bool from_cib) mainloop_set_trigger(device->work); } } - g_hash_table_replace(device_list, device->id, device); + g_hash_table_replace(device_table, device->id, device); - ndevices = g_hash_table_size(device_list); + ndevices = g_hash_table_size(device_table); crm_notice("Added '%s' to device list (%d active device%s)", device->id, ndevices, pcmk__plural_s(ndevices)); } @@ -1441,11 +1442,11 @@ fenced_device_register(const xmlNode *dev, bool from_cib) void stonith_device_remove(const char *id, bool from_cib) { - fenced_device_t *device = g_hash_table_lookup(device_list, id); + fenced_device_t *device = g_hash_table_lookup(device_table, id); guint ndevices = 0; if (device == NULL) { - ndevices = g_hash_table_size(device_list); + ndevices = g_hash_table_size(device_table); crm_info("Device '%s' not found (%u active device%s)", id, ndevices, pcmk__plural_s(ndevices)); return; @@ -1459,15 +1460,15 @@ stonith_device_remove(const char *id, bool from_cib) } if (!device->cib_registered && !device->api_registered) { - g_hash_table_remove(device_list, id); - ndevices = g_hash_table_size(device_list); + g_hash_table_remove(device_table, id); + ndevices = g_hash_table_size(device_table); crm_info("Removed '%s' from device list (%u active device%s)", id, ndevices, pcmk__plural_s(ndevices)); } else { // Exactly one is true at this point crm_trace("Not removing '%s' from device list (%u active) because " "still registered via %s", - id, g_hash_table_size(device_list), + id, g_hash_table_size(device_table), (device->cib_registered? "CIB" : "API")); } } @@ -1945,7 +1946,7 @@ execute_agent_action(xmlNode *msg, pcmk__action_result_t *result) } } - device = g_hash_table_lookup(device_list, id); + device = g_hash_table_lookup(device_table, id); if (device == NULL) { crm_info("Ignoring API '%s' action request because device %s not found", action, id); @@ -1980,7 +1981,7 @@ search_devices_record_result(struct device_search_s *search, const char *device, search->replies_received++; if (can_fence && device) { if (search->support_action_only != st_device_supports_none) { - fenced_device_t *dev = g_hash_table_lookup(device_list, device); + fenced_device_t *dev = g_hash_table_lookup(device_table, device); if (dev && !pcmk_is_set(dev->flags, search->support_action_only)) { return; } @@ -2224,7 +2225,7 @@ get_capable_devices(const char *host, const char *action, int timeout, uint32_t support_action_only) { struct device_search_s *search; - guint ndevices = g_hash_table_size(device_list); + guint ndevices = g_hash_table_size(device_table); if (ndevices == 0) { callback(NULL, user_data); @@ -2417,7 +2418,7 @@ stonith_query_capable_device_cb(GList * devices, void *user_data) crm_xml_add(list, PCMK__XA_ST_TARGET, query->target); for (lpc = devices; lpc != NULL; lpc = lpc->next) { - fenced_device_t *device = g_hash_table_lookup(device_list, lpc->data); + fenced_device_t *device = g_hash_table_lookup(device_table, lpc->data); const char *action = query->action; xmlNode *dev = NULL; @@ -2735,7 +2736,7 @@ static fenced_device_t * next_required_device(async_command_t *cmd) { for (GList *iter = cmd->next_device_iter; iter != NULL; iter = iter->next) { - fenced_device_t *next_device = g_hash_table_lookup(device_list, + fenced_device_t *next_device = g_hash_table_lookup(device_table, iter->data); if (is_action_required(cmd->action, next_device)) { @@ -2782,7 +2783,7 @@ st_child_done(int pid, const pcmk__action_result_t *result, void *user_data) && !is_action_required(cmd->action, device)) { /* if this device didn't work out, see if there are any others we can try. * if the failed device was 'required', we can't pick another device. */ - next_device = g_hash_table_lookup(device_list, + next_device = g_hash_table_lookup(device_table, cmd->next_device_iter->data); cmd->next_device_iter = cmd->next_device_iter->next; } @@ -2811,7 +2812,7 @@ stonith_fence_get_devices_cb(GList * devices, void *user_data) ndevices, pcmk__plural_s(ndevices), cmd->target); if (devices != NULL) { - device = g_hash_table_lookup(device_list, devices->data); + device = g_hash_table_lookup(device_table, devices->data); } if (device == NULL) { // No device found @@ -2865,7 +2866,7 @@ fence_locally(xmlNode *msg, pcmk__action_result_t *result) device_id = crm_element_value(dev, PCMK__XA_ST_DEVICE_ID); if (device_id != NULL) { - device = g_hash_table_lookup(device_list, device_id); + device = g_hash_table_lookup(device_table, device_id); if (device == NULL) { crm_err("Requested device '%s' is not available", device_id); pcmk__format_result(result, CRM_EX_ERROR, PCMK_EXEC_NO_FENCE_DEVICE, diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index b1d6b8c1552..fd077728d0e 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -325,7 +325,6 @@ fenced_support_flag(const char *action) return st_device_supports_none; } -extern GHashTable *device_list; extern GHashTable *topology; extern long long stonith_watchdog_timeout_ms; extern GList *stonith_watchdog_targets; From bcfe5d500ea044279b9223c5f05eca64e2cd1bee Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 22:31:51 -0700 Subject: [PATCH 23/24] Refactor: fencer: Rename free_device_list to fenced_free_device_table Proper prefix for a non-static symbol, and more importantly says "table" now. "List" was misleading. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 19 ++++++++++++------- daemons/fenced/pacemaker-fenced.c | 4 ++-- daemons/fenced/pacemaker-fenced.h | 2 +- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index c0a3d79890c..3da14102ec8 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -792,19 +792,24 @@ free_device(gpointer data) free(device); } -void free_device_list(void) +void +init_device_list(void) { - if (device_table != NULL) { - g_hash_table_destroy(device_table); - device_table = NULL; + if (device_table == NULL) { + device_table = pcmk__strkey_table(NULL, free_device); } } +/*! + * \internal + * \brief Free the table of known fence devices + */ void -init_device_list(void) +fenced_free_device_table(void) { - if (device_table == NULL) { - device_table = pcmk__strkey_table(NULL, free_device); + if (device_table != NULL) { + g_hash_table_destroy(device_table); + device_table = NULL; } } diff --git a/daemons/fenced/pacemaker-fenced.c b/daemons/fenced/pacemaker-fenced.c index 32a3f2bf562..6e85809107d 100644 --- a/daemons/fenced/pacemaker-fenced.c +++ b/daemons/fenced/pacemaker-fenced.c @@ -1,5 +1,5 @@ /* - * Copyright 2009-2024 the Pacemaker project contributors + * Copyright 2009-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -437,7 +437,7 @@ stonith_cleanup(void) pcmk__client_cleanup(); free_stonith_remote_op_list(); free_topology_list(); - free_device_list(); + fenced_free_device_table(); free_metadata_cache(); fenced_unregister_handlers(); } diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index fd077728d0e..7f546451350 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -216,7 +216,7 @@ typedef struct stonith_topology_s { void stonith_shutdown(int nsig); void init_device_list(void); -void free_device_list(void); +void fenced_free_device_table(void); bool fenced_has_watchdog_device(void); void fenced_foreach_device(GHFunc fn, gpointer user_data); void fenced_foreach_device_remove(GHRFunc fn); From fd4669afcdb12c9982b106a966e8bfac8f7223eb Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 7 Apr 2025 22:34:07 -0700 Subject: [PATCH 24/24] Refactor: fencer: Rename init_device_list to fenced_init_device_table Proper prefix for a non-static symbol, and more importantly says "table" now. "List" was misleading. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 6 +++++- daemons/fenced/pacemaker-fenced.c | 2 +- daemons/fenced/pacemaker-fenced.h | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 3da14102ec8..b31c52bf947 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -792,8 +792,12 @@ free_device(gpointer data) free(device); } +/*! + * \internal + * \brief Initialize the table of known fence devices + */ void -init_device_list(void) +fenced_init_device_table(void) { if (device_table == NULL) { device_table = pcmk__strkey_table(NULL, free_device); diff --git a/daemons/fenced/pacemaker-fenced.c b/daemons/fenced/pacemaker-fenced.c index 6e85809107d..cb862b22845 100644 --- a/daemons/fenced/pacemaker-fenced.c +++ b/daemons/fenced/pacemaker-fenced.c @@ -634,7 +634,7 @@ main(int argc, char **argv) setup_cib(); } - init_device_list(); + fenced_init_device_table(); init_topology_list(); pcmk__serve_fenced_ipc(&ipcs, &ipc_callbacks); diff --git a/daemons/fenced/pacemaker-fenced.h b/daemons/fenced/pacemaker-fenced.h index 7f546451350..c1590438cc1 100644 --- a/daemons/fenced/pacemaker-fenced.h +++ b/daemons/fenced/pacemaker-fenced.h @@ -215,7 +215,7 @@ typedef struct stonith_topology_s { void stonith_shutdown(int nsig); -void init_device_list(void); +void fenced_init_device_table(void); void fenced_free_device_table(void); bool fenced_has_watchdog_device(void); void fenced_foreach_device(GHFunc fn, gpointer user_data);