From 9c0c3d60b4df3076ff27dd176af9b8d131254337 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Thu, 2 Mar 2017 13:52:47 -0600

Subject: [PATCH] Fix: libfencing,fencing: properly remap "action" in

 configuration

"action" should never be specified in fence device configuration.

Previously, this was handled by re-inserting the proper action when creating a

fence command, if the proper action were on a "safe" list (list, monitor,

status, or meta-data).

However, that was insufficient, partly because "on" should have been on the

list, but also because action="reboot" would interfere with reboots that were

remapped to off or off+on.

Now, stonithd intelligently maps any action parameter to pcmk_off_action and/or

pcmk_reboot_action as appropriate when the device configuration is registered,

and libfencing ignores any action parameter that makes it that far.

---

 fencing/commands.c      | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-

 lib/fencing/st_client.c | 52 ++++++++++++++------------------------

 2 files changed, 83 insertions(+), 35 deletions(-)

diff --git a/fencing/commands.c b/fencing/commands.c

index e7fce78..b4e6eb5 100644

--- a/fencing/commands.c

+++ b/fencing/commands.c

@@ -783,6 +783,70 @@ read_action_metadata(stonith_device_t *device)

     freeXpathObject(xpath);

 }

+/*!

+ * \internal

+ * \brief Set a pcmk_*_action parameter if not already set

+ *

+ * \param[in,out] params  Device parameters

+ * \param[in]     action  Name of action

+ * \param[in]     value   Value to use if action is not already set

+ */

+static void

+map_action(GHashTable *params, const char *action, const char *value)

+{

+    char *key = crm_strdup_printf("pcmk_%s_action", action);

+

+    if (g_hash_table_lookup(params, key)) {

+        crm_warn("Ignoring %s='%s', see %s instead",

+                 STONITH_ATTR_ACTION_OP, value, key);

+        free(key);

+    } else {

+        crm_warn("Mapping %s='%s' to %s='%s'",

+                 STONITH_ATTR_ACTION_OP, value, key, value);

+        g_hash_table_insert(params, key, strdup(value));

+    }

+}

+

+/*!

+ * \internal

+ * \brief Create device parameter table from XML

+ *

+ * \param[in]     name    Device name (used for logging only)

+ * \param[in,out] params  Device parameters

+ */

+static GHashTable *

+xml2device_params(const char *name, xmlNode *dev)

+{

+    GHashTable *params = xml2list(dev);

+    const char *value;

+

+    /* Action should never be specified in the device configuration,

+     * but we support it for users who are familiar with other software

+     * that worked that way.

+     */

+    value = g_hash_table_lookup(params, STONITH_ATTR_ACTION_OP);

+    if (value != NULL) {

+        crm_warn("%s has '%s' parameter, which should never be specified in configuration",

+                 name, STONITH_ATTR_ACTION_OP);

+

+        if (strcmp(value, "reboot") == 0) {

+            crm_warn("Ignoring %s='reboot' (see stonith-action cluster property instead)",

+                     STONITH_ATTR_ACTION_OP);

+

+        } else if (strcmp(value, "off") == 0) {

+            map_action(params, "reboot", value);

+

+        } else {

+            map_action(params, "off", value);

+            map_action(params, "reboot", value);

+        }

+

+        g_hash_table_remove(params, STONITH_ATTR_ACTION_OP);

+    }

+

+    return params;

+}

