From c509cd07008b1f9b3402eeb7d7f3f6d504aa4fdb Mon Sep 17 00:00:00 2001
From: "Gao,Yan" <ygao@suse.com>
Date: Fri, 10 Feb 2017 15:44:16 +0100
Subject: [PATCH 1/2] Fix: libcrmcommon: Correctly compare XML comments to
prevent crmd from getting into infinite election loop
With b7fa323, crmd could still get into an infinite election loop when
there was more than one comment with the exactly same text at the same
level within a CIB XML element. For example:
'''
<!--# "Fri Feb 10 11:20:40 CET 2017-->
<!--#============================================================================-->
<!--#-->
<!--# Administrator A-->
<!--#-->
<!--# Pacemaker basic config file for Cluster A-->
<!--#============================================================================-->
<!--#-->
'''
Basically, it'd produce big messes if using the diff operation "move"
for XML comments in such a case.
With this commit, it strictly tries to match the XML comments at the
exactly same offsets when comparing v2 patchset, so that only the diff
operations "create" and "delete" will be used for XML comments.
---
lib/common/xml.c | 46 ++++++++++++++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/lib/common/xml.c b/lib/common/xml.c
index 65237c8..fd80fe1 100644
--- a/lib/common/xml.c
+++ b/lib/common/xml.c
@@ -98,7 +98,7 @@ static filter_t filter[] = {
/* *INDENT-ON* */
static xmlNode *subtract_xml_comment(xmlNode * parent, xmlNode * left, xmlNode * right, gboolean * changed);
-static xmlNode *find_xml_comment(xmlNode * root, xmlNode * search_comment);
+static xmlNode *find_xml_comment(xmlNode * root, xmlNode * search_comment, gboolean exact);
static int add_xml_comment(xmlNode * parent, xmlNode * target, xmlNode * update);
static bool __xml_acl_check(xmlNode *xml, const char *name, enum xml_private_flags mode);
const char *__xml_acl_to_text(enum xml_private_flags flags);
@@ -1555,11 +1555,11 @@ xml_accept_changes(xmlNode * xml)
}
static xmlNode *
-find_element(xmlNode *haystack, xmlNode *needle)
+find_element(xmlNode *haystack, xmlNode *needle, gboolean exact)
{
CRM_CHECK(needle != NULL, return NULL);
return (needle->type == XML_COMMENT_NODE)?
- find_xml_comment(haystack, needle)
+ find_xml_comment(haystack, needle, exact)
: find_entity(haystack, crm_element_name(needle), ID(needle));
}
@@ -1615,7 +1615,7 @@ __subtract_xml_object(xmlNode * target, xmlNode * patch)
xmlNode *target_child = cIter;
cIter = __xml_next(cIter);
- patch_child = find_element(patch, target_child);
+ patch_child = find_element(patch, target_child, FALSE);
__subtract_xml_object(target_child, patch_child);
}
free(id);
@@ -1677,7 +1677,7 @@ __add_xml_object(xmlNode * parent, xmlNode * target, xmlNode * patch)
for (patch_child = __xml_first_child(patch); patch_child != NULL;
patch_child = __xml_next(patch_child)) {
- target_child = find_element(target, patch_child);
+ target_child = find_element(target, patch_child, FALSE);
__add_xml_object(target, target_child, patch_child);
}
}
@@ -4026,7 +4026,7 @@ __xml_diff_object(xmlNode * old, xmlNode * new)
for (cIter = __xml_first_child(old); cIter != NULL; ) {
xmlNode *old_child = cIter;
- xmlNode *new_child = find_element(new, cIter);
+ xmlNode *new_child = find_element(new, cIter, TRUE);
cIter = __xml_next(cIter);
if(new_child) {
@@ -4041,7 +4041,7 @@ __xml_diff_object(xmlNode * old, xmlNode * new)
__xml_acl_apply(top); /* Make sure any ACLs are applied to 'candidate' */
free_xml(candidate);
- if (find_element(new, old_child) == NULL) {
+ if (find_element(new, old_child, TRUE) == NULL) {
xml_private_t *p = old_child->_private;
p->flags |= xpf_skip;
@@ -4051,7 +4051,7 @@ __xml_diff_object(xmlNode * old, xmlNode * new)
for (cIter = __xml_first_child(new); cIter != NULL; ) {
xmlNode *new_child = cIter;
- xmlNode *old_child = find_element(old, cIter);
+ xmlNode *old_child = find_element(old, cIter, TRUE);
cIter = __xml_next(cIter);
if(old_child == NULL) {
@@ -4226,18 +4226,36 @@ in_upper_context(int depth, int context, xmlNode * xml_node)
}
static xmlNode *
-find_xml_comment(xmlNode * root, xmlNode * search_comment)
+find_xml_comment(xmlNode * root, xmlNode * search_comment, gboolean exact)
{
xmlNode *a_child = NULL;
+ int search_offset = __xml_offset(search_comment);
CRM_CHECK(search_comment->type == XML_COMMENT_NODE, return NULL);
for (a_child = __xml_first_child(root); a_child != NULL; a_child = __xml_next(a_child)) {
- if (a_child->type != XML_COMMENT_NODE) {
- continue;
+ if (exact) {
+ int offset = __xml_offset(a_child);
+ xml_private_t *p = a_child->_private;
+
+ if (offset < search_offset) {
+ continue;
+
+ } else if (offset > search_offset) {
+ return NULL;
+ }
+
+ if (is_set(p->flags, xpf_skip)) {
+ continue;
+ }
}
- if (safe_str_eq((const char *)a_child->content, (const char *)search_comment->content)) {
+
+ if (a_child->type == XML_COMMENT_NODE
+ && safe_str_eq((const char *)a_child->content, (const char *)search_comment->content)) {
return a_child;
+
+ } else if (exact) {
+ return NULL;
}
}
@@ -4332,7 +4350,7 @@ subtract_xml_object(xmlNode * parent, xmlNode * left, xmlNode * right,
left_child = __xml_next(left_child)) {
gboolean child_changed = FALSE;
- right_child = find_element(right, left_child);
+ right_child = find_element(right, left_child, FALSE);
subtract_xml_object(diff, left_child, right_child, full, &child_changed, marker);
if (child_changed) {
*changed = TRUE;
@@ -4458,7 +4476,7 @@ add_xml_comment(xmlNode * parent, xmlNode * target, xmlNode * update)
CRM_CHECK(update->type == XML_COMMENT_NODE, return 0);
if (target == NULL) {
- target = find_xml_comment(parent, update);
+ target = find_xml_comment(parent, update, FALSE);
}
if (target == NULL) {
--
1.8.3.1
From 362f02874b6be46bf2648fb45ebc6bc636d48965 Mon Sep 17 00:00:00 2001
From: "Gao,Yan" <ygao@suse.com>
Date: Mon, 13 Feb 2017 17:47:23 +0100
Subject: [PATCH 2/2] Fix: libcrmcommon: Correctly delete XML comments
according to their positions
Previously, v2 patchset was not able to define which exact XML comments were
expected to be deleted in a CIB XML element. It was a problem if there
was more than one comment at the same level within a CIB XML element.
This commit resolves it by adding and handling a "position" field in the
diff operation "delete" for XML comments.
---
lib/common/xml.c | 121 +++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 100 insertions(+), 21 deletions(-)
diff --git a/lib/common/xml.c b/lib/common/xml.c
index fd80fe1..6dce4cb 100644
--- a/lib/common/xml.c
+++ b/lib/common/xml.c
@@ -78,7 +78,7 @@ typedef struct xml_private_s
uint32_t flags;
char *user;
GListPtr acls;
- GListPtr deleted_paths;
+ GListPtr deleted_objs;
} xml_private_t;
typedef struct xml_acl_s {
@@ -86,6 +86,11 @@ typedef struct xml_acl_s {
char *xpath;
} xml_acl_t;
+typedef struct xml_deleted_obj_s {
+ char *path;
+ int position;
+} xml_deleted_obj_t;
+
/* *INDENT-OFF* */
static filter_t filter[] = {
@@ -275,6 +280,17 @@ __xml_acl_free(void *data)
}
static void
+__xml_deleted_obj_free(void *data)
+{
+ if(data) {
+ xml_deleted_obj_t *deleted_obj = data;
+
+ free(deleted_obj->path);
+ free(deleted_obj);
+ }
+}
+
+static void
__xml_private_clean(xml_private_t *p)
{
if(p) {
@@ -288,9 +304,9 @@ __xml_private_clean(xml_private_t *p)
p->acls = NULL;
}
- if(p->deleted_paths) {
- g_list_free_full(p->deleted_paths, free);
- p->deleted_paths = NULL;
+ if(p->deleted_objs) {
+ g_list_free_full(p->deleted_objs, __xml_deleted_obj_free);
+ p->deleted_objs = NULL;
}
}
}
@@ -1094,10 +1110,10 @@ is_config_change(xmlNode *xml)
if(xml->doc && xml->doc->_private) {
p = xml->doc->_private;
- for(gIter = p->deleted_paths; gIter; gIter = gIter->next) {
- char *path = gIter->data;
+ for(gIter = p->deleted_objs; gIter; gIter = gIter->next) {
+ xml_deleted_obj_t *deleted_obj = gIter->data;
- if(strstr(path, "/"XML_TAG_CIB"/"XML_CIB_TAG_CONFIGURATION) != NULL) {
+ if(strstr(deleted_obj->path, "/"XML_TAG_CIB"/"XML_CIB_TAG_CONFIGURATION) != NULL) {
return TRUE;
}
}
@@ -1241,11 +1257,15 @@ xml_create_patchset_v2(xmlNode *source, xmlNode *target)
crm_xml_add(v, vfields[lpc], value);
}
- for(gIter = doc->deleted_paths; gIter; gIter = gIter->next) {
+ for(gIter = doc->deleted_objs; gIter; gIter = gIter->next) {
+ xml_deleted_obj_t *deleted_obj = gIter->data;
xmlNode *change = create_xml_node(patchset, XML_DIFF_CHANGE);
crm_xml_add(change, XML_DIFF_OP, "delete");
- crm_xml_add(change, XML_DIFF_PATH, gIter->data);
+ crm_xml_add(change, XML_DIFF_PATH, deleted_obj->path);
+ if (deleted_obj->position >= 0) {
+ crm_xml_add_int(change, XML_DIFF_POSITION, deleted_obj->position);
+ }
}
__xml_build_changes(target, patchset);
@@ -1473,7 +1493,15 @@ xml_log_patchset(uint8_t log_level, const char *function, xmlNode * patchset)
}
} else if(strcmp(op, "delete") == 0) {
- do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s", xpath);
+ int position = -1;
+
+ crm_element_value_int(change, XML_DIFF_POSITION, &position);
+ if (position >= 0) {
+ do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s (%d)", xpath, position);
+
+ } else {
+ do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s", xpath);
+ }
}
}
return;
@@ -1521,8 +1549,17 @@ xml_log_changes(uint8_t log_level, const char *function, xmlNode * xml)
return;
}
- for(gIter = doc->deleted_paths; gIter; gIter = gIter->next) {
- do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s", (char*)gIter->data);
+ for(gIter = doc->deleted_objs; gIter; gIter = gIter->next) {
+ xml_deleted_obj_t *deleted_obj = gIter->data;
+
+ if (deleted_obj->position >= 0) {
+ do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s (%d)",
+ deleted_obj->path, deleted_obj->position);
+
+ } else {
+ do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s",
+ deleted_obj->path);
+ }
}
log_data_element(log_level, __FILE__, function, __LINE__, "+ ", xml, 0,
@@ -1881,7 +1918,7 @@ xml_apply_patchset_v1(xmlNode *xml, xmlNode *patchset, bool check_version)
}
static xmlNode *
-__first_xml_child_match(xmlNode *parent, const char *name, const char *id)
+__first_xml_child_match(xmlNode *parent, const char *name, const char *id, int position)
{
xmlNode *cIter = NULL;
@@ -1894,13 +1931,21 @@ __first_xml_child_match(xmlNode *parent, const char *name, const char *id)
continue;
}
}
+
+ /* The "position" makes sense only for XML comments for now */
+ if (cIter->type == XML_COMMENT_NODE
+ && position >= 0
+ && __xml_offset(cIter) != position) {
+ continue;
+ }
+
return cIter;
}
return NULL;
}
static xmlNode *
-__xml_find_path(xmlNode *top, const char *key)
+__xml_find_path(xmlNode *top, const char *key, int target_position)
{
xmlNode *target = (xmlNode*)top->doc;
char *id = malloc(XML_BUFFER_SIZE);
@@ -1923,13 +1968,19 @@ __xml_find_path(xmlNode *top, const char *key)
} else if(tag && section) {
int f = sscanf (section, "%[^[][@id='%[^']", tag, id);
+ int current_position = -1;
+
+ /* The "target_position" is for the target tag */
+ if (rc == 1 && target_position >= 0) {
+ current_position = target_position;
+ }
switch(f) {
case 1:
- target = __first_xml_child_match(target, tag, NULL);
+ target = __first_xml_child_match(target, tag, NULL, current_position);
break;
case 2:
- target = __first_xml_child_match(target, tag, id);
+ target = __first_xml_child_match(target, tag, id, current_position);
break;
default:
crm_trace("Aborting on %s", section);
@@ -1975,16 +2026,20 @@ xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset, bool check_version)
xmlNode *match = NULL;
const char *op = crm_element_value(change, XML_DIFF_OP);
const char *xpath = crm_element_value(change, XML_DIFF_PATH);
+ int position = -1;
crm_trace("Processing %s %s", change->name, op);
if(op == NULL) {
continue;
}
+ if(strcmp(op, "delete") == 0) {
+ crm_element_value_int(change, XML_DIFF_POSITION, &position);
+ }
#if 0
match = get_xpath_object(xpath, xml, LOG_TRACE);
#else
- match = __xml_find_path(xml, xpath);
+ match = __xml_find_path(xml, xpath, position);
#endif
crm_trace("Performing %s on %s with %p", op, xpath, match);
@@ -2583,8 +2638,8 @@ xml_get_path(xmlNode *xml)
return NULL;
}
-void
-free_xml(xmlNode * child)
+static void
+free_xml_with_position(xmlNode * child, int position)
{
if (child != NULL) {
xmlNode *top = NULL;
@@ -2613,9 +2668,25 @@ free_xml(xmlNode * child)
char buffer[XML_BUFFER_SIZE];
if(__get_prefix(NULL, child, buffer, offset) > 0) {
+ xml_deleted_obj_t *deleted_obj = calloc(1, sizeof(xml_deleted_obj_t));
+
crm_trace("Deleting %s %p from %p", buffer, child, doc);
+
+ deleted_obj->path = strdup(buffer);
+
+ deleted_obj->position = -1;
+ /* Record the "position" only for XML comments for now */
+ if (child->type == XML_COMMENT_NODE) {
+ if (position >= 0) {
+ deleted_obj->position = position;
+
+ } else {
+ deleted_obj->position = __xml_offset(child);
+ }
+ }
+
p = doc->_private;
- p->deleted_paths = g_list_append(p->deleted_paths, strdup(buffer));
+ p->deleted_objs = g_list_append(p->deleted_objs, deleted_obj);
set_doc_flag(child, xpf_dirty);
}
}
@@ -2629,6 +2700,13 @@ free_xml(xmlNode * child)
}
}
+
+void
+free_xml(xmlNode * child)
+{
+ free_xml_with_position(child, -1);
+}
+
xmlNode *
copy_xml(xmlNode * src)
{
@@ -4039,7 +4117,8 @@ __xml_diff_object(xmlNode * old, xmlNode * new)
__xml_node_clean(candidate);
__xml_acl_apply(top); /* Make sure any ACLs are applied to 'candidate' */
- free_xml(candidate);
+ /* Record the old position */
+ free_xml_with_position(candidate, __xml_offset(old_child));
if (find_element(new, old_child, TRUE) == NULL) {
xml_private_t *p = old_child->_private;
--
1.8.3.1