From 031e8a5bac5df472796574868f31af700bfa2b2a Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Mon, 6 Jan 2025 12:59:40 -0500 Subject: [PATCH 1/4] Refactor: libcrmservice: systemd_init should return a bool. --- lib/services/systemd.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/services/systemd.c b/lib/services/systemd.c index e3545a5b123..ed517c83e3e 100644 --- a/lib/services/systemd.c +++ b/lib/services/systemd.c @@ -14,6 +14,7 @@ #include #include +#include #include // fopen(), NULL, etc. #include #include @@ -134,7 +135,7 @@ systemd_call_simple_method(const char *method) return reply; } -static gboolean +static bool systemd_init(void) { static int need_init = 1; @@ -153,9 +154,9 @@ systemd_init(void) systemd_proxy = pcmk_dbus_connect(); } if (systemd_proxy == NULL) { - return FALSE; + return false; } - return TRUE; + return true; } static inline char * @@ -546,7 +547,7 @@ systemd_unit_listall(void) DBusMessageIter elem; DBusMessage *reply = NULL; - if (systemd_init() == FALSE) { + if (!systemd_init()) { return NULL; } From 656ec99b060e56b89da08adaba454ce9ae9583ee Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Mon, 13 Jan 2025 14:40:16 -0500 Subject: [PATCH 2/4] Refactor: daemons: Unindent the goagain block in action_complete. If goagain is not set, just bail to the end of the function. Having done that, everything that was in the previous block can be unindented. I think this is a little more legible. No other code changes. --- daemons/execd/execd_commands.c | 90 ++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 42 deletions(-) diff --git a/daemons/execd/execd_commands.c b/daemons/execd/execd_commands.c index 131b7bef6f3..454f8756916 100644 --- a/daemons/execd/execd_commands.c +++ b/daemons/execd/execd_commands.c @@ -820,6 +820,9 @@ action_complete(svc_action_t * action) #ifdef PCMK__TIME_USE_CGT const char *rclass = NULL; bool goagain = false; + int time_sum = 0; + int timeout_left = 0; + int delay = 0; #endif if (!cmd) { @@ -922,59 +925,62 @@ action_complete(svc_action_t * action) #endif #ifdef PCMK__TIME_USE_CGT - if (goagain) { - int time_sum = time_diff_ms(NULL, &(cmd->t_first_run)); - int timeout_left = cmd->timeout_orig - time_sum; - int delay = cmd->timeout_orig / 10; + if (!goagain) { + goto finalize; + } - if(delay >= timeout_left && timeout_left > 20) { - delay = timeout_left/2; - } + time_sum = time_diff_ms(NULL, &(cmd->t_first_run)); + timeout_left = cmd->timeout_orig - time_sum; + delay = cmd->timeout_orig / 10; - delay = QB_MIN(2000, delay); - if (delay < timeout_left) { - cmd->start_delay = delay; - cmd->timeout = timeout_left; + if (delay >= timeout_left && timeout_left > 20) { + delay = timeout_left/2; + } - if (pcmk__result_ok(&(cmd->result))) { - crm_debug("%s %s may still be in progress: re-scheduling (elapsed=%dms, remaining=%dms, start_delay=%dms)", - cmd->rsc_id, cmd->real_action, time_sum, timeout_left, delay); + delay = QB_MIN(2000, delay); + if (delay < timeout_left) { + cmd->start_delay = delay; + cmd->timeout = timeout_left; - } else if (cmd->result.execution_status == PCMK_EXEC_PENDING) { - crm_info("%s %s is still in progress: re-scheduling (elapsed=%dms, remaining=%dms, start_delay=%dms)", - cmd->rsc_id, cmd->action, time_sum, timeout_left, delay); + if (pcmk__result_ok(&(cmd->result))) { + crm_debug("%s %s may still be in progress: re-scheduling (elapsed=%dms, remaining=%dms, start_delay=%dms)", + cmd->rsc_id, cmd->real_action, time_sum, timeout_left, delay); - } else { - crm_notice("%s %s failed '%s' (%d): re-scheduling (elapsed=%dms, remaining=%dms, start_delay=%dms)", - cmd->rsc_id, cmd->action, - services_ocf_exitcode_str(cmd->result.exit_status), - cmd->result.exit_status, time_sum, timeout_left, - delay); - } - - cmd_reset(cmd); - if(rsc) { - rsc->active = NULL; - } - schedule_lrmd_cmd(rsc, cmd); - - /* Don't finalize cmd, we're not done with it yet */ - return; + } else if (cmd->result.execution_status == PCMK_EXEC_PENDING) { + crm_info("%s %s is still in progress: re-scheduling (elapsed=%dms, remaining=%dms, start_delay=%dms)", + cmd->rsc_id, cmd->action, time_sum, timeout_left, delay); } else { - crm_notice("Giving up on %s %s (rc=%d): timeout (elapsed=%dms, remaining=%dms)", - cmd->rsc_id, - (cmd->real_action? cmd->real_action : cmd->action), - cmd->result.exit_status, time_sum, timeout_left); - pcmk__set_result(&(cmd->result), PCMK_OCF_UNKNOWN_ERROR, - PCMK_EXEC_TIMEOUT, - "Investigate reason for timeout, and adjust " - "configured operation timeout if necessary"); - cmd_original_times(cmd); + crm_notice("%s %s failed '%s' (%d): re-scheduling (elapsed=%dms, remaining=%dms, start_delay=%dms)", + cmd->rsc_id, cmd->action, + services_ocf_exitcode_str(cmd->result.exit_status), + cmd->result.exit_status, time_sum, timeout_left, + delay); } + + cmd_reset(cmd); + if (rsc) { + rsc->active = NULL; + } + schedule_lrmd_cmd(rsc, cmd); + + /* Don't finalize cmd, we're not done with it yet */ + return; + + } else { + crm_notice("Giving up on %s %s (rc=%d): timeout (elapsed=%dms, remaining=%dms)", + cmd->rsc_id, + (cmd->real_action? cmd->real_action : cmd->action), + cmd->result.exit_status, time_sum, timeout_left); + pcmk__set_result(&(cmd->result), PCMK_OCF_UNKNOWN_ERROR, + PCMK_EXEC_TIMEOUT, + "Investigate reason for timeout, and adjust " + "configured operation timeout if necessary"); + cmd_original_times(cmd); } #endif +finalize: pcmk__set_result_output(&(cmd->result), services__grab_stdout(action), services__grab_stderr(action)); cmd_finalize(cmd, rsc); From e182eb7d61fb82df2a3703b348a8f0e687985313 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Mon, 3 Feb 2025 12:25:30 -0500 Subject: [PATCH 3/4] Low: libcrmservices: Don't leak msg if systemd_proxy is NULL. --- lib/services/systemd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/services/systemd.c b/lib/services/systemd.c index ed517c83e3e..19878efaa35 100644 --- a/lib/services/systemd.c +++ b/lib/services/systemd.c @@ -105,13 +105,15 @@ systemd_send_recv(DBusMessage *msg, DBusError *error, int timeout) static DBusMessage * systemd_call_simple_method(const char *method) { - DBusMessage *msg = systemd_new_method(method); + DBusMessage *msg = NULL; DBusMessage *reply = NULL; DBusError error; /* Don't call systemd_init() here, because that calls this */ CRM_CHECK(systemd_proxy, return NULL); + msg = systemd_new_method(method); + if (msg == NULL) { crm_err("Could not create message to send %s to systemd", method); return NULL; From fc0ef6cd7a35c8a38957e1f0a5353cfa5c397979 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Mon, 3 Feb 2025 12:46:41 -0500 Subject: [PATCH 4/4] Refactor: libcrmservices: Unref the dbus connection... ...when we disconnect from the bus. We aren't allowed to close the connection since we acquired it with dbus_bus_get which makes it a shared connection. So, this is the best cleanup we can do. --- lib/services/dbus.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/services/dbus.c b/lib/services/dbus.c index 0f98ddbd294..44e2386701f 100644 --- a/lib/services/dbus.c +++ b/lib/services/dbus.c @@ -294,12 +294,12 @@ pcmk_dbus_connect(void) void pcmk_dbus_disconnect(DBusConnection *connection) { - /* Per the DBus documentation, connections created with - * dbus_connection_open() are owned by libdbus and should never be closed. - * - * @TODO Should we call dbus_connection_unref() here? + /* We acquire our dbus connection with dbus_bus_get(), which makes it a + * shared connection. Therefore, we can't close or free it here. The + * best we can do is decrement the reference count so dbus knows when + * there are no more clients connected to it. */ - return; + dbus_connection_unref(connection); } // Custom DBus error names to use