From 7056ae08bfa5cafeec9c454cb40aefa7553af6df Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Thu, 16 Jul 2020 12:53:24 -0400 Subject: [PATCH 1/4] Fix: libcrmcommon: Set out->priv to NULL in free_priv. init won't do anything if priv is not NULL, so when the private data is freed, also set it to NULL. This prevents segfaults when reset is called. --- lib/common/output_html.c | 1 + lib/common/output_log.c | 1 + lib/common/output_text.c | 1 + lib/common/output_xml.c | 1 + tools/crm_mon_curses.c | 1 + 5 files changed, 5 insertions(+) diff --git a/lib/common/output_html.c b/lib/common/output_html.c index c8f0088..fc06641 100644 --- a/lib/common/output_html.c +++ b/lib/common/output_html.c @@ -72,6 +72,7 @@ html_free_priv(pcmk__output_t *out) { g_queue_free(priv->parent_q); g_slist_free(priv->errors); free(priv); + out->priv = NULL; } static bool diff --git a/lib/common/output_log.c b/lib/common/output_log.c index 5b45ce4..0208046 100644 --- a/lib/common/output_log.c +++ b/lib/common/output_log.c @@ -44,6 +44,7 @@ log_free_priv(pcmk__output_t *out) { g_queue_free(priv->prefixes); free(priv); + out->priv = NULL; } static bool diff --git a/lib/common/output_text.c b/lib/common/output_text.c index 54c409a..8f15849 100644 --- a/lib/common/output_text.c +++ b/lib/common/output_text.c @@ -43,6 +43,7 @@ text_free_priv(pcmk__output_t *out) { g_queue_free(priv->parent_q); free(priv); + out->priv = NULL; } static bool diff --git a/lib/common/output_xml.c b/lib/common/output_xml.c index 8565bfe..858da3f 100644 --- a/lib/common/output_xml.c +++ b/lib/common/output_xml.c @@ -54,6 +54,7 @@ xml_free_priv(pcmk__output_t *out) { g_queue_free(priv->parent_q); g_slist_free(priv->errors); free(priv); + out->priv = NULL; } static bool diff --git a/tools/crm_mon_curses.c b/tools/crm_mon_curses.c index d93b847..e9cc023 100644 --- a/tools/crm_mon_curses.c +++ b/tools/crm_mon_curses.c @@ -46,6 +46,7 @@ curses_free_priv(pcmk__output_t *out) { g_queue_free(priv->parent_q); free(priv); + out->priv = NULL; } static bool -- 1.8.3.1 From 3779152993ca0e88dc407c918882568217f1b630 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Thu, 16 Jul 2020 13:50:24 -0400 Subject: [PATCH 2/4] Fix: libcrmcommon: Make reset and finish work more similarly. When finish is called for HTML and XML output formats, various extra nodes and headers are added, errors are added, etc. None of this stuff happens on reset. For the HTML format, this also means things like the CGI headers and title don't get added when reset is called. Make these two functions much more similar. Regression in 2.0.3. See: rhbz#1857728 --- lib/common/output_html.c | 26 ++++++++++++++++---------- lib/common/output_xml.c | 30 ++++++++++++++++-------------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/lib/common/output_html.c b/lib/common/output_html.c index fc06641..6127df2 100644 --- a/lib/common/output_html.c +++ b/lib/common/output_html.c @@ -113,18 +113,11 @@ add_error_node(gpointer data, gpointer user_data) { } static void -html_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy_dest) { +finish_reset_common(pcmk__output_t *out, crm_exit_t exit_status, bool print) { private_data_t *priv = out->priv; htmlNodePtr head_node = NULL; htmlNodePtr charset_node = NULL; - /* If root is NULL, html_init failed and we are being called from pcmk__output_free - * in the pcmk__output_new path. - */ - if (priv == NULL || priv->root == NULL) { - return; - } - if (cgi_output && print) { fprintf(out->dest, "Content-Type: text/html\n\n"); } @@ -174,6 +167,20 @@ html_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy if (print) { htmlDocDump(out->dest, priv->root->doc); } +} + +static void +html_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy_dest) { + private_data_t *priv = out->priv; + + /* If root is NULL, html_init failed and we are being called from pcmk__output_free + * in the pcmk__output_new path. + */ + if (priv == NULL || priv->root == NULL) { + return; + } + + finish_reset_common(out, exit_status, print); if (copy_dest != NULL) { *copy_dest = copy_xml(priv->root); @@ -185,8 +192,7 @@ html_reset(pcmk__output_t *out) { CRM_ASSERT(out != NULL); if (out->priv != NULL) { - private_data_t *priv = out->priv; - htmlDocDump(out->dest, priv->root->doc); + finish_reset_common(out, CRM_EX_OK, true); } html_free_priv(out); diff --git a/lib/common/output_xml.c b/lib/common/output_xml.c index 858da3f..b64a71d 100644 --- a/lib/common/output_xml.c +++ b/lib/common/output_xml.c @@ -106,17 +106,10 @@ add_error_node(gpointer data, gpointer user_data) { } static void -xml_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy_dest) { +finish_reset_common(pcmk__output_t *out, crm_exit_t exit_status, bool print) { xmlNodePtr node; private_data_t *priv = out->priv; - /* If root is NULL, xml_init failed and we are being called from pcmk__output_free - * in the pcmk__output_new path. - */ - if (priv == NULL || priv->root == NULL) { - return; - } - if (legacy_xml) { GSList *node = priv->errors; @@ -148,6 +141,20 @@ xml_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy_ fprintf(out->dest, "%s", buf); free(buf); } +} + +static void +xml_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy_dest) { + private_data_t *priv = out->priv; + + /* If root is NULL, xml_init failed and we are being called from pcmk__output_free + * in the pcmk__output_new path. + */ + if (priv == NULL || priv->root == NULL) { + return; + } + + finish_reset_common(out, exit_status, print); if (copy_dest != NULL) { *copy_dest = copy_xml(priv->root); @@ -156,15 +163,10 @@ xml_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy_ static void xml_reset(pcmk__output_t *out) { - char *buf = NULL; - CRM_ASSERT(out != NULL); if (out->priv != NULL) { - private_data_t *priv = out->priv; - buf = dump_xml_formatted_with_text(priv->root); - fprintf(out->dest, "%s", buf); - free(buf); + finish_reset_common(out, CRM_EX_OK, true); } xml_free_priv(out); -- 1.8.3.1 From 0f8e4ca5d9a429c934f1e91a1bdf572efd07e0db Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Thu, 16 Jul 2020 16:09:08 -0400 Subject: [PATCH 3/4] Fix: tools, libcrmcommon: Reopen the output dest on reset. This is needed when running crm_mon as a daemon. When we do a reset, we need to clear out any existing output destination and start writing again from the beginning. This really only matters when the destination is a file. The extra freopen at the end of crm_mon is to handle when crm_mon is killed. We need to reset the output file to its beginning before calling finish. --- lib/common/output_html.c | 3 +++ lib/common/output_log.c | 3 +++ lib/common/output_text.c | 3 +++ lib/common/output_xml.c | 3 +++ tools/crm_mon.c | 9 +++++++++ 5 files changed, 21 insertions(+) diff --git a/lib/common/output_html.c b/lib/common/output_html.c index 6127df2..6e21031 100644 --- a/lib/common/output_html.c +++ b/lib/common/output_html.c @@ -191,6 +191,9 @@ static void html_reset(pcmk__output_t *out) { CRM_ASSERT(out != NULL); + out->dest = freopen(NULL, "w", out->dest); + CRM_ASSERT(out->dest != NULL); + if (out->priv != NULL) { finish_reset_common(out, CRM_EX_OK, true); } diff --git a/lib/common/output_log.c b/lib/common/output_log.c index 0208046..8422ac2 100644 --- a/lib/common/output_log.c +++ b/lib/common/output_log.c @@ -72,6 +72,9 @@ static void log_reset(pcmk__output_t *out) { CRM_ASSERT(out != NULL); + out->dest = freopen(NULL, "w", out->dest); + CRM_ASSERT(out->dest != NULL); + log_free_priv(out); log_init(out); } diff --git a/lib/common/output_text.c b/lib/common/output_text.c index 8f15849..2f7e5b0 100644 --- a/lib/common/output_text.c +++ b/lib/common/output_text.c @@ -75,6 +75,9 @@ static void text_reset(pcmk__output_t *out) { CRM_ASSERT(out != NULL); + out->dest = freopen(NULL, "w", out->dest); + CRM_ASSERT(out->dest != NULL); + text_free_priv(out); text_init(out); } diff --git a/lib/common/output_xml.c b/lib/common/output_xml.c index b64a71d..9f8e01b 100644 --- a/lib/common/output_xml.c +++ b/lib/common/output_xml.c @@ -165,6 +165,9 @@ static void xml_reset(pcmk__output_t *out) { CRM_ASSERT(out != NULL); + out->dest = freopen(NULL, "w", out->dest); + CRM_ASSERT(out->dest != NULL); + if (out->priv != NULL) { finish_reset_common(out, CRM_EX_OK, true); } diff --git a/tools/crm_mon.c b/tools/crm_mon.c index b2e143b..10624c1 100644 --- a/tools/crm_mon.c +++ b/tools/crm_mon.c @@ -2014,6 +2014,10 @@ mon_refresh_display(gpointer user_data) break; } + if (options.daemonize) { + out->reset(out); + } + stonith_history_free(stonith_history); stonith_history = NULL; pe_reset_working_set(mon_data_set); @@ -2179,6 +2183,11 @@ clean_up(crm_exit_t exit_code) * crm_mon to be able to do so. */ if (out != NULL) { + if (options.daemonize) { + out->dest = freopen(NULL, "w", out->dest); + CRM_ASSERT(out->dest != NULL); + } + switch (output_format) { case mon_output_cgi: case mon_output_html: -- 1.8.3.1 From b655c039414d2c7af77c3532222b04684ef1f3d0 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 17 Jul 2020 10:58:32 -0400 Subject: [PATCH 4/4] Fix: tools: Add the http-equiv header to crm_mon at the right time. This header is only getting added on termination, which is not a lot of help if you're running crm_mon in daemonize mode. Putting this header in at the right time requires a couple changes: * pcmk__html_add_header doesn't need a parent argument. It was not being used in the first place. * The extra_headers list in output_html.c should not be freed in the reset function. This means it would get freed after every time the daemonized output is refreshed, which means the header would have to be added every time too. extra_headers will now only be freed when the program exits. This is a behavior change, but I can't see why it's going to be a problem. * To support that, we need to copy each item in the extra_headers list when it gets added to the output XML document. This prevents segfaults when we later free that document. * handle_html_output no longer needs to exist. That function only existed to add the http-equiv header at the end, which is wrong. --- include/crm/common/output.h | 5 ++--- lib/common/output_html.c | 7 ++++--- tools/crm_mon.c | 26 +++++++------------------- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/include/crm/common/output.h b/include/crm/common/output.h index e7c9417..186bcfe 100644 --- a/include/crm/common/output.h +++ b/include/crm/common/output.h @@ -703,15 +703,14 @@ pcmk__output_create_html_node(pcmk__output_t *out, const char *element_name, con * the following code would generate the tag "": * * \code - * pcmk__html_add_header(parent, "meta", "http-equiv", "refresh", "content", "19", NULL); + * pcmk__html_add_header("meta", "http-equiv", "refresh", "content", "19", NULL); * \endcode * - * \param[in,out] parent The node that will be the parent of the new node. * \param[in] name The HTML tag for the new node. * \param[in] ... A NULL-terminated key/value list of attributes. */ void -pcmk__html_add_header(xmlNodePtr parent, const char *name, ...) +pcmk__html_add_header(const char *name, ...) G_GNUC_NULL_TERMINATED; #ifdef __cplusplus diff --git a/lib/common/output_html.c b/lib/common/output_html.c index 6e21031..259e412 100644 --- a/lib/common/output_html.c +++ b/lib/common/output_html.c @@ -139,7 +139,7 @@ finish_reset_common(pcmk__output_t *out, crm_exit_t exit_status, bool print) { /* Add any extra header nodes the caller might have created. */ for (int i = 0; i < g_slist_length(extra_headers); i++) { - xmlAddChild(head_node, g_slist_nth_data(extra_headers, i)); + xmlAddChild(head_node, xmlCopyNode(g_slist_nth_data(extra_headers, i), 1)); } /* Stylesheets are included two different ways. The first is via a built-in @@ -185,6 +185,8 @@ html_finish(pcmk__output_t *out, crm_exit_t exit_status, bool print, void **copy if (copy_dest != NULL) { *copy_dest = copy_xml(priv->root); } + + g_slist_free_full(extra_headers, (GDestroyNotify) xmlFreeNode); } static void @@ -199,7 +201,6 @@ html_reset(pcmk__output_t *out) { } html_free_priv(out); - g_slist_free_full(extra_headers, (GDestroyNotify) xmlFreeNode); html_init(out); } @@ -412,7 +413,7 @@ pcmk__output_create_html_node(pcmk__output_t *out, const char *element_name, con } void -pcmk__html_add_header(xmlNodePtr parent, const char *name, ...) { +pcmk__html_add_header(const char *name, ...) { htmlNodePtr header_node; va_list ap; diff --git a/tools/crm_mon.c b/tools/crm_mon.c index 10624c1..7fd2b9c 100644 --- a/tools/crm_mon.c +++ b/tools/crm_mon.c @@ -1346,6 +1346,12 @@ main(int argc, char **argv) options.mon_ops |= mon_op_print_timing | mon_op_inactive_resources; } + if ((output_format == mon_output_html || output_format == mon_output_cgi) && + out->dest != stdout) { + pcmk__html_add_header("meta", "http-equiv", "refresh", "content", + crm_itoa(options.reconnect_msec/1000), NULL); + } + crm_info("Starting %s", crm_system_name); if (cib) { @@ -2106,15 +2112,6 @@ clean_up_connections(void) } } -static void -handle_html_output(crm_exit_t exit_code) { - xmlNodePtr html = NULL; - - pcmk__html_add_header(html, "meta", "http-equiv", "refresh", "content", - crm_itoa(options.reconnect_msec/1000), NULL); - out->finish(out, exit_code, true, (void **) &html); -} - /* * De-init ncurses, disconnect from the CIB manager, disconnect fencing, * deallocate memory and show usage-message if requested. @@ -2188,16 +2185,7 @@ clean_up(crm_exit_t exit_code) CRM_ASSERT(out->dest != NULL); } - switch (output_format) { - case mon_output_cgi: - case mon_output_html: - handle_html_output(exit_code); - break; - - default: - out->finish(out, exit_code, true, NULL); - break; - } + out->finish(out, exit_code, true, NULL); pcmk__output_free(out); pcmk__unregister_formats(); -- 1.8.3.1