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