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