From 66e5e4d83e90be3cecab7bf5f50d0e10fbaa7cea Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Fri, 26 Apr 2019 11:52:59 +0200 Subject: [PATCH 1/3] Fix: libcrmcommon: correctly apply XML diffs with multiple move/create changes Given a resource group: ``` ``` , if we'd like to change it to: ``` ``` , the generated XML diff would be like: ``` ``` Previously after applying the XML diff, the resulting XML would be a mess: ``` ``` It's because the positions of the already moved XML objects could be affected by the later moved objects. This commit fixes it by temporarily putting "move" objects after the last sibling and also delaying the adding of any "create" objects, then placing them to the target positions in the right order. --- lib/common/xml.c | 126 ++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 97 insertions(+), 29 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 66b5f66..d815a48 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1466,11 +1466,40 @@ __xml_find_path(xmlNode *top, const char *key, int target_position) return target; } +typedef struct xml_change_obj_s { + xmlNode *change; + xmlNode *match; +} xml_change_obj_t; + +static gint +sort_change_obj_by_position(gconstpointer a, gconstpointer b) +{ + const xml_change_obj_t *change_obj_a = a; + const xml_change_obj_t *change_obj_b = b; + int position_a = -1; + int position_b = -1; + + crm_element_value_int(change_obj_a->change, XML_DIFF_POSITION, &position_a); + crm_element_value_int(change_obj_b->change, XML_DIFF_POSITION, &position_b); + + if (position_a < position_b) { + return -1; + + } else if (position_a > position_b) { + return 1; + } + + return 0; +} + static int xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset) { int rc = pcmk_ok; xmlNode *change = NULL; + GListPtr change_objs = NULL; + GListPtr gIter = NULL; + for (change = __xml_first_child(patchset); change != NULL; change = __xml_next(change)) { xmlNode *match = NULL; const char *op = crm_element_value(change, XML_DIFF_OP); @@ -1482,6 +1511,7 @@ xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset) continue; } + // "delete" changes for XML comments are generated with "position" if(strcmp(op, "delete") == 0) { crm_element_value_int(change, XML_DIFF_POSITION, &position); } @@ -1497,7 +1527,71 @@ xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset) rc = -pcmk_err_diff_failed; continue; - } else if(strcmp(op, "create") == 0) { + } else if (strcmp(op, "create") == 0 || strcmp(op, "move") == 0) { + // Delay the adding of a "create" object + xml_change_obj_t *change_obj = calloc(1, sizeof(xml_change_obj_t)); + + CRM_ASSERT(change_obj != NULL); + + change_obj->change = change; + change_obj->match = match; + + change_objs = g_list_append(change_objs, change_obj); + + if (strcmp(op, "move") == 0) { + // Temporarily put the "move" object after the last sibling + if (match->parent != NULL && match->parent->last != NULL) { + xmlAddNextSibling(match->parent->last, match); + } + } + + } else if(strcmp(op, "delete") == 0) { + free_xml(match); + + } else if(strcmp(op, "modify") == 0) { + xmlAttr *pIter = pcmk__first_xml_attr(match); + xmlNode *attrs = __xml_first_child(first_named_child(change, XML_DIFF_RESULT)); + + if(attrs == NULL) { + rc = -ENOMSG; + continue; + } + while(pIter != NULL) { + const char *name = (const char *)pIter->name; + + pIter = pIter->next; + xml_remove_prop(match, name); + } + + for (pIter = pcmk__first_xml_attr(attrs); pIter != NULL; pIter = pIter->next) { + const char *name = (const char *)pIter->name; + const char *value = crm_element_value(attrs, name); + + crm_xml_add(match, name, value); + } + + } else { + crm_err("Unknown operation: %s", op); + } + } + + // Changes should be generated in the right order. Double checking. + change_objs = g_list_sort(change_objs, sort_change_obj_by_position); + + for (gIter = change_objs; gIter; gIter = gIter->next) { + xml_change_obj_t *change_obj = gIter->data; + xmlNode *match = change_obj->match; + const char *op = NULL; + const char *xpath = NULL; + + change = change_obj->change; + + op = crm_element_value(change, XML_DIFF_OP); + xpath = crm_element_value(change, XML_DIFF_PATH); + + crm_trace("Continue performing %s on %s with %p", op, xpath, match); + + if(strcmp(op, "create") == 0) { int position = 0; xmlNode *child = NULL; xmlNode *match_child = NULL; @@ -1565,36 +1659,10 @@ xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset) match->name, ID(match), __xml_offset(match), position, match->prev); rc = -pcmk_err_diff_failed; } - - } else if(strcmp(op, "delete") == 0) { - free_xml(match); - - } else if(strcmp(op, "modify") == 0) { - xmlAttr *pIter = pcmk__first_xml_attr(match); - xmlNode *attrs = __xml_first_child(first_named_child(change, XML_DIFF_RESULT)); - - if(attrs == NULL) { - rc = -ENOMSG; - continue; - } - while(pIter != NULL) { - const char *name = (const char *)pIter->name; - - pIter = pIter->next; - xml_remove_prop(match, name); - } - - for (pIter = pcmk__first_xml_attr(attrs); pIter != NULL; pIter = pIter->next) { - const char *name = (const char *)pIter->name; - const char *value = crm_element_value(attrs, name); - - crm_xml_add(match, name, value); - } - - } else { - crm_err("Unknown operation: %s", op); } } + + g_list_free_full(change_objs, free); return rc; } -- 1.8.3.1 From f8d008d8d3a29900ee0c6decbb71a243fa4c2d8c Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Tue, 30 Apr 2019 00:15:03 +0200 Subject: [PATCH 2/3] Fix: libcrmcommon: avoid possible use-of-NULL when applying XML diffs --- lib/common/xml.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index d815a48..fe87de6 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1506,11 +1506,12 @@ xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset) 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; } + crm_trace("Processing %s %s", change->name, op); + // "delete" changes for XML comments are generated with "position" if(strcmp(op, "delete") == 0) { crm_element_value_int(change, XML_DIFF_POSITION, &position); -- 1.8.3.1 From e6b2bf0cf7e7ed839583d529b190a7a6cd1bd594 Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Tue, 30 Apr 2019 00:19:46 +0200 Subject: [PATCH 3/3] Fix: libcrmcommon: return error when applying XML diffs containing unknown operations --- lib/common/xml.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/common/xml.c b/lib/common/xml.c index fe87de6..940c4b9 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1573,6 +1573,7 @@ xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset) } else { crm_err("Unknown operation: %s", op); + rc = -pcmk_err_diff_failed; } } -- 1.8.3.1