wip

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

Author: nrwahl2

daemons/based/based_callbacks.c

@@ -333,6 +333,14 @@ static bool
 parse_peer_options(const cib__operation_t *operation, pcmk__request_t *request,
                    bool *local_notify, bool *needs_reply, bool *process)
 {
+    /* Don't send replies for modifying ops because they already get forwarded
+     * to all nodes. Also don't send a reply if the client specifically told us
+     * not to.
+     */
+    const bool can_reply = !operation->modifies_cib
+                           && !pcmk__is_set(request->call_options,
+                                            cib_discard_reply);
+
     const char *host = pcmk__xe_get(request->xml, PCMK__XA_CIB_HOST);
     const char *delegated = pcmk__xe_get(request->xml,
                                          PCMK__XA_CIB_DELEGATED_FROM);
@@ -366,7 +374,6 @@ parse_peer_options(const cib__operation_t *operation, pcmk__request_t *request,
         pcmk__trace("Will notify local clients for %s reply from %s",
                     request->op, originator);
         *process = false;
-        *needs_reply = false;
         *local_notify = true;
         return true;
     }
@@ -378,6 +385,11 @@ parse_peer_options(const cib__operation_t *operation, pcmk__request_t *request,
         // sync_our_cib() sets PCMK__XA_CIB_ISREPLYTO
         delegated = reply_to;
 
+        /* @FIXME can_reply is always false here because cib__op_replace has
+         * modifies_cib=true. Should we be sending a reply here?
+         */
+        *needs_reply = can_reply;
+
     } else if (pcmk__str_eq(request->op, PCMK__CIB_REQUEST_UPGRADE,
                             pcmk__str_none)) {
         /* Only the DC (node with the oldest software) should process
@@ -407,7 +419,6 @@ parse_peer_options(const cib__operation_t *operation, pcmk__request_t *request,
             pcmk__trace("Will notify local clients for %s reply from %s",
                         request->op, originator);
             *process = false;
-            *needs_reply = false;
             *local_notify = true;
             return true;
         }
@@ -423,6 +434,7 @@ parse_peer_options(const cib__operation_t *operation, pcmk__request_t *request,
     if (pcmk__str_eq(host, OUR_NODENAME, pcmk__str_casei)) {
         pcmk__trace("Processing %s request sent to us from %s", request->op,
                     originator);
+        *needs_reply = can_reply;
         return true;
     }
 
@@ -433,10 +445,10 @@ parse_peer_options(const cib__operation_t *operation, pcmk__request_t *request,
     }
 
     if (!is_reply && pcmk__str_eq(request->op, CRM_OP_PING, pcmk__str_none)) {
+        *needs_reply = can_reply;
         return true;
     }
 
-    *needs_reply = false;
     pcmk__trace("Processing %s request broadcast by %s call %s on %s "
                 "(local clients will%s be notified)", request->op,
                 pcmk__s(pcmk__xe_get(request->xml, PCMK__XA_CIB_CLIENTNAME),
@@ -643,11 +655,9 @@ based_handle_request(pcmk__request_t *request)
 {
     // @TODO: Break into multiple smaller functions
     bool process = true;        // Whether to process request locally now
-    bool needs_reply = true;    // Whether to build a reply
+    bool needs_reply = false;   // Whether to build a reply
     bool local_notify = false;  // Whether to notify (local) requester
 
-    xmlNode *reply = NULL;
-
     int rc = pcmk_rc_ok;
     const char *originator = pcmk__xe_get(request->xml, PCMK__XA_SRC);
     const char *host = pcmk__xe_get(request->xml, PCMK__XA_CIB_HOST);
@@ -708,7 +718,6 @@ based_handle_request(pcmk__request_t *request)
          * CIB copy, and we don't notify for individual requests because the
          * entire transaction is atomic.
          */
-        needs_reply = false;
         pcmk__trace("Processing %s op from %s/%s on %s locally because it's "
                     "part of a transaction", request->op, client_name, call_id,
                     pcmk__xe_get(request->xml, PCMK__XA_SRC));
@@ -726,7 +735,6 @@ based_handle_request(pcmk__request_t *request)
         }
 
         // Process locally and notify local client; no peer to reply to
-        needs_reply = false;
         local_notify = true;
 
         log_local_options(request->ipc_client, host, request->op);
@@ -738,7 +746,6 @@ based_handle_request(pcmk__request_t *request)
     }
 
     if (pcmk__is_set(request->call_options, cib_discard_reply)) {
-        needs_reply = false;
         local_notify = false;
         pcmk__trace("Client is not interested in the reply");
     }
@@ -747,11 +754,7 @@ based_handle_request(pcmk__request_t *request)
         rc = cib_status;
         pcmk__err("Ignoring request because cluster configuration is invalid "
                   "(please repair and restart): %s", pcmk_rc_str(rc));
-
-        if (!pcmk__is_set(request->call_options, cib_discard_reply)) {
-            reply = create_cib_reply(request->xml, rc, based_cib);
-        }
-
+        output = based_cib;
         goto done;
     }
 
@@ -770,13 +773,12 @@ based_handle_request(pcmk__request_t *request)
 
     log_op_result(request->xml, operation, rc, (time(NULL) - start_time));
 
-    if (!pcmk__is_set(request->call_options, cib_discard_reply)) {
-        reply = create_cib_reply(request->xml, rc, output);
-    }
-
 done:
-    if (!operation->modifies_cib && needs_reply && !based_stand_alone()) {
+    if (needs_reply) {
+        xmlNode *reply = create_cib_reply(request->xml, rc, output);
+
         send_peer_reply(reply, originator);
+        pcmk__xml_free(reply);
     }
 
     if (local_notify && (client_id != NULL)) {

daemons/based/based_messages.c

@@ -277,6 +277,7 @@ based_process_upgrade(xmlNode *req, xmlNode **cib, xmlNode **answer)
 static xmlNode *
 cib_msg_copy(const xmlNode *msg)
 {
+    // @FIXME Copying CIB_CALLOPT seems problematic if it has cib_discard_reply
     static const char *field_list[] = {
         PCMK__XA_T,
         PCMK__XA_CIB_CLIENTID,