From 9c0c3d60b4df3076ff27dd176af9b8d131254337 Mon Sep 17 00:00:00 2001 From: Ken Gaillot 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