From dd7dfc413eb6e6dc0c8ead6e16a989ad419d0c0a Mon Sep 17 00:00:00 2001 From: Ken Gaillot Date: Jun 10 2020 17:56:11 +0000 Subject: Refactor: scheduler: reduce code duplication when displaying resources Refactor native_output_string() to use GString, for readability and maintainability. Refactor common_print() to use it, to reduce duplication and ensure displays are consistent. This makes a couple small changes in how things are shown: * If pe_print_dev is enabled (a debugging flag not actually used by anything), the additional resource fields are shown with the resource flags rather than their own parenthesized list. * The new output model is now consistent with the legacy print model in displaying resource flags with commas (not spaces) between them. --- diff --git a/include/crm/pengine/common.h b/include/crm/pengine/common.h index e497f9c..48c2b66 100644 --- a/include/crm/pengine/common.h +++ b/include/crm/pengine/common.h @@ -1,22 +1,12 @@ -/* - * Copyright 2004-2018 the Pacemaker project contributors +/* + * Copyright 2004-2019 the Pacemaker project contributors * * The version control history for this file may have further details. - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2 of the License, or (at your option) any later version. - * - * This software is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * This source code is licensed under the GNU Lesser General Public License + * version 2.1 or later (LGPLv2.1+) WITHOUT ANY WARRANTY. */ + #ifndef PE_COMMON__H # define PE_COMMON__H @@ -104,7 +94,7 @@ enum pe_print_options { pe_print_html = 0x0002, pe_print_ncurses = 0x0004, pe_print_printf = 0x0008, - pe_print_dev = 0x0010, + pe_print_dev = 0x0010, // Debugging (@COMPAT probably not useful) pe_print_details = 0x0020, pe_print_max_details = 0x0040, pe_print_rsconly = 0x0080, diff --git a/lib/pengine/native.c b/lib/pengine/native.c index 2f45768..7cad451 100644 --- a/lib/pengine/native.c +++ b/lib/pengine/native.c @@ -1,5 +1,5 @@ /* - * Copyright 2004-2019 the Pacemaker project contributors + * Copyright 2004-2020 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -491,165 +491,172 @@ native_print_xml(resource_t * rsc, const char *pre_text, long options, void *pri } } -/* making this inline rather than a macro prevents a coverity "unreachable" - * warning on the first usage - */ -static inline const char * -comma_if(int i) +// Append a flag to resource description string's flags list +static bool +add_output_flag(GString *s, const char *flag_desc, bool have_flags) { - return i? ", " : ""; + g_string_append(s, (have_flags? ", " : " (")); + g_string_append(s, flag_desc); + return true; } -static char * -flags_string(pe_resource_t *rsc, pe_node_t *node, long options, - const char *target_role) +// Append a node name to resource description string's node list +static bool +add_output_node(GString *s, const char *node, bool have_nodes) { - char *flags[6] = { NULL, }; - char *result = NULL; - int ndx = 0; + g_string_append(s, (have_nodes? " " : " [ ")); + g_string_append(s, node); + return true; +} + +/*! + * \internal + * \brief Create a string description of a resource + * + * \param[in] rsc Resource to describe + * \param[in] name Desired identifier for the resource + * \param[in] node If not NULL, node that resource is "on" + * \param[in] options Bitmask of pe_print_* + * \param[in] target_role Resource's target role + * \param[in] show_nodes Whether to display nodes when multiply active + * + * \return Newly allocated string description of resource + * \note Caller must free the result with g_free(). + */ +static gchar * +native_output_string(pe_resource_t *rsc, const char *name, pe_node_t *node, + long options, const char *target_role, bool show_nodes) +{ + const char *class = crm_element_value(rsc->xml, XML_AGENT_ATTR_CLASS); + const char *provider = NULL; + const char *kind = crm_element_value(rsc->xml, XML_ATTR_TYPE); + char *retval = NULL; + GString *outstr = NULL; + bool have_flags = false; + + CRM_CHECK(name != NULL, name = "unknown"); + CRM_CHECK(kind != NULL, kind = "unknown"); + CRM_CHECK(class != NULL, class = "unknown"); + + if (is_set(pcmk_get_ra_caps(class), pcmk_ra_cap_provider)) { + provider = crm_element_value(rsc->xml, XML_AGENT_ATTR_PROVIDER); + } - if (node && node->details->online == FALSE && node->details->unclean) { - flags[ndx++] = strdup("UNCLEAN"); + if (is_set(options, pe_print_rsconly) + || pcmk__list_of_multiple(rsc->running_on)) { + node = NULL; } + // We need a string of at least this size + outstr = g_string_sized_new(strlen(name) + strlen(class) + strlen(kind) + + (provider? (strlen(provider) + 2) : 0) + + (node? strlen(node->details->uname) + 1 : 0) + + 11); + + // Resource name and agent + g_string_printf(outstr, "%s\t(%s%s%s:%s):\t", name, class, + /* @COMPAT This should be a single ':' (see CLBZ#5395) but + * to avoid breaking anything relying on it, we're keeping + * it like this until the next minor version bump. + */ + (provider? "::" : ""), (provider? provider : ""), kind); + + // State on node + if (is_set(rsc->flags, pe_rsc_orphan)) { + g_string_append(outstr, " ORPHANED"); + } + if (is_set(rsc->flags, pe_rsc_failed)) { + enum rsc_role_e role = native_displayable_role(rsc); + + if (role > RSC_ROLE_SLAVE) { + g_string_append_printf(outstr, " FAILED %s", role2text(role)); + } else { + g_string_append(outstr, " FAILED"); + } + } else { + g_string_append(outstr, native_displayable_state(rsc, options)); + } + if (node) { + g_string_append_printf(outstr, " %s", node->details->uname); + } + + // Flags, as: ( [...]) + if (node && !(node->details->online) && node->details->unclean) { + have_flags = add_output_flag(outstr, "UNCLEAN", have_flags); + } if (is_set(options, pe_print_pending)) { const char *pending_task = native_pending_task(rsc); if (pending_task) { - flags[ndx++] = strdup(pending_task); + have_flags = add_output_flag(outstr, pending_task, have_flags); } } - if (target_role) { enum rsc_role_e target_role_e = text2role(target_role); - /* Ignore target role Started, as it is the default anyways - * (and would also allow a Master to be Master). - * Show if target role limits our abilities. */ + /* Only show target role if it limits our abilities (i.e. ignore + * Started, as it is the default anyways, and doesn't prevent the + * resource from becoming Master). + */ if (target_role_e == RSC_ROLE_STOPPED) { - flags[ndx++] = strdup("disabled"); + have_flags = add_output_flag(outstr, "disabled", have_flags); } else if (is_set(uber_parent(rsc)->flags, pe_rsc_promotable) && target_role_e == RSC_ROLE_SLAVE) { - flags[ndx++] = crm_strdup_printf("target-role:%s", target_role); + have_flags = add_output_flag(outstr, "target-role:", have_flags); + g_string_append(outstr, target_role); } } - if (is_set(rsc->flags, pe_rsc_block)) { - flags[ndx++] = strdup("blocked"); - + have_flags = add_output_flag(outstr, "blocked", have_flags); } else if (is_not_set(rsc->flags, pe_rsc_managed)) { - flags[ndx++] = strdup("unmanaged"); + have_flags = add_output_flag(outstr, "unmanaged", have_flags); } - if (is_set(rsc->flags, pe_rsc_failure_ignored)) { - flags[ndx++] = strdup("failure ignored"); + have_flags = add_output_flag(outstr, "failure ignored", have_flags); } - - if (ndx > 0) { - char *total = g_strjoinv(" ", flags); - - result = crm_strdup_printf(" (%s)", total); - g_free(total); - } - - while (--ndx >= 0) { - free(flags[ndx]); - } - return result; -} - -static char * -native_output_string(resource_t *rsc, const char *name, node_t *node, long options, - const char *target_role) { - const char *desc = NULL; - const char *class = crm_element_value(rsc->xml, XML_AGENT_ATTR_CLASS); - const char *kind = crm_element_value(rsc->xml, XML_ATTR_TYPE); - enum rsc_role_e role = native_displayable_role(rsc); - - char *retval = NULL; - - char *unames = NULL; - char *provider = NULL; - const char *orphan = NULL; - char *role_s = NULL; - char *node_s = NULL; - char *print_dev_s = NULL; - char *flags_s = NULL; - - CRM_ASSERT(kind != NULL); - - if (is_set(pcmk_get_ra_caps(class), pcmk_ra_cap_provider)) { - provider = crm_strdup_printf("::%s", crm_element_value(rsc->xml, XML_AGENT_ATTR_PROVIDER)); + if (is_set(options, pe_print_dev)) { + if (is_set(options, pe_rsc_provisional)) { + have_flags = add_output_flag(outstr, "provisional", have_flags); + } + if (is_not_set(options, pe_rsc_runnable)) { + have_flags = add_output_flag(outstr, "non-startable", have_flags); + } + have_flags = add_output_flag(outstr, "variant:", have_flags); + g_string_append_printf(outstr, "%s priority:%f", + crm_element_name(rsc->xml), + (double) (rsc->priority)); } - - if (is_set(rsc->flags, pe_rsc_orphan)) { - orphan = " ORPHANED"; + if (have_flags) { + g_string_append(outstr, ")"); } - if (role > RSC_ROLE_SLAVE && is_set(rsc->flags, pe_rsc_failed)) { - role_s = crm_strdup_printf(" FAILED %s", role2text(role)); - } else if (is_set(rsc->flags, pe_rsc_failed)) { - role_s = crm_strdup_printf(" FAILED"); - } else { - role_s = crm_strdup_printf(" %s", native_displayable_state(rsc, options)); - } + // User-supplied description + if (is_set(options, pe_print_rsconly) + || pcmk__list_of_multiple(rsc->running_on)) { + const char *desc = crm_element_value(rsc->xml, XML_ATTR_DESC); - if (node) { - node_s = crm_strdup_printf(" %s", node->details->uname); + if (desc) { + g_string_append_printf(outstr, " %s", desc); + } } - if (is_set(options, pe_print_rsconly) || g_list_length(rsc->running_on) > 1) { - desc = crm_element_value(rsc->xml, XML_ATTR_DESC); - } + if (show_nodes && is_not_set(options, pe_print_rsconly) + && pcmk__list_of_multiple(rsc->running_on)) { + bool have_nodes = false; - if (is_not_set(options, pe_print_rsconly) && g_list_length(rsc->running_on) > 1) { - GListPtr gIter = rsc->running_on; - gchar **arr = calloc(g_list_length(rsc->running_on)+1, sizeof(gchar *)); - int i = 0; - char *total = NULL; + for (GList *iter = rsc->running_on; iter != NULL; iter = iter->next) { + pe_node_t *n = (pe_node_t *) iter->data; - for (; gIter != NULL; gIter = gIter->next) { - node_t *n = (node_t *) gIter->data; - arr[i] = (gchar *) strdup(n->details->uname); - i++; + have_nodes = add_output_node(outstr, n->details->uname, have_nodes); + } + if (have_nodes) { + g_string_append(outstr, " ]"); } - - total = g_strjoinv(" ", arr); - unames = crm_strdup_printf(" [ %s ]", total); - - g_free(total); - g_strfreev(arr); } - if (is_set(options, pe_print_dev)) { - print_dev_s = crm_strdup_printf(" (%s%svariant=%s, priority=%f)", - is_set(rsc->flags, pe_rsc_provisional) ? "provisional, " : "", - is_set(rsc->flags, pe_rsc_runnable) ? "" : "non-startable, ", - crm_element_name(rsc->xml), (double)rsc->priority); - } - - flags_s = flags_string(rsc, node, options, target_role); - - retval = crm_strdup_printf("%s\t(%s%s:%s):\t%s%s%s%s%s%s%s%s", - name, class, - provider ? provider : "", - kind, - orphan ? orphan : "", - role_s, - node_s ? node_s : "", - print_dev_s ? print_dev_s : "", - flags_s ? flags_s : "", - desc ? " " : "", desc ? desc : "", - unames ? unames : ""); - - free(provider); - free(role_s); - free(node_s); - free(unames); - free(print_dev_s); - free(flags_s); - + retval = outstr->str; + g_string_free(outstr, FALSE); return retval; } @@ -657,7 +664,6 @@ void pe__common_output_html(pcmk__output_t *out, resource_t * rsc, const char *name, node_t *node, long options) { - char *s = NULL; const char *kind = crm_element_value(rsc->xml, XML_ATTR_TYPE); const char *target_role = NULL; @@ -676,10 +682,6 @@ pe__common_output_html(pcmk__output_t *out, resource_t * rsc, target_role = g_hash_table_lookup(rsc->meta, XML_RSC_ATTR_TARGET_ROLE); } - if ((options & pe_print_rsconly) || g_list_length(rsc->running_on) > 1) { - node = NULL; - } - if (is_not_set(rsc->flags, pe_rsc_managed)) { cl = "rsc-managed"; @@ -699,10 +701,14 @@ pe__common_output_html(pcmk__output_t *out, resource_t * rsc, cl = "rsc-ok"; } - s = native_output_string(rsc, name, node, options, target_role); - list_node = pcmk__output_create_html_node(out, "li", NULL, NULL, NULL); - pcmk_create_html_node(list_node, "span", NULL, cl, s); - free(s); + { + gchar *s = native_output_string(rsc, name, node, options, target_role, + true); + + list_node = pcmk__output_create_html_node(out, "li", NULL, NULL, NULL); + pcmk_create_html_node(list_node, "span", NULL, cl, s); + g_free(s); + } if (is_set(options, pe_print_details)) { GHashTableIter iter; @@ -745,7 +751,6 @@ void pe__common_output_text(pcmk__output_t *out, resource_t * rsc, const char *name, node_t *node, long options) { - char *s = NULL; const char *target_role = NULL; CRM_ASSERT(rsc->variant == pe_native); @@ -759,13 +764,13 @@ pe__common_output_text(pcmk__output_t *out, resource_t * rsc, target_role = g_hash_table_lookup(rsc->meta, XML_RSC_ATTR_TARGET_ROLE); } - if (is_set(options, pe_print_rsconly) || g_list_length(rsc->running_on) > 1) { - node = NULL; - } + { + gchar *s = native_output_string(rsc, name, node, options, target_role, + true); - s = native_output_string(rsc, name, node, options, target_role); - out->list_item(out, NULL, "%s", s); - free(s); + out->list_item(out, NULL, "%s", s); + g_free(s); + } if (is_set(options, pe_print_details)) { GHashTableIter iter; @@ -807,22 +812,14 @@ pe__common_output_text(pcmk__output_t *out, resource_t * rsc, void common_print(resource_t * rsc, const char *pre_text, const char *name, node_t *node, long options, void *print_data) { - const char *desc = NULL; - const char *class = crm_element_value(rsc->xml, XML_AGENT_ATTR_CLASS); - const char *kind = crm_element_value(rsc->xml, XML_ATTR_TYPE); const char *target_role = NULL; - enum rsc_role_e role = native_displayable_role(rsc); - - int offset = 0; - int flagOffset = 0; - char buffer[LINE_MAX]; - char flagBuffer[LINE_MAX]; CRM_ASSERT(rsc->variant == pe_native); - CRM_ASSERT(kind != NULL); if (rsc->meta) { - const char *is_internal = g_hash_table_lookup(rsc->meta, XML_RSC_ATTR_INTERNAL_RSC); + const char *is_internal = g_hash_table_lookup(rsc->meta, + XML_RSC_ATTR_INTERNAL_RSC); + if (crm_is_true(is_internal) && is_not_set(options, pe_print_implicit)) { crm_trace("skipping print of internal resource %s", rsc->id); return; @@ -830,17 +827,13 @@ common_print(resource_t * rsc, const char *pre_text, const char *name, node_t *n target_role = g_hash_table_lookup(rsc->meta, XML_RSC_ATTR_TARGET_ROLE); } - if (pre_text == NULL && (options & pe_print_printf)) { - pre_text = " "; - } - if (options & pe_print_xml) { native_print_xml(rsc, pre_text, options, print_data); return; } - if ((options & pe_print_rsconly) || g_list_length(rsc->running_on) > 1) { - node = NULL; + if ((pre_text == NULL) && (options & pe_print_printf)) { + pre_text = " "; } if (options & pe_print_html) { @@ -850,10 +843,10 @@ common_print(resource_t * rsc, const char *pre_text, const char *name, node_t *n } else if (is_set(rsc->flags, pe_rsc_failed)) { status_print(""); - } else if (rsc->variant == pe_native && (rsc->running_on == NULL)) { + } else if (rsc->running_on == NULL) { status_print(""); - } else if (g_list_length(rsc->running_on) > 1) { + } else if (pcmk__list_of_multiple(rsc->running_on)) { status_print(""); } else if (is_set(rsc->flags, pe_rsc_failure_ignored)) { @@ -864,106 +857,29 @@ common_print(resource_t * rsc, const char *pre_text, const char *name, node_t *n } } - if(pre_text) { - offset += snprintf(buffer + offset, LINE_MAX - offset, "%s", pre_text); - } - offset += snprintf(buffer + offset, LINE_MAX - offset, "%s", name); - offset += snprintf(buffer + offset, LINE_MAX - offset, "\t(%s", class); - if (is_set(pcmk_get_ra_caps(class), pcmk_ra_cap_provider)) { - const char *prov = crm_element_value(rsc->xml, XML_AGENT_ATTR_PROVIDER); - offset += snprintf(buffer + offset, LINE_MAX - offset, "::%s", prov); - } - offset += snprintf(buffer + offset, LINE_MAX - offset, ":%s):\t", kind); - if(is_set(rsc->flags, pe_rsc_orphan)) { - offset += snprintf(buffer + offset, LINE_MAX - offset, " ORPHANED "); - } - if(role > RSC_ROLE_SLAVE && is_set(rsc->flags, pe_rsc_failed)) { - offset += snprintf(buffer + offset, LINE_MAX - offset, "FAILED %s", role2text(role)); - } else if(is_set(rsc->flags, pe_rsc_failed)) { - offset += snprintf(buffer + offset, LINE_MAX - offset, "FAILED"); - } else { - const char *rsc_state = native_displayable_state(rsc, options); - - offset += snprintf(buffer + offset, LINE_MAX - offset, "%s", rsc_state); - } - - if(node) { - offset += snprintf(buffer + offset, LINE_MAX - offset, " %s", node->details->uname); - - if (node->details->online == FALSE && node->details->unclean) { - flagOffset += snprintf(flagBuffer + flagOffset, LINE_MAX - flagOffset, - "%sUNCLEAN", comma_if(flagOffset)); - } - } - - if (options & pe_print_pending) { - const char *pending_task = native_pending_task(rsc); - - if (pending_task) { - flagOffset += snprintf(flagBuffer + flagOffset, LINE_MAX - flagOffset, - "%s%s", comma_if(flagOffset), pending_task); - } - } - - if (target_role) { - enum rsc_role_e target_role_e = text2role(target_role); - - /* Ignore target role Started, as it is the default anyways - * (and would also allow a Master to be Master). - * Show if target role limits our abilities. */ - if (target_role_e == RSC_ROLE_STOPPED) { - flagOffset += snprintf(flagBuffer + flagOffset, LINE_MAX - flagOffset, - "%sdisabled", comma_if(flagOffset)); - - } else if (is_set(uber_parent(rsc)->flags, pe_rsc_promotable) - && target_role_e == RSC_ROLE_SLAVE) { - flagOffset += snprintf(flagBuffer + flagOffset, LINE_MAX - flagOffset, - "%starget-role:%s", comma_if(flagOffset), target_role); - } - } - - if (is_set(rsc->flags, pe_rsc_block)) { - flagOffset += snprintf(flagBuffer + flagOffset, LINE_MAX - flagOffset, - "%sblocked", comma_if(flagOffset)); - - } else if (is_not_set(rsc->flags, pe_rsc_managed)) { - flagOffset += snprintf(flagBuffer + flagOffset, LINE_MAX - flagOffset, - "%sunmanaged", comma_if(flagOffset)); - } - - if(is_set(rsc->flags, pe_rsc_failure_ignored)) { - flagOffset += snprintf(flagBuffer + flagOffset, LINE_MAX - flagOffset, - "%sfailure ignored", comma_if(flagOffset)); - } - - if ((options & pe_print_rsconly) || g_list_length(rsc->running_on) > 1) { - desc = crm_element_value(rsc->xml, XML_ATTR_DESC); - } - - CRM_LOG_ASSERT(offset > 0); - if(flagOffset > 0) { - status_print("%s (%s)%s%s", buffer, flagBuffer, desc?" ":"", desc?desc:""); - } else { - status_print("%s%s%s", buffer, desc?" ":"", desc?desc:""); + { + gchar *resource_s = native_output_string(rsc, name, node, options, + target_role, false); + status_print("%s%s", (pre_text? pre_text : ""), resource_s); + g_free(resource_s); } #if CURSES_ENABLED - if ((options & pe_print_rsconly) || g_list_length(rsc->running_on) > 1) { - /* Done */ - - } else if (options & pe_print_ncurses) { + if (is_set(options, pe_print_ncurses) + && is_not_set(options, pe_print_rsconly) + && !pcmk__list_of_multiple(rsc->running_on)) { /* coverity[negative_returns] False positive */ move(-1, 0); } #endif - if (options & pe_print_html) { + if (is_set(options, pe_print_html)) { status_print(" "); } - if ((options & pe_print_rsconly)) { + if (is_not_set(options, pe_print_rsconly) + && pcmk__list_of_multiple(rsc->running_on)) { - } else if (g_list_length(rsc->running_on) > 1) { GListPtr gIter = rsc->running_on; int counter = 0; @@ -1026,10 +942,6 @@ common_print(resource_t * rsc, const char *pre_text, const char *name, node_t *n GHashTableIter iter; node_t *n = NULL; - status_print("%s\t(%s%svariant=%s, priority=%f)", pre_text, - is_set(rsc->flags, pe_rsc_provisional) ? "provisional, " : "", - is_set(rsc->flags, pe_rsc_runnable) ? "" : "non-startable, ", - crm_element_name(rsc->xml), (double)rsc->priority); status_print("%s\tAllowed Nodes", pre_text); g_hash_table_iter_init(&iter, rsc->allowed_nodes); while (g_hash_table_iter_next(&iter, NULL, (void **)&n)) {