From 2c91b4a54d5b70204409ea2f7e92fbc7748e4257 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 17 Mar 2017 16:32:45 -0500

Subject: [PATCH 1/5] Log: various: minor improvements

add missing newline, reword to avoid singular/plural issue,

and correct misspellings (in log messages and comments)

---

 configure.ac                 | 2 +-

 crmd/join_dc.c               | 2 +-

 crmd/lrm.c                   | 4 ++--

 fencing/remote.c             | 4 ++--

 include/crm/pengine/status.h | 8 ++++----

 lib/cluster/election.c       | 2 +-

 lib/common/xml.c             | 2 +-

 lib/pengine/utils.c          | 4 ++--

 lrmd/lrmd.c                  | 5 +++--

 pengine/graph.c              | 2 +-

 pengine/native.c             | 2 +-

 tools/fake_transition.c      | 4 ++--

 12 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/configure.ac b/configure.ac

index b1465a3..687d3fe 100644

--- a/configure.ac

+++ b/configure.ac

@@ -1759,7 +1759,7 @@ if test "$GCC" != yes; then

 else

         CFLAGS="$CFLAGS -ggdb"

-dnl when we don't have diagnostic push / pull we can't explicitely disable

+dnl When we don't have diagnostic push / pull, we can't explicitly disable

 dnl checking for nonliteral formats in the places where they occur on purpose

 dnl thus we disable nonliteral format checking globally as we are aborting

 dnl on warnings. 

diff --git a/crmd/join_dc.c b/crmd/join_dc.c

index 6af0884..d5ecc55 100644

--- a/crmd/join_dc.c

+++ b/crmd/join_dc.c