+

 static stonith_device_t *

 build_device_from_xml(xmlNode * msg)

 {

@@ -794,7 +858,7 @@ build_device_from_xml(xmlNode * msg)

     device->id = crm_element_value_copy(dev, XML_ATTR_ID);

     device->agent = crm_element_value_copy(dev, "agent");

     device->namespace = crm_element_value_copy(dev, "namespace");

-    device->params = xml2list(dev);

+    device->params = xml2device_params(device->id, dev);

     value = g_hash_table_lookup(device->params, STONITH_ATTR_HOSTLIST);

     if (value) {

diff --git a/lib/fencing/st_client.c b/lib/fencing/st_client.c

index c7f4079..34bba88 100644

--- a/lib/fencing/st_client.c

+++ b/lib/fencing/st_client.c

@@ -394,11 +394,10 @@ stonith_api_register_level(stonith_t * st, int options, const char *node, int le

 }

 static void

-append_arg(gpointer key, gpointer value, gpointer user_data)

+append_arg(const char *key, const char *value, char **args)

 {

     int len = 3;                /* =, \n, \0 */

     int last = 0;

-    char **args = user_data;

     CRM_CHECK(key != NULL, return);

     CRM_CHECK(value != NULL, return);

@@ -418,22 +417,20 @@ append_arg(gpointer key, gpointer value, gpointer user_data)

     }

     *args = realloc_safe(*args, last + len);

-    crm_trace("Appending: %s=%s", (char *)key, (char *)value);

-    sprintf((*args) + last, "%s=%s\n", (char *)key, (char *)value);

+    crm_trace("Appending: %s=%s", key, value);

+    sprintf((*args) + last, "%s=%s\n", key, value);

 }

 static void

-append_const_arg(const char *key, const char *value, char **arg_list)

+append_config_arg(gpointer key, gpointer value, gpointer user_data)

 {

-    CRM_LOG_ASSERT(key && value);

-    if(key && value) {

-        char *glib_sucks_key = strdup(key);

-        char *glib_sucks_value = strdup(value);

-

-        append_arg(glib_sucks_key, glib_sucks_value, arg_list);

-

-        free(glib_sucks_value);

-        free(glib_sucks_key);

+    /* stonithd will filter action out when it registers the device,

+     * but ignore it here just in case any other library callers

+     * fail to do so.

+     */

+    if (safe_str_neq(key, STONITH_ATTR_ACTION_OP)) {

+        append_arg(key, value, user_data);

+        return;

     }

 }

@@ -446,7 +443,7 @@ append_host_specific_args(const char *victim, const char *map, GHashTable * para

     if (map == NULL) {

         /* The best default there is for now... */

         crm_debug("Using default arg map: port=uname");

-        append_const_arg("port", victim, arg_list);

+        append_arg("port", victim, arg_list);

         return;

     }

@@ -489,7 +486,7 @@ append_host_specific_args(const char *victim, const char *map, GHashTable * para

             if (value) {

                 crm_debug("Setting '%s'='%s' (%s) for %s", name, value, param, victim);

-                append_const_arg(name, value, arg_list);

+                append_arg(name, value, arg_list);

             } else {

                 crm_err("No node attribute '%s' for '%s'", name, victim);

@@ -516,7 +513,6 @@ make_args(const char *agent, const char *action, const char *victim, uint32_t vi

     char buffer[512];

     char *arg_list = NULL;

     const char *value = NULL;

-    const char *_action = action;

     CRM_CHECK(action != NULL, return NULL);

@@ -542,7 +538,7 @@ make_args(const char *agent, const char *action, const char *victim, uint32_t vi

         action = value;

     }

-    append_const_arg(STONITH_ATTR_ACTION_OP, action, &arg_list);

+    append_arg(STONITH_ATTR_ACTION_OP, action, &arg_list);

     if (victim && device_args) {

         const char *alias = victim;

         const char *param = g_hash_table_lookup(device_args, STONITH_ATTR_HOSTARG);

@@ -554,13 +550,13 @@ make_args(const char *agent, const char *action, const char *victim, uint32_t vi

         /* Always supply the node's name too:

          *    https://fedorahosted.org/cluster/wiki/FenceAgentAPI

          */

-        append_const_arg("nodename", victim, &arg_list);

+        append_arg("nodename", victim, &arg_list);

         if (victim_nodeid) {

             char nodeid_str[33] = { 0, };

             if (snprintf(nodeid_str, 33, "%u", (unsigned int)victim_nodeid)) {

                 crm_info("For stonith action (%s) for victim %s, adding nodeid (%s) to parameters",

                          action, victim, nodeid_str);

-                append_const_arg("nodeid", nodeid_str, &arg_list);

+                append_arg("nodeid", nodeid_str, &arg_list);

             }

         }

@@ -592,24 +588,12 @@ make_args(const char *agent, const char *action, const char *victim, uint32_t vi

         if (value == NULL || safe_str_eq(value, "dynamic")) {

             crm_debug("Performing %s action for node '%s' as '%s=%s'", action, victim, param,

                       alias);

-            append_const_arg(param, alias, &arg_list);

+            append_arg(param, alias, &arg_list);

         }

     }

     if (device_args) {

-        g_hash_table_foreach(device_args, append_arg, &arg_list);

-    }

-

-    if(device_args && g_hash_table_lookup(device_args, STONITH_ATTR_ACTION_OP)) {

-        if(safe_str_eq(_action,"list")

-           || safe_str_eq(_action,"status")

-           || safe_str_eq(_action,"monitor")

-           || safe_str_eq(_action,"metadata")) {

-            /* Force use of the calculated command for support ops

-             * We don't want list or monitor ops initiating fencing, regardless of what the admin configured

-             */

-            append_const_arg(STONITH_ATTR_ACTION_OP, action, &arg_list);

-        }

+        g_hash_table_foreach(device_args, append_config_arg, &arg_list);

     }

     return arg_list;

-- 

1.8.3.1