Merge pull request #714 from gao-yan/patchset-process-digest

Refactor: cib: Calculate and add digest for a patchset after accepting changes for the target xml
ClusterLabs · May 18, 2015 · 5e94bac · 5e94bac
2 parents 8ae4530 + 00b20b8
commit 5e94bac
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 20 deletions.
diff --git a/include/crm/common/xml.h b/include/crm/common/xml.h
@@ -267,9 +267,11 @@ void xml_log_patchset(uint8_t level, const char *function, xmlNode *xml);
 bool xml_patch_versions(xmlNode *patchset, int add[3], int del[3]);
 
 xmlNode *xml_create_patchset(
-    int format, xmlNode *source, xmlNode *target, bool *config, bool manage_version, bool with_digest);
+    int format, xmlNode *source, xmlNode *target, bool *config, bool manage_version);
 int xml_apply_patchset(xmlNode *xml, xmlNode *patchset, bool check_version);
 
+void patchset_process_digest(xmlNode *patch, xmlNode *source, xmlNode *target, bool with_digest);
+
 void save_xml_to_file(xmlNode * xml, const char *desc, const char *filename);
 char *xml_get_path(xmlNode *xml);
 

diff --git a/lib/cib/cib_utils.c b/lib/cib/cib_utils.c
@@ -302,6 +302,7 @@ cib_perform_op(const char *op, int call_options, cib_op_t * fn, gboolean is_quer
     const char *new_version = NULL;
     static struct qb_log_callsite *diff_cs = NULL;
     const char *user = crm_element_value(req, F_CIB_USER);
+    bool with_digest = FALSE;
 
     crm_trace("Begin %s%s op", is_query ? "read-only " : "", op);
 
@@ -449,26 +450,30 @@ cib_perform_op(const char *op, int call_options, cib_op_t * fn, gboolean is_quer
          * The v1 format would barf on this, but we know the v2 patch
          * format only needs it for the top-level version fields
          */
-        local_diff = xml_create_patchset(2, current_cib, scratch, (bool*)config_changed, manage_counters, FALSE);
+        local_diff = xml_create_patchset(2, current_cib, scratch, (bool*)config_changed, manage_counters);
 
     } else {
         static time_t expires = 0;
         time_t tm_now = time(NULL);
-        bool with_digest = FALSE;
 
         if (expires < tm_now) {
             expires = tm_now + 60;  /* Validate clients are correctly applying v2-style diffs at most once a minute */
             with_digest = TRUE;
         }
 
-        local_diff = xml_create_patchset(0, current_cib, scratch, (bool*)config_changed, manage_counters, with_digest);
+        local_diff = xml_create_patchset(0, current_cib, scratch, (bool*)config_changed, manage_counters);
     }
 
+    xml_log_changes(LOG_TRACE, __FUNCTION__, scratch);
+    xml_accept_changes(scratch);
+
     if (diff_cs == NULL) {
         diff_cs = qb_log_callsite_get(__PRETTY_FUNCTION__, __FILE__, "diff-validation", LOG_DEBUG, __LINE__, crm_trace_nonlog);
     }
 
     if(local_diff) {
+        patchset_process_digest(local_diff, current_cib, scratch, with_digest);
+
         xml_log_patchset(LOG_INFO, __FUNCTION__, local_diff);
         crm_log_xml_trace(local_diff, "raw patch");
     }
@@ -494,9 +499,6 @@ cib_perform_op(const char *op, int call_options, cib_op_t * fn, gboolean is_quer
         free_xml(c);
     }
 
-    xml_log_changes(LOG_TRACE, __FUNCTION__, scratch);
-    xml_accept_changes(scratch);
-
     if (safe_str_eq(section, XML_CIB_TAG_STATUS)) {
         /* Throttle the amount of costly validation we perform due to status updates
          * a) we don't really care whats in the status section

diff --git a/lib/common/xml.c b/lib/common/xml.c
@@ -1439,9 +1439,9 @@ xml_repair_v1_diff(xmlNode * last, xmlNode * next, xmlNode * local_diff, gboolea
 }
 
 static xmlNode *
-xml_create_patchset_v1(xmlNode *source, xmlNode *target, bool config, bool with_digest)
+xml_create_patchset_v1(xmlNode *source, xmlNode *target, bool config, bool suppress)
 {
-    xmlNode *patchset = diff_xml_object(source, target, !with_digest);
+    xmlNode *patchset = diff_xml_object(source, target, suppress);
 
     if(patchset) {
         CRM_LOG_ASSERT(xml_document_dirty(target));
@@ -1527,7 +1527,7 @@ static gboolean patch_legacy_mode(void)
 }
 
 xmlNode *
-xml_create_patchset(int format, xmlNode *source, xmlNode *target, bool *config_changed, bool manage_version, bool with_digest)
+xml_create_patchset(int format, xmlNode *source, xmlNode *target, bool *config_changed, bool manage_version)
 {
     int counter = 0;
     bool config = FALSE;
@@ -1573,8 +1573,7 @@ xml_create_patchset(int format, xmlNode *source, xmlNode *target, bool *config_c
 
     switch(format) {
         case 1:
-            with_digest = TRUE;
-            patch = xml_create_patchset_v1(source, target, config, with_digest);
+            patch = xml_create_patchset_v1(source, target, config, FALSE);
             break;
         case 2:
             patch = xml_create_patchset_v2(source, target);
@@ -1584,13 +1583,36 @@ xml_create_patchset(int format, xmlNode *source, xmlNode *target, bool *config_c
             return NULL;
     }
 
-    if(patch && with_digest) {
-        char *digest = calculate_xml_versioned_digest(target, FALSE, TRUE, version);
+    return patch;
+}
+
+void
+patchset_process_digest(xmlNode *patch, xmlNode *source, xmlNode *target, bool with_digest)
+{
+    int format = 1;
+    const char *version = NULL;
+    char *digest = NULL;
 
-        crm_xml_add(patch, XML_ATTR_DIGEST, digest);
-        free(digest);
+    if (patch == NULL || source == NULL || target == NULL) {
+        return;
     }
-    return patch;
+
+    /* NOTE: We should always call xml_accept_changes() before calculating digest. */
+    /* Otherwise, with an on-tracking dirty target, we could get a wrong digest. */
+    CRM_LOG_ASSERT(xml_document_dirty(target) == FALSE);
+
+    crm_element_value_int(patch, "format", &format);
+    if (format > 1 && with_digest == FALSE) {
+        return;
+    }
+
+    version = crm_element_value(source, XML_ATTR_CRM_VERSION);
+    digest = calculate_xml_versioned_digest(target, FALSE, TRUE, version);
+
+    crm_xml_add(patch, XML_ATTR_DIGEST, digest);
+    free(digest);
+
+    return;
 }
 
 static void

diff --git a/tools/cib_shadow.c b/tools/cib_shadow.c
@@ -433,7 +433,14 @@ main(int argc, char **argv)
             goto done;
         }
 
