From 9936127ab4eff0384aac2f3d1a26ca2e547f5969 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 14:35:47 -0700 Subject: [PATCH 01/32] Refactor: tools: Drop forward declarations in crm_diff.c And limit line length of SUMMARY Signed-off-by: Reid Wahl --- tools/crm_diff.c | 55 +++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 173e87f5cee..40c7194e118 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -24,8 +24,9 @@ #include #include -#define SUMMARY "Compare two Pacemaker configurations (in XML format) to produce a custom diff-like output, " \ - "or apply such an output as a patch" +#define SUMMARY "Compare two Pacemaker configurations (in XML format) to " \ + "produce a custom diff-like output, or apply such an output " \ + "as a patch" struct { gboolean apply; @@ -38,9 +39,32 @@ struct { char *xml_file_new; } options; -gboolean new_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, GError **error); -gboolean original_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, GError **error); -gboolean patch_cb(const gchar *option_name, const gchar *optarg, gpointer data, GError **error); +static gboolean +new_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, + GError **error) +{ + options.raw_new = TRUE; + pcmk__str_update(&options.xml_file_new, optarg); + return TRUE; +} + +static gboolean +original_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, + GError **error) +{ + options.raw_original = TRUE; + pcmk__str_update(&options.xml_file_original, optarg); + return TRUE; +} + +static gboolean +patch_cb(const gchar *option_name, const gchar *optarg, gpointer data, + GError **error) +{ + options.apply = TRUE; + pcmk__str_update(&options.xml_file_new, optarg); + return TRUE; +} static GOptionEntry original_xml_entries[] = { { "original", 'o', 0, G_OPTION_ARG_STRING, &options.xml_file_original, @@ -88,27 +112,6 @@ static GOptionEntry addl_entries[] = { { NULL } }; -gboolean -new_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, GError **error) { - options.raw_new = TRUE; - pcmk__str_update(&options.xml_file_new, optarg); - return TRUE; -} - -gboolean -original_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, GError **error) { - options.raw_original = TRUE; - pcmk__str_update(&options.xml_file_original, optarg); - return TRUE; -} - -gboolean -patch_cb(const gchar *option_name, const gchar *optarg, gpointer data, GError **error) { - options.apply = TRUE; - pcmk__str_update(&options.xml_file_new, optarg); - return TRUE; -} - static void print_patch(xmlNode *patch) { From e2cb3e47f84ebd3a9000fff79b0d2b861744e1e9 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 14:42:19 -0700 Subject: [PATCH 02/32] Doc: tools: Document input source precedence in crm_diff help output This is highly unfortunate, but changing it would break backward compatibility. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 40c7194e118..24ddb0fed70 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -27,6 +27,7 @@ #define SUMMARY "Compare two Pacemaker configurations (in XML format) to " \ "produce a custom diff-like output, or apply such an output " \ "as a patch" +#define INDENT " " struct { gboolean apply; @@ -66,12 +67,17 @@ patch_cb(const gchar *option_name, const gchar *optarg, gpointer data, return TRUE; } +// @COMPAT Use last-one-wins for original/new/patch input sources static GOptionEntry original_xml_entries[] = { { "original", 'o', 0, G_OPTION_ARG_STRING, &options.xml_file_original, - "XML is contained in the named file", + "XML is contained in the named file. Currently --original-string and\n" + INDENT "--stdin both override this. In a future release, the last one\n" + INDENT "specified will be used.", "FILE" }, { "original-string", 'O', 0, G_OPTION_ARG_CALLBACK, original_string_cb, - "XML is contained in the supplied string", + "XML is contained in the supplied string. Currently this takes\n" + INDENT "precedence over both --stdin and --original. In a future\n" + INDENT "release, the last one specified will be used.", "STRING" }, { NULL } @@ -79,13 +85,22 @@ static GOptionEntry original_xml_entries[] = { static GOptionEntry operation_entries[] = { { "new", 'n', 0, G_OPTION_ARG_STRING, &options.xml_file_new, - "Compare the original XML to the contents of the named file", + "Compare the original XML to the contents of the named file. Currently\n" + INDENT "--new-string and --stdin both override this. In a future\n" + INDENT "release, the last one specified will be used.", "FILE" }, { "new-string", 'N', 0, G_OPTION_ARG_CALLBACK, new_string_cb, - "Compare the original XML with the contents of the supplied string", + "Compare the original XML with the contents of the supplied string.\n" + INDENT "Currently this takes precedence over --stdin, --patch, and\n" + INDENT "--new. In a future release, the last one specified will be used.", "STRING" }, { "patch", 'p', 0, G_OPTION_ARG_CALLBACK, patch_cb, - "Patch the original XML with the contents of the named file", + "Patch the original XML with the contents of the named file. Currently\n" + INDENT "--new-string, --stdin, and (if specified later) --new override\n" + INDENT "the input source specified here. In a future release, the last\n" + INDENT "one specified will be used. Note: even if this input source is\n" + INDENT "overridden, the input source will be applied as a patch to the\n" + INDENT "original XML.", "FILE" }, { NULL } @@ -100,13 +115,16 @@ static GOptionEntry addl_entries[] = { * the versions from the patchset unless --no-version is given. */ { "cib", 'c', 0, G_OPTION_ARG_NONE, &options.as_cib, - "Compare/patch the inputs as a CIB (includes versions details)", + "Compare/patch the inputs as a CIB (includes version details)", NULL }, { "stdin", 's', 0, G_OPTION_ARG_NONE, &options.use_stdin, - "", + "Get the original XML and new (or patch) XML from stdin. Currently\n" + INDENT "--original-string and --new-string override this for original\n" + INDENT "and new/patch XML, respectively. In a future release, the last\n" + INDENT "one specified will be used.", NULL }, { "no-version", 'u', 0, G_OPTION_ARG_NONE, &options.no_version, - "Generate the difference without versions details", + "Generate the difference without version details", NULL }, { NULL } From ba537cb13eda8db31ae5009641c9cc4a420795d7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Tue, 24 Jun 2025 22:33:20 -0700 Subject: [PATCH 03/32] Refactor: tools: Drop xml_file_new arg of crm_diff.c:generate_patch() It was for logging only, and it's not particularly helpful information, especially since this is invoked only from the CLI currently. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 24ddb0fed70..ceb913a65bf 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -190,8 +190,8 @@ log_patch_cib_versions(xmlNode *patch) // \return Standard Pacemaker return code static int -generate_patch(xmlNode *object_original, xmlNode *object_new, const char *xml_file_new, - gboolean as_cib, gboolean no_version) +generate_patch(xmlNode *object_original, xmlNode *object_new, bool as_cib, + bool no_version) { const char *vfields[] = { PCMK_XA_ADMIN_EPOCH, @@ -215,7 +215,7 @@ generate_patch(xmlNode *object_original, xmlNode *object_new, const char *xml_fi pcmk__xml_doc_set_flags(object_new->doc, pcmk__xf_ignore_attr_pos); } pcmk__xml_mark_changes(object_original, object_new); - crm_log_xml_debug(object_new, (xml_file_new? xml_file_new: "target")); + crm_log_xml_debug(object_new, "target"); output = xml_create_patchset(0, object_original, object_new, NULL, FALSE); @@ -346,7 +346,8 @@ main(int argc, char **argv) if (options.apply) { rc = apply_patch(object_original, object_new, options.as_cib); } else { - rc = generate_patch(object_original, object_new, options.xml_file_new, options.as_cib, options.no_version); + rc = generate_patch(object_original, object_new, options.as_cib, + options.no_version); } exit_code = pcmk_rc2exitc(rc); From e02acdbe09566bf3d603a7b3c19ea91237180565 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 14:52:43 -0700 Subject: [PATCH 04/32] Refactor: tools: Overhaul input option storage in crm_diff.c It's simpler to use multiple strings than to have a boolean represent whether a file name should be treated as a raw string. When we can make the input precedence sane at a compatibility break, an enum for input source will likely make the most sense. Then we can bring back callbacks to set the enum value. Also rename "apply" to "patch" for clarity, and use G_OPTION_FLAG_NONE. Use G_OPTION_ARG_NONE where appropriate. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 81 +++++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 45 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index ceb913a65bf..6257b8086df 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -9,6 +9,7 @@ #include +#include // bool, true #include #include #include @@ -30,51 +31,36 @@ #define INDENT " " struct { - gboolean apply; + gchar *source_file; + gchar *target_file; + gchar *source_string; + gchar *target_string; + bool patch; gboolean as_cib; gboolean no_version; - gboolean raw_original; - gboolean raw_new; gboolean use_stdin; - char *xml_file_original; - char *xml_file_new; } options; -static gboolean -new_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, - GError **error) -{ - options.raw_new = TRUE; - pcmk__str_update(&options.xml_file_new, optarg); - return TRUE; -} - -static gboolean -original_string_cb(const gchar *option_name, const gchar *optarg, gpointer data, - GError **error) -{ - options.raw_original = TRUE; - pcmk__str_update(&options.xml_file_original, optarg); - return TRUE; -} - static gboolean patch_cb(const gchar *option_name, const gchar *optarg, gpointer data, GError **error) { - options.apply = TRUE; - pcmk__str_update(&options.xml_file_new, optarg); + options.patch = true; + g_free(options.target_file); + options.target_file = g_strdup(optarg); return TRUE; } // @COMPAT Use last-one-wins for original/new/patch input sources static GOptionEntry original_xml_entries[] = { - { "original", 'o', 0, G_OPTION_ARG_STRING, &options.xml_file_original, + { "original", 'o', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, + &options.source_file, "XML is contained in the named file. Currently --original-string and\n" INDENT "--stdin both override this. In a future release, the last one\n" INDENT "specified will be used.", "FILE" }, - { "original-string", 'O', 0, G_OPTION_ARG_CALLBACK, original_string_cb, + { "original-string", 'O', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, + &options.source_string, "XML is contained in the supplied string. Currently this takes\n" INDENT "precedence over both --stdin and --original. In a future\n" INDENT "release, the last one specified will be used.", @@ -84,17 +70,18 @@ static GOptionEntry original_xml_entries[] = { }; static GOptionEntry operation_entries[] = { - { "new", 'n', 0, G_OPTION_ARG_STRING, &options.xml_file_new, + { "new", 'n', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, &options.target_file, "Compare the original XML to the contents of the named file. Currently\n" INDENT "--new-string and --stdin both override this. In a future\n" INDENT "release, the last one specified will be used.", "FILE" }, - { "new-string", 'N', 0, G_OPTION_ARG_CALLBACK, new_string_cb, + { "new-string", 'N', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, + &options.target_string, "Compare the original XML with the contents of the supplied string.\n" INDENT "Currently this takes precedence over --stdin, --patch, and\n" INDENT "--new. In a future release, the last one specified will be used.", "STRING" }, - { "patch", 'p', 0, G_OPTION_ARG_CALLBACK, patch_cb, + { "patch", 'p', G_OPTION_FLAG_NONE, G_OPTION_ARG_CALLBACK, patch_cb, "Patch the original XML with the contents of the named file. Currently\n" INDENT "--new-string, --stdin, and (if specified later) --new override\n" INDENT "the input source specified here. In a future release, the last\n" @@ -114,16 +101,17 @@ static GOptionEntry addl_entries[] = { * we don't update the target versions in the patchset, and we don't drop * the versions from the patchset unless --no-version is given. */ - { "cib", 'c', 0, G_OPTION_ARG_NONE, &options.as_cib, + { "cib", 'c', G_OPTION_FLAG_NONE, G_OPTION_ARG_NONE, &options.as_cib, "Compare/patch the inputs as a CIB (includes version details)", NULL }, - { "stdin", 's', 0, G_OPTION_ARG_NONE, &options.use_stdin, + { "stdin", 's', G_OPTION_FLAG_NONE, G_OPTION_ARG_NONE, &options.use_stdin, "Get the original XML and new (or patch) XML from stdin. Currently\n" INDENT "--original-string and --new-string override this for original\n" INDENT "and new/patch XML, respectively. In a future release, the last\n" INDENT "one specified will be used.", NULL }, - { "no-version", 'u', 0, G_OPTION_ARG_NONE, &options.no_version, + { "no-version", 'u', G_OPTION_FLAG_NONE, G_OPTION_ARG_NONE, + &options.no_version, "Generate the difference without version details", NULL }, @@ -302,7 +290,7 @@ main(int argc, char **argv) pcmk__cli_help('v'); } - if (options.apply && options.no_version) { + if (options.patch && options.no_version) { fprintf(stderr, "warning: -u/--no-version ignored with -p/--patch\n"); } else if (options.as_cib && options.no_version) { fprintf(stderr, "error: -u/--no-version incompatible with -c/--cib\n"); @@ -310,26 +298,26 @@ main(int argc, char **argv) goto done; } - if (options.raw_original) { - object_original = pcmk__xml_parse(options.xml_file_original); + if (options.source_string != NULL) { + object_original = pcmk__xml_parse(options.source_string); } else if (options.use_stdin) { fprintf(stderr, "Input first XML fragment:"); object_original = pcmk__xml_read(NULL); - } else if (options.xml_file_original != NULL) { - object_original = pcmk__xml_read(options.xml_file_original); + } else if (options.source_file != NULL) { + object_original = pcmk__xml_read(options.source_file); } - if (options.raw_new) { - object_new = pcmk__xml_parse(options.xml_file_new); + if (options.target_string != NULL) { + object_new = pcmk__xml_parse(options.target_string); } else if (options.use_stdin) { fprintf(stderr, "Input second XML fragment:"); object_new = pcmk__xml_read(NULL); - } else if (options.xml_file_new != NULL) { - object_new = pcmk__xml_read(options.xml_file_new); + } else if (options.target_file != NULL) { + object_new = pcmk__xml_read(options.target_file); } if (object_original == NULL) { @@ -343,7 +331,7 @@ main(int argc, char **argv) goto done; } - if (options.apply) { + if (options.patch) { rc = apply_patch(object_original, object_new, options.as_cib); } else { rc = generate_patch(object_original, object_new, options.as_cib, @@ -354,8 +342,11 @@ main(int argc, char **argv) done: g_strfreev(processed_args); pcmk__free_arg_context(context); - free(options.xml_file_original); - free(options.xml_file_new); + + g_free(options.source_file); + g_free(options.target_file); + g_free(options.source_string); + g_free(options.target_string); pcmk__xml_free(object_original); pcmk__xml_free(object_new); From aa0f968c9856e857577cf1d663222cfa86acaa95 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 14:59:48 -0700 Subject: [PATCH 05/32] Refactor: tools: Use source/target consistently in crm_diff.c Signed-off-by: Reid Wahl --- tools/crm_diff.c | 48 +++++++++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 6257b8086df..1e25e998a96 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -178,8 +178,7 @@ log_patch_cib_versions(xmlNode *patch) // \return Standard Pacemaker return code static int -generate_patch(xmlNode *object_original, xmlNode *object_new, bool as_cib, - bool no_version) +generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) { const char *vfields[] = { PCMK_XA_ADMIN_EPOCH, @@ -195,27 +194,27 @@ generate_patch(xmlNode *object_original, xmlNode *object_new, bool as_cib, int lpc; for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - crm_copy_xml_element(object_original, object_new, vfields[lpc]); + crm_copy_xml_element(source, target, vfields[lpc]); } } if (as_cib) { - pcmk__xml_doc_set_flags(object_new->doc, pcmk__xf_ignore_attr_pos); + pcmk__xml_doc_set_flags(target->doc, pcmk__xf_ignore_attr_pos); } - pcmk__xml_mark_changes(object_original, object_new); - crm_log_xml_debug(object_new, "target"); + pcmk__xml_mark_changes(source, target); + crm_log_xml_debug(target, "target"); - output = xml_create_patchset(0, object_original, object_new, NULL, FALSE); + output = xml_create_patchset(0, source, target, NULL, FALSE); - pcmk__log_xml_changes(LOG_INFO, object_new); - pcmk__xml_commit_changes(object_new->doc); + pcmk__log_xml_changes(LOG_INFO, target); + pcmk__xml_commit_changes(target->doc); if (output == NULL) { return pcmk_rc_ok; // No changes } if (as_cib) { - pcmk__xml_patchset_add_digest(output, object_new); + pcmk__xml_patchset_add_digest(output, target); log_patch_cib_versions(output); } else if (no_version) { @@ -264,8 +263,8 @@ build_arg_context(pcmk__common_args_t *args) { int main(int argc, char **argv) { - xmlNode *object_original = NULL; - xmlNode *object_new = NULL; + xmlNode *source = NULL; + xmlNode *target = NULL; crm_exit_t exit_code = CRM_EX_OK; GError *error = NULL; @@ -299,43 +298,42 @@ main(int argc, char **argv) } if (options.source_string != NULL) { - object_original = pcmk__xml_parse(options.source_string); + source = pcmk__xml_parse(options.source_string); } else if (options.use_stdin) { fprintf(stderr, "Input first XML fragment:"); - object_original = pcmk__xml_read(NULL); + source = pcmk__xml_read(NULL); } else if (options.source_file != NULL) { - object_original = pcmk__xml_read(options.source_file); + source = pcmk__xml_read(options.source_file); } if (options.target_string != NULL) { - object_new = pcmk__xml_parse(options.target_string); + target = pcmk__xml_parse(options.target_string); } else if (options.use_stdin) { fprintf(stderr, "Input second XML fragment:"); - object_new = pcmk__xml_read(NULL); + target = pcmk__xml_read(NULL); } else if (options.target_file != NULL) { - object_new = pcmk__xml_read(options.target_file); + target = pcmk__xml_read(options.target_file); } - if (object_original == NULL) { + if (source == NULL) { fprintf(stderr, "Could not parse the first XML fragment\n"); exit_code = CRM_EX_DATAERR; goto done; } - if (object_new == NULL) { + if (target == NULL) { fprintf(stderr, "Could not parse the second XML fragment\n"); exit_code = CRM_EX_DATAERR; goto done; } if (options.patch) { - rc = apply_patch(object_original, object_new, options.as_cib); + rc = apply_patch(source, target, options.as_cib); } else { - rc = generate_patch(object_original, object_new, options.as_cib, - options.no_version); + rc = generate_patch(source, target, options.as_cib, options.no_version); } exit_code = pcmk_rc2exitc(rc); @@ -347,8 +345,8 @@ main(int argc, char **argv) g_free(options.target_file); g_free(options.source_string); g_free(options.target_string); - pcmk__xml_free(object_original); - pcmk__xml_free(object_new); + pcmk__xml_free(source); + pcmk__xml_free(target); pcmk__output_and_clear_error(&error, NULL); crm_exit(exit_code); From 8d9aa67e26663af38448a2000a860eb8ec85c70c Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:07:17 -0700 Subject: [PATCH 06/32] Refactor: various: Drop crm_copy_xml_element() internally It doesn't copy an element, so I'd like to deprecate it. It's useful, but only slightly, and we only call it four times. Signed-off-by: Reid Wahl --- daemons/controld/controld_te_actions.c | 11 ++++++----- tools/crm_diff.c | 3 ++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/daemons/controld/controld_te_actions.c b/daemons/controld/controld_te_actions.c index 1df83e82b08..fba2463d1e0 100644 --- a/daemons/controld/controld_te_actions.c +++ b/daemons/controld/controld_te_actions.c @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -299,10 +299,11 @@ controld_record_action_event(pcmk__graph_action_t *action, rsc = pcmk__xe_create(rsc, PCMK__XE_LRM_RESOURCE); crm_xml_add(rsc, PCMK_XA_ID, rsc_id); - - crm_copy_xml_element(action_rsc, rsc, PCMK_XA_TYPE); - crm_copy_xml_element(action_rsc, rsc, PCMK_XA_CLASS); - crm_copy_xml_element(action_rsc, rsc, PCMK_XA_PROVIDER); + crm_xml_add(rsc, PCMK_XA_TYPE, crm_element_value(action_rsc, PCMK_XA_TYPE)); + crm_xml_add(rsc, PCMK_XA_CLASS, + crm_element_value(action_rsc, PCMK_XA_CLASS)); + crm_xml_add(rsc, PCMK_XA_PROVIDER, + crm_element_value(action_rsc, PCMK_XA_PROVIDER)); pcmk__create_history_xml(rsc, op, CRM_FEATURE_SET, target_rc, target, __func__); diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 1e25e998a96..062e0390749 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -194,7 +194,8 @@ generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) int lpc; for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - crm_copy_xml_element(source, target, vfields[lpc]); + crm_xml_add(target, vfields[lpc], + crm_element_value(source, vfields[lpc])); } } From 1fbf269eae0f094ae6882cdc4ea3c4e01b96882e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:10:47 -0700 Subject: [PATCH 07/32] API: libcrmcommon: Deprecate crm_copy_xml_element() We also de-inline it to prevent test-headers from complaining about undefined reference to crm_element_value(). I'd like to avoid circular includes in xml_element_compat.h, and this way seems better than re-declaring crm_element_value(). Signed-off-by: Reid Wahl --- include/crm/common/xml_element.h | 20 +------------------- include/crm/common/xml_element_compat.h | 6 +++++- lib/common/xml_element.c | 9 +++++++++ 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/include/crm/common/xml_element.h b/include/crm/common/xml_element.h index c49603b5454..7487cce1bf8 100644 --- a/include/crm/common/xml_element.h +++ b/include/crm/common/xml_element.h @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -42,24 +42,6 @@ int crm_element_value_timeval(const xmlNode *data, const char *name_sec, const char *name_usec, struct timeval *dest); char *crm_element_value_copy(const xmlNode *data, const char *name); -/*! - * \brief Copy an element from one XML object to another - * - * \param[in] obj1 Source XML - * \param[in,out] obj2 Destination XML - * \param[in] element Name of element to copy - * - * \return Pointer to copied value (from source) - */ -static inline const char * -crm_copy_xml_element(const xmlNode *obj1, xmlNode *obj2, const char *element) -{ - const char *value = crm_element_value(obj1, element); - - crm_xml_add(obj2, element, value); - return value; -} - #ifdef __cplusplus } #endif diff --git a/include/crm/common/xml_element_compat.h b/include/crm/common/xml_element_compat.h index 7c6e1f1c841..50e677afbcb 100644 --- a/include/crm/common/xml_element_compat.h +++ b/include/crm/common/xml_element_compat.h @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -35,6 +35,10 @@ void crm_xml_set_id(xmlNode *xml, const char *format, ...) G_GNUC_PRINTF(2, 3); //! \deprecated Do not use xmlNode *sorted_xml(xmlNode *input, xmlNode *parent, gboolean recursive); +//! \deprecated Do not use +const char *crm_copy_xml_element(const xmlNode *obj1, xmlNode *obj2, + const char *element); + #ifdef __cplusplus } #endif diff --git a/lib/common/xml_element.c b/lib/common/xml_element.c index 3b97e0f37c2..1875efd50ab 100644 --- a/lib/common/xml_element.c +++ b/lib/common/xml_element.c @@ -1599,5 +1599,14 @@ sorted_xml(xmlNode *input, xmlNode *parent, gboolean recursive) return result; } +const char * +crm_copy_xml_element(const xmlNode *obj1, xmlNode *obj2, const char *element) +{ + const char *value = crm_element_value(obj1, element); + + crm_xml_add(obj2, element, value); + return value; +} + // LCOV_EXCL_STOP // End deprecated API From 3591f401246b85c2c43df1c57f7689473510abc7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:18:38 -0700 Subject: [PATCH 08/32] Refactor: tools: Best practices in crm_diff.c:generate_patch() Not much to do here. Mainly doxygen, renaming output to patchset, a sanity check for future-proofing. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 52 +++++++++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 062e0390749..89058d382df 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -176,26 +176,42 @@ log_patch_cib_versions(xmlNode *patch) } } -// \return Standard Pacemaker return code +/*! + * \internal + * \brief Create an XML patchset from the given source and target XML trees + * + * \param[in,out] source Source XML + * \param[in,out] target Target XML + * \param[in] as_cib If \c true, treat the XML trees as CIBs. In + * particular, ignore attribute position changes, + * include the target digest in the patchset, and log + * the source and target CIB versions. + * \param[in] no_version If \c true, ignore changes to the CIB version + * (must be \c false if \p as_cib is \c true) + * + * \return Standard Pacemaker return code + */ static int generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) { - const char *vfields[] = { + static const char *const vfields[] = { PCMK_XA_ADMIN_EPOCH, PCMK_XA_EPOCH, PCMK_XA_NUM_UPDATES, }; - xmlNode *output = NULL; + xmlNode *patchset = NULL; - /* If we're ignoring the version, make the version information - * identical, so it isn't detected as a change. */ - if (no_version) { - int lpc; + // Currently impossible; just a reminder for when we move to libpacemaker + pcmk__assert(!as_cib || !no_version); - for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - crm_xml_add(target, vfields[lpc], - crm_element_value(source, vfields[lpc])); + /* If we're ignoring the version, make the version information identical, so + * it isn't detected as a change. + */ + if (no_version) { + for (int i = 0; i < PCMK__NELEM(vfields); i++) { + crm_xml_add(target, vfields[i], + crm_element_value(source, vfields[i])); } } @@ -205,27 +221,27 @@ generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) pcmk__xml_mark_changes(source, target); crm_log_xml_debug(target, "target"); - output = xml_create_patchset(0, source, target, NULL, FALSE); + patchset = xml_create_patchset(0, source, target, NULL, false); pcmk__log_xml_changes(LOG_INFO, target); pcmk__xml_commit_changes(target->doc); - if (output == NULL) { + if (patchset == NULL) { return pcmk_rc_ok; // No changes } if (as_cib) { - pcmk__xml_patchset_add_digest(output, target); - log_patch_cib_versions(output); + pcmk__xml_patchset_add_digest(patchset, target); + log_patch_cib_versions(patchset); } else if (no_version) { - pcmk__xml_free(pcmk__xe_first_child(output, PCMK_XE_VERSION, NULL, + pcmk__xml_free(pcmk__xe_first_child(patchset, PCMK_XE_VERSION, NULL, NULL)); } - pcmk__log_xml_patchset(LOG_NOTICE, output); - print_patch(output); - pcmk__xml_free(output); + pcmk__log_xml_patchset(LOG_NOTICE, patchset); + print_patch(patchset); + pcmk__xml_free(patchset); /* pcmk_rc_error means there's a non-empty diff. * @COMPAT Choose a more descriptive return code, like one that maps to From 8f208c7616a0736756e31b054f4c73bfb1dd54da Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:21:01 -0700 Subject: [PATCH 09/32] Refactor: libcrmcommon: NULL-check XML private data in display functions Sanity Signed-off-by: Reid Wahl --- lib/common/xml_display.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/common/xml_display.c b/lib/common/xml_display.c index d886d34c4bb..d6865f7474a 100644 --- a/lib/common/xml_display.c +++ b/lib/common/xml_display.c @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -111,7 +111,8 @@ show_xml_element(pcmk__output_t *out, GString *buffer, const char *prefix, const char *p_value = pcmk__xml_attr_value(attr); gchar *p_copy = NULL; - if (pcmk_is_set(nodepriv->flags, pcmk__xf_deleted)) { + if ((nodepriv == NULL) + || pcmk_is_set(nodepriv->flags, pcmk__xf_deleted)) { continue; } @@ -265,6 +266,10 @@ show_xml_changes_recursive(pcmk__output_t *out, const xmlNode *data, int depth, int rc = pcmk_rc_no_output; int temp_rc = pcmk_rc_no_output; + if (nodepriv == NULL) { + return pcmk_rc_no_output; + } + if (pcmk_all_flags_set(nodepriv->flags, pcmk__xf_dirty|pcmk__xf_created)) { // Newly created return pcmk__xml_show(out, PCMK__XML_PREFIX_CREATED, data, depth, From c2fe4c3f1f2d1436041ec8086e8542a9ed4971ad Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:22:52 -0700 Subject: [PATCH 10/32] API: libcrmcommon: xml_apply_patchset() patchset argument is now const Signed-off-by: Reid Wahl --- include/crm/common/xml.h | 3 ++- lib/common/patchset.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/crm/common/xml.h b/include/crm/common/xml.h index a4cdcde69d9..b5a5b08f0e5 100644 --- a/include/crm/common/xml.h +++ b/include/crm/common/xml.h @@ -35,7 +35,8 @@ extern "C" { xmlNode *xml_create_patchset( int format, xmlNode *source, xmlNode *target, bool *config, bool manage_version); -int xml_apply_patchset(xmlNode *xml, xmlNode *patchset, bool check_version); +int xml_apply_patchset(xmlNode *xml, const xmlNode *patchset, + bool check_version); #ifdef __cplusplus } diff --git a/lib/common/patchset.c b/lib/common/patchset.c index 3ed81f0e3af..bdc1e51d5ca 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -825,7 +825,7 @@ apply_v2_patchset(xmlNode *xml, const xmlNode *patchset) } int -xml_apply_patchset(xmlNode *xml, xmlNode *patchset, bool check_version) +xml_apply_patchset(xmlNode *xml, const xmlNode *patchset, bool check_version) { int format = 1; int rc = pcmk_ok; From 83460fe30845da5c3bba85630949a91b62f53db0 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:26:37 -0700 Subject: [PATCH 11/32] Refactor: libcrmcommon: Make vfields file-scope in patchset.c Reduce some duplication Signed-off-by: Reid Wahl --- lib/common/patchset.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/lib/common/patchset.c b/lib/common/patchset.c index bdc1e51d5ca..595ca72ab34 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -26,6 +26,12 @@ #include // CRM_XML_LOG_BASE, etc. #include "crmcommon_private.h" +static const char *const vfields[] = { + PCMK_XA_ADMIN_EPOCH, + PCMK_XA_EPOCH, + PCMK_XA_NUM_UPDATES, +}; + /* Add changes for specified XML to patchset. * For patchset format, refer to diff schema. */ @@ -185,11 +191,6 @@ xml_create_patchset_v2(xmlNode *source, xmlNode *target) xmlNode *v = NULL; xmlNode *version = NULL; xmlNode *patchset = NULL; - const char *vfields[] = { - PCMK_XA_ADMIN_EPOCH, - PCMK_XA_EPOCH, - PCMK_XA_NUM_UPDATES, - }; pcmk__assert(target != NULL); @@ -335,12 +336,6 @@ int pcmk__xml_patchset_versions(const xmlNode *patchset, int source[3], int target[3]) { - static const char *const vfields[] = { - PCMK_XA_ADMIN_EPOCH, - PCMK_XA_EPOCH, - PCMK_XA_NUM_UPDATES, - }; - int format = 0; const xmlNode *version = NULL; const xmlNode *source_xml = NULL; @@ -429,12 +424,6 @@ xml_patch_version_check(const xmlNode *xml, const xmlNode *patchset) int del[] = { 0, 0, 0 }; int rc = pcmk_rc_ok; - const char *vfields[] = { - PCMK_XA_ADMIN_EPOCH, - PCMK_XA_EPOCH, - PCMK_XA_NUM_UPDATES, - }; - for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { crm_element_value_int(xml, vfields[lpc], &(this[lpc])); crm_trace("Got %d for this[%s]", this[lpc], vfields[lpc]); @@ -968,12 +957,6 @@ pcmk__cib_element_in_patchset(const xmlNode *patchset, const char *element) bool xml_patch_versions(const xmlNode *patchset, int add[3], int del[3]) { - static const char *const vfields[] = { - PCMK_XA_ADMIN_EPOCH, - PCMK_XA_EPOCH, - PCMK_XA_NUM_UPDATES, - }; - const xmlNode *version = pcmk__xe_first_child(patchset, PCMK_XE_VERSION, NULL, NULL); const xmlNode *source = pcmk__xe_first_child(version, PCMK_XE_SOURCE, NULL, From 1439e9f9e181e98e7bd7de5fcf28926b74b97fa0 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:31:02 -0700 Subject: [PATCH 12/32] Refactor: libcrmcommon: Some best practices in xml_patch_version_check() Rename to check_patchset_versions() and rename arrays for clarity Signed-off-by: Reid Wahl --- lib/common/patchset.c | 51 ++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/lib/common/patchset.c b/lib/common/patchset.c index 595ca72ab34..f310b934e06 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -414,67 +414,72 @@ pcmk__xml_patchset_versions(const xmlNode *patchset, int source[3], * \return Standard Pacemaker return code */ static int -xml_patch_version_check(const xmlNode *xml, const xmlNode *patchset) +check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) { int lpc = 0; bool changed = FALSE; - int this[] = { 0, 0, 0 }; - int add[] = { 0, 0, 0 }; - int del[] = { 0, 0, 0 }; + int current[] = { 0, 0, 0 }; + int source[] = { 0, 0, 0 }; + int target[] = { 0, 0, 0 }; int rc = pcmk_rc_ok; for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - crm_element_value_int(xml, vfields[lpc], &(this[lpc])); - crm_trace("Got %d for this[%s]", this[lpc], vfields[lpc]); - if (this[lpc] < 0) { - this[lpc] = 0; + crm_element_value_int(xml, vfields[lpc], &(current[lpc])); + crm_trace("Got %d for current[%s]", current[lpc], vfields[lpc]); + if (current[lpc] < 0) { + current[lpc] = 0; } } /* Set some defaults in case nothing is present */ - add[0] = this[0]; - add[1] = this[1]; - add[2] = this[2] + 1; + target[0] = current[0]; + target[1] = current[1]; + target[2] = current[2] + 1; for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - del[lpc] = this[lpc]; + source[lpc] = current[lpc]; } - rc = pcmk__xml_patchset_versions(patchset, del, add); + rc = pcmk__xml_patchset_versions(patchset, source, target); if (rc != pcmk_rc_ok) { return rc; } for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - if (this[lpc] < del[lpc]) { + if (current[lpc] < source[lpc]) { crm_debug("Current %s is too low (%d.%d.%d < %d.%d.%d --> %d.%d.%d)", - vfields[lpc], this[0], this[1], this[2], - del[0], del[1], del[2], add[0], add[1], add[2]); + vfields[lpc], + current[0], current[1], current[2], + source[0], source[1], source[2], + target[0], target[1], target[2]); return pcmk_rc_diff_resync; - } else if (this[lpc] > del[lpc]) { + } else if (current[lpc] > source[lpc]) { crm_info("Current %s is too high (%d.%d.%d > %d.%d.%d --> %d.%d.%d) %p", - vfields[lpc], this[0], this[1], this[2], - del[0], del[1], del[2], add[0], add[1], add[2], patchset); + vfields[lpc], + current[0], current[1], current[2], + source[0], source[1], source[2], + target[0], target[1], target[2], patchset); crm_log_xml_info(patchset, "OldPatch"); return pcmk_rc_old_data; } } for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - if (add[lpc] > del[lpc]) { + if (target[lpc] > source[lpc]) { changed = TRUE; } } if (!changed) { crm_notice("Versions did not change in patch %d.%d.%d", - add[0], add[1], add[2]); + target[0], target[1], target[2]); return pcmk_rc_old_data; } crm_debug("Can apply patch %d.%d.%d to %d.%d.%d", - add[0], add[1], add[2], this[0], this[1], this[2]); + target[0], target[1], target[2], + current[0], current[1], current[2]); return pcmk_rc_ok; } @@ -828,7 +833,7 @@ xml_apply_patchset(xmlNode *xml, const xmlNode *patchset, bool check_version) pcmk__log_xml_patchset(LOG_TRACE, patchset); if (check_version) { - rc = pcmk_rc2legacy(xml_patch_version_check(xml, patchset)); + rc = pcmk_rc2legacy(check_patchset_versions(xml, patchset)); if (rc != pcmk_ok) { return rc; } From 3d0633e1404e6e071bc539f80382de25e79c86b3 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 15:33:12 -0700 Subject: [PATCH 13/32] Refactor: libcrmcommon: Rename lpc to i in xml_patch_version_check() And limit it to loop-scope. Signed-off-by: Reid Wahl --- lib/common/patchset.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/lib/common/patchset.c b/lib/common/patchset.c index f310b934e06..60c4859c636 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -416,7 +416,6 @@ pcmk__xml_patchset_versions(const xmlNode *patchset, int source[3], static int check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) { - int lpc = 0; bool changed = FALSE; int current[] = { 0, 0, 0 }; @@ -424,11 +423,11 @@ check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) int target[] = { 0, 0, 0 }; int rc = pcmk_rc_ok; - for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - crm_element_value_int(xml, vfields[lpc], &(current[lpc])); - crm_trace("Got %d for current[%s]", current[lpc], vfields[lpc]); - if (current[lpc] < 0) { - current[lpc] = 0; + for (int i = 0; i < PCMK__NELEM(vfields); i++) { + crm_element_value_int(xml, vfields[i], &(current[i])); + crm_trace("Got %d for current[%s]", current[i], vfields[i]); + if (current[i] < 0) { + current[i] = 0; } } @@ -436,8 +435,8 @@ check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) target[0] = current[0]; target[1] = current[1]; target[2] = current[2] + 1; - for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - source[lpc] = current[lpc]; + for (int i = 0; i < PCMK__NELEM(vfields); i++) { + source[i] = current[i]; } rc = pcmk__xml_patchset_versions(patchset, source, target); @@ -445,18 +444,18 @@ check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) return rc; } - for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - if (current[lpc] < source[lpc]) { + for (int i = 0; i < PCMK__NELEM(vfields); i++) { + if (current[i] < source[i]) { crm_debug("Current %s is too low (%d.%d.%d < %d.%d.%d --> %d.%d.%d)", - vfields[lpc], + vfields[i], current[0], current[1], current[2], source[0], source[1], source[2], target[0], target[1], target[2]); return pcmk_rc_diff_resync; - } else if (current[lpc] > source[lpc]) { + } else if (current[i] > source[i]) { crm_info("Current %s is too high (%d.%d.%d > %d.%d.%d --> %d.%d.%d) %p", - vfields[lpc], + vfields[i], current[0], current[1], current[2], source[0], source[1], source[2], target[0], target[1], target[2], patchset); @@ -465,8 +464,8 @@ check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) } } - for (lpc = 0; lpc < PCMK__NELEM(vfields); lpc++) { - if (target[lpc] > source[lpc]) { + for (int i = 0; i < PCMK__NELEM(vfields); i++) { + if (target[i] > source[i]) { changed = TRUE; } } From ce9842fb75a7bc890ec0930850f1136b19f145bf Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 20:38:33 -0700 Subject: [PATCH 14/32] Refactor: libcrmcommon: Clarify check_patchset_versions() Do not change behavior yet, as it will affect xml_apply_patchset(). Signed-off-by: Reid Wahl --- lib/common/patchset.c | 68 ++++++++++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/lib/common/patchset.c b/lib/common/patchset.c index 60c4859c636..81c6521bb51 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -408,30 +408,46 @@ pcmk__xml_patchset_versions(const xmlNode *patchset, int source[3], * \internal * \brief Check whether patchset can be applied to current CIB * - * \param[in] xml Root of current CIB + * \param[in] cib_root Root of current CIB * \param[in] patchset Patchset to check * * \return Standard Pacemaker return code */ static int -check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) +check_patchset_versions(const xmlNode *cib_root, const xmlNode *patchset) { - bool changed = FALSE; - int current[] = { 0, 0, 0 }; int source[] = { 0, 0, 0 }; int target[] = { 0, 0, 0 }; int rc = pcmk_rc_ok; for (int i = 0; i < PCMK__NELEM(vfields); i++) { - crm_element_value_int(xml, vfields[i], &(current[i])); - crm_trace("Got %d for current[%s]", current[i], vfields[i]); + /* @COMPAT We should probably fail with EINVAL for negative or invalid. + * + * Preserve behavior for xml_apply_patchset(). Use new behavior in a + * future replacement. + */ + if (crm_element_value_int(cib_root, vfields[i], &(current[i])) == 0) { + crm_trace("Got %d for current[%s]%s", + current[i], vfields[i], + ((current[i] < 0)? ", using 0" : "")); + } else { + crm_debug("Failed to get value for current[%s], using 0", + vfields[i]); + } if (current[i] < 0) { current[i] = 0; } } - /* Set some defaults in case nothing is present */ + /* Set some defaults in case nothing is present. + * + * @COMPAT We should probably skip this step, and fail immediately below if + * target[i] < source[i]. + * + * Preserve behavior for xml_apply_patchset(). Use new behavior in a future + * replacement. + */ target[0] = current[0]; target[1] = current[1]; target[2] = current[2] + 1; @@ -444,42 +460,40 @@ check_patchset_versions(const xmlNode *xml, const xmlNode *patchset) return rc; } + // Ensure current version matches patchset source version for (int i = 0; i < PCMK__NELEM(vfields); i++) { if (current[i] < source[i]) { - crm_debug("Current %s is too low (%d.%d.%d < %d.%d.%d --> %d.%d.%d)", - vfields[i], - current[0], current[1], current[2], + crm_debug("Current %s is too low " + "(%d.%d.%d < %d.%d.%d --> %d.%d.%d)", + vfields[i], current[0], current[1], current[2], source[0], source[1], source[2], target[0], target[1], target[2]); return pcmk_rc_diff_resync; - - } else if (current[i] > source[i]) { - crm_info("Current %s is too high (%d.%d.%d > %d.%d.%d --> %d.%d.%d) %p", - vfields[i], - current[0], current[1], current[2], + } + if (current[i] > source[i]) { + crm_info("Current %s is too high " + "(%d.%d.%d > %d.%d.%d --> %d.%d.%d)", + vfields[i], current[0], current[1], current[2], source[0], source[1], source[2], - target[0], target[1], target[2], patchset); + target[0], target[1], target[2]); crm_log_xml_info(patchset, "OldPatch"); return pcmk_rc_old_data; } } + // Ensure target version is newer than source version for (int i = 0; i < PCMK__NELEM(vfields); i++) { if (target[i] > source[i]) { - changed = TRUE; + crm_debug("Can apply patch %d.%d.%d to %d.%d.%d", + target[0], target[1], target[2], + current[0], current[1], current[2]); + return pcmk_rc_ok; } } - if (!changed) { - crm_notice("Versions did not change in patch %d.%d.%d", - target[0], target[1], target[2]); - return pcmk_rc_old_data; - } - - crm_debug("Can apply patch %d.%d.%d to %d.%d.%d", - target[0], target[1], target[2], - current[0], current[1], current[2]); - return pcmk_rc_ok; + crm_notice("Versions did not change in patch %d.%d.%d", + target[0], target[1], target[2]); + return pcmk_rc_old_data; } // Return first child matching element name and optionally id or position From db7f615ac59384b3f2f15f68cc52a300b42b4b63 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Tue, 24 Jun 2025 23:24:32 -0700 Subject: [PATCH 15/32] Refactor: libcrmcommon: Null-check digest in pcmk__digest_xml() Signed-off-by: Reid Wahl --- lib/common/digest.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/common/digest.c b/lib/common/digest.c index 3479f8a4258..b9213bd5608 100644 --- a/lib/common/digest.c +++ b/lib/common/digest.c @@ -176,6 +176,9 @@ pcmk__digest_xml(const xmlNode *xml, bool filter) pcmk__xml_string(xml, (filter? pcmk__xml_fmt_filtered : 0), buf, 0); digest = crm_md5sum(buf->str); + if (digest == NULL) { + goto done; + } pcmk__if_tracing( { @@ -192,6 +195,8 @@ pcmk__digest_xml(const xmlNode *xml, bool filter) }, {} ); + +done: g_string_free(buf, TRUE); return digest; } From 283a4c1bfa5bbf81c50026cc13fec6ea78f9eb2b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:00:33 -0700 Subject: [PATCH 16/32] Refactor: tools: Clean up crm_diff.c:apply_patch() We don't do anything with input afterward, and this is static for a CLI tool, so no need to make a copy. Don't calculate a digest. It's only for tracing, and it's unclear what we would use it for. The commit that added it (2cff40d2) doesn't give any explanation. Don't bother adding doxygen. We're about to drop this function. I'm doing it in two steps to clarify what it's doing and that it's no longer worth functionizing. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 89058d382df..3b9d4ef7deb 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -130,30 +130,18 @@ print_patch(xmlNode *patch) fflush(stdout); } -// \return Standard Pacemaker return code static int -apply_patch(xmlNode *input, xmlNode *patch, gboolean as_cib) +apply_patchset(xmlNode *input, const xmlNode *patch, bool check_version) { - xmlNode *output = pcmk__xml_copy(NULL, input); - int rc = xml_apply_patchset(output, patch, as_cib); + int rc = xml_apply_patchset(input, patch, check_version); rc = pcmk_legacy2rc(rc); if (rc != pcmk_rc_ok) { fprintf(stderr, "Could not apply patch: %s\n", pcmk_rc_str(rc)); - pcmk__xml_free(output); return rc; } - if (output != NULL) { - char *buffer; - - print_patch(output); - - buffer = pcmk__digest_xml(output, true); - crm_trace("Digest: %s", pcmk__s(buffer, "\n")); - free(buffer); - pcmk__xml_free(output); - } + print_patch(input); return pcmk_rc_ok; } @@ -348,7 +336,7 @@ main(int argc, char **argv) } if (options.patch) { - rc = apply_patch(source, target, options.as_cib); + rc = apply_patchset(source, target, options.as_cib); } else { rc = generate_patch(source, target, options.as_cib, options.no_version); } From 40b4acbd237b4054a46e854832c541f3b6e4a65a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:04:38 -0700 Subject: [PATCH 17/32] Refactor: tools: Drop crm_diff.c:apply_patchset() It's not really doing anything besides calling xml_apply_patchset() at this point. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 3b9d4ef7deb..11fc07325e1 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -130,21 +130,6 @@ print_patch(xmlNode *patch) fflush(stdout); } -static int -apply_patchset(xmlNode *input, const xmlNode *patch, bool check_version) -{ - int rc = xml_apply_patchset(input, patch, check_version); - - rc = pcmk_legacy2rc(rc); - if (rc != pcmk_rc_ok) { - fprintf(stderr, "Could not apply patch: %s\n", pcmk_rc_str(rc)); - return rc; - } - - print_patch(input); - return pcmk_rc_ok; -} - static void log_patch_cib_versions(xmlNode *patch) { @@ -336,7 +321,14 @@ main(int argc, char **argv) } if (options.patch) { - rc = apply_patchset(source, target, options.as_cib); + rc = xml_apply_patchset(source, target, options.as_cib); + rc = pcmk_legacy2rc(rc); + if (rc != pcmk_rc_ok) { + fprintf(stderr, "Could not apply patch: %s\n", pcmk_rc_str(rc)); + } else { + print_patch(source); + } + } else { rc = generate_patch(source, target, options.as_cib, options.no_version); } From 2f46983cd5ba0394d605f5455a655d7b4bd7a35b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:12:06 -0700 Subject: [PATCH 18/32] Refactor: tools: Drop log_patchset_cib_versions() pcmk__log_xml_patchset() already logs these, almost identically, at notice level. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 11fc07325e1..3a57308d384 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -130,25 +130,6 @@ print_patch(xmlNode *patch) fflush(stdout); } -static void -log_patch_cib_versions(xmlNode *patch) -{ - int add[] = { 0, 0, 0 }; - int del[] = { 0, 0, 0 }; - - const char *fmt = NULL; - const char *digest = NULL; - - pcmk__xml_patchset_versions(patch, del, add); - fmt = crm_element_value(patch, PCMK_XA_FORMAT); - digest = crm_element_value(patch, PCMK__XA_DIGEST); - - if (add[2] != del[2] || add[1] != del[1] || add[0] != del[0]) { - crm_info("Patch: --- %d.%d.%d %s", del[0], del[1], del[2], fmt); - crm_info("Patch: +++ %d.%d.%d %s", add[0], add[1], add[2], digest); - } -} - /*! * \internal * \brief Create an XML patchset from the given source and target XML trees @@ -205,7 +186,6 @@ generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) if (as_cib) { pcmk__xml_patchset_add_digest(patchset, target); - log_patch_cib_versions(patchset); } else if (no_version) { pcmk__xml_free(pcmk__xe_first_child(patchset, PCMK_XE_VERSION, NULL, From 2ebf936f58e1e447efce1bffc123e52bcbb86f6b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:20:14 -0700 Subject: [PATCH 19/32] Doc: tools: Doxygen for crm_diff.c:print_patch() Not really necessary but easy. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 3a57308d384..3f69da3b623 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -118,12 +118,24 @@ static GOptionEntry addl_entries[] = { { NULL } }; +/*! + * \internal + * \brief Print an XML tree serialized to text + * + * \param[in] xml XML tree to print + * + * \todo Use \c pcmk__output_t with message functions and drop this. + * + * \note This is basically a simplified version of \c pcmk__xml_write_fd(), but + * that function closes the stream before returning. We could modify it in + * the future. But we don't want to close stdout. + */ static void -print_patch(xmlNode *patch) +print_xml(const xmlNode *xml) { GString *buffer = g_string_sized_new(1024); - pcmk__xml_string(patch, pcmk__xml_fmt_pretty, buffer, 0); + pcmk__xml_string(xml, pcmk__xml_fmt_pretty, buffer, 0); printf("%s", buffer->str); g_string_free(buffer, TRUE); @@ -193,7 +205,7 @@ generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) } pcmk__log_xml_patchset(LOG_NOTICE, patchset); - print_patch(patchset); + print_xml(patchset); pcmk__xml_free(patchset); /* pcmk_rc_error means there's a non-empty diff. @@ -306,7 +318,7 @@ main(int argc, char **argv) if (rc != pcmk_rc_ok) { fprintf(stderr, "Could not apply patch: %s\n", pcmk_rc_str(rc)); } else { - print_patch(source); + print_xml(source); } } else { From dfc1f8f131eedb11bd628c66375bfede800f68eb Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:30:31 -0700 Subject: [PATCH 20/32] Refactor: tools: Clean up crm_diff.c includes Signed-off-by: Reid Wahl --- include/crm_internal.h | 3 ++- tools/crm_diff.c | 23 ++++++++--------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/include/crm_internal.h b/include/crm_internal.h index 891f2e5b686..337d309b991 100644 --- a/include/crm_internal.h +++ b/include/crm_internal.h @@ -1,5 +1,5 @@ /* - * Copyright 2006-2024 the Pacemaker project contributors + * Copyright 2006-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -38,6 +38,7 @@ #include #include +#include #include #include #include diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 3f69da3b623..137435637ea 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -9,21 +9,14 @@ #include -#include // bool, true -#include -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include -#include +#include // bool, true +#include // NULL, printf(), fprintf(), etc. +#include // free() + +#include // GOption, etc. +#include // xmlNode + +#include // xml_{create,apply}_patchset() #define SUMMARY "Compare two Pacemaker configurations (in XML format) to " \ "produce a custom diff-like output, or apply such an output " \ From 85c772e967479a7424be4ee0a35cecb94c3894ad Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:36:38 -0700 Subject: [PATCH 21/32] Low: tools: Be stricter about crm_diff --cib/--no-version Previously, we allowed --cib and --no-version to be specified together as long as --patch was also specified. In that case, --no-version was ignored. Our option validation (or lack thereof) in crm_diff is a big mess. This is one small improvement that SHOULDN'T break anyone's workflow... if it does, they can fix it, or if it's something really insane, we can revert. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 137435637ea..ecda9d1e341 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -264,12 +264,17 @@ main(int argc, char **argv) pcmk__cli_help('v'); } - if (options.patch && options.no_version) { - fprintf(stderr, "warning: -u/--no-version ignored with -p/--patch\n"); - } else if (options.as_cib && options.no_version) { - fprintf(stderr, "error: -u/--no-version incompatible with -c/--cib\n"); - exit_code = CRM_EX_USAGE; - goto done; + if (options.no_version) { + if (options.as_cib) { + exit_code = CRM_EX_USAGE; + g_set_error(&error, PCMK__EXITC_ERROR, exit_code, + "-u/--no-version incompatible with -c/--cib"); + goto done; + } + if (options.patch) { + fprintf(stderr, + "Warning: -u/--no-version ignored with -p/--patch\n"); + } } if (options.source_string != NULL) { From f6781fcaf172596a7ce5240da9ec1100c26359d7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:43:50 -0700 Subject: [PATCH 22/32] Log: tools: Improve error handling in crm_diff.c Signed-off-by: Reid Wahl --- tools/crm_diff.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index ecda9d1e341..832dfe88dc9 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -286,6 +286,13 @@ main(int argc, char **argv) } else if (options.source_file != NULL) { source = pcmk__xml_read(options.source_file); + + } else { + exit_code = CRM_EX_USAGE; + g_set_error(&error, PCMK__EXITC_ERROR, exit_code, + "Either --original, --original-string, or --stdin must be " + "specified"); + goto done; } if (options.target_string != NULL) { @@ -297,16 +304,25 @@ main(int argc, char **argv) } else if (options.target_file != NULL) { target = pcmk__xml_read(options.target_file); + + } else { + exit_code = CRM_EX_USAGE; + g_set_error(&error, PCMK__EXITC_ERROR, exit_code, + "Either --new, --new-string, --patch, or --stdin must be " + "specified"); + goto done; } if (source == NULL) { - fprintf(stderr, "Could not parse the first XML fragment\n"); exit_code = CRM_EX_DATAERR; + g_set_error(&error, PCMK__EXITC_ERROR, exit_code, + "Failed to parse original XML"); goto done; } if (target == NULL) { - fprintf(stderr, "Could not parse the second XML fragment\n"); exit_code = CRM_EX_DATAERR; + g_set_error(&error, PCMK__EXITC_ERROR, exit_code, + "Failed to parse %s XML", (options.patch? "patch" : "new")); goto done; } @@ -314,7 +330,8 @@ main(int argc, char **argv) rc = xml_apply_patchset(source, target, options.as_cib); rc = pcmk_legacy2rc(rc); if (rc != pcmk_rc_ok) { - fprintf(stderr, "Could not apply patch: %s\n", pcmk_rc_str(rc)); + g_set_error(&error, PCMK__RC_ERROR, rc, + "Could not apply patch: %s", pcmk_rc_str(rc)); } else { print_xml(source); } From 3f06da9342b3223aba33ba2886ca88a1743bab36 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 21:48:38 -0700 Subject: [PATCH 23/32] Refactor: tools: Add pcmk__output_t scaffolding to crm_diff.c No pcmk__message_entry_t array or pcmk__register_messages() call, because it looks like ouput_xml() will be sufficient, with no need for message functions. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 832dfe88dc9..573d3e7db69 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -208,6 +208,14 @@ generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) return pcmk_rc_error; } +static const pcmk__supported_format_t formats[] = { + PCMK__SUPPORTED_FORMAT_NONE, + PCMK__SUPPORTED_FORMAT_TEXT, + PCMK__SUPPORTED_FORMAT_XML, + + { NULL, NULL, NULL } +}; + static GOptionContext * build_arg_context(pcmk__common_args_t *args) { GOptionContext *context = NULL; @@ -238,17 +246,22 @@ build_arg_context(pcmk__common_args_t *args) { int main(int argc, char **argv) { + crm_exit_t exit_code = CRM_EX_OK; + int rc = pcmk_rc_ok; + xmlNode *source = NULL; xmlNode *target = NULL; - crm_exit_t exit_code = CRM_EX_OK; + pcmk__output_t *out = NULL; + GError *error = NULL; + GOptionGroup *output_group = NULL; pcmk__common_args_t *args = pcmk__new_common_args(SUMMARY); gchar **processed_args = pcmk__cmdline_preproc(argv, "nopNO"); GOptionContext *context = build_arg_context(args); - int rc = pcmk_rc_ok; + pcmk__register_formats(output_group, formats); if (!g_option_context_parse_strv(context, &processed_args, &error)) { exit_code = CRM_EX_USAGE; @@ -257,11 +270,18 @@ main(int argc, char **argv) pcmk__cli_init_logging("crm_diff", args->verbosity); + rc = pcmk__output_new(&out, args->output_ty, args->output_dest, argv); + if (rc != pcmk_rc_ok) { + exit_code = CRM_EX_ERROR; + g_set_error(&error, PCMK__EXITC_ERROR, exit_code, + "Error creating output format %s: %s", args->output_ty, + pcmk_rc_str(rc)); + goto done; + } + if (args->version) { - g_strfreev(processed_args); - pcmk__free_arg_context(context); - /* FIXME: When crm_diff is converted to use formatted output, this can go. */ - pcmk__cli_help('v'); + out->version(out, false); + goto done; } if (options.no_version) { @@ -352,6 +372,11 @@ main(int argc, char **argv) pcmk__xml_free(source); pcmk__xml_free(target); - pcmk__output_and_clear_error(&error, NULL); + pcmk__output_and_clear_error(&error, out); + + if (out != NULL) { + out->finish(out, exit_code, true, NULL); + pcmk__output_free(out); + } crm_exit(exit_code); } From 2e7edd4ae1bcebd250ee625b61bb2fb8fea75d88 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 23:11:46 -0700 Subject: [PATCH 24/32] Refactor: tools: Use out->err() in crm_diff, and drop prompts The prompts would only make sense for text output format. They can't be sent to out->err() because it would show up as an error when the XML output is finished. If they're sent to out->info(), they won't be output at all for XML output format, and stdout can't easily be flushed for text output without checking the output format explicitly or adding new functionality to the pcmk__output_t API. We could continue printing to stderr directly, but I'd rather not. Besides, the prompts are not user-friendly at all, and I'm not sure who (if anyone) has ever used them. What they don't tell you is that you have to press Ctrl+D to mark end of input when you're done. Otherwise it will keep waiting for more input forever. I would like to eventually get rid of the --stdin option (deprecating and hiding it first), unless we can come up with a sane way to get TWO pieces of XML via stdin. Taking input from stdin makes some amount of sense when it's one piece. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 573d3e7db69..c60a1ac752b 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -10,7 +10,7 @@ #include #include // bool, true -#include // NULL, printf(), fprintf(), etc. +#include // NULL, printf(), etc. #include // free() #include // GOption, etc. @@ -292,8 +292,7 @@ main(int argc, char **argv) goto done; } if (options.patch) { - fprintf(stderr, - "Warning: -u/--no-version ignored with -p/--patch\n"); + out->err(out, "Warning: -u/--no-version ignored with -p/--patch"); } } @@ -301,7 +300,6 @@ main(int argc, char **argv) source = pcmk__xml_parse(options.source_string); } else if (options.use_stdin) { - fprintf(stderr, "Input first XML fragment:"); source = pcmk__xml_read(NULL); } else if (options.source_file != NULL) { @@ -319,7 +317,6 @@ main(int argc, char **argv) target = pcmk__xml_parse(options.target_string); } else if (options.use_stdin) { - fprintf(stderr, "Input second XML fragment:"); target = pcmk__xml_read(NULL); } else if (options.target_file != NULL) { From 2e32afe471be7957047d3828ffe7fa4c0ede8399 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Mon, 17 Mar 2025 23:56:51 -0700 Subject: [PATCH 25/32] Feature: tools: Deprecate crm_diff --stdin I'm not aware of any tool in our ecosystem that uses this option. It's not user-friendly at all. Nothing tells you that you have to press Ctrl+D (on Linux) at the beginning of a line to mark the end of an XML string and indicate that you're ready to input the next one. And then press Ctrl+D again to end the second XML string. So this isn't conducive to piping input into the command, which is typically the use case for a --stdin option. There's no obvious good way to delimit the first XML input string from the second one. The prompts (removed by a previous commit) also complicated the conversion to using pcmk__output_t. Basically, there's no great way to print the prompts when using XML-formatted output, and the output isn't flushed automatically for text-formatted output, so we can't guarantee that the prompts will appear on time. We could work around this by continuing to fprintf() to stderr and then flush, but that's ugly. Signed-off-by: Reid Wahl --- tools/crm_diff.c | 53 +++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index c60a1ac752b..00498d7c503 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -31,7 +31,7 @@ struct { bool patch; gboolean as_cib; gboolean no_version; - gboolean use_stdin; + gboolean use_stdin; //!< \deprecated } options; static gboolean @@ -44,41 +44,48 @@ patch_cb(const gchar *option_name, const gchar *optarg, gpointer data, return TRUE; } -// @COMPAT Use last-one-wins for original/new/patch input sources +/* @COMPAT Use last-one-wins for original/new/patch input sources + * + * @COMPAT Precedence is --original-string > --stdin > --original. --stdin is + * now deprecated and hidden, so we don't mention it in the help text. + */ static GOptionEntry original_xml_entries[] = { { "original", 'o', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, &options.source_file, - "XML is contained in the named file. Currently --original-string and\n" - INDENT "--stdin both override this. In a future release, the last one\n" - INDENT "specified will be used.", + "XML is contained in the named file. Currently --original-string\n" + INDENT "overrides this. In a future release, the last one specified\n" + INDENT "will be used.", "FILE" }, { "original-string", 'O', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, &options.source_string, "XML is contained in the supplied string. Currently this takes\n" - INDENT "precedence over both --stdin and --original. In a future\n" - INDENT "release, the last one specified will be used.", + INDENT "precedence over --original. In a future release, the last one\n" + INDENT "specified will be used.", "STRING" }, { NULL } }; +/* @COMPAT Precedence is --original-string > --stdin > --original. --stdin is + * now deprecated and hidden, so we don't mention it in the help text. + */ static GOptionEntry operation_entries[] = { { "new", 'n', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, &options.target_file, "Compare the original XML to the contents of the named file. Currently\n" - INDENT "--new-string and --stdin both override this. In a future\n" - INDENT "release, the last one specified will be used.", + INDENT "--new-string overrides this. In a future release, the last one\n" + INDENT "specified will be used.", "FILE" }, { "new-string", 'N', G_OPTION_FLAG_NONE, G_OPTION_ARG_STRING, &options.target_string, "Compare the original XML with the contents of the supplied string.\n" - INDENT "Currently this takes precedence over --stdin, --patch, and\n" - INDENT "--new. In a future release, the last one specified will be used.", + INDENT "Currently this takes precedence over --patch and --new. In a\n" + INDENT "future release, the last one specified will be used.", "STRING" }, { "patch", 'p', G_OPTION_FLAG_NONE, G_OPTION_ARG_CALLBACK, patch_cb, "Patch the original XML with the contents of the named file. Currently\n" - INDENT "--new-string, --stdin, and (if specified later) --new override\n" - INDENT "the input source specified here. In a future release, the last\n" - INDENT "one specified will be used. Note: even if this input source is\n" + INDENT "--new-string and (if specified later) --new override the input\n" + INDENT "source specified here. In a future release, the last one\n" + INDENT "specified will be used. Note: even if this input source is\n" INDENT "overridden, the input source will be applied as a patch to the\n" INDENT "original XML.", "FILE" }, @@ -97,16 +104,18 @@ static GOptionEntry addl_entries[] = { { "cib", 'c', G_OPTION_FLAG_NONE, G_OPTION_ARG_NONE, &options.as_cib, "Compare/patch the inputs as a CIB (includes version details)", NULL }, - { "stdin", 's', G_OPTION_FLAG_NONE, G_OPTION_ARG_NONE, &options.use_stdin, + { "no-version", 'u', G_OPTION_FLAG_NONE, G_OPTION_ARG_NONE, + &options.no_version, + "Generate the difference without version details", + NULL }, + + // @COMPAT Deprecated + { "stdin", 's', G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_NONE, &options.use_stdin, "Get the original XML and new (or patch) XML from stdin. Currently\n" INDENT "--original-string and --new-string override this for original\n" INDENT "and new/patch XML, respectively. In a future release, the last\n" INDENT "one specified will be used.", NULL }, - { "no-version", 'u', G_OPTION_FLAG_NONE, G_OPTION_ARG_NONE, - &options.no_version, - "Generate the difference without version details", - NULL }, { NULL } }; @@ -308,8 +317,7 @@ main(int argc, char **argv) } else { exit_code = CRM_EX_USAGE; g_set_error(&error, PCMK__EXITC_ERROR, exit_code, - "Either --original, --original-string, or --stdin must be " - "specified"); + "Either --original or --original-string must be specified"); goto done; } @@ -325,8 +333,7 @@ main(int argc, char **argv) } else { exit_code = CRM_EX_USAGE; g_set_error(&error, PCMK__EXITC_ERROR, exit_code, - "Either --new, --new-string, --patch, or --stdin must be " - "specified"); + "Either --new, --new-string, or --patch must be specified"); goto done; } From 6c5b306ace229ebd7ed55c08c7f5b3c40be8429a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 25 Jun 2025 16:57:30 -0700 Subject: [PATCH 26/32] API: libcrmcommon: New PCMK_XE_PATCHSET string constant Signed-off-by: Reid Wahl --- include/crm/common/xml_names.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/crm/common/xml_names.h b/include/crm/common/xml_names.h index 6dfd23af8b5..50e4120c931 100644 --- a/include/crm/common/xml_names.h +++ b/include/crm/common/xml_names.h @@ -157,6 +157,7 @@ extern "C" { #define PCMK_XE_PACEMAKERD "pacemakerd" #define PCMK_XE_PARAMETER "parameter" #define PCMK_XE_PARAMETERS "parameters" +#define PCMK_XE_PATCHSET "patchset" #define PCMK_XE_PERIOD "period" #define PCMK_XE_PODMAN "podman" #define PCMK_XE_PORT_MAPPING "port-mapping" From d36be6ca092b2fa2fb6a63f527d03085c44840e8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 25 Jun 2025 16:56:56 -0700 Subject: [PATCH 27/32] API: libcrmcommon: New PCMK_XE_UPDATED string constant Signed-off-by: Reid Wahl --- include/crm/common/xml_names.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/crm/common/xml_names.h b/include/crm/common/xml_names.h index 50e4120c931..2e937660dac 100644 --- a/include/crm/common/xml_names.h +++ b/include/crm/common/xml_names.h @@ -1,5 +1,5 @@ /* - * Copyright 2004-2024 the Pacemaker project contributors + * Copyright 2004-2025 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -215,6 +215,7 @@ extern "C" { #define PCMK_XE_TIMING "timing" #define PCMK_XE_TIMINGS "timings" #define PCMK_XE_TRANSITION "transition" +#define PCMK_XE_UPDATED "updated" #define PCMK_XE_UTILIZATION "utilization" #define PCMK_XE_UTILIZATIONS "utilizations" #define PCMK_XE_VALIDATE "validate" From 1e987c91cecac91acf6075b6a736ab64864fc135 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 25 Jun 2025 22:46:29 -0700 Subject: [PATCH 28/32] Test: cts-cli: Avoid Python exception for integer precision When logging an error: "ValueError: Precision not allowed in integer format specifier" Here we use width instead of precision, with zero-padding. Signed-off-by: Reid Wahl --- cts/cts-cli.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cts/cts-cli.in b/cts/cts-cli.in index b2d2218f773..5ec9b260908 100644 --- a/cts/cts-cli.in +++ b/cts/cts-cli.in @@ -450,7 +450,7 @@ class Test: def _log_test_failed(self, app, rc): """Log a message when a test fails.""" - self._output.append(f"* Failed (rc={rc:.3d}): {app:<23} - {self.desc}") + self._output.append(f"* Failed (rc={rc:03d}): {app:<23} - {self.desc}") def _log_test_passed(self, app): """Log a message when a test passes.""" From 5cc0790f8084bfd315b3754b9abb911a10fd84a0 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 25 Jun 2025 22:38:05 -0700 Subject: [PATCH 29/32] Test: cts-cli: crm_diff regression tests Signed-off-by: Reid Wahl --- cts/cli/crm_diff_new.xml | 2 +- cts/cli/crm_diff_patchset.xml | 63 +++++ cts/cli/regression.crm_diff.exp | 439 +++++++++++++++++++++++++++++++- cts/cts-cli.in | 42 ++- 4 files changed, 539 insertions(+), 7 deletions(-) create mode 100644 cts/cli/crm_diff_patchset.xml diff --git a/cts/cli/crm_diff_new.xml b/cts/cli/crm_diff_new.xml index 7fa0cbbd1ba..be9e9aba7fa 100644 --- a/cts/cli/crm_diff_new.xml +++ b/cts/cli/crm_diff_new.xml @@ -1,4 +1,4 @@ - + diff --git a/cts/cli/crm_diff_patchset.xml b/cts/cli/crm_diff_patchset.xml new file mode 100644 index 00000000000..7cb38288d6e --- /dev/null +++ b/cts/cli/crm_diff_patchset.xml @@ -0,0 +1,63 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/cts/cli/regression.crm_diff.exp b/cts/cli/regression.crm_diff.exp index 6b1a7109dda..01e859842f0 100644 --- a/cts/cli/regression.crm_diff.exp +++ b/cts/cli/regression.crm_diff.exp @@ -1,4 +1,4 @@ -=#=#=#= Begin test: Create an XML patchset =#=#=#= +=#=#=#= Begin test: Create an XML patchset from files =#=#=#= @@ -62,5 +62,438 @@ -=#=#=#= End test: Create an XML patchset - Error occurred (1) =#=#=#= -* Passed: crm_diff - Create an XML patchset +=#=#=#= End test: Create an XML patchset from files - Error occurred (1) =#=#=#= +* Passed: crm_diff - Create an XML patchset from files +=#=#=#= Begin test: Create an XML patchset from strings =#=#=#= + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +=#=#=#= End test: Create an XML patchset from strings - Error occurred (1) =#=#=#= +* Passed: crm_diff - Create an XML patchset from strings +=#=#=#= Begin test: Create an XML patchset from old file, new string =#=#=#= + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +=#=#=#= End test: Create an XML patchset from old file, new string - Error occurred (1) =#=#=#= +* Passed: crm_diff - Create an XML patchset from old file, new string +=#=#=#= Begin test: Create an XML patchset from old string, new file =#=#=#= + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +=#=#=#= End test: Create an XML patchset from old string, new file - Error occurred (1) =#=#=#= +* Passed: crm_diff - Create an XML patchset from old string, new file +=#=#=#= Begin test: Create an XML patchset from files =#=#=#= + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +=#=#=#= End test: Create an XML patchset from files - Error occurred (1) =#=#=#= +* Passed: crm_diff - Create an XML patchset from files +=#=#=#= Begin test: Create an XML patchset from files =#=#=#= + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +=#=#=#= End test: Create an XML patchset from files - Error occurred (1) =#=#=#= +* Passed: crm_diff - Create an XML patchset from files +=#=#=#= Begin test: Create an XML patchset from files =#=#=#= +crm_diff: -u/--no-version incompatible with -c/--cib +=#=#=#= End test: Create an XML patchset from files - Incorrect usage (64) =#=#=#= +* Passed: crm_diff - Create an XML patchset from files +=#=#=#= Begin test: Apply an XML patchset to a file =#=#=#= + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +=#=#=#= End test: Apply an XML patchset to a file - OK (0) =#=#=#= +* Passed: crm_diff - Apply an XML patchset to a file +=#=#=#= Begin test: Apply an XML patchset to a string =#=#=#= + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +=#=#=#= End test: Apply an XML patchset to a string - OK (0) =#=#=#= +* Passed: crm_diff - Apply an XML patchset to a string diff --git a/cts/cts-cli.in b/cts/cts-cli.in index 5ec9b260908..d594a6f612a 100644 --- a/cts/cts-cli.in +++ b/cts/cts-cli.in @@ -2385,10 +2385,46 @@ class CrmDiffRegressionTest(RegressionTest): @property def tests(self): """A list of Test instances to be run as part of this regression test.""" + + old_file = f"{cts_cli_data}/crm_diff_old.xml" + new_file = f"{cts_cli_data}/crm_diff_new.xml" + patch_file = f"{cts_cli_data}/crm_diff_patchset.xml" + + with open(f"{cts_cli_data}/crm_diff_old.xml", "r") as file: + old_str = f"'{file.read()}'" + with open(f"{cts_cli_data}/crm_diff_new.xml", "r") as file: + new_str = f"'{file.read()}'" + return [ - Test("Create an XML patchset", - f"crm_diff -o {cts_cli_data}/crm_diff_old.xml -n {cts_cli_data}/crm_diff_new.xml", - expected_rc=ExitStatus.ERROR) + Test("Create an XML patchset from files", + f"crm_diff -o {old_file} -n {new_file}", + expected_rc=ExitStatus.ERROR), + Test("Create an XML patchset from strings", + f"crm_diff -O {old_str} -N {new_str}", + expected_rc=ExitStatus.ERROR), + Test("Create an XML patchset from old file, new string", + f"crm_diff -o {old_file} -N {new_str}", + expected_rc=ExitStatus.ERROR), + Test("Create an XML patchset from old string, new file", + f"crm_diff -O {old_str} -n {new_file}", + expected_rc=ExitStatus.ERROR), + + Test("Create an XML patchset from files", + f"crm_diff -o {old_file} -n {new_file} --cib", + expected_rc=ExitStatus.ERROR), + Test("Create an XML patchset from files", + f"crm_diff -o {old_file} -n {new_file} --no-version", + expected_rc=ExitStatus.ERROR), + Test("Create an XML patchset from files", + f"crm_diff -o {old_file} -n {new_file} --cib --no-version", + expected_rc=ExitStatus.USAGE), + + Test("Apply an XML patchset to a file", + f"crm_diff -o {old_file} -p {patch_file}"), + Test("Apply an XML patchset to a string", + f"crm_diff -O {old_str} -p {patch_file}"), + Test("Apply an XML patchset to a file", + f"crm_diff -o {old_file} -p {patch_file} --cib"), ] From eddad0c110888568d5548c77b1bdb184ef5917e9 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Tue, 18 Mar 2025 00:09:59 -0700 Subject: [PATCH 30/32] Feature: tools: Use formatted output in crm_diff.c It's a shame that the pcmk__output_t:output_xml() method requires preformatting the XML into a string. Since it's an internal API, maybe we can revisit that soon. Also, we already have a diff schema in xml/api. We **could** use it and output the patchset XML directly, instead of outputting the patchset XML as CDATA. Ken was pretty adamant about outputting CDATA here. I don't know why, or can't remember. The argument can't be that the XML isn't generated by crm_diff itself, because that's true for almost all of our output formatters. Something about how the format is supposed to be meaningful only to Pacemaker... though I don't see why that ought to be the case. I'm fine with CDATA, regardless. Ref T114 Signed-off-by: Reid Wahl --- tools/crm_diff.c | 52 +++++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 31 deletions(-) diff --git a/tools/crm_diff.c b/tools/crm_diff.c index 00498d7c503..239afc4c0d8 100644 --- a/tools/crm_diff.c +++ b/tools/crm_diff.c @@ -120,34 +120,11 @@ static GOptionEntry addl_entries[] = { { NULL } }; -/*! - * \internal - * \brief Print an XML tree serialized to text - * - * \param[in] xml XML tree to print - * - * \todo Use \c pcmk__output_t with message functions and drop this. - * - * \note This is basically a simplified version of \c pcmk__xml_write_fd(), but - * that function closes the stream before returning. We could modify it in - * the future. But we don't want to close stdout. - */ -static void -print_xml(const xmlNode *xml) -{ - GString *buffer = g_string_sized_new(1024); - - pcmk__xml_string(xml, pcmk__xml_fmt_pretty, buffer, 0); - - printf("%s", buffer->str); - g_string_free(buffer, TRUE); - fflush(stdout); -} - /*! * \internal * \brief Create an XML patchset from the given source and target XML trees * + * \param[in,out] out Output object * \param[in,out] source Source XML * \param[in,out] target Target XML * \param[in] as_cib If \c true, treat the XML trees as CIBs. In @@ -160,7 +137,8 @@ print_xml(const xmlNode *xml) * \return Standard Pacemaker return code */ static int -generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) +generate_patch(pcmk__output_t *out, xmlNode *source, xmlNode *target, + bool as_cib, bool no_version) { static const char *const vfields[] = { PCMK_XA_ADMIN_EPOCH, @@ -169,6 +147,7 @@ generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) }; xmlNode *patchset = NULL; + GString *buffer = NULL; // Currently impossible; just a reminder for when we move to libpacemaker pcmk__assert(!as_cib || !no_version); @@ -207,8 +186,13 @@ generate_patch(xmlNode *source, xmlNode *target, bool as_cib, bool no_version) } pcmk__log_xml_patchset(LOG_NOTICE, patchset); - print_xml(patchset); + + buffer = g_string_sized_new(1024); + pcmk__xml_string(patchset, pcmk__xml_fmt_pretty, buffer, 0); + out->output_xml(out, PCMK_XE_PATCHSET, buffer->str); + pcmk__xml_free(patchset); + g_string_free(buffer, TRUE); /* pcmk_rc_error means there's a non-empty diff. * @COMPAT Choose a more descriptive return code, like one that maps to @@ -226,7 +210,8 @@ static const pcmk__supported_format_t formats[] = { }; static GOptionContext * -build_arg_context(pcmk__common_args_t *args) { +build_arg_context(pcmk__common_args_t *args, GOptionGroup **group) +{ GOptionContext *context = NULL; const char *description = "Examples:\n\n" @@ -240,7 +225,7 @@ build_arg_context(pcmk__common_args_t *args) { "Apply the patch to the running cluster:\n\n" "\t# cibadmin --patch -x patch.xml\n"; - context = pcmk__build_arg_context(args, NULL, NULL, NULL); + context = pcmk__build_arg_context(args, "text (default), xml", group, NULL); g_option_context_set_description(context, description); pcmk__add_arg_group(context, "xml", "Original XML:", @@ -268,7 +253,7 @@ main(int argc, char **argv) GOptionGroup *output_group = NULL; pcmk__common_args_t *args = pcmk__new_common_args(SUMMARY); gchar **processed_args = pcmk__cmdline_preproc(argv, "nopNO"); - GOptionContext *context = build_arg_context(args); + GOptionContext *context = build_arg_context(args, &output_group); pcmk__register_formats(output_group, formats); @@ -357,11 +342,16 @@ main(int argc, char **argv) g_set_error(&error, PCMK__RC_ERROR, rc, "Could not apply patch: %s", pcmk_rc_str(rc)); } else { - print_xml(source); + GString *buffer = g_string_sized_new(1024); + + pcmk__xml_string(source, pcmk__xml_fmt_pretty, buffer, 0); + out->output_xml(out, PCMK_XE_UPDATED, buffer->str); + g_string_free(buffer, TRUE); } } else { - rc = generate_patch(source, target, options.as_cib, options.no_version); + rc = generate_patch(out, source, target, options.as_cib, + options.no_version); } exit_code = pcmk_rc2exitc(rc); From 0394f59a4ac3ec07686de6b72769ade60f411531 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 25 Jun 2025 17:03:02 -0700 Subject: [PATCH 31/32] API: schemas: Add a schema for crm_diff This is pretty basic. We can have either a "patchset" element or an "updated" element, and each just contains a CDATA block. It would be possible to use the diff API schema and include the patchset XML directly rather than as CDATA. This is less straightforward to do for the "updated" XML. The input is expected to be a CIB, so I suppose we could use the CIB schema there. Unfortunately the patchset itself already uses "diff" as its root element, so I wanted to avoid using "diff" as a top-level element name for crm_diff output. It also wouldn't make a lot of sense when applying a patchset (that is, where we're displaying an "updated" element). Closes T114 Signed-off-by: Reid Wahl --- xml/Makefile.am | 1 + xml/api/crm_diff-2.39.rng | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 xml/api/crm_diff-2.39.rng diff --git a/xml/Makefile.am b/xml/Makefile.am index dcbd9db0547..5b218a91484 100644 --- a/xml/Makefile.am +++ b/xml/Makefile.am @@ -55,6 +55,7 @@ version_pairs_last = $(wordlist \ # Names of API schemas that form the choices for pacemaker-result content API_request_base = command-output \ crm_attribute \ + crm_diff \ crm_error \ crm_mon \ crm_node \ diff --git a/xml/api/crm_diff-2.39.rng b/xml/api/crm_diff-2.39.rng new file mode 100644 index 00000000000..296539c24ba --- /dev/null +++ b/xml/api/crm_diff-2.39.rng @@ -0,0 +1,29 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 22ae578c97dfbca99767a8ef34fc9f4d633b735d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Wed, 2 Jul 2025 23:32:41 -0700 Subject: [PATCH 32/32] Feature: libcrmcommon: Bump feature set to 3.20.2 crm_diff --stdin is now deprecated, crm_diff now accepts --output-as=xml (and defaults to --output-as=text), and there is a new crm_diff API schema. Signed-off-by: Reid Wahl --- include/crm/crm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/crm/crm.h b/include/crm/crm.h index b8a213cb4d0..a819902cc9b 100644 --- a/include/crm/crm.h +++ b/include/crm/crm.h @@ -63,7 +63,7 @@ extern "C" { * >=3.2.0: DC supports PCMK_EXEC_INVALID and PCMK_EXEC_NOT_CONNECTED * >=3.19.0: DC supports PCMK__CIB_REQUEST_COMMIT_TRANSACT */ -#define CRM_FEATURE_SET "3.20.1" +#define CRM_FEATURE_SET "3.20.2" /* Pacemaker's CPG protocols use fixed-width binary fields for the sender and * recipient of a CPG message. This imposes an arbitrary limit on cluster node