From a30fccfb849822d657b7747ea1cf604aeadc7325 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 01:47:39 -0800 Subject: [PATCH 01/22] Refactor: libcrmcommon: Use a switch statement in is_mode_allowed() I prefer to list the three meaningful values explicitly. Otherwise we could accept an arbitrary flag that happens to be set in the flag group. Signed-off-by: Reid Wahl --- lib/common/acl.c | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index ce34b93d6cc..b1bf444bbe6 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -764,30 +764,28 @@ is_mode_allowed(uint32_t flags, enum pcmk__xml_flags mode) return false; } - if (pcmk__is_set(flags, mode)) { - // The access we requested is explicitly allowed - return true; - } - - if ((mode == pcmk__xf_acl_read) - && pcmk__is_set(flags, pcmk__xf_acl_write)) { + switch (mode) { + case pcmk__xf_acl_read: + // Write access provides read access + return pcmk__any_flags_set(flags, + pcmk__xf_acl_read|pcmk__xf_acl_write); - // Write access provides read access - return true; - } + case pcmk__xf_acl_write: + return pcmk__is_set(flags, pcmk__xf_acl_write); - if ((mode == pcmk__xf_acl_create) - && pcmk__any_flags_set(flags, pcmk__xf_acl_write|pcmk__xf_created)) { + case pcmk__xf_acl_create: + /* Write access provides create access. + * + * @TODO Why does the \c pcmk__xf_created flag provide create + * access? This was introduced by commit e2ed85fe. + */ + return pcmk__any_flags_set(flags, + pcmk__xf_acl_write|pcmk__xf_created); - /* Write access provides create access. - * - * @TODO Why does the \c pcmk__xf_created flag provide create access? - * This was introduced by commit e2ed85fe. - */ - return true; + default: + // Invalid mode + return false; } - - return false; } /*! From cf815b12679004210d310800f735d5b814213697 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 02:10:30 -0800 Subject: [PATCH 02/22] Doc: libcrmcommon: Clarify that pcmk__element_xpath() returns non-NULL Signed-off-by: Reid Wahl --- lib/common/xpath.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/common/xpath.c b/lib/common/xpath.c index 8bff702dcb0..f3ff8bf2ac0 100644 --- a/lib/common/xpath.c +++ b/lib/common/xpath.c @@ -272,10 +272,11 @@ pcmk__xpath_find_one(xmlDoc *doc, const char *path, uint8_t level) * * \param[in] xml The XML element for which to build an XPath string * - * \return A \p GString that matches \p xml, or \p NULL if \p xml is \p NULL. + * \return \c GString that matches \p xml, or \c NULL if \p xml is \c NULL + * (guaranteed not to be \c NULL if \p xml is not \c NULL) * * \note The caller is responsible for freeing the string using - * \p g_string_free(). + * \c g_string_free(). */ GString * pcmk__element_xpath(const xmlNode *xml) From 012636be89e271cf48d281baa2c4b17f103c2ed8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 02:17:44 -0800 Subject: [PATCH 03/22] Refactor: libcrmcommon: Use convenience helpers in implicitly_allowed() Use pcmk__xe_first_attr() and attr_is_not_id(). Signed-off-by: Reid Wahl --- lib/common/acl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index b1bf444bbe6..77f9acb47a8 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -949,23 +949,24 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, * * \param[in] xml XML element to check * - * \return true if XML element is implicitly allowed, false otherwise + * \return \c true if XML element is implicitly allowed, or \c false otherwise */ static bool -implicitly_allowed(const xmlNode *xml) +implicitly_allowed(xmlNode *xml) { GString *path = NULL; - for (xmlAttr *prop = xml->properties; prop != NULL; prop = prop->next) { - if (strcmp((const char *) prop->name, PCMK_XA_ID) != 0) { + for (xmlAttr *attr = pcmk__xe_first_attr(xml); attr != NULL; + attr = attr->next) { + + if (attr_is_not_id(attr, NULL)) { return false; } } path = pcmk__element_xpath(xml); - pcmk__assert(path != NULL); - if (strstr((const char *) path->str, "/" PCMK_XE_ACLS "/") != NULL) { + if (strstr(path->str, "/" PCMK_XE_ACLS "/") != NULL) { g_string_free(path, TRUE); return false; } From 6e872c65f2967e425fbd25bbc8a21c9803dcdb4e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 02:35:02 -0800 Subject: [PATCH 04/22] Refactor: libcrmcommon: Walk up the tree in implicitly_allowed() Instead of creating an XPath string and looking for a substring. It seems clearer this way, though that is debatable. Signed-off-by: Reid Wahl --- lib/common/acl.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 77f9acb47a8..0349dc10d50 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -954,8 +954,6 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, static bool implicitly_allowed(xmlNode *xml) { - GString *path = NULL; - for (xmlAttr *attr = pcmk__xe_first_attr(xml); attr != NULL; attr = attr->next) { @@ -964,14 +962,16 @@ implicitly_allowed(xmlNode *xml) } } - path = pcmk__element_xpath(xml); - - if (strstr(path->str, "/" PCMK_XE_ACLS "/") != NULL) { - g_string_free(path, TRUE); - return false; + /* Creation is not implicitly allowed for a descendant of PCMK_XE_ACLS, but + * it may be for PCMK_XE_ACLS itself. Start checking at xml->parent and walk + * up the tree. + */ + for (xml = xml->parent; xml != NULL; xml = xml->parent) { + if (pcmk__xe_is(xml, PCMK_XE_ACLS)) { + return false; + } } - g_string_free(path, TRUE); return true; } From 680e7f2aece5cb6fc4854d4fe1c4b8a1be9742e3 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 13:56:34 -0800 Subject: [PATCH 05/22] Refactor: libcrmcommon, libpe_status: Drop strncmp() calls Replace with g_str_has_prefix() in all but one place for clarity. The remaining place is pcmk__ipc_is_authentic_process_active(). In that case, use pcmk__str_eq(). Note that the former length argument was sizeof(last_asked_name), not sizeof(last_asked_name) - 1. This means we were checking whether the two strings were the same length and every character matched up to that length -- in other words, we were checking whether the strings were equal. Signed-off-by: Reid Wahl --- include/crm/pengine/internal.h | 2 +- lib/common/actions.c | 3 ++- lib/common/digest.c | 4 ++-- lib/common/ipc_client.c | 5 +++-- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/crm/pengine/internal.h b/include/crm/pengine/internal.h index 543d1a92023..cb31eaca444 100644 --- a/include/crm/pengine/internal.h +++ b/include/crm/pengine/internal.h @@ -246,7 +246,7 @@ pe_base_name_eq(const pcmk_resource_t *rsc, const char *id) // Number of characters in rsc->id before any clone suffix size_t base_len = pe_base_name_end(rsc->id) - rsc->id + 1; - return (strlen(id) == base_len) && !strncmp(id, rsc->id, base_len); + return (strlen(id) == base_len) && g_str_has_prefix(rsc->id, id); } return false; } diff --git a/lib/common/actions.c b/lib/common/actions.c index b312c2ac612..26a7d2c8b1e 100644 --- a/lib/common/actions.c +++ b/lib/common/actions.c @@ -266,7 +266,8 @@ match_before(const char *key, size_t position, const char **matches) const size_t possible = position - match_len - 1; if ((key[possible] == '_') - && (strncmp(key + possible + 1, matches[i], match_len) == 0)) { + && g_str_has_prefix(key + possible + 1, matches[i])) { + return possible; } } diff --git a/lib/common/digest.c b/lib/common/digest.c index 3552ba5ccce..80448e62f54 100644 --- a/lib/common/digest.c +++ b/lib/common/digest.c @@ -300,10 +300,10 @@ pcmk__xa_filterable(const char *name) static bool should_filter_for_digest(xmlAttrPtr a, void *user_data) { - if (strncmp((const char *) a->name, CRM_META "_", - sizeof(CRM_META " ") - 1) == 0) { + if (g_str_has_prefix((const char *) a->name, CRM_META "_")) { return true; } + return pcmk__str_any_of((const char *) a->name, PCMK_XA_ID, PCMK_XA_CRM_FEATURE_SET, diff --git a/lib/common/ipc_client.c b/lib/common/ipc_client.c index fd8a6058290..4f782e9b8a4 100644 --- a/lib/common/ipc_client.c +++ b/lib/common/ipc_client.c @@ -1747,8 +1747,9 @@ pcmk__ipc_is_authentic_process_active(const char *name, uid_t refuid, } rc = pcmk_rc_ok; - if ((found_uid != refuid || found_gid != refgid) - && strncmp(last_asked_name, name, sizeof(last_asked_name))) { + if (((found_uid != refuid) || (found_gid != refgid)) + && !pcmk__str_eq(name, last_asked_name, pcmk__str_none)) { + if ((found_uid == 0) && (refuid != 0)) { pcmk__warn("Daemon (IPC %s) runs as root, whereas the expected " "credentials are %lld:%lld, hazard of violating the " From 53df26a8d4f9968c132a3f608031f0f8bbfe880e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 14:16:10 -0800 Subject: [PATCH 06/22] Refactor: libcrmcommon: Drop a redundant check in pcmk__xa_remove() We return at the very beginning if attr->parent is NULL. element is then assigned attr->parent, so we know that element is not NULL. Signed-off-by: Reid Wahl --- lib/common/xml_attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/common/xml_attr.c b/lib/common/xml_attr.c index c7993bbab2f..d3b0785ab36 100644 --- a/lib/common/xml_attr.c +++ b/lib/common/xml_attr.c @@ -59,7 +59,7 @@ pcmk__xa_remove(xmlAttr *attr, bool force) return EPERM; } - if (!force && (element != NULL) + if (!force && pcmk__xml_doc_all_flags_set(element->doc, pcmk__xf_tracking)) { // Leave in place (marked for removal) until after diff is calculated From ec009a7530e028bb6c65a9810b5b64ac2e1b7790 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 14:20:13 -0800 Subject: [PATCH 07/22] Refactor: libcrmcommon: Check force arg sooner in pcmk__xa_remove() The whole function seems clearer to me this way. Signed-off-by: Reid Wahl --- lib/common/xml_attr.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/common/xml_attr.c b/lib/common/xml_attr.c index d3b0785ab36..4ccfbdeb91a 100644 --- a/lib/common/xml_attr.c +++ b/lib/common/xml_attr.c @@ -50,26 +50,30 @@ pcmk__xa_remove(xmlAttr *attr, bool force) return pcmk_rc_ok; } + if (force) { + goto remove; + } + element = attr->parent; - if (!force && !pcmk__check_acl(element, NULL, pcmk__xf_acl_write)) { + if (!pcmk__check_acl(element, NULL, pcmk__xf_acl_write)) { // ACLs apply to element, not to particular attributes pcmk__trace("ACLs prevent removal of attributes from %s element", element->name); return EPERM; } - if (!force - && pcmk__xml_doc_all_flags_set(element->doc, pcmk__xf_tracking)) { - + if (pcmk__xml_doc_all_flags_set(element->doc, pcmk__xf_tracking)) { // Leave in place (marked for removal) until after diff is calculated pcmk__xml_set_parent_flags(element, pcmk__xf_dirty); pcmk__set_xml_flags((xml_node_private_t *) attr->_private, pcmk__xf_deleted); - } else { - pcmk__xml_free_private_data((xmlNode *) attr); - xmlRemoveProp(attr); + return pcmk_rc_ok; } + +remove: + pcmk__xml_free_private_data((xmlNode *) attr); + xmlRemoveProp(attr); return pcmk_rc_ok; } From 4b68578dd168fbffd90883bb0106102e6fbd899b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 15:28:03 -0800 Subject: [PATCH 08/22] Refactor: libcrmcommon: Functionize cases of new_private_data() Notes: * The tracking flag can never be set when the argument is a document node. If node->_private is not NULL, we return before the switch statement. But pcmk__xml_doc_all_flags_set() returns false when the document's private data is NULL. So tracking is relevant only for element, attribute, and comment nodes. * pcmk__mark_xml_node_dirty() sets the pcmk__xf_dirty flag on the node itself as well as all of its parents. For an element, it doesn't matter whether we call it before or after creating the attributes' private data. Signed-off-by: Reid Wahl --- lib/common/xml.c | 97 +++++++++++++++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 34 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 5b34ba2fe5a..e34676e7d4f 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -262,6 +262,62 @@ reset_xml_private_data(xml_doc_private_t *docpriv) } } +/*! + * \internal + * \brief Allocate and initialize private data for an XML document + * + * \param[in,out] doc XML document + */ +static void +new_doc_private_data(xmlDoc *doc) +{ + xml_doc_private_t *priv = pcmk__assert_alloc(1, sizeof(xml_doc_private_t)); + + priv->check = PCMK__XML_DOC_PRIVATE_MAGIC; + doc->_private = priv; +} + +/*! + * \internal + * \brief Allocate and initialize private data for a non-document XML node + * + * \param[in,out] xml XML node + */ +static void +new_node_private_data(xmlNode *xml) +{ + const bool tracking = pcmk__xml_doc_all_flags_set(xml->doc, + pcmk__xf_tracking); + xml_node_private_t *priv = pcmk__assert_alloc(1, + sizeof(xml_node_private_t)); + + priv->check = PCMK__XML_NODE_PRIVATE_MAGIC; + xml->_private = priv; + + if (tracking) { + pcmk__set_xml_flags(priv, pcmk__xf_created); + pcmk__mark_xml_node_dirty(xml); + } +} + +/*! + * \internal + * \brief Allocate and initialize private data for an XML element + * + * \param[in,out] xml XML element + */ +static void +new_element_private_data(xmlNode *xml) +{ + new_node_private_data(xml); + + for (xmlAttr *iter = pcmk__xe_first_attr(xml); iter != NULL; + iter = iter->next) { + + new_node_private_data((xmlNode *) iter); + } +} + /*! * \internal * \brief Allocate and initialize private data for an XML node @@ -276,47 +332,25 @@ reset_xml_private_data(xml_doc_private_t *docpriv) static bool new_private_data(xmlNode *node, void *user_data) { - bool tracking = false; - CRM_CHECK(node != NULL, return true); if (node->_private != NULL) { return true; } - tracking = pcmk__xml_doc_all_flags_set(node->doc, pcmk__xf_tracking); - switch (node->type) { case XML_DOCUMENT_NODE: - { - xml_doc_private_t *docpriv = - pcmk__assert_alloc(1, sizeof(xml_doc_private_t)); - - docpriv->check = PCMK__XML_DOC_PRIVATE_MAGIC; - node->_private = docpriv; - } - break; + new_doc_private_data((xmlDoc *) node); + return true; - case XML_ELEMENT_NODE: case XML_ATTRIBUTE_NODE: case XML_COMMENT_NODE: - { - xml_node_private_t *nodepriv = - pcmk__assert_alloc(1, sizeof(xml_node_private_t)); - - nodepriv->check = PCMK__XML_NODE_PRIVATE_MAGIC; - node->_private = nodepriv; - if (tracking) { - pcmk__set_xml_flags(nodepriv, pcmk__xf_dirty|pcmk__xf_created); - } - - for (xmlAttr *iter = pcmk__xe_first_attr(node); iter != NULL; - iter = iter->next) { + new_node_private_data(node); + return true; - new_private_data((xmlNode *) iter, user_data); - } - } - break; + case XML_ELEMENT_NODE: + new_element_private_data(node); + return true; case XML_TEXT_NODE: case XML_DTD_NODE: @@ -327,11 +361,6 @@ new_private_data(xmlNode *node, void *user_data) CRM_LOG_ASSERT(node->type == XML_ELEMENT_NODE); return true; } - - if (tracking) { - pcmk__mark_xml_node_dirty(node); - } - return true; } /*! From 6a37ef2c6f8a1ea111c7be2350ba1495c1462adc Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 16:50:48 -0800 Subject: [PATCH 09/22] Refactor: libcrmcommon: New pcmk__xe_foreach{,_const}_attr() Nothing uses these yet. Signed-off-by: Reid Wahl --- include/crm/common/xml_element_internal.h | 6 +++ lib/common/xml_element.c | 61 +++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/include/crm/common/xml_element_internal.h b/include/crm/common/xml_element_internal.h index ec2964c2504..4878cc0ffaa 100644 --- a/include/crm/common/xml_element_internal.h +++ b/include/crm/common/xml_element_internal.h @@ -37,6 +37,12 @@ extern "C" { const char *pcmk__xe_add_last_written(xmlNode *xe); +bool pcmk__xe_foreach_attr(xmlNode *xml, bool (*fn)(xmlAttr *, void *), + void *user_data); +bool pcmk__xe_foreach_const_attr(const xmlNode *xml, + bool (*fn)(const xmlAttr *, void *), + void *user_data); + xmlNode *pcmk__xe_first_child(const xmlNode *parent, const char *node_name, const char *attr_n, const char *attr_v); diff --git a/lib/common/xml_element.c b/lib/common/xml_element.c index 78a2a084963..4bafab83425 100644 --- a/lib/common/xml_element.c +++ b/lib/common/xml_element.c @@ -26,6 +26,67 @@ #include #include "crmcommon_private.h" +/*! + * \internal + * \brief Call a function for each of an XML element's attributes + * + * \param[in,out] xml XML element + * \param[in] fn Function to call for each attribute (returns + * \c true to continue iterating over attributes or + * \c false to stop) + * \param[in,out] user_data User data argument for \p fn + * + * \return \c false if any \p fn call returned \c false, or \c true otherwise + * + * \note \p fn may remove its attribute argument. + */ +bool +pcmk__xe_foreach_attr(xmlNode *xml, bool (*fn)(xmlAttr *, void *), + void *user_data) +{ + xmlAttr *attr = pcmk__xe_first_attr(xml); + + while (attr != NULL) { + xmlAttr *next = attr->next; + + if (!fn(attr, user_data)) { + return false; + } + + attr = next; + } + + return true; +} + +/*! + * \internal + * \brief Call a function for each of a \c const XML element's attributes + * + * \param[in] xml XML element + * \param[in] fn Function to call for each attribute (returns + * \c true to continue iterating over attributes or + * \c false to stop) + * \param[in,out] user_data User data argument for \p fn + * + * \return \c false if any \p fn call returned \c false, or \c true otherwise + */ +bool +pcmk__xe_foreach_const_attr(const xmlNode *xml, + bool (*fn)(const xmlAttr *, void *), + void *user_data) +{ + for (const xmlAttr *attr = pcmk__xe_first_attr(xml); attr != NULL; + attr = attr->next) { + + if (!fn(attr, user_data)) { + return false; + } + } + + return true; +} + /*! * \internal * \brief Find first XML child element matching given criteria From 9b07fefa4db50dcad510571ec147ed8eb9329532 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 14:35:28 -0800 Subject: [PATCH 10/22] Refactor: libcrmcommon: pcmk__xe_foreach_attr() in new_private_data() Signed-off-by: Reid Wahl --- lib/common/xml.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index e34676e7d4f..c68755c1f87 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -300,6 +300,24 @@ new_node_private_data(xmlNode *xml) } } +/*! + * \internal + * \brief Allocate and initialize private data for an XML attribute + * + * \param[in,out] attr XML attribute + * \param[in] user_data Ignored + * + * \return \c true (to continue iterating) + * + * \note This is compatible with \c pcmk__xe_foreach_attr(). + */ +static bool +new_attr_private_data(xmlAttr *attr, void *user_data) +{ + new_node_private_data((xmlNode *) attr); + return true; +} + /*! * \internal * \brief Allocate and initialize private data for an XML element @@ -310,12 +328,7 @@ static void new_element_private_data(xmlNode *xml) { new_node_private_data(xml); - - for (xmlAttr *iter = pcmk__xe_first_attr(xml); iter != NULL; - iter = iter->next) { - - new_node_private_data((xmlNode *) iter); - } + pcmk__xe_foreach_attr(xml, new_attr_private_data, NULL); } /*! From 35e75c99c31656118c8b61305536ae2f3aefb110 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 17:03:00 -0800 Subject: [PATCH 11/22] Refactor: libcrmcommon: Clear flags in reset_xml_private_data() Also rename to reset_doc_private_data(), move the definition to a position below new_private_data() and above its first caller, and add Doxygen. Signed-off-by: Reid Wahl --- lib/common/xml.c | 49 ++++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index c68755c1f87..739931bab4b 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -245,23 +245,6 @@ free_deleted_object(void *data) } } -// Free and NULL user, ACLs, and deleted objects in an XML node's private data -static void -reset_xml_private_data(xml_doc_private_t *docpriv) -{ - if (docpriv != NULL) { - pcmk__assert(docpriv->check == PCMK__XML_DOC_PRIVATE_MAGIC); - - g_clear_pointer(&docpriv->acl_user, free); - g_clear_pointer(&docpriv->acls, pcmk__free_acls); - - if(docpriv->deleted_objs) { - g_list_free_full(docpriv->deleted_objs, free_deleted_object); - docpriv->deleted_objs = NULL; - } - } -} - /*! * \internal * \brief Allocate and initialize private data for an XML document @@ -376,6 +359,33 @@ new_private_data(xmlNode *node, void *user_data) } } +/*! + * \internal + * \brief Free and zero all data fields of an XML document's private data + * + * This function does not clear the \c check field or free the private data + * object itself. + * + * \param[in,out] docpriv XML document private data + */ +static void +reset_doc_private_data(xml_doc_private_t *docpriv) +{ + if (docpriv == NULL) { + return; + } + + pcmk__assert(docpriv->check == PCMK__XML_DOC_PRIVATE_MAGIC); + + docpriv->flags = pcmk__xf_none; + + g_clear_pointer(&docpriv->acl_user, free); + g_clear_pointer(&docpriv->acls, pcmk__free_acls); + + g_list_free_full(docpriv->deleted_objs, free_deleted_object); + docpriv->deleted_objs = NULL; +} + /*! * \internal * \brief Free private data for an XML node @@ -397,7 +407,7 @@ free_private_data(xmlNode *node, void *user_data) } if (node->type == XML_DOCUMENT_NODE) { - reset_xml_private_data((xml_doc_private_t *) node->_private); + reset_doc_private_data((xml_doc_private_t *) node->_private); } else { xml_node_private_t *nodepriv = node->_private; @@ -520,8 +530,7 @@ pcmk__xml_commit_changes(xmlDoc *doc) pcmk__xml_tree_foreach(xmlDocGetRootElement(doc), commit_attr_deletions, NULL); } - reset_xml_private_data(docpriv); - docpriv->flags = pcmk__xf_none; + reset_doc_private_data(docpriv); } /*! From b20629c5bc2057387f565af9242b2fe0870be5fe Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 17:19:55 -0800 Subject: [PATCH 12/22] Refactor: libcrmcommon: pcmk__xe_foreach_attr() in free_private_data() To enable this, functionize the pieces of free_private_data(). Signed-off-by: Reid Wahl --- lib/common/xml.c | 84 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 70 insertions(+), 14 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 739931bab4b..98f362291de 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -386,6 +386,66 @@ reset_doc_private_data(xml_doc_private_t *docpriv) docpriv->deleted_objs = NULL; } +/*! + * \internal + * \brief Free and clear private data for an XML document + * + * \param[in,out] doc XML document + */ +static void +free_doc_private_data(xmlDoc *doc) +{ + reset_doc_private_data(doc->_private); + g_clear_pointer(&doc->_private, free); +} + +/*! + * \internal + * \brief Free and clear private data for a non-document XML node + * + * \param[in,out] xml XML node + */ +static void +free_node_private_data(xmlNode *xml) +{ + xml_node_private_t *nodepriv = xml->_private; + + pcmk__assert(nodepriv->check == PCMK__XML_NODE_PRIVATE_MAGIC); + + g_clear_pointer(&xml->_private, free); +} + +/*! + * \internal + * \brief Free and clear private data for an XML attribute + * + * \param[in,out] attr XML attribute + * \param[in] user_data Ignored + * + * \return \c true (to continue iterating) + * + * \note This is compatible with \c pcmk__xe_foreach_attr(). + */ +static bool +free_attr_private_data(xmlAttr *xml, void *user_data) +{ + free_node_private_data((xmlNode *) xml); + return true; +} + +/*! + * \internal + * \brief Free and clear private data for an XML element and its attributes + * + * \param[in,out] xml XML element + */ +static void +free_element_private_data(xmlNode *xml) +{ + free_node_private_data(xml); + pcmk__xe_foreach_attr(xml, free_attr_private_data, NULL); +} + /*! * \internal * \brief Free private data for an XML node @@ -406,23 +466,19 @@ free_private_data(xmlNode *node, void *user_data) return true; } - if (node->type == XML_DOCUMENT_NODE) { - reset_doc_private_data((xml_doc_private_t *) node->_private); - - } else { - xml_node_private_t *nodepriv = node->_private; - - pcmk__assert(nodepriv->check == PCMK__XML_NODE_PRIVATE_MAGIC); + switch (node->type) { + case XML_DOCUMENT_NODE: + free_doc_private_data((xmlDoc *) node); + return true; - for (xmlAttr *iter = pcmk__xe_first_attr(node); iter != NULL; - iter = iter->next) { + case XML_ELEMENT_NODE: + free_element_private_data(node); + return true; - free_private_data((xmlNode *) iter, user_data); - } + default: + free_node_private_data(node); + return true; } - free(node->_private); - node->_private = NULL; - return true; } /*! From cea7ac88310fa2c35dbf93c81ab63a1a8be0883b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 17:26:01 -0800 Subject: [PATCH 13/22] Refactor: libcrmcommon: Use a for-loop in xml_diff_old_attrs() Also rename attr_iter to old_attr and drop the declaration of old_attr within the body. Signed-off-by: Reid Wahl --- lib/common/xml.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 98f362291de..b2ba0191e8d 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1295,15 +1295,13 @@ mark_attr_moved(xmlNode *new_xml, const char *element, xmlAttr *old_attr, static void xml_diff_old_attrs(xmlNode *old_xml, xmlNode *new_xml) { - xmlAttr *attr_iter = pcmk__xe_first_attr(old_xml); + for (xmlAttr *old_attr = pcmk__xe_first_attr(old_xml); old_attr != NULL; + old_attr = old_attr->next) { - while (attr_iter != NULL) { - const char *name = (const char *) attr_iter->name; - xmlAttr *old_attr = attr_iter; - xmlAttr *new_attr = xmlHasProp(new_xml, attr_iter->name); - const char *old_value = pcmk__xml_attr_value(attr_iter); + const char *name = (const char *) old_attr->name; + const char *old_value = pcmk__xml_attr_value(old_attr); + xmlAttr *new_attr = xmlHasProp(new_xml, old_attr->name); - attr_iter = attr_iter->next; if (new_attr == NULL) { mark_attr_deleted(new_xml, (const char *) old_xml->name, name, old_value); From 659c1a93b1e73f649551ff521a43288d8162cc8b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 17:34:00 -0800 Subject: [PATCH 14/22] Refactor: libcrmcommon: Unindent else block in xml_diff_old_attrs() Signed-off-by: Reid Wahl --- lib/common/xml.c | 60 ++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index b2ba0191e8d..a52533f39fa 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1299,39 +1299,45 @@ xml_diff_old_attrs(xmlNode *old_xml, xmlNode *new_xml) old_attr = old_attr->next) { const char *name = (const char *) old_attr->name; - const char *old_value = pcmk__xml_attr_value(old_attr); + xmlAttr *new_attr = xmlHasProp(new_xml, old_attr->name); + xml_node_private_t *new_priv = NULL; + + const char *old_value = pcmk__xml_attr_value(old_attr); + const char *new_value = NULL; + + int old_pos = 0; + int new_pos = 0; if (new_attr == NULL) { mark_attr_deleted(new_xml, (const char *) old_xml->name, name, old_value); + continue; + } - } else { - xml_node_private_t *nodepriv = new_attr->_private; - int new_pos = pcmk__xml_position((xmlNode*) new_attr, - pcmk__xf_skip); - int old_pos = pcmk__xml_position((xmlNode*) old_attr, - pcmk__xf_skip); - const char *new_value = pcmk__xe_get(new_xml, name); - - // This attribute isn't new - pcmk__clear_xml_flags(nodepriv, pcmk__xf_created); - - if (strcmp(new_value, old_value) != 0) { - mark_attr_changed(new_xml, (const char *) old_xml->name, name, - old_value); - - } else if ((old_pos != new_pos) - && !pcmk__xml_doc_all_flags_set(new_xml->doc, - pcmk__xf_ignore_attr_pos - |pcmk__xf_tracking)) { - /* pcmk__xf_tracking is always set by pcmk__xml_mark_changes() - * before this function is called, so only the - * pcmk__xf_ignore_attr_pos check is truly relevant. - */ - mark_attr_moved(new_xml, (const char *) old_xml->name, - old_attr, new_attr, old_pos, new_pos); - } + new_priv = new_attr->_private; + new_value = pcmk__xe_get(new_xml, name); + + old_pos = pcmk__xml_position((xmlNode *) old_attr, pcmk__xf_skip); + new_pos = pcmk__xml_position((xmlNode *) new_attr, pcmk__xf_skip); + + // This attribute isn't new + pcmk__clear_xml_flags(new_priv, pcmk__xf_created); + + if (strcmp(new_value, old_value) != 0) { + mark_attr_changed(new_xml, (const char *) old_xml->name, name, + old_value); + + } else if ((old_pos != new_pos) + && !pcmk__xml_doc_all_flags_set(new_xml->doc, + pcmk__xf_ignore_attr_pos + |pcmk__xf_tracking)) { + /* pcmk__xf_tracking is always set by pcmk__xml_mark_changes() + * before this function is called, so only the + * pcmk__xf_ignore_attr_pos check is truly relevant. + */ + mark_attr_moved(new_xml, (const char *) old_xml->name, + old_attr, new_attr, old_pos, new_pos); } } } From 2a99e790b8e4f0b46fc0ddc1357002baeb8fe0bd Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 17:42:58 -0800 Subject: [PATCH 15/22] Refactor: libcrmcommon: Unindent a bit more of xml_diff_old_attrs() Signed-off-by: Reid Wahl --- lib/common/xml.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index a52533f39fa..0579567cdcd 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1318,27 +1318,31 @@ xml_diff_old_attrs(xmlNode *old_xml, xmlNode *new_xml) new_priv = new_attr->_private; new_value = pcmk__xe_get(new_xml, name); - old_pos = pcmk__xml_position((xmlNode *) old_attr, pcmk__xf_skip); - new_pos = pcmk__xml_position((xmlNode *) new_attr, pcmk__xf_skip); - // This attribute isn't new pcmk__clear_xml_flags(new_priv, pcmk__xf_created); - if (strcmp(new_value, old_value) != 0) { + if (!pcmk__str_eq(old_value, new_value, pcmk__str_none)) { mark_attr_changed(new_xml, (const char *) old_xml->name, name, old_value); + continue; + } - } else if ((old_pos != new_pos) - && !pcmk__xml_doc_all_flags_set(new_xml->doc, - pcmk__xf_ignore_attr_pos - |pcmk__xf_tracking)) { - /* pcmk__xf_tracking is always set by pcmk__xml_mark_changes() - * before this function is called, so only the - * pcmk__xf_ignore_attr_pos check is truly relevant. - */ - mark_attr_moved(new_xml, (const char *) old_xml->name, - old_attr, new_attr, old_pos, new_pos); + old_pos = pcmk__xml_position((xmlNode *) old_attr, pcmk__xf_skip); + new_pos = pcmk__xml_position((xmlNode *) new_attr, pcmk__xf_skip); + + /* pcmk__xf_tracking is always set by pcmk__xml_mark_changes() before + * this function is called, so only the pcmk__xf_ignore_attr_pos check + * is truly relevant. + */ + if ((old_pos == new_pos) + || pcmk__xml_doc_all_flags_set(new_xml->doc, + pcmk__xf_ignore_attr_pos + |pcmk__xf_tracking)) { + continue; } + + mark_attr_moved(new_xml, (const char *) old_xml->name, old_attr, + new_attr, old_pos, new_pos); } } From 9d269d6de239cc9514702b4ffcd41839bbbfebc8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 18:00:35 -0800 Subject: [PATCH 16/22] Refactor: libcrmcommon: pcmk__xe_foreach_attr() in xml_diff_old_attrs() More precisely, replace xml_diff_old_attrs(). Functionize the for loop body and call the new function through pcmk__xe_foreach_attr(). Signed-off-by: Reid Wahl --- lib/common/xml.c | 96 ++++++++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 44 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 0579567cdcd..62e43d15feb 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1287,63 +1287,71 @@ mark_attr_moved(xmlNode *new_xml, const char *element, xmlAttr *old_attr, /*! * \internal - * \brief Calculate differences in all previously existing XML attributes + * \brief Mark an XML attribute as deleted, changed, or moved if appropriate * - * \param[in,out] old_xml Original XML to compare - * \param[in,out] new_xml New XML to compare + * Given an attribute (from an old XML element) and a new XML element, check + * whether the attribute has been deleted, changed, or moved between the old and + * new elements. If so, mark the new XML element to indicate what changed. + * + * \param[in,out] old_attr XML attribute from old element + * \param[in,out] user_data New XML element + * + * \return \c true (to continue iterating) + * + * \note This is compatible with \c pcmk__xe_foreach_attr(). */ -static void -xml_diff_old_attrs(xmlNode *old_xml, xmlNode *new_xml) +static bool +mark_attr_diff(xmlAttr *old_attr, void *user_data) { - for (xmlAttr *old_attr = pcmk__xe_first_attr(old_xml); old_attr != NULL; - old_attr = old_attr->next) { - - const char *name = (const char *) old_attr->name; + xmlNode *old_xml = old_attr->parent; + xmlNode *new_xml = user_data; - xmlAttr *new_attr = xmlHasProp(new_xml, old_attr->name); - xml_node_private_t *new_priv = NULL; + const char *name = (const char *) old_attr->name; - const char *old_value = pcmk__xml_attr_value(old_attr); - const char *new_value = NULL; + xmlAttr *new_attr = xmlHasProp(new_xml, old_attr->name); + xml_node_private_t *new_priv = NULL; - int old_pos = 0; - int new_pos = 0; + const char *old_value = pcmk__xml_attr_value(old_attr); + const char *new_value = NULL; - if (new_attr == NULL) { - mark_attr_deleted(new_xml, (const char *) old_xml->name, name, - old_value); - continue; - } + int old_pos = 0; + int new_pos = 0; - new_priv = new_attr->_private; - new_value = pcmk__xe_get(new_xml, name); + if (new_attr == NULL) { + mark_attr_deleted(new_xml, (const char *) old_xml->name, name, + old_value); + return true; + } - // This attribute isn't new - pcmk__clear_xml_flags(new_priv, pcmk__xf_created); + new_priv = new_attr->_private; + new_value = pcmk__xe_get(new_xml, name); - if (!pcmk__str_eq(old_value, new_value, pcmk__str_none)) { - mark_attr_changed(new_xml, (const char *) old_xml->name, name, - old_value); - continue; - } + // This attribute isn't new + pcmk__clear_xml_flags(new_priv, pcmk__xf_created); - old_pos = pcmk__xml_position((xmlNode *) old_attr, pcmk__xf_skip); - new_pos = pcmk__xml_position((xmlNode *) new_attr, pcmk__xf_skip); + if (!pcmk__str_eq(old_value, new_value, pcmk__str_none)) { + mark_attr_changed(new_xml, (const char *) old_xml->name, name, + old_value); + return true; + } - /* pcmk__xf_tracking is always set by pcmk__xml_mark_changes() before - * this function is called, so only the pcmk__xf_ignore_attr_pos check - * is truly relevant. - */ - if ((old_pos == new_pos) - || pcmk__xml_doc_all_flags_set(new_xml->doc, - pcmk__xf_ignore_attr_pos - |pcmk__xf_tracking)) { - continue; - } + old_pos = pcmk__xml_position((xmlNode *) old_attr, pcmk__xf_skip); + new_pos = pcmk__xml_position((xmlNode *) new_attr, pcmk__xf_skip); - mark_attr_moved(new_xml, (const char *) old_xml->name, old_attr, - new_attr, old_pos, new_pos); + /* pcmk__xf_tracking is always set by pcmk__xml_mark_changes() before this + * function is called, so only the pcmk__xf_ignore_attr_pos check is truly + * relevant. + */ + if ((old_pos == new_pos) + || pcmk__xml_doc_all_flags_set(new_xml->doc, + pcmk__xf_ignore_attr_pos + |pcmk__xf_tracking)) { + return true; } + + mark_attr_moved(new_xml, (const char *) old_xml->name, old_attr, new_attr, + old_pos, new_pos); + return true; } /*! @@ -1402,7 +1410,7 @@ xml_diff_attrs(xmlNode *old_xml, xmlNode *new_xml) pcmk__set_xml_flags(nodepriv, pcmk__xf_created); } - xml_diff_old_attrs(old_xml, new_xml); + pcmk__xe_foreach_attr(old_xml, mark_attr_diff, new_xml); mark_created_attrs(new_xml); } From 7ba8a6f0f43b9a15c6566d7bc142426ea691d77f Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 18:02:36 -0800 Subject: [PATCH 17/22] Refactor: libcrmcommon: Drop redundant check from mark_attr_diff() Signed-off-by: Reid Wahl --- lib/common/xml.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 62e43d15feb..6b51aa10784 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1338,14 +1338,9 @@ mark_attr_diff(xmlAttr *old_attr, void *user_data) old_pos = pcmk__xml_position((xmlNode *) old_attr, pcmk__xf_skip); new_pos = pcmk__xml_position((xmlNode *) new_attr, pcmk__xf_skip); - /* pcmk__xf_tracking is always set by pcmk__xml_mark_changes() before this - * function is called, so only the pcmk__xf_ignore_attr_pos check is truly - * relevant. - */ if ((old_pos == new_pos) || pcmk__xml_doc_all_flags_set(new_xml->doc, - pcmk__xf_ignore_attr_pos - |pcmk__xf_tracking)) { + pcmk__xf_ignore_attr_pos)) { return true; } From 6c6e518e6561500ee35159ee5440abbb4ff1229b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 18:05:50 -0800 Subject: [PATCH 18/22] Refactor: libcrmcommon: Drop redundant args from mark_attr_*() functions We can assume that the old and new XML elements are of the same type. Signed-off-by: Reid Wahl --- lib/common/xml.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 6b51aa10784..b9d0a3e66dd 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1200,12 +1200,11 @@ pcmk__xml_escape(const char *text, enum pcmk__xml_escape_type type) * differences have been calculated. * * \param[in,out] new_xml XML to modify - * \param[in] element Name of XML element that changed (for logging) * \param[in] attr_name Name of attribute that was deleted * \param[in] old_value Value of attribute that was deleted */ static void -mark_attr_deleted(xmlNode *new_xml, const char *element, const char *attr_name, +mark_attr_deleted(xmlNode *new_xml, const char *attr_name, const char *old_value) { xml_doc_private_t *docpriv = new_xml->doc->_private; @@ -1228,7 +1227,7 @@ mark_attr_deleted(xmlNode *new_xml, const char *element, const char *attr_name, pcmk__xa_remove(attr, false); pcmk__trace("XML attribute %s=%s was removed from %s", attr_name, old_value, - element); + (const char *) new_xml->name); } /* @@ -1236,14 +1235,14 @@ mark_attr_deleted(xmlNode *new_xml, const char *element, const char *attr_name, * \brief Check ACLs for a changed XML attribute */ static void -mark_attr_changed(xmlNode *new_xml, const char *element, const char *attr_name, +mark_attr_changed(xmlNode *new_xml, const char *attr_name, const char *old_value) { xml_doc_private_t *docpriv = new_xml->doc->_private; char *vcopy = pcmk__xe_get_copy(new_xml, attr_name); pcmk__trace("XML attribute %s was changed from '%s' to '%s' in %s", - attr_name, old_value, vcopy, element); + attr_name, old_value, vcopy, (const char *) new_xml->name); // Restore the original value (without checking ACLs) pcmk__clear_xml_flags(docpriv, pcmk__xf_tracking); @@ -1260,20 +1259,19 @@ mark_attr_changed(xmlNode *new_xml, const char *element, const char *attr_name, * \brief Mark an XML attribute as having changed position * * \param[in,out] new_xml XML to modify - * \param[in] element Name of XML element that changed (for logging) * \param[in,out] old_attr Attribute that moved, in original XML * \param[in,out] new_attr Attribute that moved, in \p new_xml * \param[in] p_old Ordinal position of \p old_attr in original XML * \param[in] p_new Ordinal position of \p new_attr in \p new_xml */ static void -mark_attr_moved(xmlNode *new_xml, const char *element, xmlAttr *old_attr, - xmlAttr *new_attr, int p_old, int p_new) +mark_attr_moved(xmlNode *new_xml, xmlAttr *old_attr, xmlAttr *new_attr, + int p_old, int p_new) { xml_node_private_t *nodepriv = new_attr->_private; pcmk__trace("XML attribute %s moved from position %d to %d in %s", - old_attr->name, p_old, p_new, element); + old_attr->name, p_old, p_new, (const char *) new_xml->name); // Mark document, element, and all element's parents as changed pcmk__mark_xml_node_dirty(new_xml); @@ -1303,7 +1301,6 @@ mark_attr_moved(xmlNode *new_xml, const char *element, xmlAttr *old_attr, static bool mark_attr_diff(xmlAttr *old_attr, void *user_data) { - xmlNode *old_xml = old_attr->parent; xmlNode *new_xml = user_data; const char *name = (const char *) old_attr->name; @@ -1318,8 +1315,7 @@ mark_attr_diff(xmlAttr *old_attr, void *user_data) int new_pos = 0; if (new_attr == NULL) { - mark_attr_deleted(new_xml, (const char *) old_xml->name, name, - old_value); + mark_attr_deleted(new_xml, name, old_value); return true; } @@ -1330,8 +1326,7 @@ mark_attr_diff(xmlAttr *old_attr, void *user_data) pcmk__clear_xml_flags(new_priv, pcmk__xf_created); if (!pcmk__str_eq(old_value, new_value, pcmk__str_none)) { - mark_attr_changed(new_xml, (const char *) old_xml->name, name, - old_value); + mark_attr_changed(new_xml, name, old_value); return true; } @@ -1344,8 +1339,7 @@ mark_attr_diff(xmlAttr *old_attr, void *user_data) return true; } - mark_attr_moved(new_xml, (const char *) old_xml->name, old_attr, new_attr, - old_pos, new_pos); + mark_attr_moved(new_xml, old_attr, new_attr, old_pos, new_pos); return true; } From ec1359a812c6c78f63062ade09975f831c595c35 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 18:09:43 -0800 Subject: [PATCH 19/22] Refactor: libcrmcommon: Use for loop in mark_created_attrs() Also rename attr_iter to attr, rename attr_name to name, and drop new_attr variable. Signed-off-by: Reid Wahl --- lib/common/xml.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index b9d0a3e66dd..1642486770c 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1355,27 +1355,25 @@ mark_attr_diff(xmlAttr *old_attr, void *user_data) static void mark_created_attrs(xmlNode *new_xml) { - xmlAttr *attr_iter = pcmk__xe_first_attr(new_xml); + for (xmlAttr *attr = pcmk__xe_first_attr(new_xml); attr != NULL; + attr = attr->next) { - while (attr_iter != NULL) { - xmlAttr *new_attr = attr_iter; - xml_node_private_t *nodepriv = attr_iter->_private; + xml_node_private_t *nodepriv = attr->_private; - attr_iter = attr_iter->next; if (pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { - const char *attr_name = (const char *) new_attr->name; + const char *name = (const char *) attr->name; - pcmk__trace("Created new attribute %s=%s in %s", attr_name, - pcmk__xml_attr_value(new_attr), new_xml->name); + pcmk__trace("Created new attribute %s=%s in %s", name, + pcmk__xml_attr_value(attr), new_xml->name); /* Check ACLs (we can't use the remove-then-create trick because it * would modify the attribute position). */ - if (pcmk__check_acl(new_xml, attr_name, pcmk__xf_acl_write)) { - pcmk__mark_xml_attr_dirty(new_attr); + if (pcmk__check_acl(new_xml, name, pcmk__xf_acl_write)) { + pcmk__mark_xml_attr_dirty(attr); } else { // Creation was not allowed, so remove the attribute - pcmk__xa_remove(new_attr, true); + pcmk__xa_remove(attr, true); } } } From a5f814f22fa38be63aaa1bb8fb3a4c823225caaa Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 18:13:22 -0800 Subject: [PATCH 20/22] Refactor: libcrmcommon: Unindent most of for loop in mark_created_attrs Signed-off-by: Reid Wahl --- lib/common/xml.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 1642486770c..ef1ecaf4856 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1358,23 +1358,25 @@ mark_created_attrs(xmlNode *new_xml) for (xmlAttr *attr = pcmk__xe_first_attr(new_xml); attr != NULL; attr = attr->next) { + const char *name = (const char *) attr->name; xml_node_private_t *nodepriv = attr->_private; - if (pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { - const char *name = (const char *) attr->name; + if (!pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { + continue; + } - pcmk__trace("Created new attribute %s=%s in %s", name, - pcmk__xml_attr_value(attr), new_xml->name); + pcmk__trace("Created new attribute %s=%s in %s", name, + pcmk__xml_attr_value(attr), (const char *) new_xml->name); - /* Check ACLs (we can't use the remove-then-create trick because it - * would modify the attribute position). - */ - if (pcmk__check_acl(new_xml, name, pcmk__xf_acl_write)) { - pcmk__mark_xml_attr_dirty(attr); - } else { - // Creation was not allowed, so remove the attribute - pcmk__xa_remove(attr, true); - } + /* Check ACLs (we can't use the remove-then-create trick because it + * would modify the attribute position). + */ + if (pcmk__check_acl(new_xml, name, pcmk__xf_acl_write)) { + pcmk__mark_xml_attr_dirty(attr); + + } else { + // Creation was not allowed, so remove the attribute + pcmk__xa_remove(attr, true); } } } From ccec087f65696382aa13ecb0a074c8b2bac7310f Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 21:11:39 -0800 Subject: [PATCH 21/22] Refactor: libcrmcommon: pcmk__xe_foreach_attr() for mark_created_attrs() Replace mark_created_attrs(). Functionize the for loop body and call the new function through pcmk__xe_foreach_attr(). Signed-off-by: Reid Wahl --- lib/common/xml.c | 68 ++++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index ef1ecaf4856..0543281775b 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1345,40 +1345,52 @@ mark_attr_diff(xmlAttr *old_attr, void *user_data) /*! * \internal - * \brief Check all attributes in new XML for creation + * \brief Mark a new attribute dirty if ACLs allow creation, or remove otherwise * - * For each of a given XML element's attributes marked as newly created, accept - * (and mark as dirty) or reject the creation according to ACLs. + * We set the \c pcmk__xf_created flag on all attributes in the new XML at an + * earlier stage of change calculation. Then we checked whether each attribute + * was present in the old XML, and we cleared the flag if so. If the flag is + * still set, then the attribute is truly new. * - * \param[in,out] new_xml XML to check + * Now we check whether ACLs allow the attribute's creation. If so, we "accept" + * it: we mark the attribute as dirty and modified, and we mark all of its + * parents as dirty. Otherwise, we reject it by removing the attribute (ignoring + * ACLs and change tracking for the removal). + * + * \param[in,out] attr XML attribute to mark dirty or remove + * \param[in] user_data Ignored + * + * \return \c true (to continue iterating) + * + * \note This is compatible with \c pcmk__xe_foreach_attr(). */ -static void -mark_created_attrs(xmlNode *new_xml) +static bool +check_new_attr_acls(xmlAttr *attr, void *user_data) { - for (xmlAttr *attr = pcmk__xe_first_attr(new_xml); attr != NULL; - attr = attr->next) { - - const char *name = (const char *) attr->name; - xml_node_private_t *nodepriv = attr->_private; + const char *name = (const char *) attr->name; + const char *value = pcmk__xml_attr_value(attr); + const xml_node_private_t *nodepriv = attr->_private; + xmlNode *new_xml = attr->parent; + const char *new_xml_id = pcmk__s(pcmk__xe_id(new_xml), "without ID"); - if (!pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { - continue; - } - - pcmk__trace("Created new attribute %s=%s in %s", name, - pcmk__xml_attr_value(attr), (const char *) new_xml->name); - - /* Check ACLs (we can't use the remove-then-create trick because it - * would modify the attribute position). - */ - if (pcmk__check_acl(new_xml, name, pcmk__xf_acl_write)) { - pcmk__mark_xml_attr_dirty(attr); + if (!pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { + return true; + } - } else { - // Creation was not allowed, so remove the attribute - pcmk__xa_remove(attr, true); - } + /* Check ACLs (we can't use the remove-then-create trick because it + * would modify the attribute position). + */ + if (!pcmk__check_acl(new_xml, name, pcmk__xf_acl_write)) { + pcmk__trace("ACLs prevent creation of attribute %s=%s in %s %s", name, + value, (const char *) new_xml->name, new_xml_id); + pcmk__xa_remove(attr, true); + return true; } + + pcmk__trace("Created new attribute %s=%s in %s %s", name, value, + (const char *) new_xml->name, new_xml_id); + pcmk__mark_xml_attr_dirty(attr); + return true; } /*! @@ -1400,7 +1412,7 @@ xml_diff_attrs(xmlNode *old_xml, xmlNode *new_xml) } pcmk__xe_foreach_attr(old_xml, mark_attr_diff, new_xml); - mark_created_attrs(new_xml); + pcmk__xe_foreach_attr(new_xml, check_new_attr_acls, NULL); } /*! From 336a189ed2821ebd552278781cc7e6ece3e23374 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 21:14:45 -0800 Subject: [PATCH 22/22] Refactor: libcrmcommon: New mark_attr_created() Use with pcmk__xe_foreach_attr(). Signed-off-by: Reid Wahl --- lib/common/xml.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 0543281775b..8ad4e190ec8 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1190,6 +1190,26 @@ pcmk__xml_escape(const char *text, enum pcmk__xml_escape_type type) return g_string_free(copy, FALSE); } +/*! + * \internal + * \brief Set the \c pcmk__xf_created flag on an attribute + * + * \param[in,out] attr XML attribute + * \param[in] user_data Ignored + * + * \return \c true (to continue iterating) + * + * \note This is compatible with \c pcmk__xe_foreach_attr(). + */ +static bool +mark_attr_created(xmlAttr *attr, void *user_data) +{ + xml_node_private_t *nodepriv = attr->_private; + + pcmk__set_xml_flags(nodepriv, pcmk__xf_created); + return true; +} + /*! * \internal * \brief Add an XML attribute to a node, marked as deleted @@ -1404,12 +1424,7 @@ static void xml_diff_attrs(xmlNode *old_xml, xmlNode *new_xml) { // Cleared later if attributes are not really new - for (xmlAttr *attr = pcmk__xe_first_attr(new_xml); attr != NULL; - attr = attr->next) { - xml_node_private_t *nodepriv = attr->_private; - - pcmk__set_xml_flags(nodepriv, pcmk__xf_created); - } + pcmk__xe_foreach_attr(new_xml, mark_attr_created, NULL); pcmk__xe_foreach_attr(old_xml, mark_attr_diff, new_xml); pcmk__xe_foreach_attr(new_xml, check_new_attr_acls, NULL);