From 6c529bb624ad548f66ce6ef1fa80b77c688918f4 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 22 Nov 2019 16:39:54 -0600

Subject: [PATCH 1/4] Refactor: controller: rename struct recurring_op_s to

 active_op_t

... because it holds both recurring and pending non-recurring actions,

and the name was confusing

---

 daemons/controld/controld_execd.c       | 18 +++++++++---------

 daemons/controld/controld_execd_state.c |  4 ++--

 daemons/controld/controld_lrm.h         |  8 ++++----

 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/daemons/controld/controld_execd.c b/daemons/controld/controld_execd.c

index 9e8dd36..48f35dd 100644

--- a/daemons/controld/controld_execd.c

+++ b/daemons/controld/controld_execd.c

@@ -403,7 +403,7 @@ lrm_state_verify_stopped(lrm_state_t * lrm_state, enum crmd_fsa_state cur_state,

     GHashTableIter gIter;

     const char *key = NULL;

     rsc_history_t *entry = NULL;

-    struct recurring_op_s *pending = NULL;

+    active_op_t *pending = NULL;

     crm_debug("Checking for active resources before exit");

@@ -909,7 +909,7 @@ static gboolean

 lrm_remove_deleted_op(gpointer key, gpointer value, gpointer user_data)

 {

     const char *rsc = user_data;

-    struct recurring_op_s *pending = value;

+    active_op_t *pending = value;

     if (crm_str_eq(rsc, pending->rsc_id, TRUE)) {

         crm_info("Removing op %s:%d for deleted resource %s",

@@ -1137,7 +1137,7 @@ cancel_op(lrm_state_t * lrm_state, const char *rsc_id, const char *key, int op,

 {

     int rc = pcmk_ok;

     char *local_key = NULL;

-    struct recurring_op_s *pending = NULL;

+    active_op_t *pending = NULL;

     CRM_CHECK(op != 0, return FALSE);

     CRM_CHECK(rsc_id != NULL, return FALSE);

@@ -1203,7 +1203,7 @@ cancel_action_by_key(gpointer key, gpointer value, gpointer user_data)

 {

     gboolean remove = FALSE;

     struct cancel_data *data = user_data;

-    struct recurring_op_s *op = (struct recurring_op_s *)value;

+    active_op_t *op = value;

     if (crm_str_eq(op->op_key, data->key, TRUE)) {

         data->done = TRUE;

@@ -2107,7 +2107,7 @@ stop_recurring_action_by_rsc(gpointer key, gpointer value, gpointer user_data)

 {

     gboolean remove = FALSE;

     struct stop_recurring_action_s *event = user_data;

-    struct recurring_op_s *op = (struct recurring_op_s *)value;

+    active_op_t *op = value;

     if ((op->interval_ms != 0)

         && crm_str_eq(op->rsc_id, event->rsc->id, TRUE)) {

@@ -2124,7 +2124,7 @@ stop_recurring_actions(gpointer key, gpointer value, gpointer user_data)

 {

     gboolean remove = FALSE;

     lrm_state_t *lrm_state = user_data;

-    struct recurring_op_s *op = (struct recurring_op_s *)value;

+    active_op_t *op = value;

     if (op->interval_ms != 0) {

         crm_info("Cancelling op %d for %s (%s)", op->call_id, op->rsc_id,

@@ -2297,9 +2297,9 @@ do_lrm_rsc_op(lrm_state_t * lrm_state, lrmd_rsc_info_t * rsc, const char *operat

          * for them to complete during shutdown

          */

         char *call_id_s = make_stop_id(rsc->id, call_id);

-        struct recurring_op_s *pending = NULL;

+        active_op_t *pending = NULL;

-        pending = calloc(1, sizeof(struct recurring_op_s));

+        pending = calloc(1, sizeof(active_op_t));

         crm_trace("Recording pending op: %d - %s %s", call_id, op_id, call_id_s);

         pending->call_id = call_id;

@@ -2517,7 +2517,7 @@ did_lrm_rsc_op_fail(lrm_state_t *lrm_state, const char * rsc_id,

 void

 process_lrm_event(lrm_state_t *lrm_state, lrmd_event_data_t *op,

-                  struct recurring_op_s *pending, xmlNode *action_xml)

+                  active_op_t *pending, xmlNode *action_xml)

 {

     char *op_id = NULL;

     char *op_key = NULL;

diff --git a/daemons/controld/controld_execd_state.c b/daemons/controld/controld_execd_state.c

index 0e21d18..473da97 100644

--- a/daemons/controld/controld_execd_state.c

+++ b/daemons/controld/controld_execd_state.c

@@ -44,7 +44,7 @@ free_deletion_op(gpointer value)

 static void

 free_recurring_op(gpointer value)

 {

-    struct recurring_op_s *op = (struct recurring_op_s *)value;

+    active_op_t *op = value;

     free(op->user_data);

     free(op->rsc_id);

@@ -61,7 +61,7 @@ fail_pending_op(gpointer key, gpointer value, gpointer user_data)

 {

     lrmd_event_data_t event = { 0, };

     lrm_state_t *lrm_state = user_data;

-    struct recurring_op_s *op = (struct recurring_op_s *)value;

+    active_op_t *op = value;

     crm_trace("Pre-emptively failing " CRM_OP_FMT " on %s (call=%s, %s)",

               op->rsc_id, op->op_type, op->interval_ms,

diff --git a/daemons/controld/controld_lrm.h b/daemons/controld/controld_lrm.h

index 598682b..27df5d7 100644

--- a/daemons/controld/controld_lrm.h

+++ b/daemons/controld/controld_lrm.h

@@ -33,8 +33,8 @@ typedef struct resource_history_s {

 void history_free(gpointer data);

-/* TODO - Replace this with lrmd_event_data_t */

-struct recurring_op_s {

+// In-flight action (recurring or pending)

+typedef struct active_op_s {

     guint interval_ms;

     int call_id;

     gboolean remove;

@@ -45,7 +45,7 @@ struct recurring_op_s {

     char *op_key;

     char *user_data;

     GHashTable *params;

-};

+} active_op_t;

 typedef struct lrm_state_s {

     const char *node_name;

@@ -164,4 +164,4 @@ void remote_ra_process_maintenance_nodes(xmlNode *xml);

 gboolean remote_ra_controlling_guest(lrm_state_t * lrm_state);

 void process_lrm_event(lrm_state_t *lrm_state, lrmd_event_data_t *op,

-                       struct recurring_op_s *pending, xmlNode *action_xml);

+                       active_op_t *pending, xmlNode *action_xml);

-- 

1.8.3.1

From 93a59f1df8fe11d365032d75f10cb4189ad2f1f8 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 22 Nov 2019 16:45:31 -0600

Subject: [PATCH 2/4] Refactor: controller: convert active_op_t booleans to

 bitmask

---

 daemons/controld/controld_execd.c | 11 +++++------

 daemons/controld/controld_lrm.h   |  8 ++++++--

 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/daemons/controld/controld_execd.c b/daemons/controld/controld_execd.c

index 48f35dd..2c9d9c0 100644

--- a/daemons/controld/controld_execd.c

+++ b/daemons/controld/controld_execd.c

@@ -1148,18 +1148,17 @@ cancel_op(lrm_state_t * lrm_state, const char *rsc_id, const char *key, int op,

     pending = g_hash_table_lookup(lrm_state->pending_ops, key);

     if (pending) {

-        if (remove && pending->remove == FALSE) {

-            pending->remove = TRUE;

+        if (remove && is_not_set(pending->flags, active_op_remove)) {

+            set_bit(pending->flags, active_op_remove);

             crm_debug("Scheduling %s for removal", key);

         }

-        if (pending->cancelled) {

+        if (is_set(pending->flags, active_op_cancelled)) {

             crm_debug("Operation %s already cancelled", key);

             free(local_key);

             return FALSE;

         }

-

-        pending->cancelled = TRUE;

+        set_bit(pending->flags, active_op_cancelled);

     } else {

         crm_info("No pending op found for %s", key);

@@ -2652,7 +2651,7 @@ process_lrm_event(lrm_state_t *lrm_state, lrmd_event_data_t *op,

         crm_err("Recurring operation %s was cancelled without transition information",

                 op_key);

-    } else if (pending->remove) {

+    } else if (is_set(pending->flags, active_op_remove)) {

         /* This recurring operation was cancelled (by us) and pending, and we

          * have been waiting for it to finish.

          */

diff --git a/daemons/controld/controld_lrm.h b/daemons/controld/controld_lrm.h

index 27df5d7..3ab7048 100644

--- a/daemons/controld/controld_lrm.h

+++ b/daemons/controld/controld_lrm.h

@@ -33,12 +33,16 @@ typedef struct resource_history_s {

 void history_free(gpointer data);

+enum active_op_e {

+    active_op_remove    = (1 << 0),

+    active_op_cancelled = (1 << 1),

+};

+

 // In-flight action (recurring or pending)

 typedef struct active_op_s {

     guint interval_ms;

     int call_id;

-    gboolean remove;

-    gboolean cancelled;

+    uint32_t flags; // bitmask of active_op_e

     time_t start_time;

     char *rsc_id;

     char *op_type;

-- 

1.8.3.1

From 4d087d021d325e26b41a9b36b5b190dc7b25334c Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 22 Nov 2019 16:58:25 -0600

Subject: [PATCH 3/4] Refactor: controller: remove unused argument

---

 daemons/controld/controld_execd.c | 10 +++++-----

 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/daemons/controld/controld_execd.c b/daemons/controld/controld_execd.c

index 2c9d9c0..46c1958 100644

--- a/daemons/controld/controld_execd.c

+++ b/daemons/controld/controld_execd.c

@@ -43,8 +43,8 @@ static int delete_rsc_status(lrm_state_t * lrm_state, const char *rsc_id, int ca

 static lrmd_event_data_t *construct_op(lrm_state_t * lrm_state, xmlNode * rsc_op,

                                        const char *rsc_id, const char *operation);

-static void do_lrm_rsc_op(lrm_state_t * lrm_state, lrmd_rsc_info_t * rsc, const char *operation,

-                          xmlNode * msg, xmlNode * request);

+static void do_lrm_rsc_op(lrm_state_t *lrm_state, lrmd_rsc_info_t *rsc,

+                          const char *operation, xmlNode *msg);

 void send_direct_ack(const char *to_host, const char *to_sys,

                      lrmd_rsc_info_t * rsc, lrmd_event_data_t * op, const char *rsc_id);

@@ -1858,7 +1858,7 @@ do_lrm_invoke(long long action,

                           crm_rsc_delete, user_name);

         } else {

-            do_lrm_rsc_op(lrm_state, rsc, operation, input->xml, input->msg);

+            do_lrm_rsc_op(lrm_state, rsc, operation, input->xml);

         }

         lrmd_free_rsc_info(rsc);

@@ -2170,8 +2170,8 @@ record_pending_op(const char *node_name, lrmd_rsc_info_t *rsc, lrmd_event_data_t

 }

 static void

-do_lrm_rsc_op(lrm_state_t * lrm_state, lrmd_rsc_info_t * rsc, const char *operation, xmlNode * msg,

-              xmlNode * request)

+do_lrm_rsc_op(lrm_state_t *lrm_state, lrmd_rsc_info_t *rsc,

+              const char *operation, xmlNode *msg)

 {

     int call_id = 0;

     char *op_id = NULL;

-- 

1.8.3.1

From 356b417274918b7da6cdd9c72c036c923160b318 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 6 Dec 2019 12:15:05 -0600

Subject: [PATCH 4/4] Refactor: scheduler: combine two "if" statements

... for readability, and ease of adding another block later

---

 lib/pacemaker/pcmk_sched_graph.c | 120 +++++++++++++++++++--------------------

 1 file changed, 60 insertions(+), 60 deletions(-)

diff --git a/lib/pacemaker/pcmk_sched_graph.c b/lib/pacemaker/pcmk_sched_graph.c

index e5a8a01..a6967fe 100644

--- a/lib/pacemaker/pcmk_sched_graph.c

+++ b/lib/pacemaker/pcmk_sched_graph.c

@@ -1088,71 +1088,71 @@ action2xml(action_t * action, gboolean as_input, pe_working_set_t *data_set)

         return action_xml;

     }

-    /* List affected resource */

-    if (action->rsc) {

-        if (is_set(action->flags, pe_action_pseudo) == FALSE) {

-            int lpc = 0;

-

-            xmlNode *rsc_xml = create_xml_node(action_xml, crm_element_name(action->rsc->xml));

-

-            const char *attr_list[] = {

-                XML_AGENT_ATTR_CLASS,

-                XML_AGENT_ATTR_PROVIDER,

-                XML_ATTR_TYPE

-            };

-

-            if (is_set(action->rsc->flags, pe_rsc_orphan) && action->rsc->clone_name) {

-                /* Do not use the 'instance free' name here as that

-                 * might interfere with the instance we plan to keep.

-                 * Ie. if there are more than two named /anonymous/

-                 * instances on a given node, we need to make sure the

-                 * command goes to the right one.

-                 *

-                 * Keep this block, even when everyone is using

-                 * 'instance free' anonymous clone names - it means

-                 * we'll do the right thing if anyone toggles the

-                 * unique flag to 'off'

-                 */

-                crm_debug("Using orphan clone name %s instead of %s", action->rsc->id,

-                          action->rsc->clone_name);

-                crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->clone_name);

-                crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id);

+    if (action->rsc && is_not_set(action->flags, pe_action_pseudo)) {

+        int lpc = 0;

+        xmlNode *rsc_xml = NULL;

+        const char *attr_list[] = {

+            XML_AGENT_ATTR_CLASS,

+            XML_AGENT_ATTR_PROVIDER,

+            XML_ATTR_TYPE

+        };

+

+        // List affected resource

+

+        rsc_xml = create_xml_node(action_xml,

+                                  crm_element_name(action->rsc->xml));

+        if (is_set(action->rsc->flags, pe_rsc_orphan)

+            && action->rsc->clone_name) {

+            /* Do not use the 'instance free' name here as that

+             * might interfere with the instance we plan to keep.

+             * Ie. if there are more than two named /anonymous/

+             * instances on a given node, we need to make sure the

+             * command goes to the right one.

+             *

+             * Keep this block, even when everyone is using

+             * 'instance free' anonymous clone names - it means

+             * we'll do the right thing if anyone toggles the

+             * unique flag to 'off'

+             */

+            crm_debug("Using orphan clone name %s instead of %s", action->rsc->id,

+                      action->rsc->clone_name);

+            crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->clone_name);

+            crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id);

-            } else if (is_not_set(action->rsc->flags, pe_rsc_unique)) {

-                const char *xml_id = ID(action->rsc->xml);

-

-                crm_debug("Using anonymous clone name %s for %s (aka. %s)", xml_id, action->rsc->id,

-                          action->rsc->clone_name);

-

-                /* ID is what we'd like client to use

-                 * ID_LONG is what they might know it as instead

-                 *

-                 * ID_LONG is only strictly needed /here/ during the

-                 * transition period until all nodes in the cluster

-                 * are running the new software /and/ have rebooted

-                 * once (meaning that they've only ever spoken to a DC

-                 * supporting this feature).

-                 *

-                 * If anyone toggles the unique flag to 'on', the

-                 * 'instance free' name will correspond to an orphan

-                 * and fall into the clause above instead

-                 */

-                crm_xml_add(rsc_xml, XML_ATTR_ID, xml_id);

-                if (action->rsc->clone_name && safe_str_neq(xml_id, action->rsc->clone_name)) {

-                    crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->clone_name);

-                } else {

-                    crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id);

-                }

+        } else if (is_not_set(action->rsc->flags, pe_rsc_unique)) {

+            const char *xml_id = ID(action->rsc->xml);

+

+            crm_debug("Using anonymous clone name %s for %s (aka. %s)", xml_id, action->rsc->id,

+                      action->rsc->clone_name);

+            /* ID is what we'd like client to use

+             * ID_LONG is what they might know it as instead

+             *

+             * ID_LONG is only strictly needed /here/ during the

+             * transition period until all nodes in the cluster

+             * are running the new software /and/ have rebooted

+             * once (meaning that they've only ever spoken to a DC

+             * supporting this feature).

+             *

+             * If anyone toggles the unique flag to 'on', the

+             * 'instance free' name will correspond to an orphan

+             * and fall into the clause above instead

+             */

+            crm_xml_add(rsc_xml, XML_ATTR_ID, xml_id);

+            if (action->rsc->clone_name && safe_str_neq(xml_id, action->rsc->clone_name)) {

+                crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->clone_name);

             } else {

-                CRM_ASSERT(action->rsc->clone_name == NULL);

-                crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->id);

+                crm_xml_add(rsc_xml, XML_ATTR_ID_LONG, action->rsc->id);

             }

-            for (lpc = 0; lpc < DIMOF(attr_list); lpc++) {

-                crm_xml_add(rsc_xml, attr_list[lpc],

-                            g_hash_table_lookup(action->rsc->meta, attr_list[lpc]));

-            }

+        } else {

+            CRM_ASSERT(action->rsc->clone_name == NULL);

+            crm_xml_add(rsc_xml, XML_ATTR_ID, action->rsc->id);

+        }

+

+        for (lpc = 0; lpc < DIMOF(attr_list); lpc++) {

+            crm_xml_add(rsc_xml, attr_list[lpc],

+                        g_hash_table_lookup(action->rsc->meta, attr_list[lpc]));

         }

     }

-- 

1.8.3.1