-        diff = xml_create_patchset(0, old_config, new_config, NULL, FALSE, FALSE);
+        xml_track_changes(new_config, NULL, new_config, FALSE);
+        xml_calculate_changes(old_config, new_config);
+
+        diff = xml_create_patchset(0, old_config, new_config, NULL, FALSE);
+
+        xml_log_changes(LOG_INFO, __FUNCTION__, new_config);
+        xml_accept_changes(new_config);
+
         if (diff != NULL) {
             xml_log_patchset(0, "  ", diff);
             rc = 1;

diff --git a/tools/xml_diff.c b/tools/xml_diff.c
@@ -203,7 +203,14 @@ main(int argc, char **argv)
         xml_calculate_changes(object_1, object_2);
         crm_log_xml_debug(object_2, xml_file_2?xml_file_2:"target");
 
-        output = xml_create_patchset(0, object_1, object_2, NULL, FALSE, as_cib);
+        output = xml_create_patchset(0, object_1, object_2, NULL, FALSE);
+
+        xml_log_changes(LOG_INFO, __FUNCTION__, object_2);
+        xml_accept_changes(object_2);
+
+        if (output) {
+            patchset_process_digest(output, object_1, object_2, as_cib);
+        }
 
         if(as_cib && output) {
             int add[] = { 0, 0, 0 };
@@ -267,7 +274,6 @@ main(int argc, char **argv)
                 }
             }
         }
-        xml_log_changes(LOG_INFO, __FUNCTION__, object_2);
         xml_log_patchset(LOG_NOTICE, __FUNCTION__, output);
     }