Merge pull request #4124 from nrwahl2/nrwahl2-acls_first

Refactor: libcrmcommon: Use XML attribute foreach functions in more places

Author: clumens

include/crm/common/internal.h

@@ -60,6 +60,7 @@
 #include <crm/common/servers_internal.h>
 #include <crm/common/tls_internal.h>
 #include <crm/common/utils_internal.h>
+// xml_attr_internal.h intentionally left out
 // xml_comment_internal.h intentionally left out
 // xml_element_internal.h intentionally left out
 // xml_idref_internal.h intentionally left out

include/crm/common/xml_attr_internal.h

@@ -0,0 +1,37 @@
+/*
+ * Copyright 2025-2026 the Pacemaker project contributors
+ *
+ * The version control history for this file may have further details.
+ *
+ * This source code is licensed under the GNU Lesser General Public License
+ * version 2.1 or later (LGPLv2.1+) WITHOUT ANY WARRANTY.
+ */
+
+#ifndef PCMK__INCLUDED_CRM_COMMON_INTERNAL_H
+#error "Include <crm/common/internal.h> instead of <xml_attr_internal.h> "  \
+       "directly"
+#endif
+
+#ifndef PCMK__CRM_COMMON_XML_ATTR_INTERNAL__H
+#define PCMK__CRM_COMMON_XML_ATTR_INTERNAL__H
+
+/*
+ * Internal-only wrappers for and extensions to libxml2 for processing XML
+ * attributes
+ */
+
+#include <stdbool.h>        // bool
+
+#include <libxml/tree.h>    // xmlAttr
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+bool pcmk__xa_insert_dup(const xmlAttr *attr, void *user_data);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif  // PCMK__XML_ATTR_INTERNAL__H

include/crm/common/xml_element_internal.h

@@ -48,7 +48,7 @@ xmlNode *pcmk__xe_first_child(const xmlNode *parent, const char *node_name,
 void pcmk__xe_remove_attr(xmlNode *element, const char *name);
 bool pcmk__xe_remove_attr_cb(xmlNode *xml, void *user_data);
 void pcmk__xe_remove_matching_attrs(xmlNode *element, bool force,
-                                    bool (*match)(xmlAttrPtr, void *),
+                                    bool (*match)(xmlAttr *, void *),
                                     void *user_data);
 int pcmk__xe_delete_match(xmlNode *xml, xmlNode *search);
 int pcmk__xe_replace_match(xmlNode *xml, xmlNode *replace);

include/crm/common/xml_internal.h

@@ -28,6 +28,7 @@
 #include <crm/common/xml_names.h>             // PCMK_XA_ID, PCMK_XE_CLONE
 
 // This file is a wrapper for other {xml_*,xpath}_internal.h headers
+#include <crm/common/xml_attr_internal.h>
 #include <crm/common/xml_comment_internal.h>
 #include <crm/common/xml_element_internal.h>
 #include <crm/common/xml_idref_internal.h>
@@ -432,11 +433,23 @@ void pcmk__xml_mark_changes(xmlNode *old_xml, xmlNode *new_xml);
 bool pcmk__xml_tree_foreach(xmlNode *xml, bool (*fn)(xmlNode *, void *),
                             void *user_data);
 
+/*!
+ * \internal
+ * \brief Get an XML attribute's value
+ *
+ * \param[in] attr  XML attribute
+ *
+ * \return Value of \p attr, or \c NULL if \p attr is \c NULL or its value is
+ *         unset
+ */
 static inline const char *
 pcmk__xml_attr_value(const xmlAttr *attr)
 {
-    return ((attr == NULL) || (attr->children == NULL))? NULL
-           : (const char *) attr->children->content;
+    if ((attr == NULL) || (attr->children == NULL)) {
+        return NULL;
+    }
+
+    return (const char *) attr->children->content;
 }
 
 void pcmk__xml_patchset_add_digest(xmlNode *patchset, const xmlNode *target);

lib/common/acl.c

@@ -788,6 +788,24 @@ is_mode_allowed(uint32_t flags, enum pcmk__xml_flags mode)
     }
 }
 
+/*!
+ * \internal
+ * \brief Check whether an XML attribute's name is \c PCMK_XA_ID
+ *
+ * \param[in] attr       Attribute to check
+ * \param[in] user_data  Ignored
+ *
+ * \return \c true if the attribute's name is \c PCMK_XA_ID, or \c false
+ *         otherwise
+ *
+ * \note This is compatible with \c pcmk__xe_foreach_const_attr().
+ */
+static bool
+attr_is_id(const xmlAttr *attr, void *user_data)
+{
+    return pcmk__str_eq((const char *) attr->name, PCMK_XA_ID, pcmk__str_none);
+}
+
 /*!
  * \internal
  * \brief Check whether an XML attribute's name is not \c PCMK_XA_ID
@@ -803,7 +821,7 @@ is_mode_allowed(uint32_t flags, enum pcmk__xml_flags mode)
 static bool
 attr_is_not_id(xmlAttr *attr, void *user_data)
 {
-    return !pcmk__str_eq((const char *) attr->name, PCMK_XA_ID, pcmk__str_none);
+    return !attr_is_id(attr, user_data);
 }
 
 /*!
@@ -952,14 +970,10 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml,
  * \return \c true if XML element is implicitly allowed, or \c false otherwise
  */
 static bool
-implicitly_allowed(xmlNode *xml)
+implicitly_allowed(const xmlNode *xml)
 {
-    for (xmlAttr *attr = pcmk__xe_first_attr(xml); attr != NULL;
-         attr = attr->next) {
-
-        if (attr_is_not_id(attr, NULL)) {
-            return false;
-        }
+    if (!pcmk__xe_foreach_const_attr(xml, attr_is_id, NULL)) {
+        return false;
     }
 
     /* Creation is not implicitly allowed for a descendant of PCMK_XE_ACLS, but

lib/common/crmcommon_private.h

@@ -176,7 +176,7 @@ G_GNUC_INTERNAL
 bool pcmk__marked_as_deleted(xmlAttrPtr a, void *user_data);
 
 G_GNUC_INTERNAL
-void pcmk__dump_xml_attr(const xmlAttr *attr, GString *buffer);
+bool pcmk__dump_xml_attr(const xmlAttr *attr, void *user_data);
 
 G_GNUC_INTERNAL
 int pcmk__xe_set_score(xmlNode *target, const char *name, const char *value);

lib/common/nvpair.c

@@ -340,7 +340,6 @@ GHashTable *
 xml2list(const xmlNode *parent)
 {
     xmlNode *child = NULL;
-    xmlAttrPtr pIter = NULL;
     xmlNode *nvpair_list = NULL;
     GHashTable *nvpair_hash = pcmk__strkey_table(free, free);
 
@@ -354,16 +353,7 @@ xml2list(const xmlNode *parent)
 
     pcmk__log_xml_trace(nvpair_list, "Unpacking");
 
-    for (pIter = pcmk__xe_first_attr(nvpair_list); pIter != NULL;
-         pIter = pIter->next) {
-
-        const char *p_name = (const char *)pIter->name;
-        const char *p_value = pcmk__xml_attr_value(pIter);
-
-        pcmk__trace("Added %s=%s", p_name, p_value);
-
-        pcmk__insert_dup(nvpair_hash, p_name, p_value);
-    }
+    pcmk__xe_foreach_const_attr(nvpair_list, pcmk__xa_insert_dup, nvpair_hash);
 
     for (child = pcmk__xe_first_child(nvpair_list, PCMK__XE_PARAM, NULL, NULL);
          child != NULL; child = pcmk__xe_next(child, PCMK__XE_PARAM)) {