From ef711f968ba143b12fb0654b6d75045a04f83d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Thu, 9 Feb 2017 22:30:17 +0100 Subject: [PATCH 1/6] Refactor: lib/services: unify/add include guards for *.h (wild guess has it that MH stood for long-dead Matahari project) --- lib/services/pcmk-dbus.h | 8 ++++---- lib/services/services_private.h | 6 +++--- lib/services/systemd.h | 5 +++++ lib/services/upstart.h | 7 +++---- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/services/pcmk-dbus.h b/lib/services/pcmk-dbus.h index c575dda..31bcf0e 100644 --- a/lib/services/pcmk-dbus.h +++ b/lib/services/pcmk-dbus.h @@ -8,9 +8,9 @@ #ifndef PCMK_DBUS__H # define PCMK_DBUS__H -#ifndef DBUS_TIMEOUT_USE_DEFAULT -# define DBUS_TIMEOUT_USE_DEFAULT -1 -#endif +# ifndef DBUS_TIMEOUT_USE_DEFAULT +# define DBUS_TIMEOUT_USE_DEFAULT -1 +# endif DBusConnection *pcmk_dbus_connect(void); void pcmk_dbus_connection_setup_with_select(DBusConnection *c); @@ -28,4 +28,4 @@ char *pcmk_dbus_get_property( bool pcmk_dbus_find_error(DBusPendingCall *pending, DBusMessage *reply, DBusError *error); -#endif /* PCMK_DBUS__H */ +#endif /* PCMK_DBUS__H */ diff --git a/lib/services/services_private.h b/lib/services/services_private.h index 43aedc6..0e12d18 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -16,8 +16,8 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ -#ifndef __MH_SERVICES_PRIVATE_H__ -# define __MH_SERVICES_PRIVATE_H__ +#ifndef SERVICES_PRIVATE__H +# define SERVICES_PRIVATE__H #if SUPPORT_DBUS # include @@ -69,4 +69,4 @@ gboolean is_op_blocked(const char *rsc); void services_set_op_pending(svc_action_t *op, DBusPendingCall *pending); #endif -#endif /* __MH_SERVICES_PRIVATE_H__ */ +#endif /* SERVICES_PRIVATE__H */ diff --git a/lib/services/systemd.h b/lib/services/systemd.h index acd832e..17e654e 100644 --- a/lib/services/systemd.h +++ b/lib/services/systemd.h @@ -16,8 +16,13 @@ * Copyright (C) 2012 Andrew Beekhof */ +#ifndef SYSTEMD__H +# define SYSTEMD__H + G_GNUC_INTERNAL GList *systemd_unit_listall(void); G_GNUC_INTERNAL int systemd_unit_exec(svc_action_t * op); G_GNUC_INTERNAL gboolean systemd_unit_exists(const gchar * name); G_GNUC_INTERNAL gboolean systemd_unit_running(const gchar * name); G_GNUC_INTERNAL void systemd_cleanup(void); + +#endif /* SYSTEMD__H */ diff --git a/lib/services/upstart.h b/lib/services/upstart.h index a837226..523d208 100644 --- a/lib/services/upstart.h +++ b/lib/services/upstart.h @@ -13,12 +13,11 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA * - * File: upstart-dbus.c * Copyright (C) 2010 Senko Rasic * Copyright (c) 2010 Ante Karamatic */ -#ifndef _UPSTART_DBUS_H_ -# define _UPSTART_DBUS_H_ +#ifndef UPSTART__H +# define UPSTART__H # include # include "crm/services.h" @@ -29,4 +28,4 @@ G_GNUC_INTERNAL gboolean upstart_job_exists(const gchar * name); G_GNUC_INTERNAL gboolean upstart_job_running(const gchar * name); G_GNUC_INTERNAL void upstart_cleanup(void); -#endif /* _UPSTART_DBUS_H_ */ +#endif /* UPSTART__H */ -- 1.8.3.1 From 5dc4937b9588b3fb1d4297eedd7c433aafd8cc81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Thu, 9 Feb 2017 22:40:39 +0100 Subject: [PATCH 2/6] Refactor: lib/services: make *.h self-sufficient wrt. other includes Also make function declaration follow the actual definitions. --- lib/services/pcmk-dbus.h | 2 ++ lib/services/services_private.h | 3 +++ lib/services/systemd.h | 5 ++++- lib/services/upstart.h | 2 +- 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/services/pcmk-dbus.h b/lib/services/pcmk-dbus.h index 31bcf0e..e071349 100644 --- a/lib/services/pcmk-dbus.h +++ b/lib/services/pcmk-dbus.h @@ -8,6 +8,8 @@ #ifndef PCMK_DBUS__H # define PCMK_DBUS__H +# include + # ifndef DBUS_TIMEOUT_USE_DEFAULT # define DBUS_TIMEOUT_USE_DEFAULT -1 # endif diff --git a/lib/services/services_private.h b/lib/services/services_private.h index 0e12d18..60b1b14 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -19,6 +19,9 @@ #ifndef SERVICES_PRIVATE__H # define SERVICES_PRIVATE__H +# include +# include "crm/services.h" + #if SUPPORT_DBUS # include #endif diff --git a/lib/services/systemd.h b/lib/services/systemd.h index 17e654e..98150dd 100644 --- a/lib/services/systemd.h +++ b/lib/services/systemd.h @@ -19,8 +19,11 @@ #ifndef SYSTEMD__H # define SYSTEMD__H +# include +# include "crm/services.h" + G_GNUC_INTERNAL GList *systemd_unit_listall(void); -G_GNUC_INTERNAL int systemd_unit_exec(svc_action_t * op); +G_GNUC_INTERNAL gboolean systemd_unit_exec(svc_action_t * op); G_GNUC_INTERNAL gboolean systemd_unit_exists(const gchar * name); G_GNUC_INTERNAL gboolean systemd_unit_running(const gchar * name); G_GNUC_INTERNAL void systemd_cleanup(void); diff --git a/lib/services/upstart.h b/lib/services/upstart.h index 523d208..8d41c11 100644 --- a/lib/services/upstart.h +++ b/lib/services/upstart.h @@ -23,7 +23,7 @@ # include "crm/services.h" G_GNUC_INTERNAL GList *upstart_job_listall(void); -G_GNUC_INTERNAL int upstart_job_exec(svc_action_t * op, gboolean synchronous); +G_GNUC_INTERNAL gboolean upstart_job_exec(svc_action_t * op, gboolean synchronous); G_GNUC_INTERNAL gboolean upstart_job_exists(const gchar * name); G_GNUC_INTERNAL gboolean upstart_job_running(const gchar * name); G_GNUC_INTERNAL void upstart_cleanup(void); -- 1.8.3.1 From 6b12e0d1284cac96e66780aa4aeb7c32619089fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Thu, 9 Feb 2017 22:46:42 +0100 Subject: [PATCH 3/6] Refactor: lib/services: ensure *.h declare externally hidden functions ...following the precedent of {upstart,systemd}.h. Note also that since hardening-related commit 658fff944 [$LD -z now], it's even more desirable to limit "symbol smog" as much as possible and symbols only used within the library like these are trivially expendable (assuming suitable toolchain). As we use G_GNU_C_INTERNAL macro from glib2, we should care to require at least that version that introduced it, i.e., 2.6 per documentation. --- lib/services/pcmk-dbus.h | 13 +++++++++++++ lib/services/services_private.h | 14 ++++++++++++++ pacemaker.spec.in | 4 +++- 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/services/pcmk-dbus.h b/lib/services/pcmk-dbus.h index e071349..3f3547e 100644 --- a/lib/services/pcmk-dbus.h +++ b/lib/services/pcmk-dbus.h @@ -14,19 +14,32 @@ # define DBUS_TIMEOUT_USE_DEFAULT -1 # endif +G_GNUC_INTERNAL DBusConnection *pcmk_dbus_connect(void); + +G_GNUC_INTERNAL void pcmk_dbus_connection_setup_with_select(DBusConnection *c); + +G_GNUC_INTERNAL void pcmk_dbus_disconnect(DBusConnection *connection); +G_GNUC_INTERNAL DBusPendingCall *pcmk_dbus_send(DBusMessage *msg, DBusConnection *connection, void(*done)(DBusPendingCall *pending, void *user_data), void *user_data, int timeout); + +G_GNUC_INTERNAL DBusMessage *pcmk_dbus_send_recv(DBusMessage *msg, DBusConnection *connection, DBusError *error, int timeout); + +G_GNUC_INTERNAL bool pcmk_dbus_type_check(DBusMessage *msg, DBusMessageIter *field, int expected, const char *function, int line); + +G_GNUC_INTERNAL char *pcmk_dbus_get_property( DBusConnection *connection, const char *target, const char *obj, const gchar * iface, const char *name, void (*callback)(const char *name, const char *value, void *userdata), void *userdata, DBusPendingCall **pending, int timeout); +G_GNUC_INTERNAL bool pcmk_dbus_find_error(DBusPendingCall *pending, DBusMessage *reply, DBusError *error); diff --git a/lib/services/services_private.h b/lib/services/services_private.h index 60b1b14..41895a9 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -45,30 +45,44 @@ struct svc_action_private_s { #endif }; +G_GNUC_INTERNAL GList *services_os_get_directory_list(const char *root, gboolean files, gboolean executable); +G_GNUC_INTERNAL gboolean services_os_action_execute(svc_action_t * op, gboolean synchronous); +G_GNUC_INTERNAL GList *resources_os_list_lsb_agents(void); +G_GNUC_INTERNAL GList *resources_os_list_ocf_providers(void); +G_GNUC_INTERNAL GList *resources_os_list_ocf_agents(const char *provider); +G_GNUC_INTERNAL GList *resources_os_list_nagios_agents(void); +G_GNUC_INTERNAL gboolean cancel_recurring_action(svc_action_t * op); +G_GNUC_INTERNAL gboolean recurring_action_timer(gpointer data); + +G_GNUC_INTERNAL gboolean operation_finalize(svc_action_t * op); +G_GNUC_INTERNAL void services_add_inflight_op(svc_action_t *op); +G_GNUC_INTERNAL void handle_blocked_ops(void); +G_GNUC_INTERNAL gboolean is_op_blocked(const char *rsc); #if SUPPORT_DBUS +G_GNUC_INTERNAL void services_set_op_pending(svc_action_t *op, DBusPendingCall *pending); #endif diff --git a/pacemaker.spec.in b/pacemaker.spec.in index 34a1076..6385350 100644 --- a/pacemaker.spec.in +++ b/pacemaker.spec.in @@ -190,7 +190,9 @@ BuildRequires: coreutils findutils grep sed # Required for core functionality BuildRequires: automake autoconf libtool pkgconfig libtool-ltdl-devel -BuildRequires: pkgconfig(glib-2.0) libxml2-devel libxslt-devel libuuid-devel +## version lower bound for: G_GNUC_INTERNAL +BuildRequires: pkgconfig(glib-2.0) >= 2.6 +BuildRequires: libxml2-devel libxslt-devel libuuid-devel BuildRequires: bzip2-devel pam-devel # Required for agent_config.h which specifies the correct scratch directory -- 1.8.3.1 From 31adceb587c1beb90a0d61bb3bf1b634e5516d54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Fri, 10 Feb 2017 10:50:59 +0100 Subject: [PATCH 4/6] Refactor: lib/services: ensure some objects not exported accidentally They are not tracked by neither public nor private API anyway. See also a note about "symbol smog" in the previous commit. --- lib/services/services.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index 4be425c..b1e3b26 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -37,11 +37,11 @@ /* TODO: Develop a rollover strategy */ static int operations = 0; -GHashTable *recurring_actions = NULL; +static GHashTable *recurring_actions = NULL; /* ops waiting to run async because of conflicting active - * pending ops*/ -GList *blocked_ops = NULL; + * pending ops */ +static GList *blocked_ops = NULL; /* ops currently active (in-flight) */ GList *inflight_ops = NULL; -- 1.8.3.1 From eeeabd732b67d478bb50101e1bb30324ca2c3d9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Thu, 9 Feb 2017 23:06:36 +0100 Subject: [PATCH 5/6] Refactor: lib/services: unify on single source of "synchronous" flag Forerunner systemd.[ch] is followed by the rest of squad in decision based merely on svc_action_t operand with suitably flipped member item and no more on redundant extraneous parameter that in turn is dropped. It was (for the former) and is possible because both a/synchronous paths leading to such decision contains a priori setting of the flag as expected (in services_action_async/services_action_sync, respectively). --- lib/services/services.c | 8 ++++---- lib/services/services_linux.c | 16 ++++++++-------- lib/services/services_private.h | 2 +- lib/services/upstart.c | 2 +- lib/services/upstart.h | 2 +- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index b1e3b26..decb2cb 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -573,14 +573,14 @@ action_async_helper(svc_action_t * op) { if (op->standard && strcasecmp(op->standard, "upstart") == 0) { #if SUPPORT_UPSTART - return upstart_job_exec(op, FALSE); + return upstart_job_exec(op); #endif } else if (op->standard && strcasecmp(op->standard, "systemd") == 0) { #if SUPPORT_SYSTEMD return systemd_unit_exec(op); #endif } else { - return services_os_action_execute(op, FALSE); + return services_os_action_execute(op); } /* The 'op' has probably been freed if the execution functions return TRUE. */ /* Avoid using the 'op' in here. */ @@ -701,14 +701,14 @@ services_action_sync(svc_action_t * op) op->synchronous = true; if (op->standard && strcasecmp(op->standard, "upstart") == 0) { #if SUPPORT_UPSTART - rc = upstart_job_exec(op, TRUE); + rc = upstart_job_exec(op); #endif } else if (op->standard && strcasecmp(op->standard, "systemd") == 0) { #if SUPPORT_SYSTEMD rc = systemd_unit_exec(op); #endif } else { - rc = services_os_action_execute(op, TRUE); + rc = services_os_action_execute(op); } crm_trace(" > %s_%s_%d: %s = %d", op->rsc, op->action, op->interval, op->opaque->exec, op->rc); if (op->stdout_data) { diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c index 16f25f3..adfefaa 100644 --- a/lib/services/services_linux.c +++ b/lib/services/services_linux.c @@ -606,7 +606,7 @@ action_synced_wait(svc_action_t * op, sigset_t *mask) /* For an asynchronous 'op', returns FALSE if 'op' should be free'd by the caller */ /* For a synchronous 'op', returns FALSE if 'op' fails */ gboolean -services_os_action_execute(svc_action_t * op, gboolean synchronous) +services_os_action_execute(svc_action_t * op) { int stdout_fd[2]; int stderr_fd[2]; @@ -641,7 +641,7 @@ services_os_action_execute(svc_action_t * op, gboolean synchronous) int rc = errno; crm_warn("Cannot execute '%s': %s (%d)", op->opaque->exec, pcmk_strerror(rc), rc); services_handle_exec_error(op, rc); - if (!synchronous) { + if (!op->synchronous) { return operation_finalize(op); } return FALSE; @@ -653,7 +653,7 @@ services_os_action_execute(svc_action_t * op, gboolean synchronous) crm_err("pipe(stdout_fd) failed. '%s': %s (%d)", op->opaque->exec, pcmk_strerror(rc), rc); services_handle_exec_error(op, rc); - if (!synchronous) { + if (!op->synchronous) { return operation_finalize(op); } return FALSE; @@ -668,13 +668,13 @@ services_os_action_execute(svc_action_t * op, gboolean synchronous) crm_err("pipe(stderr_fd) failed. '%s': %s (%d)", op->opaque->exec, pcmk_strerror(rc), rc); services_handle_exec_error(op, rc); - if (!synchronous) { + if (!op->synchronous) { return operation_finalize(op); } return FALSE; } - if (synchronous) { + if (op->synchronous) { #ifdef HAVE_SYS_SIGNALFD_H sigemptyset(&mask); sigaddset(&mask, SIGCHLD); @@ -717,7 +717,7 @@ services_os_action_execute(svc_action_t * op, gboolean synchronous) crm_err("Could not execute '%s': %s (%d)", op->opaque->exec, pcmk_strerror(rc), rc); services_handle_exec_error(op, rc); - if (!synchronous) { + if (!op->synchronous) { return operation_finalize(op); } @@ -740,7 +740,7 @@ services_os_action_execute(svc_action_t * op, gboolean synchronous) close(stderr_fd[1]); } - if (synchronous) { + if (op->synchronous) { sigchld_cleanup(); } @@ -758,7 +758,7 @@ services_os_action_execute(svc_action_t * op, gboolean synchronous) op->opaque->stderr_fd = stderr_fd[0]; set_fd_opts(op->opaque->stderr_fd, O_NONBLOCK); - if (synchronous) { + if (op->synchronous) { action_synced_wait(op, pmask); sigchld_cleanup(); } else { diff --git a/lib/services/services_private.h b/lib/services/services_private.h index 41895a9..ec9e1c4 100644 --- a/lib/services/services_private.h +++ b/lib/services/services_private.h @@ -49,7 +49,7 @@ G_GNUC_INTERNAL GList *services_os_get_directory_list(const char *root, gboolean files, gboolean executable); G_GNUC_INTERNAL -gboolean services_os_action_execute(svc_action_t * op, gboolean synchronous); +gboolean services_os_action_execute(svc_action_t * op); G_GNUC_INTERNAL GList *resources_os_list_lsb_agents(void); diff --git a/lib/services/upstart.c b/lib/services/upstart.c index 5651a14..a17c2bf 100644 --- a/lib/services/upstart.c +++ b/lib/services/upstart.c @@ -414,7 +414,7 @@ upstart_async_dispatch(DBusPendingCall *pending, void *user_data) /* For an asynchronous 'op', returns FALSE if 'op' should be free'd by the caller */ /* For a synchronous 'op', returns FALSE if 'op' fails */ gboolean -upstart_job_exec(svc_action_t * op, gboolean synchronous) +upstart_job_exec(svc_action_t * op) { char *job = NULL; int arg_wait = TRUE; diff --git a/lib/services/upstart.h b/lib/services/upstart.h index 8d41c11..6954943 100644 --- a/lib/services/upstart.h +++ b/lib/services/upstart.h @@ -23,7 +23,7 @@ # include "crm/services.h" G_GNUC_INTERNAL GList *upstart_job_listall(void); -G_GNUC_INTERNAL gboolean upstart_job_exec(svc_action_t * op, gboolean synchronous); +G_GNUC_INTERNAL gboolean upstart_job_exec(svc_action_t * op); G_GNUC_INTERNAL gboolean upstart_job_exists(const gchar * name); G_GNUC_INTERNAL gboolean upstart_job_running(const gchar * name); G_GNUC_INTERNAL void upstart_cleanup(void); -- 1.8.3.1 From 6e64b585ba628ef93d99fc17a6a880a76505e27d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= Date: Thu, 9 Feb 2017 23:28:24 +0100 Subject: [PATCH 6/6] Refactor: lib/services: async_helper -> versatile exec_helper Also mark it inline just in case the compiler won't get it that it's just another form of up-to-singly-nested ternary operator making for a tail call. --- lib/services/services.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/lib/services/services.c b/lib/services/services.c index decb2cb..0b535e6 100644 --- a/lib/services/services.c +++ b/lib/services/services.c @@ -568,9 +568,10 @@ handle_duplicate_recurring(svc_action_t * op, void (*action_callback) (svc_actio return FALSE; } -static gboolean -action_async_helper(svc_action_t * op) +inline static gboolean +action_exec_helper(svc_action_t * op) { + /* Whether a/synchronous must be decided (op->synchronous) beforehand. */ if (op->standard && strcasecmp(op->standard, "upstart") == 0) { #if SUPPORT_UPSTART return upstart_job_exec(op); @@ -582,7 +583,8 @@ action_async_helper(svc_action_t * op) } else { return services_os_action_execute(op); } - /* The 'op' has probably been freed if the execution functions return TRUE. */ + /* The 'op' has probably been freed if the execution functions return TRUE + for the asynchronous 'op'. */ /* Avoid using the 'op' in here. */ return FALSE; @@ -625,7 +627,7 @@ services_action_async(svc_action_t * op, void (*action_callback) (svc_action_t * return TRUE; } - return action_async_helper(op); + return action_exec_helper(op); } @@ -670,7 +672,7 @@ handle_blocked_ops(void) continue; } executed_ops = g_list_append(executed_ops, op); - res = action_async_helper(op); + res = action_exec_helper(op); if (res == FALSE) { op->status = PCMK_LRM_OP_ERROR; /* this can cause this function to be called recursively @@ -699,17 +701,7 @@ services_action_sync(svc_action_t * op) } op->synchronous = true; - if (op->standard && strcasecmp(op->standard, "upstart") == 0) { -#if SUPPORT_UPSTART - rc = upstart_job_exec(op); -#endif - } else if (op->standard && strcasecmp(op->standard, "systemd") == 0) { -#if SUPPORT_SYSTEMD - rc = systemd_unit_exec(op); -#endif - } else { - rc = services_os_action_execute(op); - } + rc = action_exec_helper(op); crm_trace(" > %s_%s_%d: %s = %d", op->rsc, op->action, op->interval, op->opaque->exec, op->rc); if (op->stdout_data) { crm_trace(" > stdout: %s", op->stdout_data); -- 1.8.3.1