@@ -385,7 +385,7 @@ do_dc_join_finalize(long long action,

     /* This we can do straight away and avoid clients timing us out

      *  while we compute the latest CIB

      */

-    crm_debug("Finializing join-%d for %d clients",

+    crm_debug("Finalizing join-%d for %d clients",

               current_join_id, crmd_join_phase_count(crm_join_integrated));

     crmd_join_phase_log(LOG_INFO);

diff --git a/crmd/lrm.c b/crmd/lrm.c

index 2d553cd..f7aefbf 100644

--- a/crmd/lrm.c

+++ b/crmd/lrm.c

@@ -231,8 +231,8 @@ update_history_cache(lrm_state_t * lrm_state, lrmd_rsc_info_t * rsc, lrmd_event_

         }

     } else if (did_rsc_op_fail(op, target_rc)) {

-        /* We must store failed monitors here

-         * - otherwise the block below will cause them to be forgetten them when a stop happens

+        /* Store failed monitors here, otherwise the block below will cause them

+         * to be forgotten when a stop happens.

          */

         if (entry->failed) {

             lrmd_free_event(entry->failed);

diff --git a/fencing/remote.c b/fencing/remote.c

index 6c47f65..4a47d49 100644

--- a/fencing/remote.c

+++ b/fencing/remote.c

@@ -1558,13 +1558,13 @@ call_remote_stonith(remote_fencing_op_t * op, st_query_result_t * peer)

         }

         if (op->state == st_query) {

-           crm_info("None of the %d peers have devices capable of fencing (%s) %s for %s (%d)",

+           crm_info("No peers (out of %d) have devices capable of fencing (%s) %s for %s (%d)",

                    op->replies, op->action, op->target, op->client_name,

                    op->state);

             rc = -ENODEV;

         } else {

-           crm_info("None of the %d peers are capable of fencing (%s) %s for %s (%d)",

+           crm_info("No peers (out of %d) are capable of fencing (%s) %s for %s (%d)",

                    op->replies, op->action, op->target, op->client_name,

                    op->state);

         }

diff --git a/include/crm/pengine/status.h b/include/crm/pengine/status.h

index 79e4572..e748792 100644

--- a/include/crm/pengine/status.h

+++ b/include/crm/pengine/status.h

@@ -343,8 +343,8 @@ struct pe_action_s {

      * requires at minimum X number of cloned instances to be running

      * before an order dependency can run. Another option that uses

      * this is 'require-all=false' in ordering constrants. This option

-     * says "only required one instance of a resource to start before

-     * allowing dependencies to start" basicall require-all=false is

+     * says "only require one instance of a resource to start before

+     * allowing dependencies to start" -- basically, require-all=false is

      * the same as clone-min=1.

      */

@@ -354,8 +354,8 @@ struct pe_action_s {

      * to be considered runnable */ 

     int required_runnable_before;

-    GListPtr actions_before;    /* action_warpper_t* */

-    GListPtr actions_after;     /* action_warpper_t* */

+    GListPtr actions_before;    /* action_wrapper_t* */

+    GListPtr actions_after;     /* action_wrapper_t* */

 };

 struct ticket_s {

diff --git a/lib/cluster/election.c b/lib/cluster/election.c

index a8902d3..a5bb1da 100644

--- a/lib/cluster/election.c

+++ b/lib/cluster/election.c

@@ -461,7 +461,7 @@ election_count_vote(election_t *e, xmlNode *vote, bool can_win)

         int peers = 1 + g_hash_table_size(crm_peer_cache);

         /* If every node has to vote down every other node, thats N*(N-1) total elections

-         * Allow some leway before _really_ complaining

+         * Allow some leeway before _really_ complaining

          */

         election_wins++;

         if (election_wins > (peers * peers)) {

diff --git a/lib/common/xml.c b/lib/common/xml.c

index 6dce4cb..c566956 100644

--- a/lib/common/xml.c

+++ b/lib/common/xml.c

@@ -3190,7 +3190,7 @@ crm_xml_escape(const char *text)

      * converted back to their escape sequences.

      *

      * However xmlNodeDump() is randomly dog slow, even with the same

-     * input. So we need to replicate the escapeing in our custom

+     * input. So we need to replicate the escaping in our custom

      * version so that the result can be re-parsed by xmlCtxtReadDoc()

      * when necessary.

      */

diff --git a/lib/pengine/utils.c b/lib/pengine/utils.c

index 2b53999..74856fe 100644

--- a/lib/pengine/utils.c

+++ b/lib/pengine/utils.c

@@ -1099,8 +1099,8 @@ pe_free_action(action_t * action)

     if (action == NULL) {

         return;

     }

-    g_list_free_full(action->actions_before, free);     /* action_warpper_t* */

-    g_list_free_full(action->actions_after, free);      /* action_warpper_t* */

+    g_list_free_full(action->actions_before, free);     /* action_wrapper_t* */

+    g_list_free_full(action->actions_after, free);      /* action_wrapper_t* */

     if (action->extra) {

         g_hash_table_destroy(action->extra);

     }

diff --git a/lrmd/lrmd.c b/lrmd/lrmd.c

index 5669d34..c874aa5 100644

--- a/lrmd/lrmd.c

+++ b/lrmd/lrmd.c

@@ -1464,9 +1464,10 @@ free_rsc(gpointer data)

         if (is_stonith) {

             cmd->lrmd_op_status = PCMK_LRM_OP_CANCELLED;

-            /* if a stonith cmd is in-flight, mark just mark it as cancelled,

+            /* If a stonith command is in-flight, just mark it as cancelled;

              * it is not safe to finalize/free the cmd until the stonith api

-             * says it has either completed or timed out.*/ 

+             * says it has either completed or timed out.

+             */

             if (rsc->active != cmd) {

                 cmd_finalize(cmd, NULL);

             }

diff --git a/pengine/graph.c b/pengine/graph.c

index 81d8355..8098f9e 100644

--- a/pengine/graph.c

+++ b/pengine/graph.c

@@ -1005,7 +1005,7 @@ action2xml(action_t * action, gboolean as_input, pe_working_set_t *data_set)

                  *

                  * If anyone toggles the unique flag to 'on', the

                  * 'instance free' name will correspond to an orphan

-                 * and fall into the claus above instead

+                 * and fall into the clause above instead

                  */

                 crm_xml_add(rsc_xml, XML_ATTR_ID, xml_id);

                 if (action->rsc->clone_name && safe_str_neq(xml_id, action->rsc->clone_name)) {

diff --git a/pengine/native.c b/pengine/native.c

index 52eba4f..a968fc7 100644

--- a/pengine/native.c

+++ b/pengine/native.c

@@ -1647,7 +1647,7 @@ colocation_match(resource_t * rsc_lh, resource_t * rsc_rh, rsc_colocation_t * co

     } else if (constraint->score < 0) {

         /* nothing to do:

-         *   anti-colocation with something thats not running

+         *   anti-colocation with something that is not running

          */

         return;

     }

diff --git a/tools/fake_transition.c b/tools/fake_transition.c

index 251f9bb..26f55a2 100644

--- a/tools/fake_transition.c

+++ b/tools/fake_transition.c

@@ -70,8 +70,8 @@ inject_transient_attr(xmlNode * cib_node, const char *name, const char *value)

     char *nvp_id = crm_concat(name, node_uuid, '-');

     node_path = xmlGetNodePath(cib_node);

-    quiet_log("Injecting attribute %s=%s into %s '%s'", name, value, node_path,

-             ID(cib_node));

+    quiet_log(" + Injecting attribute %s=%s into %s '%s'\n",

+              name, value, node_path, ID(cib_node));

     free(node_path);

     attrs = first_named_child(cib_node, XML_TAG_TRANSIENT_NODEATTRS);

-- 

1.8.3.1

From 9251c17d97250103679aada63729d54a9537874f Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 17 Mar 2017 16:58:48 -0500

Subject: [PATCH 2/5] Log: libcrmcluster,crmd: log join phase as text rather

 than integer

---

 crmd/join_dc.c        | 51 +++++++++++++++------------------------------------

 include/crm/cluster.h | 14 ++++++++++++++

 2 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/crmd/join_dc.c b/crmd/join_dc.c

index d5ecc55..71311de 100644

--- a/crmd/join_dc.c

+++ b/crmd/join_dc.c

@@ -45,8 +45,9 @@ crm_update_peer_join(const char *source, crm_node_t * node, enum crm_join_phase

     enum crm_join_phase last = 0;

     if(node == NULL) {

-        crm_err("Could not update join because node not specified" CRM_XS

-                " join-%u source=%s phase=%d", source, current_join_id, phase);

+        crm_err("Could not update join because node not specified"

+                CRM_XS " join-%u source=%s phase=%s",

+                current_join_id, source, crm_join_phase_str(phase));

         return;

     }

@@ -58,23 +59,21 @@ crm_update_peer_join(const char *source, crm_node_t * node, enum crm_join_phase

     last = node->join;

     if(phase == last) {

-        crm_trace("%s: Node %s[%u] - join-%u phase still %u",

-                  source, node->uname, node->id, current_join_id, last);

+        crm_trace("%s: Node %s[%u] - join-%u phase still %s",

+                  source, node->uname, node->id, current_join_id,

+                  crm_join_phase_str(last));

-    } else if (phase <= crm_join_none) {

+    } else if ((phase <= crm_join_none) || (phase == (last + 1))) {

         node->join = phase;

-        crm_info("%s: Node %s[%u] - join-%u phase %u -> %u",

-                 source, node->uname, node->id, current_join_id, last, phase);

+        crm_info("%s: Node %s[%u] - join-%u phase %s -> %s",

+                 source, node->uname, node->id, current_join_id,

+                 crm_join_phase_str(last), crm_join_phase_str(phase));

-    } else if(phase == last + 1) {

-        node->join = phase;

-        crm_info("%s: Node %s[%u] - join-%u phase %u -> %u",

-                 source, node->uname, node->id, current_join_id, last, phase);

     } else {

         crm_err("Could not update join for node %s because phase transition invalid "

-                CRM_XS " join-%u source=%s node_id=%u last=%u new=%u",

-                node->uname, current_join_id, source, node->id, last, phase);

-

+                CRM_XS " join-%u source=%s node_id=%u last=%s new=%s",

+                node->uname, current_join_id, source, node->id,

+                crm_join_phase_str(last), crm_join_phase_str(phase));

     }

 }

@@ -691,27 +690,7 @@ void crmd_join_phase_log(int level)

     g_hash_table_iter_init(&iter, crm_peer_cache);

     while (g_hash_table_iter_next(&iter, NULL, (gpointer *) &peer)) {

-        const char *state = "unknown";

-        switch(peer->join) {

-            case crm_join_nack:

-                state = "nack";

-                break;

-            case crm_join_none:

-                state = "none";

-                break;

-            case crm_join_welcomed:

-                state = "welcomed";

-                break;

-            case crm_join_integrated:

-                state = "integrated";

-                break;

-            case crm_join_finalized:

-                state = "finalized";

-                break;

-            case crm_join_confirmed:

-                state = "confirmed";

-                break;

-        }

-        do_crm_log(level, "join-%d: %s=%s", current_join_id, peer->uname, state);

+        do_crm_log(level, "join-%d: %s=%s", current_join_id, peer->uname,

+                   crm_join_phase_str(peer->join));

     }

 }

diff --git a/include/crm/cluster.h b/include/crm/cluster.h

index 27ee9eb..343ce08 100644

--- a/include/crm/cluster.h

+++ b/include/crm/cluster.h

@@ -235,4 +235,18 @@ char *pcmk_message_common_cs(cpg_handle_t handle, uint32_t nodeid, uint32_t pid,

                         uint32_t *kind, const char **from);

 #  endif

+static inline const char *

+crm_join_phase_str(enum crm_join_phase phase)

+{

+    switch (phase) {

+        case crm_join_nack:         return "nack";

+        case crm_join_none:         return "none";

+        case crm_join_welcomed:     return "welcomed";

+        case crm_join_integrated:   return "integrated";

+        case crm_join_finalized:    return "finalized";

+        case crm_join_confirmed:    return "confirmed";

+    }

+    return "invalid";

+}

+

 #endif

-- 

1.8.3.1

From dc977b614de9ce4018fa44c4f00c2b6b43c65c75 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 17 Mar 2017 17:23:22 -0500

Subject: [PATCH 3/5] Feature: libcrmcommon: add convenience functions for

 managing XML IDs

These handle ID sanitization and memory management for callers,

reducing the chance for mistakes.

---

 include/crm/common/xml.h |  4 ++++

 lib/common/xml.c         | 45 +++++++++++++++++++++++++++++++++++++++++++++

 2 files changed, 49 insertions(+)

diff --git a/include/crm/common/xml.h b/include/crm/common/xml.h

index a948915..f896464 100644

--- a/include/crm/common/xml.h

+++ b/include/crm/common/xml.h

@@ -373,4 +373,8 @@ void save_xml_to_file(xmlNode * xml, const char *desc, const char *filename);

 char *xml_get_path(xmlNode *xml);

 char * crm_xml_escape(const char *text);

+void crm_xml_sanitize_id(char *id);

+void crm_xml_set_id(xmlNode *xml, const char *format, ...)

+    __attribute__ ((__format__ (__printf__, 2, 3)));

+

 #endif

diff --git a/lib/common/xml.c b/lib/common/xml.c

index c566956..0ea33d9 100644

--- a/lib/common/xml.c

+++ b/lib/common/xml.c

@@ -3036,6 +3036,51 @@ crm_xml_add_last_written(xmlNode *xml_node)

     return crm_xml_add(xml_node, XML_CIB_ATTR_WRITTEN, now_str);

 }

+/*!

+ * \brief Sanitize a string so it is usable as an XML ID

+ *

+ * \param[in,out] id  String to sanitize

+ */

+void

+crm_xml_sanitize_id(char *id)

+{

+    char *c;

+

+    for (c = id; *c; ++c) {

+        /* @TODO Sanitize more comprehensively */

+        switch (*c) {

+            case ':':

+            case '#':

+                *c = '.';

+        }

+    }

+}

+

+/*!

+ * \brief Set the ID of an XML element using a format

+ *

+ * \param[in,out] xml  XML element

+ * \param[in]     fmt  printf-style format

+ * \param[in]     ...  any arguments required by format

+ */

+void

+crm_xml_set_id(xmlNode *xml, const char *format, ...)

+{

+    va_list ap;

+    int len = 0;

+    char *id = NULL;

+

+    /* equivalent to crm_strdup_printf() */

+    va_start(ap, format);

+    len = vasprintf(&id, format, ap);

+    va_end(ap);

+    CRM_ASSERT(len > 0);

+

+    crm_xml_sanitize_id(id);

+    crm_xml_add(xml, XML_ATTR_ID, id);

+    free(id);

+}

+

 static int

 write_xml_stream(xmlNode * xml_node, const char *filename, FILE * stream, gboolean compress)

 {

-- 

1.8.3.1

From bf39184154405621a07ce05a201fded861db2357 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 17 Mar 2017 18:21:05 -0500

Subject: [PATCH 4/5] Refactor: various: use new convenience functions for

 generating XML IDs

---

 attrd/commands.c         | 41 +++++++++++-------------------------

 crmd/pengine.c           |  8 +++----

 lib/cluster/corosync.c   |  5 +----

 lib/pengine/unpack.c     | 37 ++++++++-------------------------

 tools/crm_node.c         | 12 +++--------

 tools/crm_resource_ban.c | 54 +++++++++++-------------------------------------

 tools/fake_transition.c  |  5 +----

 7 files changed, 41 insertions(+), 121 deletions(-)

diff --git a/attrd/commands.c b/attrd/commands.c

index 486efb6..98b4215 100644

--- a/attrd/commands.c

+++ b/attrd/commands.c

@@ -928,31 +928,9 @@ write_attributes(bool all, bool peer_discovered)

 static void

 build_update_element(xmlNode *parent, attribute_t *a, const char *nodeid, const char *value)

 {

-    char *set = NULL;

-    char *uuid = NULL;

+    const char *set = NULL;

     xmlNode *xml_obj = NULL;

-    if(a->set) {

-        set = strdup(a->set);

-    } else {

-        set = crm_strdup_printf("%s-%s", XML_CIB_TAG_STATUS, nodeid);

-    }

-

-    if(a->uuid) {

-        uuid = strdup(a->uuid);

-    } else {

-        int lpc;

-        uuid = crm_strdup_printf("%s-%s", set, a->id);

-

-        /* Minimal attempt at sanitizing automatic IDs */

-        for (lpc = 0; uuid[lpc] != 0; lpc++) {

-            switch (uuid[lpc]) {

-                case ':':

-                    uuid[lpc] = '.';

-            }

-        }

-    }

-

     xml_obj = create_xml_node(parent, XML_CIB_TAG_STATE);

     crm_xml_add(xml_obj, XML_ATTR_ID, nodeid);

@@ -960,10 +938,19 @@ build_update_element(xmlNode *parent, attribute_t *a, const char *nodeid, const

     crm_xml_add(xml_obj, XML_ATTR_ID, nodeid);

     xml_obj = create_xml_node(xml_obj, XML_TAG_ATTR_SETS);

-    crm_xml_add(xml_obj, XML_ATTR_ID, set);

+    if (a->set) {

+        crm_xml_set_id(xml_obj, "%s", a->set);

+    } else {

+        crm_xml_set_id(xml_obj, "%s-%s", XML_CIB_TAG_STATUS, nodeid);

+    }

+    set = ID(xml_obj);

     xml_obj = create_xml_node(xml_obj, XML_CIB_TAG_NVPAIR);

-    crm_xml_add(xml_obj, XML_ATTR_ID, uuid);

+    if (a->uuid) {

+        crm_xml_set_id(xml_obj, "%s", a->uuid);

+    } else {

+        crm_xml_set_id(xml_obj, "%s-%s", set, a->id);

+    }

     crm_xml_add(xml_obj, XML_NVPAIR_ATTR_NAME, a->id);

     if(value) {

@@ -973,9 +960,6 @@ build_update_element(xmlNode *parent, attribute_t *a, const char *nodeid, const

         crm_xml_add(xml_obj, XML_NVPAIR_ATTR_VALUE, "");

         crm_xml_add(xml_obj, "__delete__", XML_NVPAIR_ATTR_VALUE);

     }

-

-    free(uuid);

-    free(set);

 }

 void

diff --git a/crmd/pengine.c b/crmd/pengine.c

index 5a301c5..17613cf 100644

--- a/crmd/pengine.c

+++ b/crmd/pengine.c

@@ -264,12 +264,12 @@ force_local_option(xmlNode *xml, const char *attr_name, const char *attr_value)

     }

     if(max == 0) {

-        char *attr_id = crm_concat(CIB_OPTIONS_FIRST, attr_name, '-');

         xmlNode *configuration = NULL;

         xmlNode *crm_config = NULL;

         xmlNode *cluster_property_set = NULL;

-        crm_trace("Creating %s/%s = %s", attr_id, attr_name, attr_value);

+        crm_trace("Creating %s-%s for %s=%s",

+                  CIB_OPTIONS_FIRST, attr_name, attr_name, attr_value);

         configuration = find_entity(xml, XML_CIB_TAG_CONFIGURATION, NULL);

         if (configuration == NULL) {

@@ -289,11 +289,9 @@ force_local_option(xmlNode *xml, const char *attr_name, const char *attr_value)

         xml = create_xml_node(cluster_property_set, XML_CIB_TAG_NVPAIR);

-        crm_xml_add(xml, XML_ATTR_ID, attr_id);

+        crm_xml_set_id(xml, "%s-%s", CIB_OPTIONS_FIRST, attr_name);

         crm_xml_add(xml, XML_NVPAIR_ATTR_NAME, attr_name);

         crm_xml_add(xml, XML_NVPAIR_ATTR_VALUE, attr_value);

-

-        free(attr_id);

     }

     freeXpathObject(xpathObj);

 }

diff --git a/lib/cluster/corosync.c b/lib/cluster/corosync.c

index b02ae73..dfc781c 100644

--- a/lib/cluster/corosync.c

+++ b/lib/cluster/corosync.c

@@ -548,12 +548,9 @@ corosync_initialize_nodelist(void *cluster, gboolean force_member, xmlNode * xml

             any = TRUE;

             if (xml_parent) {

-                char buffer[64];

                 xmlNode *node = create_xml_node(xml_parent, XML_CIB_TAG_NODE);

-                if(snprintf(buffer, 63, "%u", nodeid) > 0) {

-                    crm_xml_add(node, XML_ATTR_ID, buffer);

-                }

+                crm_xml_set_id(node, "%u", nodeid);

                 crm_xml_add(node, XML_ATTR_UNAME, name);

                 if (force_member) {

                     crm_xml_add(node, XML_ATTR_TYPE, CRM_NODE_MEMBER);

diff --git a/lib/pengine/unpack.c b/lib/pengine/unpack.c

index e6a8f58..57aa5f2 100644

--- a/lib/pengine/unpack.c

+++ b/lib/pengine/unpack.c

@@ -373,7 +373,6 @@ expand_remote_rsc_meta(xmlNode *xml_obj, xmlNode *parent, GHashTable **rsc_name_

     const char *remote_port = NULL;

     const char *connect_timeout = "60s";

     const char *remote_allow_migrate=NULL;

-    char *tmp_id = NULL;

     for (attr_set = __xml_first_child(xml_obj); attr_set != NULL; attr_set = __xml_next_element(attr_set)) {

         if (safe_str_neq((const char *)attr_set->name, XML_TAG_META_SETS)) {

@@ -427,73 +426,55 @@ expand_remote_rsc_meta(xmlNode *xml_obj, xmlNode *parent, GHashTable **rsc_name_

     crm_xml_add(xml_rsc, XML_ATTR_TYPE, "remote");

     xml_tmp = create_xml_node(xml_rsc, XML_TAG_META_SETS);

-    tmp_id = crm_concat(remote_name, XML_TAG_META_SETS, '_');

-    crm_xml_add(xml_tmp, XML_ATTR_ID, tmp_id);

-    free(tmp_id);

+    crm_xml_set_id(xml_tmp, "%s_%s", remote_name, XML_TAG_META_SETS);

     attr = create_xml_node(xml_tmp, XML_CIB_TAG_NVPAIR);

-    tmp_id = crm_concat(remote_name, "meta-attributes-container", '_');

-    crm_xml_add(attr, XML_ATTR_ID, tmp_id);

+    crm_xml_set_id(attr, "%s_%s", remote_name, "meta-attributes-container");

     crm_xml_add(attr, XML_NVPAIR_ATTR_NAME, XML_RSC_ATTR_CONTAINER);

     crm_xml_add(attr, XML_NVPAIR_ATTR_VALUE, container_id);

-    free(tmp_id);

     attr = create_xml_node(xml_tmp, XML_CIB_TAG_NVPAIR);

-    tmp_id = crm_concat(remote_name, "meta-attributes-internal", '_');

-    crm_xml_add(attr, XML_ATTR_ID, tmp_id);

+    crm_xml_set_id(attr, "%s_%s", remote_name, "meta-attributes-internal");

     crm_xml_add(attr, XML_NVPAIR_ATTR_NAME, XML_RSC_ATTR_INTERNAL_RSC);

     crm_xml_add(attr, XML_NVPAIR_ATTR_VALUE, "true");

-    free(tmp_id);

     if (remote_allow_migrate) {

         attr = create_xml_node(xml_tmp, XML_CIB_TAG_NVPAIR);

-        tmp_id = crm_concat(remote_name, "meta-attributes-container", '_');

-        crm_xml_add(attr, XML_ATTR_ID, tmp_id);

+        crm_xml_set_id(attr, "%s_%s", remote_name, "meta-attributes-container");

         crm_xml_add(attr, XML_NVPAIR_ATTR_NAME, XML_OP_ATTR_ALLOW_MIGRATE);

         crm_xml_add(attr, XML_NVPAIR_ATTR_VALUE, remote_allow_migrate);

-        free(tmp_id);

     }

     xml_tmp = create_xml_node(xml_rsc, "operations");

     attr = create_xml_node(xml_tmp, XML_ATTR_OP);

-    tmp_id = crm_concat(remote_name, "monitor-interval-30s", '_');

-    crm_xml_add(attr, XML_ATTR_ID, tmp_id);

+    crm_xml_set_id(attr, "%s_%s", remote_name, "monitor-interval-30s");

     crm_xml_add(attr, XML_ATTR_TIMEOUT, "30s");

     crm_xml_add(attr, XML_LRM_ATTR_INTERVAL, "30s");

     crm_xml_add(attr, XML_NVPAIR_ATTR_NAME, "monitor");

-    free(tmp_id);

     if (connect_timeout) {

         attr = create_xml_node(xml_tmp, XML_ATTR_OP);

-        tmp_id = crm_concat(remote_name, "start-interval-0", '_');

-        crm_xml_add(attr, XML_ATTR_ID, tmp_id);

+        crm_xml_set_id(attr, "%s_%s", remote_name, "start-interval-0");

         crm_xml_add(attr, XML_ATTR_TIMEOUT, connect_timeout);

         crm_xml_add(attr, XML_LRM_ATTR_INTERVAL, "0");

         crm_xml_add(attr, XML_NVPAIR_ATTR_NAME, "start");

-        free(tmp_id);

     }

     if (remote_port || remote_server) {

         xml_tmp = create_xml_node(xml_rsc, XML_TAG_ATTR_SETS);

-        tmp_id = crm_concat(remote_name, XML_TAG_ATTR_SETS, '_');

-        crm_xml_add(xml_tmp, XML_ATTR_ID, tmp_id);

-        free(tmp_id);

+        crm_xml_set_id(xml_tmp, "%s_%s", remote_name, XML_TAG_ATTR_SETS);

         if (remote_server) {

             attr = create_xml_node(xml_tmp, XML_CIB_TAG_NVPAIR);

-            tmp_id = crm_concat(remote_name, "instance-attributes-addr", '_');

-            crm_xml_add(attr, XML_ATTR_ID, tmp_id);

+            crm_xml_set_id(attr, "%s_%s", remote_name, "instance-attributes-addr");

             crm_xml_add(attr, XML_NVPAIR_ATTR_NAME, "addr");

             crm_xml_add(attr, XML_NVPAIR_ATTR_VALUE, remote_server);

-            free(tmp_id);

         }

         if (remote_port) {

             attr = create_xml_node(xml_tmp, XML_CIB_TAG_NVPAIR);

-            tmp_id = crm_concat(remote_name, "instance-attributes-port", '_');

-            crm_xml_add(attr, XML_ATTR_ID, tmp_id);

+            crm_xml_set_id(attr, "%s_%s", remote_name, "instance-attributes-port");

             crm_xml_add(attr, XML_NVPAIR_ATTR_NAME, "port");

             crm_xml_add(attr, XML_NVPAIR_ATTR_VALUE, remote_port);

-            free(tmp_id);

         }

     }

diff --git a/tools/crm_node.c b/tools/crm_node.c

index 7092db4..9b60c55 100644

--- a/tools/crm_node.c

+++ b/tools/crm_node.c

@@ -105,11 +105,8 @@ cib_remove_node(uint32_t id, const char *name)

     crm_xml_add(node, XML_ATTR_UNAME, name);

     crm_xml_add(node_state, XML_ATTR_UNAME, name);

     if(id) {

-        char buffer[64];

-        if(snprintf(buffer, 63, "%u", id) > 0) {

-            crm_xml_add(node, XML_ATTR_ID, buffer);

-            crm_xml_add(node_state, XML_ATTR_ID, buffer);

-        }

+        crm_xml_set_id(node, "%u", id);

+        crm_xml_add(node_state, XML_ATTR_ID, ID(node));

     }

     cib = cib_new();

@@ -200,10 +197,7 @@ int tools_remove_node_cache(const char *node, const char *target)

         cmd = create_request(CRM_OP_RM_NODE_CACHE,

                              NULL, NULL, target, crm_system_name, admin_uuid);

         if (n) {

-            char buffer[64];

-            if(snprintf(buffer, 63, "%u", n) > 0) {

-                crm_xml_add(cmd, XML_ATTR_ID, buffer);

-            }

+            crm_xml_set_id(cmd, "%u", n);

         }

         crm_xml_add(cmd, XML_ATTR_UNAME, name);

     }

diff --git a/tools/crm_resource_ban.c b/tools/crm_resource_ban.c

index 2f7b366..69b49bf 100644

--- a/tools/crm_resource_ban.c

+++ b/tools/crm_resource_ban.c

@@ -62,7 +62,6 @@ cli_resource_ban(const char *rsc_id, const char *host, GListPtr allnodes, cib_t

 {

     char *later_s = NULL;

     int rc = pcmk_ok;

-    char *id = NULL;

     xmlNode *fragment = NULL;

     xmlNode *location = NULL;

@@ -83,10 +82,8 @@ cli_resource_ban(const char *rsc_id, const char *host, GListPtr allnodes, cib_t

     fragment = create_xml_node(NULL, XML_CIB_TAG_CONSTRAINTS);

-    id = crm_strdup_printf("cli-ban-%s-on-%s", rsc_id, host);

     location = create_xml_node(fragment, XML_CONS_TAG_RSC_LOCATION);

-    crm_xml_add(location, XML_ATTR_ID, id);

-    free(id);

+    crm_xml_set_id(location, "cli-ban-%s-on-%s", rsc_id, host);

     if (BE_QUIET == FALSE) {

         CMD_ERR("WARNING: Creating rsc_location constraint '%s'"

@@ -117,27 +114,18 @@ cli_resource_ban(const char *rsc_id, const char *host, GListPtr allnodes, cib_t

         xmlNode *rule = create_xml_node(location, XML_TAG_RULE);

         xmlNode *expr = create_xml_node(rule, XML_TAG_EXPRESSION);

-        id = crm_strdup_printf("cli-ban-%s-on-%s-rule", rsc_id, host);

-        crm_xml_add(rule, XML_ATTR_ID, id);

-        free(id);

-

+        crm_xml_set_id(rule, "cli-ban-%s-on-%s-rule", rsc_id, host);

         crm_xml_add(rule, XML_RULE_ATTR_SCORE, MINUS_INFINITY_S);

         crm_xml_add(rule, XML_RULE_ATTR_BOOLEAN_OP, "and");

-        id = crm_strdup_printf("cli-ban-%s-on-%s-expr", rsc_id, host);

-        crm_xml_add(expr, XML_ATTR_ID, id);

-        free(id);

-

+        crm_xml_set_id(expr, "cli-ban-%s-on-%s-expr", rsc_id, host);

         crm_xml_add(expr, XML_EXPR_ATTR_ATTRIBUTE, "#uname");

         crm_xml_add(expr, XML_EXPR_ATTR_OPERATION, "eq");