From 363e2478ddc9854ecaffb2f87e78e3ed79ab5c07 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 01:51:05 -0700 Subject: [PATCH 01/42] Refactor: libcrmcommon: Use g_strsplit() in add_possible_values_* funcs For readability. Similarly for the use of pcmk__str_eq() -- using strcmp() in add_possible_values_default() was not a bug in this case, as both arguments were guaranteed to be non-NULL, but this was not obvious. Signed-off-by: Reid Wahl --- lib/common/options_display.c | 42 ++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/lib/common/options_display.c b/lib/common/options_display.c index 8e33aec908f..d03002fc556 100644 --- a/lib/common/options_display.c +++ b/lib/common/options_display.c @@ -34,26 +34,27 @@ add_possible_values_default(pcmk__output_t *out, } if ((option->values != NULL) && (strcmp(option->type, "select") == 0)) { - const char *delim = ", "; + // If there is no default value, don't look for one bool found_default = (option->default_value == NULL); - char *str = pcmk__str_copy(option->values); - for (const char *value = strtok(str, delim); value != NULL; - value = strtok(NULL, delim)) { + gchar **values = g_strsplit(option->values, ", ", 0); + for (gchar **value = values; *value != NULL; value++) { if (buf->len > 0) { - g_string_append(buf, delim); + g_string_append(buf, ", "); } g_string_append_c(buf, '"'); - g_string_append(buf, value); + g_string_append(buf, *value); g_string_append_c(buf, '"'); - if (!found_default && (strcmp(value, option->default_value) == 0)) { + if (!found_default + && pcmk__str_eq(*value, option->default_value, + pcmk__str_none)) { found_default = true; g_string_append(buf, _(" (default)")); } } - free(str); + g_strfreev(values); } else if (option->default_value != NULL) { pcmk__g_strcat(buf, @@ -289,19 +290,22 @@ static void add_possible_values_xml(pcmk__output_t *out, const pcmk__cluster_option_t *option) { - if ((option->values != NULL) && (strcmp(option->type, "select") == 0)) { - const char *delim = ", "; - char *str = pcmk__str_copy(option->values); - const char *ptr = strtok(str, delim); + gchar **values = NULL; - while (ptr != NULL) { - pcmk__output_create_xml_node(out, PCMK_XE_OPTION, - PCMK_XA_VALUE, ptr, - NULL); - ptr = strtok(NULL, delim); - } - free(str); + if ((option->values == NULL) + || !pcmk__str_eq(option->type, "select", pcmk__str_none)) { + return; } + + values = g_strsplit(option->values, ", ", 0); + + for (gchar **value = values; *value != NULL; value++) { + pcmk__output_create_xml_node(out, PCMK_XE_OPTION, + PCMK_XA_VALUE, *value, + NULL); + } + + g_strfreev(values); } /*! From 21faadc6da7a16f9c22c7354578d90498f4dad8e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 02:49:42 -0700 Subject: [PATCH 02/42] Refactor: libcrmcommon: Use g_strsplit() in add_desc_xml() For readability and reliability. strtok() modifies its argument string, but according to the C11 standard (per the following Stack Overflow answer), the setlocale() return value should not be modified by the program. * https://stackoverflow.com/a/29116455 Signed-off-by: Reid Wahl --- lib/common/options_display.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/common/options_display.c b/lib/common/options_display.c index d03002fc556..7ceaf6f2951 100644 --- a/lib/common/options_display.c +++ b/lib/common/options_display.c @@ -261,18 +261,14 @@ add_desc_xml(pcmk__output_t *out, bool for_long, const char *desc) pcmk__xe_set(node, PCMK_XA_LANG, PCMK__VALUE_EN); #ifdef ENABLE_NLS - { - static const char *locale = NULL; + if (!pcmk__str_eq(desc, _(desc), pcmk__str_none)) { + // The locale should have been set already by crm_log_preinit() + gchar **parts = g_strsplit(setlocale(LC_ALL, NULL), "_", 2); - if (strcmp(desc, _(desc)) == 0) { - return; - } - - if (locale == NULL) { - locale = strtok(setlocale(LC_ALL, NULL), "_"); - } node = pcmk__output_create_xml_text_node(out, tag, _(desc)); - pcmk__xe_set(node, PCMK_XA_LANG, locale); + pcmk__xe_set(node, PCMK_XA_LANG, parts[0]); + + g_strfreev(parts); } #endif } From d6432aa44e8d6b5fb17cc23d7df0e70756120950 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 14:45:03 -0700 Subject: [PATCH 03/42] Refactor: libcrmservice: Rename services_os_get_single_directory_list() The name is overly long and uses the public API prefix ("services_" with a single underscore). Also rename the lpc variable to "i" and scope it to the loop, rename the "root" argument to "dir", and use bool instead of gboolean. I'm guessing that the "os" prefix, as well as the separate "services_linux.c" file, were meant to allow different implementations of certain functions depending on the OS. GLib does similar things. However, the commit that introduced it all (cc2110d7) sheds no light on this question. Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 29 +++++++++++++++-------------- lib/services/services_ocf.c | 2 +- lib/services/services_private.h | 4 ++-- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 0a84f74edad..047a5577040 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -9,6 +9,7 @@ #include +#include // bool, true, false #include #include #include @@ -1429,28 +1430,28 @@ services__execute_file(svc_action_t *op) } GList * -services_os_get_single_directory_list(const char *root, gboolean files, gboolean executable) +services__list_dir(const char *dir, bool files, bool executable) { GList *list = NULL; struct dirent **namelist = NULL; - int entries = 0, lpc = 0; + int entries = 0; - entries = scandir(root, &namelist, NULL, alphasort); + entries = scandir(dir, &namelist, NULL, alphasort); if (entries <= 0) { return list; } - for (lpc = 0; lpc < entries; lpc++) { + for (int i = 0; i < entries; i++) { char *buffer = NULL; struct stat sb; int rc = 0; - if ('.' == namelist[lpc]->d_name[0]) { - free(namelist[lpc]); + if ('.' == namelist[i]->d_name[0]) { + free(namelist[i]); continue; } - buffer = pcmk__assert_asprintf("%s/%s", root, namelist[lpc]->d_name); + buffer = pcmk__assert_asprintf("%s/%s", dir, namelist[i]->d_name); rc = stat(buffer, &sb); free(buffer); @@ -1460,26 +1461,26 @@ services_os_get_single_directory_list(const char *root, gboolean files, gboolean if (S_ISDIR(sb.st_mode)) { if (files) { - free(namelist[lpc]); + free(namelist[i]); continue; } } else if (S_ISREG(sb.st_mode)) { - if (files == FALSE) { - free(namelist[lpc]); + if (!files) { + free(namelist[i]); continue; } else if (executable && (sb.st_mode & S_IXUSR) == 0 && (sb.st_mode & S_IXGRP) == 0 && (sb.st_mode & S_IXOTH) == 0) { - free(namelist[lpc]); + free(namelist[i]); continue; } } - list = g_list_append(list, strdup(namelist[lpc]->d_name)); + list = g_list_append(list, strdup(namelist[i]->d_name)); - free(namelist[lpc]); + free(namelist[i]); } free(namelist); @@ -1499,7 +1500,7 @@ services_os_get_directory_list(const char *root, gboolean files, gboolean execut } for (dir = strtok(dirs, ":"); dir != NULL; dir = strtok(NULL, ":")) { - GList *tmp = services_os_get_single_directory_list(dir, files, executable); + GList *tmp = services__list_dir(dir, files, executable); if (tmp) { result = g_list_concat(result, tmp); diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index 2ace59e95dc..bc9802cbd15 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -44,7 +44,7 @@ services_os_get_directory_list_provider(const char *root, const char *provider, GList *tmp = NULL; sprintf(buffer, "%s/%s", dir, provider); - tmp = services_os_get_single_directory_list(buffer, files, executable); + tmp = services__list_dir(buffer, files, executable); if (tmp) { result = g_list_concat(result, tmp); diff --git a/lib/services/services_private.h b/lib/services/services_private.h index de7e7a4573c..bedd2a29d59 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -11,6 +11,7 @@ #ifndef PCMK__SERVICES_SERVICES_PRIVATE__H #define PCMK__SERVICES_SERVICES_PRIVATE__H +#include // bool #include // uid_t, gid_t #include // G_GNUC_INTERNAL, gboolean, guint, etc. @@ -59,8 +60,7 @@ G_GNUC_INTERNAL const char *services__action_kind(const svc_action_t *action); G_GNUC_INTERNAL -GList *services_os_get_single_directory_list(const char *root, gboolean files, - gboolean executable); +GList *services__list_dir(const char *dir, bool files, bool executable); G_GNUC_INTERNAL GList *services_os_get_directory_list(const char *root, gboolean files, gboolean executable); From d4f8bf2431e8f6977226bb3d393e02b0bca7f9ce Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:00:00 -0700 Subject: [PATCH 04/42] Low: libcrmservice: Fix memory leaks when listing directory contents * namelist was not being freed when entries == 0, indicating an empty directory. * A namelist entry was not being freed if stat() failed. Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 047a5577040..f81e79b88a8 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1434,11 +1434,10 @@ services__list_dir(const char *dir, bool files, bool executable) { GList *list = NULL; struct dirent **namelist = NULL; - int entries = 0; + int entries = scandir(dir, &namelist, NULL, alphasort); - entries = scandir(dir, &namelist, NULL, alphasort); - if (entries <= 0) { - return list; + if (entries < 0) { + return NULL; } for (int i = 0; i < entries; i++) { @@ -1447,7 +1446,6 @@ services__list_dir(const char *dir, bool files, bool executable) int rc = 0; if ('.' == namelist[i]->d_name[0]) { - free(namelist[i]); continue; } @@ -1461,28 +1459,26 @@ services__list_dir(const char *dir, bool files, bool executable) if (S_ISDIR(sb.st_mode)) { if (files) { - free(namelist[i]); continue; } } else if (S_ISREG(sb.st_mode)) { if (!files) { - free(namelist[i]); continue; } else if (executable && (sb.st_mode & S_IXUSR) == 0 && (sb.st_mode & S_IXGRP) == 0 && (sb.st_mode & S_IXOTH) == 0) { - free(namelist[i]); continue; } } list = g_list_append(list, strdup(namelist[i]->d_name)); + } + for (int i = 0; i < entries; i++) { free(namelist[i]); } - free(namelist); return list; } From 061281c7066df06bc33988d360aefbf2e47c6df0 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:05:11 -0700 Subject: [PATCH 05/42] Refactor: libcrmservice: Assert on memory error in services__list_dir() Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index f81e79b88a8..051e38acace 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1473,7 +1473,7 @@ services__list_dir(const char *dir, bool files, bool executable) } } - list = g_list_append(list, strdup(namelist[i]->d_name)); + list = g_list_append(list, pcmk__str_copy(namelist[i]->d_name)); } for (int i = 0; i < entries; i++) { From 85e56b3af961c414b8e96c3e82474f94b47d8ad7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:09:48 -0700 Subject: [PATCH 06/42] Refactor: libcrmservice: Use pcmk__any_flags_set() in services__list_dir For readability Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 051e38acace..9fbd80303cd 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1465,10 +1465,10 @@ services__list_dir(const char *dir, bool files, bool executable) } else if (S_ISREG(sb.st_mode)) { if (!files) { continue; + } - } else if (executable - && (sb.st_mode & S_IXUSR) == 0 - && (sb.st_mode & S_IXGRP) == 0 && (sb.st_mode & S_IXOTH) == 0) { + if (executable + && !pcmk__any_flags_set(sb.st_mode, S_IXUSR|S_IXGRP|S_IXOTH)) { continue; } } From bf66d15ed764c184e333ae4ad97a74189c22f230 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:25:03 -0700 Subject: [PATCH 07/42] Refactor: libcrmservice: Rename services_os_get_directory_list() The name is overly long and uses the public API prefix ("services_" with a single underscore). Use bool instead of gboolean, and rename the "root" argument to "dirs". "dirs" is a colon-delimited list of directories. I'm guessing that the "os" prefix, as well as the separate "services_linux.c" file, were meant to allow different implementations of certain functions depending on the OS. GLib does similar things. However, the commit that introduced it all (cc2110d7) sheds no light on this question. Signed-off-by: Reid Wahl --- lib/services/services.c | 2 +- lib/services/services_linux.c | 12 ++++++------ lib/services/services_lsb.c | 2 +- lib/services/services_private.h | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index d1055951022..c8eadeca469 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -1030,7 +1030,7 @@ services_action_sync(svc_action_t * op) GList * get_directory_list(const char *root, gboolean files, gboolean executable) { - return services_os_get_directory_list(root, files, executable); + return services__list_dirs(root, files, executable); } GList * diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 9fbd80303cd..47883b48a6d 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1484,18 +1484,18 @@ services__list_dir(const char *dir, bool files, bool executable) } GList * -services_os_get_directory_list(const char *root, gboolean files, gboolean executable) +services__list_dirs(const char *dirs, bool files, bool executable) { GList *result = NULL; - char *dirs = strdup(root); + char *dirs_copy = strdup(dirs); char *dir = NULL; - if (pcmk__str_empty(dirs)) { - free(dirs); + if (pcmk__str_empty(dirs_copy)) { + free(dirs_copy); return result; } - for (dir = strtok(dirs, ":"); dir != NULL; dir = strtok(NULL, ":")) { + for (dir = strtok(dirs_copy, ":"); dir != NULL; dir = strtok(NULL, ":")) { GList *tmp = services__list_dir(dir, files, executable); if (tmp) { @@ -1503,7 +1503,7 @@ services_os_get_directory_list(const char *root, gboolean files, gboolean execut } } - free(dirs); + free(dirs_copy); return result; } diff --git a/lib/services/services_lsb.c b/lib/services/services_lsb.c index 05eeb62f241..394b0a6b721 100644 --- a/lib/services/services_lsb.c +++ b/lib/services/services_lsb.c @@ -244,7 +244,7 @@ services__get_lsb_metadata(const char *type, char **output) GList * services__list_lsb_agents(void) { - return services_os_get_directory_list(PCMK__LSB_INIT_DIR, TRUE, TRUE); + return services__list_dirs(PCMK__LSB_INIT_DIR, true, true); } bool diff --git a/lib/services/services_private.h b/lib/services/services_private.h index bedd2a29d59..6c395206bb0 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -63,7 +63,7 @@ G_GNUC_INTERNAL GList *services__list_dir(const char *dir, bool files, bool executable); G_GNUC_INTERNAL -GList *services_os_get_directory_list(const char *root, gboolean files, gboolean executable); +GList *services__list_dirs(const char *dirs, bool files, bool executable); G_GNUC_INTERNAL int services__execute_file(svc_action_t *op); From bca5058bf92d71db827b1d0648fd95b906a99db6 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 03:10:12 -0700 Subject: [PATCH 08/42] Refactor: libcrmservice: Use g_strsplit() for getting directory list For readability versus strtok(). Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 47883b48a6d..05fca1793c5 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1486,24 +1486,19 @@ services__list_dir(const char *dir, bool files, bool executable) GList * services__list_dirs(const char *dirs, bool files, bool executable) { - GList *result = NULL; - char *dirs_copy = strdup(dirs); - char *dir = NULL; + gchar **dir_paths = NULL; + GList *list = NULL; - if (pcmk__str_empty(dirs_copy)) { - free(dirs_copy); - return result; + if (pcmk__str_empty(dirs)) { + return NULL; } - for (dir = strtok(dirs_copy, ":"); dir != NULL; dir = strtok(NULL, ":")) { - GList *tmp = services__list_dir(dir, files, executable); + dir_paths = g_strsplit(dirs, ":", 0); - if (tmp) { - result = g_list_concat(result, tmp); - } + for (gchar **dir = dir_paths; *dir != NULL; dir++) { + list = g_list_concat(list, services__list_dir(*dir, files, executable)); } - free(dirs_copy); - - return result; + g_strfreev(dir_paths); + return list; } From 0b892f8fb4349aa82ddb820693f81ce0f674ea2b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:31:40 -0700 Subject: [PATCH 09/42] Low: build, libcrmservice: initdir must be a single directory Previously, the libcrmservice code supported initdir being a colon-delimited list of directories. However, this was not documented in the configure help text or comments. All of the documentation referred to initdir being a single directory. The default behavior, if not building with "--with-initdir=", was to check a list of directories and set INITDIR to the first one that existed. Signed-off-by: Reid Wahl --- lib/services/services_lsb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/services/services_lsb.c b/lib/services/services_lsb.c index 394b0a6b721..f482d58f970 100644 --- a/lib/services/services_lsb.c +++ b/lib/services/services_lsb.c @@ -244,7 +244,7 @@ services__get_lsb_metadata(const char *type, char **output) GList * services__list_lsb_agents(void) { - return services__list_dirs(PCMK__LSB_INIT_DIR, true, true); + return services__list_dir(PCMK__LSB_INIT_DIR, true, true); } bool From 9d9184d4ed5026c8bc9cbc569a1cbfdd9115be30 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:35:42 -0700 Subject: [PATCH 10/42] Refactor: libcrmservice: Drop services__list_dirs() Its sole remaining caller did nothing except call it. Move the code there. Signed-off-by: Reid Wahl --- lib/services/services.c | 16 +++++++++++++++- lib/services/services_linux.c | 20 -------------------- lib/services/services_private.h | 3 --- 3 files changed, 15 insertions(+), 24 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index c8eadeca469..70a8a1fbce2 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -1030,7 +1030,21 @@ services_action_sync(svc_action_t * op) GList * get_directory_list(const char *root, gboolean files, gboolean executable) { - return services__list_dirs(root, files, executable); + gchar **dir_paths = NULL; + GList *list = NULL; + + if (pcmk__str_empty(root)) { + return NULL; + } + + dir_paths = g_strsplit(root, ":", 0); + + for (gchar **dir = dir_paths; *dir != NULL; dir++) { + list = g_list_concat(list, services__list_dir(*dir, files, executable)); + } + + g_strfreev(dir_paths); + return list; } GList * diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 05fca1793c5..fc6fe07dfbb 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1482,23 +1482,3 @@ services__list_dir(const char *dir, bool files, bool executable) free(namelist); return list; } - -GList * -services__list_dirs(const char *dirs, bool files, bool executable) -{ - gchar **dir_paths = NULL; - GList *list = NULL; - - if (pcmk__str_empty(dirs)) { - return NULL; - } - - dir_paths = g_strsplit(dirs, ":", 0); - - for (gchar **dir = dir_paths; *dir != NULL; dir++) { - list = g_list_concat(list, services__list_dir(*dir, files, executable)); - } - - g_strfreev(dir_paths); - return list; -} diff --git a/lib/services/services_private.h b/lib/services/services_private.h index 6c395206bb0..af7699add90 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -62,9 +62,6 @@ const char *services__action_kind(const svc_action_t *action); G_GNUC_INTERNAL GList *services__list_dir(const char *dir, bool files, bool executable); -G_GNUC_INTERNAL -GList *services__list_dirs(const char *dirs, bool files, bool executable); - G_GNUC_INTERNAL int services__execute_file(svc_action_t *op); From f75b1e058c3ce5537d02cb338c6c1e4ae38b3d9c Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:48:47 -0700 Subject: [PATCH 11/42] Refactor: libcrmservice: Drop internal call to get_directory_list() The resources_os_list_ocf_providers() is listing top-level subdirectories of each directory in PCMK__OCF_RA_PATH. Signed-off-by: Reid Wahl --- lib/services/services_ocf.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index bc9802cbd15..c1672d8131f 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -9,6 +9,7 @@ #include +#include // true, false #include #include #include @@ -23,7 +24,15 @@ GList * resources_os_list_ocf_providers(void) { - return get_directory_list(PCMK__OCF_RA_PATH, FALSE, TRUE); + GList *list = NULL; + gchar **dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); + + for (gchar **dir = dirs; *dir != NULL; dir++) { + list = g_list_concat(list, services__list_dir(*dir, false, true)); + } + + g_strfreev(dirs); + return list; } static GList * From a108090154de1a5b4d6059e12265e7cc5eaea62a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 16:59:53 -0700 Subject: [PATCH 12/42] API: libcrmservice: Deprecate get_directory_list() Signed-off-by: Reid Wahl --- include/crm/services.h | 16 +----------- include/crm/services_compat.h | 6 +++++ lib/services/services.c | 48 ++++++++++++++++++++--------------- 3 files changed, 35 insertions(+), 35 deletions(-) diff --git a/include/crm/services.h b/include/crm/services.h index 64f3d3eeaed..61a06b2fda4 100644 --- a/include/crm/services.h +++ b/include/crm/services.h @@ -1,5 +1,5 @@ /* - * Copyright 2010-2024 the Pacemaker project contributors + * Copyright 2010-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -159,20 +159,6 @@ typedef struct svc_action_s { svc_action_private_t *opaque; } svc_action_t; -/*! - * \brief Get a list of files or directories in a given path - * - * \param[in] root Full path to a directory to read - * \param[in] files Return list of files if TRUE or directories if FALSE - * \param[in] executable If TRUE and files is TRUE, only return executable files - * - * \return List of what was found as char * items. - * \note The caller is responsibile for freeing the result with - * g_list_free_full(list, free). - */ -GList *get_directory_list(const char *root, gboolean files, - gboolean executable); - /*! * \brief Get a list of providers * diff --git a/include/crm/services_compat.h b/include/crm/services_compat.h index 62c503abdcd..0966fcf79ca 100644 --- a/include/crm/services_compat.h +++ b/include/crm/services_compat.h @@ -10,6 +10,8 @@ #ifndef PCMK__CRM_SERVICES_COMPAT__H #define PCMK__CRM_SERVICES_COMPAT__H +#include // GList, gboolean + #include // enum ocf_exitcode, PCMK_OCF_OK, etc. #ifdef __cplusplus @@ -50,6 +52,10 @@ services_ocf_exitcode_str(enum ocf_exitcode code) } } +//! \deprecated Do not use +GList *get_directory_list(const char *root, gboolean files, + gboolean executable); + # ifdef __cplusplus } # endif diff --git a/lib/services/services.c b/lib/services/services.c index 70a8a1fbce2..9c81df246e9 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -1027,26 +1027,6 @@ services_action_sync(svc_action_t * op) return rc; } -GList * -get_directory_list(const char *root, gboolean files, gboolean executable) -{ - gchar **dir_paths = NULL; - GList *list = NULL; - - if (pcmk__str_empty(root)) { - return NULL; - } - - dir_paths = g_strsplit(root, ":", 0); - - for (gchar **dir = dir_paths; *dir != NULL; dir++) { - list = g_list_concat(list, services__list_dir(*dir, files, executable)); - } - - g_strfreev(dir_paths); - return list; -} - GList * resources_list_standards(void) { @@ -1391,3 +1371,31 @@ services__grab_stderr(svc_action_t *action) action->stderr_data = NULL; return output; } + +// Deprecated functions kept only for backward API compatibility +// LCOV_EXCL_START + +#include + +GList * +get_directory_list(const char *root, gboolean files, gboolean executable) +{ + gchar **dir_paths = NULL; + GList *list = NULL; + + if (pcmk__str_empty(root)) { + return NULL; + } + + dir_paths = g_strsplit(root, ":", 0); + + for (gchar **dir = dir_paths; *dir != NULL; dir++) { + list = g_list_concat(list, services__list_dir(*dir, files, executable)); + } + + g_strfreev(dir_paths); + return list; +} + +// LCOV_EXCL_STOP +// End deprecated API From 1b79d4f1a23ff187aa7363c8f162ebaa5316a760 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 13:58:46 -0700 Subject: [PATCH 13/42] Refactor: libcrmservice: Clean services_os_get_directory_list_provider ...up. * Give it a shorter and clearer name that doesn't look like public API. * Drop arguments that always take the same value. (There is only a single caller.) * Avoid a fixed-size buffer where we don't control the input. * Use g_strsplit() for readability instead of strtok(). Signed-off-by: Reid Wahl --- lib/services/services_ocf.c | 50 ++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index c1672d8131f..1366b4cc4dc 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -35,34 +35,39 @@ resources_os_list_ocf_providers(void) return list; } +/*! + * \internal + * \brief List the agents from the given OCF provider + * + * For each directory along \c PCMK__OCF_RA_PATH (a colon-delimited list), this + * function looks for a subdirectory called \p provider. It then finds the top- + * level executable files inside that subdirectory, excluding those beginning + * with \c '.', and adds them to the list. + * + * \param[in] provider OCF provider + * + * \return Newly allocated list of OCF agents as newly allocated strings + * + * \note The caller is responsible for freeing the return value using + * g_list_free_full(list, free). + */ static GList * -services_os_get_directory_list_provider(const char *root, const char *provider, - gboolean files, gboolean executable) +list_provider_agents(const char *provider) { - GList *result = NULL; - char *dirs = strdup(root); - char *dir = NULL; - char buffer[PATH_MAX]; - - if (pcmk__str_empty(dirs)) { - free(dirs); - return result; - } + gchar **dirs = NULL; + GList *list = NULL; - for (dir = strtok(dirs, ":"); dir != NULL; dir = strtok(NULL, ":")) { - GList *tmp = NULL; + dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); - sprintf(buffer, "%s/%s", dir, provider); - tmp = services__list_dir(buffer, files, executable); + for (gchar **dir = dirs; *dir != NULL; dir++) { + char *buf = pcmk__assert_asprintf("%s/%s", *dir, provider); - if (tmp) { - result = g_list_concat(result, tmp); - } + list = g_list_concat(list, services__list_dir(buf, true, true)); + free(buf); } - free(dirs); - - return result; + g_strfreev(dirs); + return list; } GList * @@ -73,8 +78,7 @@ resources_os_list_ocf_agents(const char *provider) GList *providers = NULL; if (provider) { - return services_os_get_directory_list_provider(PCMK__OCF_RA_PATH, provider, - TRUE, TRUE); + return list_provider_agents(provider); } providers = resources_os_list_ocf_providers(); From 11efc0adb1f6fe0e281f2cf56d659ba6d5c1d84a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 17:30:29 -0700 Subject: [PATCH 14/42] Refactor: libcrmservice: Simplify resources_os_list_ocf_agents() Signed-off-by: Reid Wahl --- lib/services/services_ocf.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index 1366b4cc4dc..36b39ad884a 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -73,25 +73,21 @@ list_provider_agents(const char *provider) GList * resources_os_list_ocf_agents(const char *provider) { - GList *gIter = NULL; - GList *result = NULL; + GList *list = NULL; GList *providers = NULL; - if (provider) { + if (provider != NULL) { return list_provider_agents(provider); } providers = resources_os_list_ocf_providers(); - for (gIter = providers; gIter != NULL; gIter = gIter->next) { - GList *tmp1 = result; - GList *tmp2 = resources_os_list_ocf_agents(gIter->data); - - if (tmp2) { - result = g_list_concat(tmp1, tmp2); - } + for (const GList *iter = providers; iter != NULL; iter = iter->next) { + provider = (const char *) iter->data; + list = g_list_concat(list, resources_os_list_ocf_agents(provider)); } + g_list_free_full(providers, free); - return result; + return list; } gboolean From cdf01dead5d02f545857859d0e98f88b0413d7ea Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 17:33:52 -0700 Subject: [PATCH 15/42] Refactor: libcrmservice: Avoid a services__list_dir() call ...in get_directory_list(). To do this, copy the current services__list_dir() function into a static helper called gdl_helper(). This is the only call to services__list_dir() that may take a false value for the executable argument. Signed-off-by: Reid Wahl --- lib/services/services.c | 57 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/lib/services/services.c b/lib/services/services.c index 9c81df246e9..0a11912abdf 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -9,6 +9,7 @@ #include +#include // bool, true, false #include #include #include @@ -1377,6 +1378,60 @@ services__grab_stderr(svc_action_t *action) #include +static GList * +gdl_helper(const char *dir, bool files, bool executable) +{ + GList *list = NULL; + struct dirent **namelist = NULL; + int entries = scandir(dir, &namelist, NULL, alphasort); + + if (entries < 0) { + return NULL; + } + + for (int i = 0; i < entries; i++) { + char *buffer = NULL; + struct stat sb; + int rc = 0; + + if ('.' == namelist[i]->d_name[0]) { + continue; + } + + buffer = pcmk__assert_asprintf("%s/%s", dir, namelist[i]->d_name); + rc = stat(buffer, &sb); + free(buffer); + + if (rc != 0) { + continue; + } + + if (S_ISDIR(sb.st_mode)) { + if (files) { + continue; + } + + } else if (S_ISREG(sb.st_mode)) { + if (!files) { + continue; + } + + if (executable + && !pcmk__any_flags_set(sb.st_mode, S_IXUSR|S_IXGRP|S_IXOTH)) { + continue; + } + } + + list = g_list_append(list, pcmk__str_copy(namelist[i]->d_name)); + } + + for (int i = 0; i < entries; i++) { + free(namelist[i]); + } + free(namelist); + return list; +} + GList * get_directory_list(const char *root, gboolean files, gboolean executable) { @@ -1390,7 +1445,7 @@ get_directory_list(const char *root, gboolean files, gboolean executable) dir_paths = g_strsplit(root, ":", 0); for (gchar **dir = dir_paths; *dir != NULL; dir++) { - list = g_list_concat(list, services__list_dir(*dir, files, executable)); + list = g_list_concat(list, gdl_helper(*dir, files, executable)); } g_strfreev(dir_paths); From d1fb2213ffccbba6b2ab82f4a901cd054c78d158 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 17:53:00 -0700 Subject: [PATCH 16/42] Refactor: libcrmservice: Drop third argument of services__list_dir() It's always true Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 30 +++++++++++++++++++++--------- lib/services/services_lsb.c | 2 +- lib/services/services_ocf.c | 4 ++-- lib/services/services_private.h | 2 +- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index fc6fe07dfbb..d9fe22b6776 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1429,8 +1429,23 @@ services__execute_file(svc_action_t *op) } } +/*! + * \internal + * \brief List directory's top-level contents of the given type + * + * Hidden files (those beginning with \c '.') are skipped. + * + * \param[in] dir Full path of directory to list + * \param[in] exec_files If \c true, list only executable files within \p dir. + * If \c false, list only directories within \p dir. + * + * \return Newly allocated list of newly allocated names of directory entries + * + * \note The caller is responsible for freeing the return value using + * g_list_free_full(list, free). + */ GList * -services__list_dir(const char *dir, bool files, bool executable) +services__list_dir(const char *dir, bool exec_files) { GList *list = NULL; struct dirent **namelist = NULL; @@ -1445,7 +1460,8 @@ services__list_dir(const char *dir, bool files, bool executable) struct stat sb; int rc = 0; - if ('.' == namelist[i]->d_name[0]) { + // Skip hidden files + if (namelist[i]->d_name[0] == '.') { continue; } @@ -1458,17 +1474,13 @@ services__list_dir(const char *dir, bool files, bool executable) } if (S_ISDIR(sb.st_mode)) { - if (files) { + if (exec_files) { continue; } } else if (S_ISREG(sb.st_mode)) { - if (!files) { - continue; - } - - if (executable - && !pcmk__any_flags_set(sb.st_mode, S_IXUSR|S_IXGRP|S_IXOTH)) { + if (!exec_files + || !pcmk__any_flags_set(sb.st_mode, S_IXUSR|S_IXGRP|S_IXOTH)) { continue; } } diff --git a/lib/services/services_lsb.c b/lib/services/services_lsb.c index f482d58f970..291b1f05c2b 100644 --- a/lib/services/services_lsb.c +++ b/lib/services/services_lsb.c @@ -244,7 +244,7 @@ services__get_lsb_metadata(const char *type, char **output) GList * services__list_lsb_agents(void) { - return services__list_dir(PCMK__LSB_INIT_DIR, true, true); + return services__list_dir(PCMK__LSB_INIT_DIR, true); } bool diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index 36b39ad884a..ffb29b47c22 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -28,7 +28,7 @@ resources_os_list_ocf_providers(void) gchar **dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); for (gchar **dir = dirs; *dir != NULL; dir++) { - list = g_list_concat(list, services__list_dir(*dir, false, true)); + list = g_list_concat(list, services__list_dir(*dir, false)); } g_strfreev(dirs); @@ -62,7 +62,7 @@ list_provider_agents(const char *provider) for (gchar **dir = dirs; *dir != NULL; dir++) { char *buf = pcmk__assert_asprintf("%s/%s", *dir, provider); - list = g_list_concat(list, services__list_dir(buf, true, true)); + list = g_list_concat(list, services__list_dir(buf, true)); free(buf); } diff --git a/lib/services/services_private.h b/lib/services/services_private.h index af7699add90..7fe84101de8 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -60,7 +60,7 @@ G_GNUC_INTERNAL const char *services__action_kind(const svc_action_t *action); G_GNUC_INTERNAL -GList *services__list_dir(const char *dir, bool files, bool executable); +GList *services__list_dir(const char *dir, bool exec_files); G_GNUC_INTERNAL int services__execute_file(svc_action_t *op); From 8f819866befdc1238a7ab1648a679aaf035ad6bb Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 17:59:38 -0700 Subject: [PATCH 17/42] Low: libcrmservice: List only the requested directory contents Previously, files of type other than directory and regular file would be listed unconditionally. Now, list only executable regular files if exec_files is true, and list only directories if exec_files is false. Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index d9fe22b6776..73783653dbb 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -1473,16 +1473,13 @@ services__list_dir(const char *dir, bool exec_files) continue; } - if (S_ISDIR(sb.st_mode)) { - if (exec_files) { - continue; - } - - } else if (S_ISREG(sb.st_mode)) { - if (!exec_files + if (exec_files) { + if (!S_ISREG(sb.st_mode) || !pcmk__any_flags_set(sb.st_mode, S_IXUSR|S_IXGRP|S_IXOTH)) { continue; } + } else if (!S_ISDIR(sb.st_mode)) { + continue; } list = g_list_append(list, pcmk__str_copy(namelist[i]->d_name)); From 487f38f33d1115c3f6b121cc4e75049852a56c88 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 19:16:04 -0700 Subject: [PATCH 18/42] Refactor: libcrmservice: Use scandir() filters for services__list_dir() Signed-off-by: Reid Wahl --- lib/services/services_linux.c | 102 ++++++++++++++++++++++++---------- 1 file changed, 72 insertions(+), 30 deletions(-) diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 73783653dbb..0be0b46fbed 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -28,6 +28,8 @@ #include "services_private.h" +static const char *filter_dir = NULL; + static void close_pipe(int fildes[]); /* We have two alternative ways of handling SIGCHLD when synchronously waiting @@ -1429,6 +1431,69 @@ services__execute_file(svc_action_t *op) } } +/*! + * \internal + * \brief \c scandir() filter for non-hidden executable regular files + * + * \param[in] entry Directory entry + * + * \retval 1 if the entry is an executable regular file and its name does not + * begin with \c "." + * \retval 0 otherwise + */ +static int +exec_file_filter(const struct dirent *entry) +{ + char *buf = NULL; + struct stat sb; + int rc = 0; + + if (entry->d_name[0] == '.') { + return rc; + } + + buf = pcmk__assert_asprintf("%s/%s", filter_dir, entry->d_name); + + if ((stat(buf, &sb) == 0) && S_ISREG(sb.st_mode) + && pcmk__any_flags_set(sb.st_mode, S_IXUSR|S_IXGRP|S_IXOTH)) { + + rc = 1; + } + + free(buf); + return rc; +} + +/*! + * \internal + * \brief \c scandir() filter for non-hidden directories + * + * \param[in] entry Directory entry + * + * \retval 1 if the entry is a directory and its name does not begin with \c "." + * \retval 0 otherwise + */ +static int +directory_filter(const struct dirent *entry) +{ + char *buf = NULL; + struct stat sb; + int rc = 0; + + if (entry->d_name[0] == '.') { + return rc; + } + + buf = pcmk__assert_asprintf("%s/%s", filter_dir, entry->d_name); + + if ((stat(buf, &sb) == 0) && S_ISDIR(sb.st_mode)) { + rc = 1; + } + + free(buf); + return rc; +} + /*! * \internal * \brief List directory's top-level contents of the given type @@ -1449,43 +1514,20 @@ services__list_dir(const char *dir, bool exec_files) { GList *list = NULL; struct dirent **namelist = NULL; - int entries = scandir(dir, &namelist, NULL, alphasort); + int entries = 0; + + filter_dir = dir; + entries = scandir(dir, &namelist, + (exec_files? exec_file_filter : directory_filter), + alphasort); + filter_dir = NULL; if (entries < 0) { return NULL; } for (int i = 0; i < entries; i++) { - char *buffer = NULL; - struct stat sb; - int rc = 0; - - // Skip hidden files - if (namelist[i]->d_name[0] == '.') { - continue; - } - - buffer = pcmk__assert_asprintf("%s/%s", dir, namelist[i]->d_name); - rc = stat(buffer, &sb); - free(buffer); - - if (rc != 0) { - continue; - } - - if (exec_files) { - if (!S_ISREG(sb.st_mode) - || !pcmk__any_flags_set(sb.st_mode, S_IXUSR|S_IXGRP|S_IXOTH)) { - continue; - } - } else if (!S_ISDIR(sb.st_mode)) { - continue; - } - list = g_list_append(list, pcmk__str_copy(namelist[i]->d_name)); - } - - for (int i = 0; i < entries; i++) { free(namelist[i]); } free(namelist); From 8984aaab9e0c4e02e4b4ba3f3362a404bd884ba1 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 21:41:07 -0700 Subject: [PATCH 19/42] Refactor: libcrmservice: Rename resources_os_list_ocf_providers() To services__list_ocf_providers(), in order to follow the library's naming conventions. Signed-off-by: Reid Wahl --- lib/services/services.c | 2 +- lib/services/services_ocf.c | 17 +++++++++++++++-- lib/services/services_ocf.h | 4 ++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index 0a11912abdf..fec289017de 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -1062,7 +1062,7 @@ GList * resources_list_providers(const char *standard) { if (pcmk__is_set(pcmk_get_ra_caps(standard), pcmk_ra_cap_provider)) { - return resources_os_list_ocf_providers(); + return services__list_ocf_providers(); } return NULL; diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index ffb29b47c22..f2153674404 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -21,8 +21,21 @@ #include "services_private.h" #include "services_ocf.h" +/*! + * \internal + * \brief List the OCF providers from \c PCMK__OCF_RA_PATH + * + * For each directory along \c PCMK__OCF_RA_PATH (a colon-delimited list), this + * function adds all top-level subdirectories to the list, excluding those + * beginning with \c '.'. + * + * \return Newly allocated list of OCF providers as newly allocated strings + * + * \note The caller is responsible for freeing the return value using + * g_list_free_full(list, free). + */ GList * -resources_os_list_ocf_providers(void) +services__list_ocf_providers(void) { GList *list = NULL; gchar **dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); @@ -80,7 +93,7 @@ resources_os_list_ocf_agents(const char *provider) return list_provider_agents(provider); } - providers = resources_os_list_ocf_providers(); + providers = services__list_ocf_providers(); for (const GList *iter = providers; iter != NULL; iter = iter->next) { provider = (const char *) iter->data; list = g_list_concat(list, resources_os_list_ocf_agents(provider)); diff --git a/lib/services/services_ocf.h b/lib/services/services_ocf.h index bc57d4e68fc..5d0001e91f2 100644 --- a/lib/services/services_ocf.h +++ b/lib/services/services_ocf.h @@ -1,6 +1,6 @@ /* * Copyright 2010-2011 Red Hat, Inc. - * Later changes copyright 2012-2024 the Pacemaker project contributors + * Later changes copyright 2012-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -21,7 +21,7 @@ extern "C" { #endif G_GNUC_INTERNAL -GList *resources_os_list_ocf_providers(void); +GList *services__list_ocf_providers(void); G_GNUC_INTERNAL GList *resources_os_list_ocf_agents(const char *provider); From 2f760c7644f423893a3e9fc3239d5ad1edc28e86 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 22:47:38 -0700 Subject: [PATCH 20/42] Refactor: libcrmservice: Rename resources_os_list_ocf_agents() To services__list_ocf_agents(), in order to follow the library's naming conventions. Also defunctionize list_provider_agents(). It seems more straightforward to have it inline. Signed-off-by: Reid Wahl --- lib/services/services.c | 4 +-- lib/services/services_ocf.c | 54 +++++++++++++++++-------------------- lib/services/services_ocf.h | 2 +- 3 files changed, 28 insertions(+), 32 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index fec289017de..17690618805 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -1083,7 +1083,7 @@ resources_list_agents(const char *standard, const char *provider) if (standard == NULL) { tmp1 = result; - tmp2 = resources_os_list_ocf_agents(NULL); + tmp2 = services__list_ocf_agents(NULL); if (tmp2) { result = g_list_concat(tmp1, tmp2); } @@ -1104,7 +1104,7 @@ resources_list_agents(const char *standard, const char *provider) return result; } else if (strcasecmp(standard, PCMK_RESOURCE_CLASS_OCF) == 0) { - return resources_os_list_ocf_agents(provider); + return services__list_ocf_agents(provider); #if PCMK__ENABLE_LSB } else if (strcasecmp(standard, PCMK_RESOURCE_CLASS_LSB) == 0) { return services__list_lsb_agents(); diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index f2153674404..8a41eee80fe 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -50,25 +50,41 @@ services__list_ocf_providers(void) /*! * \internal - * \brief List the agents from the given OCF provider + * \brief List the agents from the given OCF provider or from all OCF providers * - * For each directory along \c PCMK__OCF_RA_PATH (a colon-delimited list), this - * function looks for a subdirectory called \p provider. It then finds the top- - * level executable files inside that subdirectory, excluding those beginning - * with \c '.', and adds them to the list. + * If \p provider is not \c NULL, for each directory along \c PCMK__OCF_RA_PATH + * (a colon-delimited list), this function looks for a subdirectory called + * \p provider. It then finds the top-level executable files inside that + * subdirectory, excluding those beginning with \c '.', and adds them to the + * list. + * + * If \p provider is \c NULL, this function does the above for each provider and + * concatenates the results. * - * \param[in] provider OCF provider + * \param[in] provider OCF provider (\c NULL to list agents from all providers) * * \return Newly allocated list of OCF agents as newly allocated strings * * \note The caller is responsible for freeing the return value using * g_list_free_full(list, free). */ -static GList * -list_provider_agents(const char *provider) +GList * +services__list_ocf_agents(const char *provider) { - gchar **dirs = NULL; GList *list = NULL; + gchar **dirs = NULL; + + if (provider == NULL) { + // Make a recursive call for each provider and concatenate the results + GList *providers = services__list_ocf_providers(); + + for (const GList *iter = providers; iter != NULL; iter = iter->next) { + provider = (const char *) iter->data; + list = g_list_concat(list, services__list_ocf_agents(provider)); + } + g_list_free_full(providers, free); + return list; + } dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); @@ -83,26 +99,6 @@ list_provider_agents(const char *provider) return list; } -GList * -resources_os_list_ocf_agents(const char *provider) -{ - GList *list = NULL; - GList *providers = NULL; - - if (provider != NULL) { - return list_provider_agents(provider); - } - - providers = services__list_ocf_providers(); - for (const GList *iter = providers; iter != NULL; iter = iter->next) { - provider = (const char *) iter->data; - list = g_list_concat(list, resources_os_list_ocf_agents(provider)); - } - - g_list_free_full(providers, free); - return list; -} - gboolean services__ocf_agent_exists(const char *provider, const char *agent) { diff --git a/lib/services/services_ocf.h b/lib/services/services_ocf.h index 5d0001e91f2..999a8adc974 100644 --- a/lib/services/services_ocf.h +++ b/lib/services/services_ocf.h @@ -24,7 +24,7 @@ G_GNUC_INTERNAL GList *services__list_ocf_providers(void); G_GNUC_INTERNAL -GList *resources_os_list_ocf_agents(const char *provider); +GList *services__list_ocf_agents(const char *provider); G_GNUC_INTERNAL gboolean services__ocf_agent_exists(const char *provider, const char *agent); From 60a011e2439df2461f7caea27d973481c84e2b82 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 23:03:24 -0700 Subject: [PATCH 21/42] Refactor: libcrmservice: Simplify services__ocf_agent_exists() somewhat And return bool instead of gboolean. More to come. Signed-off-by: Reid Wahl --- lib/services/services_ocf.c | 47 +++++++++++++++++++++++-------------- lib/services/services_ocf.h | 4 +++- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index 8a41eee80fe..f5d733ae826 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -99,34 +99,45 @@ services__list_ocf_agents(const char *provider) return list; } -gboolean +/*! + * \internal + * \brief Check whether the given OCF agent from the given provider exists + * + * For each directory along \c PCMK__OCF_RA_PATH (a colon-delimited list), this + * function looks for a file called \p agent in a subdirectory called + * \p provider. It returns \c true if such a file is found. + * + * \param[in] provider OCF provider + * \param[in] agent OCF agent + * + * \return \c true if the agent is found or \c false otherwise + */ +bool services__ocf_agent_exists(const char *provider, const char *agent) { - gboolean rc = FALSE; - struct stat st; - char *dirs = strdup(PCMK__OCF_RA_PATH); - char *dir = NULL; - char *buf = NULL; + bool found = false; + char *dirs = NULL; + + if ((provider == NULL) || (agent == NULL) + || pcmk__str_empty(PCMK__OCF_RA_PATH)) { - if (provider == NULL || agent == NULL || pcmk__str_empty(dirs)) { - free(dirs); - return rc; + return false; } - for (dir = strtok(dirs, ":"); dir != NULL; dir = strtok(NULL, ":")) { - buf = pcmk__assert_asprintf("%s/%s/%s", dir, provider, agent); - if (stat(buf, &st) == 0) { - free(buf); - rc = TRUE; - break; - } + dirs = pcmk__str_copy(PCMK__OCF_RA_PATH); + + for (char *dir = strtok(dirs, ":"); !found && (dir != NULL); + dir = strtok(NULL, ":")) { + char *buf = pcmk__assert_asprintf("%s/%s/%s", dir, provider, agent); + struct stat sb; + + found = (stat(buf, &sb) == 0); free(buf); } free(dirs); - - return rc; + return found; } /*! diff --git a/lib/services/services_ocf.h b/lib/services/services_ocf.h index 999a8adc974..ca9a48d0d46 100644 --- a/lib/services/services_ocf.h +++ b/lib/services/services_ocf.h @@ -11,6 +11,8 @@ #ifndef PCMK__SERVICES_SERVICES_OCF__H #define PCMK__SERVICES_SERVICES_OCF__H +#include // bool + #include // G_GNUC_INTERNAL, GList, gboolean #include // enum ocf_exitcode @@ -27,7 +29,7 @@ G_GNUC_INTERNAL GList *services__list_ocf_agents(const char *provider); G_GNUC_INTERNAL -gboolean services__ocf_agent_exists(const char *provider, const char *agent); +bool services__ocf_agent_exists(const char *provider, const char *agent); G_GNUC_INTERNAL int services__ocf_prepare(svc_action_t *op); From 737f1364accde1cfb59ab1bb8119d2cbb3bf3a77 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 23:09:35 -0700 Subject: [PATCH 22/42] Refactor: libcrmservice: Use g_strsplit() in services__ocf_agent_exists For readability Signed-off-by: Reid Wahl --- lib/services/services_ocf.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index f5d733ae826..2e99c48ec77 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -116,27 +116,23 @@ bool services__ocf_agent_exists(const char *provider, const char *agent) { bool found = false; - char *dirs = NULL; - - if ((provider == NULL) || (agent == NULL) - || pcmk__str_empty(PCMK__OCF_RA_PATH)) { + gchar **dirs = NULL; + if ((provider == NULL) || (agent == NULL)) { return false; } - dirs = pcmk__str_copy(PCMK__OCF_RA_PATH); - - for (char *dir = strtok(dirs, ":"); !found && (dir != NULL); - dir = strtok(NULL, ":")) { + dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); - char *buf = pcmk__assert_asprintf("%s/%s/%s", dir, provider, agent); + for (gchar **dir = dirs; !found && (*dir != NULL); dir++) { + char *buf = pcmk__assert_asprintf("%s/%s/%s", *dir, provider, agent); struct stat sb; found = (stat(buf, &sb) == 0); free(buf); } - free(dirs); + g_strfreev(dirs); return found; } From aa90f0ab167a268bef75f52fc9c852a14c8890ee Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 23:24:44 -0700 Subject: [PATCH 23/42] Refactor: libcrmservice: Use g_strsplit() in services__ocf_prepare() And assert on memory errors Signed-off-by: Reid Wahl --- lib/services/services_ocf.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index 2e99c48ec77..04f965f4eb8 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -147,36 +147,29 @@ services__ocf_agent_exists(const char *provider, const char *agent) int services__ocf_prepare(svc_action_t *op) { - char *dirs = strdup(PCMK__OCF_RA_PATH); - struct stat st; - - if (dirs == NULL) { - return ENOMEM; - } + gchar **dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); // Look for agent on path - for (char *dir = strtok(dirs, ":"); dir != NULL; dir = strtok(NULL, ":")) { - char *buf = pcmk__assert_asprintf("%s/%s/%s", dir, op->provider, + for (gchar **dir = dirs; *dir != NULL; dir++) { + char *buf = pcmk__assert_asprintf("%s/%s/%s", *dir, op->provider, op->agent); + struct stat sb; - if (stat(buf, &st) == 0) { + if (stat(buf, &sb) == 0) { op->opaque->exec = buf; break; } free(buf); } - free(dirs); + + g_strfreev(dirs); if (op->opaque->exec == NULL) { return ENOENT; } - op->opaque->args[0] = strdup(op->opaque->exec); - op->opaque->args[1] = strdup(op->action); - if ((op->opaque->args[0] == NULL) || (op->opaque->args[1] == NULL)) { - return ENOMEM; - } - + op->opaque->args[0] = pcmk__str_copy(op->opaque->exec); + op->opaque->args[1] = pcmk__str_copy(op->action); return pcmk_rc_ok; } From 1d30a0daf6bce8de5434bdd041f511a0629bb2f9 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 23:32:57 -0700 Subject: [PATCH 24/42] Refactor: libcrmservice: Return path from services__ocf_agent_exists() Nothing uses this yet Signed-off-by: Reid Wahl --- lib/services/services.c | 2 +- lib/services/services_ocf.c | 22 ++++++++++++++++++---- lib/services/services_ocf.h | 3 ++- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index 17690618805..d6e160b61d8 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -1179,7 +1179,7 @@ resources_agent_exists(const char *standard, const char *provider, const char *a #endif if (pcmk__str_eq(standard, PCMK_RESOURCE_CLASS_OCF, pcmk__str_casei)) { - rc = services__ocf_agent_exists(provider, agent); + rc = services__ocf_agent_exists(provider, agent, NULL); #if PCMK__ENABLE_LSB } else if (pcmk__str_eq(standard, PCMK_RESOURCE_CLASS_LSB, pcmk__str_casei)) { diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index 04f965f4eb8..f1457cfee7a 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -107,17 +107,24 @@ services__list_ocf_agents(const char *provider) * function looks for a file called \p agent in a subdirectory called * \p provider. It returns \c true if such a file is found. * - * \param[in] provider OCF provider - * \param[in] agent OCF agent + * \param[in] provider OCF provider + * \param[in] agent OCF agent + * \param[out] path If not \c NULL, where to store full path to agent if + * found; unchanged if agent is not found * * \return \c true if the agent is found or \c false otherwise + * + * \note The caller is responsible for freeing \p *path on success using + * \c free(). */ bool -services__ocf_agent_exists(const char *provider, const char *agent) +services__ocf_agent_exists(const char *provider, const char *agent, char **path) { bool found = false; gchar **dirs = NULL; + pcmk__assert((path == NULL) || (*path == NULL)); + if ((provider == NULL) || (agent == NULL)) { return false; } @@ -128,7 +135,14 @@ services__ocf_agent_exists(const char *provider, const char *agent) char *buf = pcmk__assert_asprintf("%s/%s/%s", *dir, provider, agent); struct stat sb; - found = (stat(buf, &sb) == 0); + if (stat(buf, &sb) != 0) { + found = true; + + if (path != NULL) { + *path = buf; + buf = NULL; + } + } free(buf); } diff --git a/lib/services/services_ocf.h b/lib/services/services_ocf.h index ca9a48d0d46..61935628721 100644 --- a/lib/services/services_ocf.h +++ b/lib/services/services_ocf.h @@ -29,7 +29,8 @@ G_GNUC_INTERNAL GList *services__list_ocf_agents(const char *provider); G_GNUC_INTERNAL -bool services__ocf_agent_exists(const char *provider, const char *agent); +bool services__ocf_agent_exists(const char *provider, const char *agent, + char **path); G_GNUC_INTERNAL int services__ocf_prepare(svc_action_t *op); From 2d2f241024da9511dec2ecb721b6715c90ce7051 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 23:37:26 -0700 Subject: [PATCH 25/42] Refactor: libcrmservice: Reduce duplication in services__ocf_prepare() Signed-off-by: Reid Wahl --- lib/services/services_ocf.c | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/lib/services/services_ocf.c b/lib/services/services_ocf.c index f1457cfee7a..255777d2c4b 100644 --- a/lib/services/services_ocf.c +++ b/lib/services/services_ocf.c @@ -161,24 +161,8 @@ services__ocf_agent_exists(const char *provider, const char *agent, char **path) int services__ocf_prepare(svc_action_t *op) { - gchar **dirs = g_strsplit(PCMK__OCF_RA_PATH, ":", 0); - - // Look for agent on path - for (gchar **dir = dirs; *dir != NULL; dir++) { - char *buf = pcmk__assert_asprintf("%s/%s/%s", *dir, op->provider, - op->agent); - struct stat sb; - - if (stat(buf, &sb) == 0) { - op->opaque->exec = buf; - break; - } - free(buf); - } - - g_strfreev(dirs); - - if (op->opaque->exec == NULL) { + if (!services__ocf_agent_exists(op->provider, op->agent, + &(op->opaque->exec))) { return ENOENT; } From 975635f8fd812b50c3f6ba118d0332cf5e533871 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 23:43:28 -0700 Subject: [PATCH 26/42] Refactor: fencer: Reduce nesting in get_action_delay_base() And compare strchr() return value against NULL instead of 0. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 51 ++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 26775e2d32c..9a198c0c2a1 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -205,45 +205,50 @@ static int get_action_delay_base(const fenced_device_t *device, const char *action, const char *target) { - char *hash_value = NULL; + const char *param = NULL; guint delay_base = 0U; + char *value = NULL; + char *valptr = NULL; + if (!pcmk__is_fencing_action(action)) { return 0; } - hash_value = g_hash_table_lookup(device->params, PCMK_FENCING_DELAY_BASE); + param = g_hash_table_lookup(device->params, PCMK_FENCING_DELAY_BASE); + if (param == NULL) { + return 0; + } - if (hash_value) { - char *value = pcmk__str_copy(hash_value); - char *valptr = value; + value = pcmk__str_copy(param); + valptr = value; - if (target != NULL) { - for (char *val = strtok(value, "; \t"); val != NULL; val = strtok(NULL, "; \t")) { - char *mapval = strchr(val, ':'); + if (target != NULL) { + for (char *val = strtok(value, "; \t"); val != NULL; + val = strtok(NULL, "; \t")) { - if (mapval == NULL || mapval[1] == 0) { - crm_err("pcmk_delay_base: empty value in mapping", val); - continue; - } + char *mapval = strchr(val, ':'); - if (mapval != val && strncasecmp(target, val, (size_t)(mapval - val)) == 0) { - value = mapval + 1; - crm_debug("pcmk_delay_base mapped to %s for %s", - value, target); - break; - } + if (mapval == NULL || mapval[1] == 0) { + crm_err("pcmk_delay_base: empty value in mapping", val); + continue; } - } - if (strchr(value, ':') == 0) { - pcmk_parse_interval_spec(value, &delay_base); - delay_base /= 1000; + if (mapval != val && strncasecmp(target, val, (size_t)(mapval - val)) == 0) { + value = mapval + 1; + crm_debug("pcmk_delay_base mapped to %s for %s", value, target); + break; + } } + } - free(valptr); + if (strchr(value, ':') == NULL) { + pcmk_parse_interval_spec(value, &delay_base); + delay_base /= 1000; } + free(valptr); + return (int) delay_base; } From 2c25704cf3e1b83dd705dcec1b8a39bd1189e72e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 17 Sep 2025 23:50:55 -0700 Subject: [PATCH 27/42] Log: fencer: Fix a format string Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 9a198c0c2a1..4fecc2fc2a1 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -230,7 +230,7 @@ get_action_delay_base(const fenced_device_t *device, const char *action, char *mapval = strchr(val, ':'); if (mapval == NULL || mapval[1] == 0) { - crm_err("pcmk_delay_base: empty value in mapping", val); + crm_err("pcmk_delay_base: empty mapping for %s", val); continue; } From b1b359f7625cc38d0f3f87e688b86721dbc910b3 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 02:40:57 -0700 Subject: [PATCH 28/42] Log: fencer: Log an error for empty pcmk_delay_base mapping key Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 4fecc2fc2a1..4d7df840b65 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -229,12 +229,18 @@ get_action_delay_base(const fenced_device_t *device, const char *action, char *mapval = strchr(val, ':'); + if (mapval == val) { + // val starts with a colon, so the mapping key is empty + crm_err("pcmk_delay_base: Malformed value '%s'", val); + continue; + } + if (mapval == NULL || mapval[1] == 0) { crm_err("pcmk_delay_base: empty mapping for %s", val); continue; } - if (mapval != val && strncasecmp(target, val, (size_t)(mapval - val)) == 0) { + if (strncasecmp(target, val, (size_t)(mapval - val)) == 0) { value = mapval + 1; crm_debug("pcmk_delay_base mapped to %s for %s", value, target); break; From 3784aa455c4ce2b94166f79de02bba5a4e007dc7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 02:59:54 -0700 Subject: [PATCH 29/42] Refactor: fencer: Use g_strsplit() for pcmk_delay_base mappings It's still confusing, but hopefully a little bit less so. When delay_base_s is set, it means we've found our mapping. So we use that as a loop exit condition. We no longer need valptr, since we never change what value points to. Now that value isn't changing, it will be easier to feel confident about using g_strsplit_set() in place of strtok(). Note the similarity to pcmk__scan_nvpair(). Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 41 +++++++++++++++++--------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 4d7df840b65..2279088206d 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -206,10 +206,10 @@ get_action_delay_base(const fenced_device_t *device, const char *action, const char *target) { const char *param = NULL; + char *delay_base_s = NULL; guint delay_base = 0U; char *value = NULL; - char *valptr = NULL; if (!pcmk__is_fencing_action(action)) { return 0; @@ -221,39 +221,42 @@ get_action_delay_base(const fenced_device_t *device, const char *action, } value = pcmk__str_copy(param); - valptr = value; if (target != NULL) { - for (char *val = strtok(value, "; \t"); val != NULL; + for (char *val = strtok(value, "; \t"); + (val != NULL) && (delay_base_s == NULL); val = strtok(NULL, "; \t")) { - char *mapval = strchr(val, ':'); + gchar **nvpair = g_strsplit(val, ":", 2); - if (mapval == val) { - // val starts with a colon, so the mapping key is empty + if (pcmk__str_empty(nvpair[0]) || pcmk__str_empty(nvpair[1])) { crm_err("pcmk_delay_base: Malformed value '%s'", val); - continue; - } - if (mapval == NULL || mapval[1] == 0) { - crm_err("pcmk_delay_base: empty mapping for %s", val); - continue; + } else if (pcmk__str_eq(target, nvpair[0], pcmk__str_casei)) { + delay_base_s = pcmk__str_copy(nvpair[1]); + crm_debug("pcmk_delay_base mapped to %s for %s", delay_base_s, + target); } - if (strncasecmp(target, val, (size_t)(mapval - val)) == 0) { - value = mapval + 1; - crm_debug("pcmk_delay_base mapped to %s for %s", value, target); - break; - } + g_strfreev(nvpair); } } - if (strchr(value, ':') == NULL) { - pcmk_parse_interval_spec(value, &delay_base); + if (delay_base_s == NULL) { + /* Either target is NULL or we didn't find a mapping. Try to parse value + * itself, but take ownership so that we don't free value twice. + */ + delay_base_s = value; + value = NULL; + } + + if (strchr(delay_base_s, ':') == NULL) { + pcmk_parse_interval_spec(delay_base_s, &delay_base); delay_base /= 1000; } - free(valptr); + free(delay_base_s); + free(value); return (int) delay_base; } From 690864b0cf9bd92273d111c78d9386c80926160f Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 03:16:53 -0700 Subject: [PATCH 30/42] Refactor: fencer: Use g_strsplit_set() in get_action_delay_base() Instead of strtok(). This seems clearer. Skip empty mappings so that we don't log a "malformed" error. strtok() skipped contiguous sequences of delimiters, so we wouldn't try to parse an empty mapping. g_strsplit_set() returns an empty string between contiguous delimiters. Strip leading and trailing whitespace before parsing the param value, and don't try to parse as a sequence of mappings (delimited by ';', ' ', and '\t') if there are no more delimiters after stripping. This avoids logging an error when the value is formatted like a single interval: no colon, no semicolon, no spaces or tabs except optionally at the beginning or end of the string. It seems reasonable not to strip leading or trailing semicolons. We can ignore leading or trailing whitespace, but a leading or trailing semicolon seems like a clear indicator that this is supposed to be a mapping. This will result an error for values like ";3s " or " 3s;" but not for " 3s ". Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 66 +++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 2279088206d..ae1d87287ea 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -206,11 +206,10 @@ get_action_delay_base(const fenced_device_t *device, const char *action, const char *target) { const char *param = NULL; - char *delay_base_s = NULL; + gchar *stripped = NULL; + gchar *delay_base_s = NULL; guint delay_base = 0U; - char *value = NULL; - if (!pcmk__is_fencing_action(action)) { return 0; } @@ -220,34 +219,57 @@ get_action_delay_base(const fenced_device_t *device, const char *action, return 0; } - value = pcmk__str_copy(param); + stripped = g_strstrip(g_strdup(param)); if (target != NULL) { - for (char *val = strtok(value, "; \t"); - (val != NULL) && (delay_base_s == NULL); - val = strtok(NULL, "; \t")) { + gchar **mappings = g_strsplit_set(stripped, "; \t", 0); + + /* If there are no delimiters after stripping leading and trailing + * whitespace, then we want to parse the string as a single interval, + * rather than as a delimited list of mappings. Short-circuiting here + * avoids a "Malformed mapping" error message below. + */ + if (g_strv_length(mappings) >= 2) { + for (gchar **mapping = mappings; + (*mapping != NULL) && (delay_base_s == NULL); + mapping++) { - gchar **nvpair = g_strsplit(val, ":", 2); + gchar **nvpair = NULL; - if (pcmk__str_empty(nvpair[0]) || pcmk__str_empty(nvpair[1])) { - crm_err("pcmk_delay_base: Malformed value '%s'", val); + if (pcmk__str_empty(*mapping)) { + continue; + } - } else if (pcmk__str_eq(target, nvpair[0], pcmk__str_casei)) { - delay_base_s = pcmk__str_copy(nvpair[1]); - crm_debug("pcmk_delay_base mapped to %s for %s", delay_base_s, - target); - } + nvpair = g_strsplit(*mapping, ":", 2); - g_strfreev(nvpair); + if (pcmk__str_empty(nvpair[0]) || pcmk__str_empty(nvpair[1])) { + crm_err("pcmk_delay_base: Malformed mapping '%s'", + *mapping); + + } else if (pcmk__str_eq(target, nvpair[0], pcmk__str_casei)) { + /* Take ownership so that we don't free nvpair[1] with + * nvpair + */ + delay_base_s = nvpair[1]; + nvpair[1] = NULL; + + crm_debug("pcmk_delay_base mapped to %s for %s", + delay_base_s, target); + } + + g_strfreev(nvpair); + } } + g_strfreev(mappings); } if (delay_base_s == NULL) { - /* Either target is NULL or we didn't find a mapping. Try to parse value - * itself, but take ownership so that we don't free value twice. + /* Either target is NULL or we didn't find a mapping. Try to parse the + * stripped value itself. Take ownership so that we don't free stripped + * twice. */ - delay_base_s = value; - value = NULL; + delay_base_s = stripped; + stripped = NULL; } if (strchr(delay_base_s, ':') == NULL) { @@ -255,8 +277,8 @@ get_action_delay_base(const fenced_device_t *device, const char *action, delay_base /= 1000; } - free(delay_base_s); - free(value); + g_free(stripped); + g_free(delay_base_s); return (int) delay_base; } From 4f7d0832c2b3cf3e7ffe2a5979ea51b1d95a01b3 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 13:43:00 -0700 Subject: [PATCH 31/42] Refactor: fencer: Functionize part of get_action_delay_base() To reduce some nesting Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 63 ++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index ae1d87287ea..217c8edca3a 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -201,6 +201,38 @@ get_action_delay_max(const fenced_device_t *device, const char *action) return (int) delay_max; } +static gchar * +get_value_if_matching(const char *mapping, const char *target) +{ + gchar **nvpair = NULL; + gchar *value = NULL; + + if (pcmk__str_empty(mapping)) { + goto done; + } + + nvpair = g_strsplit(mapping, ":", 2); + + if (pcmk__str_empty(nvpair[0]) || pcmk__str_empty(nvpair[1])) { + crm_err(PCMK_FENCING_DELAY_BASE ": Malformed mapping '%s'", mapping); + goto done; + } + + if (!pcmk__str_eq(target, nvpair[0], pcmk__str_casei)) { + goto done; + } + + // Take ownership so that we don't free nvpair[1] with nvpair + value = nvpair[1]; + nvpair[1] = NULL; + + crm_debug(PCMK_FENCING_DELAY_BASE " mapped to %s for %s", value, target); + +done: + g_strfreev(nvpair); + return value; +} + static int get_action_delay_base(const fenced_device_t *device, const char *action, const char *target) @@ -230,34 +262,11 @@ get_action_delay_base(const fenced_device_t *device, const char *action, * avoids a "Malformed mapping" error message below. */ if (g_strv_length(mappings) >= 2) { - for (gchar **mapping = mappings; - (*mapping != NULL) && (delay_base_s == NULL); - mapping++) { - - gchar **nvpair = NULL; - - if (pcmk__str_empty(*mapping)) { - continue; + for (gchar **mapping = mappings; *mapping != NULL; mapping++) { + delay_base_s = get_value_if_matching(*mapping, target); + if (delay_base_s != NULL) { + break; } - - nvpair = g_strsplit(*mapping, ":", 2); - - if (pcmk__str_empty(nvpair[0]) || pcmk__str_empty(nvpair[1])) { - crm_err("pcmk_delay_base: Malformed mapping '%s'", - *mapping); - - } else if (pcmk__str_eq(target, nvpair[0], pcmk__str_casei)) { - /* Take ownership so that we don't free nvpair[1] with - * nvpair - */ - delay_base_s = nvpair[1]; - nvpair[1] = NULL; - - crm_debug("pcmk_delay_base mapped to %s for %s", - delay_base_s, target); - } - - g_strfreev(nvpair); } } g_strfreev(mappings); From a8b12f4396c5cbf6c5e877888fc825ffa2f17b80 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 13:52:19 -0700 Subject: [PATCH 32/42] Refactor: fencer: Avoid some more nesting in get_action_delay_base() Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 217c8edca3a..26d71f5ab80 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -239,6 +239,8 @@ get_action_delay_base(const fenced_device_t *device, const char *action, { const char *param = NULL; gchar *stripped = NULL; + gchar **mappings = NULL; + gchar *delay_base_s = NULL; guint delay_base = 0U; @@ -252,24 +254,21 @@ get_action_delay_base(const fenced_device_t *device, const char *action, } stripped = g_strstrip(g_strdup(param)); + mappings = g_strsplit_set(stripped, "; \t", 0); - if (target != NULL) { - gchar **mappings = g_strsplit_set(stripped, "; \t", 0); - - /* If there are no delimiters after stripping leading and trailing - * whitespace, then we want to parse the string as a single interval, - * rather than as a delimited list of mappings. Short-circuiting here - * avoids a "Malformed mapping" error message below. - */ - if (g_strv_length(mappings) >= 2) { - for (gchar **mapping = mappings; *mapping != NULL; mapping++) { - delay_base_s = get_value_if_matching(*mapping, target); - if (delay_base_s != NULL) { - break; - } + /* If there are no delimiters after stripping leading and trailing + * whitespace, then we want to parse the string as a single interval, rather + * than as a delimited list of mappings. Short-circuiting here when we split + * into fewer than two mappings avoids a "Malformed mapping" error message + * below. + */ + if ((target != NULL) && (g_strv_length(mappings) >= 2)) { + for (gchar **mapping = mappings; *mapping != NULL; mapping++) { + delay_base_s = get_value_if_matching(*mapping, target); + if (delay_base_s != NULL) { + break; } } - g_strfreev(mappings); } if (delay_base_s == NULL) { @@ -287,6 +286,7 @@ get_action_delay_base(const fenced_device_t *device, const char *action, } g_free(stripped); + g_strfreev(mappings); g_free(delay_base_s); return (int) delay_base; From 32eaebf720a3d4a317d3aaebf403eef59cfd7ae2 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 13:59:01 -0700 Subject: [PATCH 33/42] Refactor: fencer: Functionize getting delay base for target Seems cleaner since it isolates the "mappings" variable and g_strsplit_set() call. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 48 ++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 26d71f5ab80..e7d7672d20d 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -233,14 +233,40 @@ get_value_if_matching(const char *mapping, const char *target) return value; } +static gchar * +get_value_for_target(const char *target, const char *values) +{ + gchar *value = NULL; + gchar **mappings = g_strsplit_set(values, "; \t", 0); + + /* If there are no delimiters after stripping leading and trailing + * whitespace, then we want to parse the string as a single interval, rather + * than as a delimited list of mappings. Short-circuiting here when we split + * into fewer than two mappings avoids a "Malformed mapping" error message + * below. + */ + if (g_strv_length(mappings) < 2) { + goto done; + } + + for (gchar **mapping = mappings; *mapping != NULL; mapping++) { + value = get_value_if_matching(*mapping, target); + if (value != NULL) { + break; + } + } + +done: + g_strfreev(mappings); + return value; +} + static int get_action_delay_base(const fenced_device_t *device, const char *action, const char *target) { const char *param = NULL; gchar *stripped = NULL; - gchar **mappings = NULL; - gchar *delay_base_s = NULL; guint delay_base = 0U; @@ -254,21 +280,8 @@ get_action_delay_base(const fenced_device_t *device, const char *action, } stripped = g_strstrip(g_strdup(param)); - mappings = g_strsplit_set(stripped, "; \t", 0); - - /* If there are no delimiters after stripping leading and trailing - * whitespace, then we want to parse the string as a single interval, rather - * than as a delimited list of mappings. Short-circuiting here when we split - * into fewer than two mappings avoids a "Malformed mapping" error message - * below. - */ - if ((target != NULL) && (g_strv_length(mappings) >= 2)) { - for (gchar **mapping = mappings; *mapping != NULL; mapping++) { - delay_base_s = get_value_if_matching(*mapping, target); - if (delay_base_s != NULL) { - break; - } - } + if (target != NULL) { + delay_base_s = get_value_for_target(target, stripped); } if (delay_base_s == NULL) { @@ -286,7 +299,6 @@ get_action_delay_base(const fenced_device_t *device, const char *action, } g_free(stripped); - g_strfreev(mappings); g_free(delay_base_s); return (int) delay_base; From 8c20741dd2c5cff1e4617a28cae90ed09f2d8c63 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 14:01:05 -0700 Subject: [PATCH 34/42] Low: fencer: Fix ISO 8601 interval parsing in pcmk_delay_base An ISO 8601 interval can include a colon character. I can't think of any valid reason why a user would specify an interval this way for a fencing delay, but there doesn't seem to be any reason to exclude mapping values that have a colon. The "make sure there's no colon" check seemed to be an artifact of the way the parsing code worked before we started using g_strsplit(), etc. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index e7d7672d20d..66d7bbc0671 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -261,6 +261,10 @@ get_value_for_target(const char *target, const char *values) return value; } +/* @TODO Consolidate some of this with build_port_aliases(). But keep in mind + * that build_port_aliases()/pcmk__host_map supports either '=' or ':' as a + * mapping separator, while pcmk_delay_base supports only ':'. + */ static int get_action_delay_base(const fenced_device_t *device, const char *action, const char *target) @@ -293,10 +297,11 @@ get_action_delay_base(const fenced_device_t *device, const char *action, stripped = NULL; } - if (strchr(delay_base_s, ':') == NULL) { - pcmk_parse_interval_spec(delay_base_s, &delay_base); - delay_base /= 1000; - } + /* @TODO Should we accept only a simple time+units string, rather than an + * ISO 8601 interval? + */ + pcmk_parse_interval_spec(delay_base_s, &delay_base); + delay_base /= 1000; g_free(stripped); g_free(delay_base_s); From 6686a24d06a960bc575f268f1f2e51e4e3e6ab4e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 16:33:14 -0700 Subject: [PATCH 35/42] Test: cts-fencing: Test pcmk_delay_base/pcmk_delay_max properly Commit f0577783 fixed an off-by-one error that a test relied on to avoid randomness in the output. Also add a new test to ensure that pcmk_delay_base is capped by pcmk_delay_max. Signed-off-by: Reid Wahl --- cts/cts-fencing.in | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/cts/cts-fencing.in b/cts/cts-fencing.in index 04323310c90..1fe4a635683 100644 --- a/cts/cts-fencing.in +++ b/cts/cts-fencing.in @@ -510,21 +510,28 @@ class FenceTests(Tests): args='--output-as=xml -R true1 -a fence_dummy -o mode=pass -o "pcmk_host_list=node1 node2 node3" -o pcmk_delay_base=1') test.add_cmd("stonith_admin", args='--output-as=xml -R false1 -a fence_dummy -o mode=fail -o "pcmk_host_list=node1 node2 node3" -o pcmk_delay_base=1') - # Resulting "random" delay will always be 1 since (rand() % (delay_max - delay_base)) is always 0 here + + # Resulting "random" delay will be either 1 or 2 seconds test.add_cmd("stonith_admin", args='--output-as=xml -R true2 -a fence_dummy -o mode=pass -o "pcmk_host_list=node1 node2 node3" -o pcmk_delay_base=1 -o pcmk_delay_max=2') + + # Resulting delay will be 1 second (capped by pcmk_delay_max) test.add_cmd("stonith_admin", - args='--output-as=xml -R true3 -a fence_dummy -o mode=pass -o "pcmk_host_list=node1 node2 node3"') + args='--output-as=xml -R true3 -a fence_dummy -o mode=pass -o "pcmk_host_list=node1 node2 node3" -o pcmk_delay_base=2 -o pcmk_delay_max=1') + + test.add_cmd("stonith_admin", + args='--output-as=xml -R true4 -a fence_dummy -o mode=pass -o "pcmk_host_list=node1 node2 node3"') test.add_cmd("stonith_admin", args="--output-as=xml -r node3 -i 1 -v true1") test.add_cmd("stonith_admin", args="--output-as=xml -r node3 -i 1 -v false1") test.add_cmd("stonith_admin", args="--output-as=xml -r node3 -i 2 -v true2") test.add_cmd("stonith_admin", args="--output-as=xml -r node3 -i 2 -v true3") + test.add_cmd("stonith_admin", args="--output-as=xml -r node3 -i 2 -v true4") test.add_cmd("stonith_admin", args="--output-as=xml -F node3 --delay 1") # Total fencing timeout takes all fencing delays into account - test.add_log_pattern("Total timeout set to 582s") + test.add_log_pattern("Total timeout set to 727s") # Fencing timeout for the first device takes the requested fencing delay # and pcmk_delay_base into account @@ -543,9 +550,15 @@ class FenceTests(Tests): # Fencing timeout takes pcmk_delay_max into account test.add_log_pattern(r"Requesting that .* perform 'off' action targeting node3 using true2 .*146s.*", regex=True) - test.add_log_pattern("Delaying 'off' action targeting node3 using true2 for 1s | timeout=120s requested_delay=0s base=1s max=2s") + test.add_log_pattern(r"Delaying 'off' action targeting node3 using true2 for [12]s " + "| timeout=120s requested_delay=0s base=1s max=2s", + regex=True) + test.add_log_pattern(r"Requesting that .* perform 'off' action targeting node3 using true3 .*145s.*", + regex=True) + test.add_log_pattern("Delaying 'off' action targeting node3 using true3 for 1s " + "| timeout=120s requested_delay=0s base=1s max=1s") - test.add_log_pattern("Delaying 'off' action targeting node3 using true3", + test.add_log_pattern("Delaying 'off' action targeting node3 using true4", negative=True) def build_unfence_tests(self): From 3579d821a65cec9b2a59068338978279676119df Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 17:28:57 -0700 Subject: [PATCH 36/42] Refactor: fencer: Make build_port_aliases():lpc loop-scope and rename ...to i. Also rename max to len. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 33 +++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 66d7bbc0671..5e378bfb7e3 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -904,30 +904,32 @@ static GHashTable * build_port_aliases(const char *hostmap, GList ** targets) { char *name = NULL; - int last = 0, lpc = 0, max = 0, added = 0; + int last = 0; + int added = 0; + size_t len = 0; GHashTable *aliases = pcmk__strikey_table(free, free); if (hostmap == NULL) { return aliases; } - max = strlen(hostmap); - for (; lpc <= max; lpc++) { - switch (hostmap[lpc]) { + len = strlen(hostmap); + for (int i = 0; i <= len; i++) { + switch (hostmap[i]) { /* Skip escaped chars */ case '\\': - lpc++; + i++; break; /* Assignment chars */ case '=': case ':': - if (lpc > last) { + if (i > last) { free(name); - name = pcmk__assert_alloc(1, 1 + lpc - last); - memcpy(name, hostmap + last, lpc - last); + name = pcmk__assert_alloc(1, 1 + i - last); + memcpy(name, hostmap + last, i - last); } - last = lpc + 1; + last = i + 1; break; /* Delimeter chars */ @@ -940,8 +942,8 @@ build_port_aliases(const char *hostmap, GList ** targets) char *value = NULL; int k = 0; - value = pcmk__assert_alloc(1, 1 + lpc - last); - memcpy(value, hostmap + last, lpc - last); + value = pcmk__assert_alloc(1, 1 + i - last); + memcpy(value, hostmap + last, i - last); for (int i = 0; value[i] != '\0'; i++) { if (value[i] != '\\') { @@ -959,15 +961,16 @@ build_port_aliases(const char *hostmap, GList ** targets) name = NULL; added++; - } else if (lpc > last) { - crm_debug("Parse error at offset %d near '%s'", lpc - last, hostmap + last); + } else if (i > last) { + crm_debug("Parse error at offset %d near '%s'", i - last, + hostmap + last); } - last = lpc + 1; + last = i + 1; break; } - if (hostmap[lpc] == 0) { + if (hostmap[i] == 0) { break; } } From 0431aae10efc318ab00eef95b037fc4d534507a9 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 17:30:52 -0700 Subject: [PATCH 37/42] Refactor: fencer: Assume build_port_aliases():targets is non-NULL This is a static function with only one caller, and that caller always passes the address of a struct member. 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 5e378bfb7e3..89d82060a05 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -954,9 +954,7 @@ build_port_aliases(const char *hostmap, GList ** targets) crm_debug("Adding alias '%s'='%s'", name, value); g_hash_table_replace(aliases, name, value); - if (targets) { - *targets = g_list_append(*targets, pcmk__str_copy(value)); - } + *targets = g_list_append(*targets, pcmk__str_copy(value)); value = NULL; name = NULL; added++; From f9c5d0949a585e359e0b347832ff34f26ee12e49 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 17:33:36 -0700 Subject: [PATCH 38/42] Log: fencer: Drop unhelpful message from build_port_aliases() Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 89d82060a05..ae398fc53be 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -905,7 +905,6 @@ build_port_aliases(const char *hostmap, GList ** targets) { char *name = NULL; int last = 0; - int added = 0; size_t len = 0; GHashTable *aliases = pcmk__strikey_table(free, free); @@ -957,7 +956,6 @@ build_port_aliases(const char *hostmap, GList ** targets) *targets = g_list_append(*targets, pcmk__str_copy(value)); value = NULL; name = NULL; - added++; } else if (i > last) { crm_debug("Parse error at offset %d near '%s'", i - last, @@ -973,10 +971,6 @@ build_port_aliases(const char *hostmap, GList ** targets) } } - if (added == 0) { - crm_info("No host mappings detected in '%s'", hostmap); - } - free(name); return aliases; } From 4df0aef672364046c2c998b4c68edbacbc043120 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 17:38:18 -0700 Subject: [PATCH 39/42] Refactor: fencer: Don't set build_port_aliases():value to NULL It's about to go out of scope, so it doesn't get reused. It also doesn't get freed because the aliases table has taken ownership of it. 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 ae398fc53be..5faef8a951b 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -938,10 +938,9 @@ build_port_aliases(const char *hostmap, GList ** targets) case ' ': case '\t': if (name) { - char *value = NULL; int k = 0; + char *value = pcmk__assert_alloc(1, 1 + i - last); - value = pcmk__assert_alloc(1, 1 + i - last); memcpy(value, hostmap + last, i - last); for (int i = 0; value[i] != '\0'; i++) { @@ -954,7 +953,6 @@ build_port_aliases(const char *hostmap, GList ** targets) crm_debug("Adding alias '%s'='%s'", name, value); g_hash_table_replace(aliases, name, value); *targets = g_list_append(*targets, pcmk__str_copy(value)); - value = NULL; name = NULL; } else if (i > last) { From 0af49a3182bcf1fa4c9cc2f9fb7503aaeac77921 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 17:40:25 -0700 Subject: [PATCH 40/42] Refactor: various: Use correct pcmk__assert_alloc() arg order This should make absolutely no difference but uses the correct semantics. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 5 +++-- lib/cluster/cpg.c | 2 +- lib/common/actions.c | 2 +- lib/common/fuzzers/iso8601_fuzzer.c | 4 ++-- lib/common/fuzzers/scores_fuzzer.c | 4 ++-- lib/common/fuzzers/strings_fuzzer.c | 2 +- lib/common/output_text.c | 2 +- lib/common/procfs.c | 2 +- lib/common/remote.c | 2 +- lib/pacemaker/pcmk_injections.c | 4 ++-- tools/crm_mon_curses.c | 2 +- 11 files changed, 16 insertions(+), 15 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 5faef8a951b..78828585daa 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -925,7 +925,7 @@ build_port_aliases(const char *hostmap, GList ** targets) case ':': if (i > last) { free(name); - name = pcmk__assert_alloc(1, 1 + i - last); + name = pcmk__assert_alloc(1 + i - last, sizeof(char)); memcpy(name, hostmap + last, i - last); } last = i + 1; @@ -939,7 +939,8 @@ build_port_aliases(const char *hostmap, GList ** targets) case '\t': if (name) { int k = 0; - char *value = pcmk__assert_alloc(1, 1 + i - last); + char *value = pcmk__assert_alloc(1 + i - last, + sizeof(char)); memcpy(value, hostmap + last, i - last); diff --git a/lib/cluster/cpg.c b/lib/cluster/cpg.c index ae95fe93ab0..68df41aeee5 100644 --- a/lib/cluster/cpg.c +++ b/lib/cluster/cpg.c @@ -466,7 +466,7 @@ pcmk__cpg_message_data(cpg_handle_t handle, uint32_t sender_id, uint32_t pid, if (msg->is_compressed && (msg->size > 0)) { int rc = BZ_OK; unsigned int new_size = msg->size + 1; - char *uncompressed = pcmk__assert_alloc(1, new_size); + char *uncompressed = pcmk__assert_alloc(new_size, sizeof(char)); rc = BZ2_bzBuffToBuffDecompress(uncompressed, &new_size, msg->data, msg->compressed_size, 1, 0); diff --git a/lib/common/actions.c b/lib/common/actions.c index 25a9dc4cdf9..cc6053120c6 100644 --- a/lib/common/actions.c +++ b/lib/common/actions.c @@ -403,7 +403,7 @@ decode_transition_magic(const char *magic, char **uuid, int *transition_id, int res = sscanf(magic, "%d:%d;%ms", &local_op_status, &local_op_rc, &key); #else // magic must have >=4 other characters - key = pcmk__assert_alloc(1, strlen(magic) - 3); + key = pcmk__assert_alloc(strlen(magic) - 3, sizeof(char)); res = sscanf(magic, "%d:%d;%s", &local_op_status, &local_op_rc, key); #endif if (res == EOF) { diff --git a/lib/common/fuzzers/iso8601_fuzzer.c b/lib/common/fuzzers/iso8601_fuzzer.c index 0e151c6dbd8..d164d5417f7 100644 --- a/lib/common/fuzzers/iso8601_fuzzer.c +++ b/lib/common/fuzzers/iso8601_fuzzer.c @@ -1,5 +1,5 @@ /* - * Copyright 2024 the Pacemaker project contributors + * Copyright 2024-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -27,7 +27,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) if (size < 10) { return -1; // Do not add input to testing corpus } - ns = pcmk__assert_alloc(1, size + 1); + ns = pcmk__assert_alloc(size + 1, sizeof(char)); memcpy(ns, data, size); period = crm_time_parse_period(ns); diff --git a/lib/common/fuzzers/scores_fuzzer.c b/lib/common/fuzzers/scores_fuzzer.c index d20d65d1ad0..918d936a0fb 100644 --- a/lib/common/fuzzers/scores_fuzzer.c +++ b/lib/common/fuzzers/scores_fuzzer.c @@ -1,5 +1,5 @@ /* - * Copyright 2024 the Pacemaker project contributors + * Copyright 2024-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -21,7 +21,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) guint result = 0U; if (size > 0) { - ns = pcmk__assert_alloc(1, size + 1); + ns = pcmk__assert_alloc(size + 1, sizeof(char)); memcpy(ns, data, size); ns[size] = '\0'; } diff --git a/lib/common/fuzzers/strings_fuzzer.c b/lib/common/fuzzers/strings_fuzzer.c index 6f434856264..379993180ad 100644 --- a/lib/common/fuzzers/strings_fuzzer.c +++ b/lib/common/fuzzers/strings_fuzzer.c @@ -28,7 +28,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) if (size < 10) { return -1; // Do not add input to testing corpus } - ns = pcmk__assert_alloc(1, size + 1); + ns = pcmk__assert_alloc(size + 1, sizeof(char)); memcpy(ns, data, size); ns[size] = '\0'; diff --git a/lib/common/output_text.c b/lib/common/output_text.c index b19927777a8..e71c7ab5fba 100644 --- a/lib/common/output_text.c +++ b/lib/common/output_text.c @@ -481,7 +481,7 @@ pcmk__text_prompt(const char *prompt, bool echo, char **dest) #if HAVE_SSCANF_M rc = scanf("%ms", dest); #else - *dest = pcmk__assert_alloc(1, 1024); + *dest = pcmk__assert_alloc(1024, sizeof(char)); rc = scanf("%1023s", *dest); #endif fprintf(stderr, "\n"); diff --git a/lib/common/procfs.c b/lib/common/procfs.c index fd1b8100e52..393e33c8f07 100644 --- a/lib/common/procfs.c +++ b/lib/common/procfs.c @@ -398,7 +398,7 @@ pcmk__throttle_cib_load(const char *server, float *load) } if (fgets(buffer, sizeof(buffer), stream) != NULL) { - char *comm = pcmk__assert_alloc(1, 256); + char *comm = pcmk__assert_alloc(256, sizeof(char)); char state = 0; int rc = 0, pid = 0, ppid = 0, pgrp = 0, session = 0, tty_nr = 0, tpgid = 0; unsigned long flags = 0, minflt = 0, cminflt = 0, majflt = 0, cmajflt = 0, utime = 0, stime = 0; diff --git a/lib/common/remote.c b/lib/common/remote.c index 46520b8b557..76e98b902c9 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -293,7 +293,7 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) int rc = 0; unsigned int size_u = 1 + header->payload_uncompressed; char *uncompressed = - pcmk__assert_alloc(1, header->payload_offset + size_u); + pcmk__assert_alloc(header->payload_offset + size_u, sizeof(char)); crm_trace("Decompressing message data %d bytes into %d bytes", header->payload_compressed, size_u); diff --git a/lib/pacemaker/pcmk_injections.c b/lib/pacemaker/pcmk_injections.c index 498c06af0cd..64a28e01bf6 100644 --- a/lib/pacemaker/pcmk_injections.c +++ b/lib/pacemaker/pcmk_injections.c @@ -581,8 +581,8 @@ inject_action(pcmk__output_t *out, const char *spec, cib_t *cib, out->message(out, "inject-spec", spec); - key = pcmk__assert_alloc(1, strlen(spec) + 1); - node = pcmk__assert_alloc(1, strlen(spec) + 1); + key = pcmk__assert_alloc(strlen(spec) + 1, sizeof(char)); + node = pcmk__assert_alloc(strlen(spec) + 1, sizeof(char)); rc = sscanf(spec, "%[^@]@%[^=]=%d", key, node, &outcome); if (rc != 3) { out->err(out, "Invalid operation spec: %s. Only found %d fields", diff --git a/tools/crm_mon_curses.c b/tools/crm_mon_curses.c index 0792dd036cc..15e1d121b69 100644 --- a/tools/crm_mon_curses.c +++ b/tools/crm_mon_curses.c @@ -316,7 +316,7 @@ curses_prompt(const char *prompt, bool do_echo, char **dest) free(*dest); } - *dest = pcmk__assert_alloc(1, 1024); + *dest = pcmk__assert_alloc(1024, sizeof(char)); /* On older systems, scanw is defined as taking a char * for its first argument, * while newer systems rightly want a const char *. Accomodate both here due * to building with -Werror. From 8e852db9b58dfa557a64b22b571cd30b5d5dfee2 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 19:30:26 -0700 Subject: [PATCH 41/42] Low: fencer: Drop support for escaped characters in pcmk_host_map This reverts e95198f. Backslash escapes are useful in C and on the command line, but it's not clear how they would be useful in a pcmk_host_map value. Backslash escapes are not used in XML, where the configuration is stored. The removed code did the following upon finding a backslash: * If we're not inside the value part of a host-to-port mapping, skip both a backslash and the character after it. * Otherwise, skip the backslash and keep the character after it (unless that character is also a backslash). This doesn't seem to make sense. Any characters that are special in XML attribute values would need to be escaped using XML entities. Other characters, including backslashes, are allowed as literals. * https://www.w3.org/TR/REC-xml/#NT-AttValue Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 78828585daa..1fba33b695f 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -915,11 +915,6 @@ build_port_aliases(const char *hostmap, GList ** targets) len = strlen(hostmap); for (int i = 0; i <= len; i++) { switch (hostmap[i]) { - /* Skip escaped chars */ - case '\\': - i++; - break; - /* Assignment chars */ case '=': case ':': @@ -938,19 +933,11 @@ build_port_aliases(const char *hostmap, GList ** targets) case ' ': case '\t': if (name) { - int k = 0; char *value = pcmk__assert_alloc(1 + i - last, sizeof(char)); memcpy(value, hostmap + last, i - last); - for (int i = 0; value[i] != '\0'; i++) { - if (value[i] != '\\') { - value[k++] = value[i]; - } - } - value[k] = '\0'; - crm_debug("Adding alias '%s'='%s'", name, value); g_hash_table_replace(aliases, name, value); *targets = g_list_append(*targets, pcmk__str_copy(value)); From b93681741cc8e91f2c22948619bbed42fd5039bb Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Thu, 18 Sep 2025 20:31:02 -0700 Subject: [PATCH 42/42] Feature: fencer: Improve validation of pcmk_host_map Previously, if we hit a parsing error in pcmk_host_map, we logged a debug message and continued trying to parse the rest of the value. It's hard to be confident about the resulting behavior. Now, we stop parsing if we encounter an error. Also as part of this commit, we use g_strsplit_set() twice. At the first level, we use it to split the list of node-name-to-port mappings in pcmk_host_map, using ';', ' ', and '\t' as delimiters. At the second level, we use it to split each mapping, using '=' and ':' as delimiters. Support for '=' is likely to be removed in a future release. I greatly struggled to reason about the existing code based on "last = i + 1". It felt as if there were lots of potential edge cases. The behavior may not be identical as of this commit, but it should be more robust and predictable, and the code should be easier to maintain. Signed-off-by: Reid Wahl --- daemons/fenced/fenced_commands.c | 70 ++++++++++++-------------------- 1 file changed, 25 insertions(+), 45 deletions(-) diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c index 1fba33b695f..31e430e8cdf 100644 --- a/daemons/fenced/fenced_commands.c +++ b/daemons/fenced/fenced_commands.c @@ -901,63 +901,43 @@ fenced_free_device_table(void) } static GHashTable * -build_port_aliases(const char *hostmap, GList ** targets) +build_port_aliases(const char *hostmap, GList **targets) { - char *name = NULL; - int last = 0; - size_t len = 0; GHashTable *aliases = pcmk__strikey_table(free, free); + gchar *stripped = NULL; + gchar **mappings = NULL; - if (hostmap == NULL) { - return aliases; + if (pcmk__str_empty(hostmap)) { + goto done; } - len = strlen(hostmap); - for (int i = 0; i <= len; i++) { - switch (hostmap[i]) { - /* Assignment chars */ - case '=': - case ':': - if (i > last) { - free(name); - name = pcmk__assert_alloc(1 + i - last, sizeof(char)); - memcpy(name, hostmap + last, i - last); - } - last = i + 1; - break; + stripped = g_strstrip(g_strdup(hostmap)); + mappings = g_strsplit_set(stripped, "; \t", 0); - /* Delimeter chars */ - /* case ',': Potentially used to specify multiple ports */ - case 0: - case ';': - case ' ': - case '\t': - if (name) { - char *value = pcmk__assert_alloc(1 + i - last, - sizeof(char)); - - memcpy(value, hostmap + last, i - last); - - crm_debug("Adding alias '%s'='%s'", name, value); - g_hash_table_replace(aliases, name, value); - *targets = g_list_append(*targets, pcmk__str_copy(value)); - name = NULL; - - } else if (i > last) { - crm_debug("Parse error at offset %d near '%s'", i - last, - hostmap + last); - } + for (gchar **mapping = mappings; *mapping != NULL; mapping++) { + gchar **nvpair = NULL; - last = i + 1; - break; + if (pcmk__str_empty(*mapping)) { + continue; } - if (hostmap[i] == 0) { - break; + // @COMPAT Drop support for '=' as delimiter + nvpair = g_strsplit_set(*mapping, ":=", 2); + + if (pcmk__str_empty(nvpair[0]) || pcmk__str_empty(nvpair[1])) { + crm_err(PCMK_FENCING_HOST_MAP ": Malformed mapping '%s'", *mapping); + + } else { + crm_debug("Adding alias '%s'='%s'", nvpair[0], nvpair[1]); + pcmk__insert_dup(aliases, nvpair[0], nvpair[1]); + *targets = g_list_append(*targets, pcmk__str_copy(nvpair[1])); } + g_strfreev(nvpair); } - free(name); +done: + g_free(stripped); + g_strfreev(mappings); return aliases; }