Refactor: libcib: cib_perform_op() takes only one CIB argument

The new approach is to set the caller's result_cib pointer to the
current CIB instead of to NULL. Then pass &result_cib to
cib_perform_op() as its "cib" argument.
* If it doesn't make a copy, then it operates on the original CIB
  directly and then assign the original CIB to working_cib -- just as
  we were already doing.
* If it does make a copy, then it's copying the original CIB, just as we
  were already doing.
* If it makes *cib point somewhere else at the end (in the "done"
  section), then that changes the value of the caller's result_cib, but
  it DOES NOT change the value of caller's current CIB pointer (the_cib
  in pacemaker-based or private->cib_xml in cib_file.c). So the caller
  still has a reference to the original CIB.

The logic is arguably a bit more complicated, but previously we had five
CIB pointers (current_cib, result_cib, working_cib, old_versions, top)
in cib_perform_op(). Now we only have to deal with four. That feels more
manageable and thus worth it IMO.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Author: nrwahl2

daemons/based/based_callbacks.c

@@ -563,8 +563,9 @@ cib_process_command(xmlNode *request, const cib__operation_t *operation,
      * It's not important whether the client variant is cib_native or
      * cib_remote.
      */
+    result_cib = the_cib;
     rc = cib_perform_op(cib_undefined, op_function, request, &config_changed,
-                        &the_cib, &result_cib, &cib_diff, &output);
+                        &result_cib, &cib_diff, &output);
 
     /* Always write to disk for successful ops with the flag set. This also
      * negates the need to detect ordering changes.

include/crm/cib/internal.h

@@ -196,8 +196,8 @@ int cib__perform_query(cib__op_fn_t fn, xmlNode *req, xmlNode **current_cib,
                        xmlNode **output);
 
 int cib_perform_op(enum cib_variant variant, cib__op_fn_t fn, xmlNode *req,
-                   bool *config_changed, xmlNode **current_cib,
-                   xmlNode **result_cib, xmlNode **diff, xmlNode **output);
+                   bool *config_changed, xmlNode **cib, xmlNode **diff,
+                   xmlNode **output);
 
 int cib__create_op(cib_t *cib, const char *op, const char *host,
                    const char *section, xmlNode *data, int call_options,

lib/cib/cib_file.c

@@ -169,8 +169,9 @@ process_request(cib_t *cib, xmlNode *request, xmlNode **output)
         rc = cib__perform_query(op_function, request, &private->cib_xml,
                                 output);
     } else {
+        result_cib = private->cib_xml;
         rc = cib_perform_op(cib_file, op_function, request, &changed,
-                            &private->cib_xml, &result_cib, &cib_diff, output);
+                            &result_cib, &cib_diff, output);
     }
 
     if (pcmk__is_set(call_options, cib_transaction)) {

lib/cib/cib_utils.c

@@ -465,8 +465,8 @@ set_update_origin(xmlNode *new_cib, const xmlNode *request)
 
 int
 cib_perform_op(enum cib_variant variant, cib__op_fn_t fn, xmlNode *req,
-               bool *config_changed, xmlNode **current_cib,
-               xmlNode **result_cib, xmlNode **diff, xmlNode **output)
+               bool *config_changed, xmlNode **cib, xmlNode **diff,
+               xmlNode **output)
 {
     int rc = pcmk_rc_ok;
 
@@ -489,8 +489,7 @@ cib_perform_op(enum cib_variant variant, cib__op_fn_t fn, xmlNode *req,
 
     pcmk__assert((fn != NULL) && (req != NULL)
                  && (config_changed != NULL) && (!*config_changed)
-                 && (current_cib != NULL) && (*current_cib != NULL)
-                 && (result_cib != NULL) && (*result_cib == NULL)
+                 && (cib != NULL) && (*cib != NULL)
                  && (diff != NULL) && (*diff == NULL)
                  && (output != NULL) && (*output == NULL));
 
@@ -500,39 +499,38 @@ cib_perform_op(enum cib_variant variant, cib__op_fn_t fn, xmlNode *req,
     pcmk__xe_get_flags(req, PCMK__XA_CIB_CALLOPT, &call_options, cib_none);
 
     input = get_op_input(req);
-    enable_acl = cib_acl_enabled(*current_cib, user);
+    enable_acl = cib_acl_enabled(*cib, user);
 
     pcmk__trace("Processing %s for section '%s', user '%s'", op,
                 pcmk__s(section, "(null)"), pcmk__s(user, "(null)"));
     pcmk__log_xml_trace(req, "request");
 
     if (!should_copy_cib(op, section, call_options)) {
         // Make a copy of the top-level element to store version details
-        top = pcmk__xe_create(NULL, (const char *) (*current_cib)->name);
-        pcmk__xe_copy_attrs(top, *current_cib, pcmk__xaf_none);
+        top = pcmk__xe_create(NULL, (const char *) (*cib)->name);
+        pcmk__xe_copy_attrs(top, *cib, pcmk__xaf_none);
         old_versions = top;
 
-        pcmk__xml_commit_changes((*current_cib)->doc);
-        pcmk__xml_doc_set_flags((*current_cib)->doc, pcmk__xf_tracking);
+        pcmk__xml_commit_changes((*cib)->doc);
+        pcmk__xml_doc_set_flags((*cib)->doc, pcmk__xf_tracking);
         if (enable_acl) {
-            pcmk__enable_acl(*current_cib, *current_cib, user);
+            pcmk__enable_acl(*cib, *cib, user);
         }
 
-        rc = fn(op, call_options, section, req, input, current_cib, output);
+        rc = fn(op, call_options, section, req, input, cib, output);
 
-        /* Set working_cib to *current_cib after fn(), in case *current_cib
-         * points somewhere else now (for example, after a erase or full-CIB
-         * replace op).
+        /* Set working_cib to *cib after fn(), in case *cib points somewhere
+         * else now (for example, after a erase or full-CIB replace op).
          */
-        working_cib = *current_cib;
+        working_cib = *cib;
 
     } else {
-        working_cib = pcmk__xml_copy(NULL, *current_cib);
-        old_versions = *current_cib;
+        working_cib = pcmk__xml_copy(NULL, *cib);
+        old_versions = *cib;
 
         pcmk__xml_doc_set_flags(working_cib->doc, pcmk__xf_tracking);
         if (enable_acl) {
-            pcmk__enable_acl(*current_cib, working_cib, user);
+            pcmk__enable_acl(*cib, working_cib, user);
         }
 
         rc = fn(op, call_options, section, req, input, &working_cib, output);
@@ -615,15 +613,15 @@ cib_perform_op(enum cib_variant variant, cib__op_fn_t fn, xmlNode *req,
     }
 
 done:
-    *result_cib = working_cib;
+    *cib = working_cib;
 
     /* @TODO This may not work correctly when !should_copy_cib(), since we don't
      * keep the original CIB.
      */
     if ((rc != pcmk_rc_ok) && cib_acl_enabled(old_versions, user)
-        && xml_acl_filtered_copy(user, old_versions, working_cib, result_cib)) {
+        && xml_acl_filtered_copy(user, old_versions, working_cib, cib)) {
 
-        if (*result_cib == NULL) {
+        if (*cib == NULL) {
             pcmk__debug("Pre-filtered the entire cib result");
         }
         pcmk__xml_free(working_cib);