Best practices for a few scheduler functions

include/crm/common/scheduler_internal.h

@@ -279,6 +279,7 @@ extern uint32_t pcmk__warnings;
             (scheduler)->flags, (flags_to_clear), #flags_to_clear);         \
     } while (0)
 
+void pcmk__set_scheduler_defaults(pcmk_scheduler_t *scheduler);
 time_t pcmk__scheduler_epoch_time(pcmk_scheduler_t *scheduler);
 
 #ifdef __cplusplus

lib/common/scheduler.c

@@ -18,6 +18,36 @@
 
 uint32_t pcmk__warnings = 0;
 
+/*!
+ * \internal
+ * \brief Set non-zero default values in scheduler data
+ *
+ * \param[in,out] scheduler  Scheduler data to modify
+ *
+ * \note Values that default to NULL or 0 will remain unchanged
+ */
+void
+pcmk__set_scheduler_defaults(pcmk_scheduler_t *scheduler)
+{
+    pcmk__assert(scheduler != NULL);
+    scheduler->flags = 0U;
+#if PCMK__CONCURRENT_FENCING_DEFAULT_TRUE
+    pcmk__set_scheduler_flags(scheduler,
+                              pcmk__sched_symmetric_cluster
+                              |pcmk__sched_concurrent_fencing
+                              |pcmk__sched_stop_removed_resources
+                              |pcmk__sched_cancel_removed_actions);
+#else
+    pcmk__set_scheduler_flags(scheduler,
+                              pcmk__sched_symmetric_cluster
+                              |pcmk__sched_stop_removed_resources
+                              |pcmk__sched_cancel_removed_actions);
+#endif
+    scheduler->no_quorum_policy = pcmk_no_quorum_stop;
+    scheduler->priv->next_action_id = 1;
+    scheduler->priv->next_ordering_id = 1;
+}
+
 /*!
  * \internal
  * \brief Get the Designated Controller node from scheduler data

lib/common/tests/scheduler/Makefile.am

@@ -12,7 +12,8 @@ include $(top_srcdir)/mk/tap.mk
 include $(top_srcdir)/mk/unittest.mk
 
 # Add "_test" to the end of all test program names to simplify .gitignore.
-check_PROGRAMS = pcmk_get_dc_test			\
+check_PROGRAMS = pcmk__set_scheduler_defaults_test	\
+		 pcmk_get_dc_test			\
 		 pcmk_get_no_quorum_policy_test		\
 		 pcmk_has_quorum_test			\
 		 pcmk_set_scheduler_cib_test

lib/pengine/tests/status/set_working_set_defaults_test.c → lib/common/tests/scheduler/pcmk__set_scheduler_defaults_test.c

@@ -18,13 +18,20 @@
 #include "mock_private.h"
 
 static void
-check_defaults(void **state) {
-    uint32_t flags;
-    pcmk_scheduler_t *scheduler = pcmk__assert_alloc(1,
-                                                     sizeof(pcmk_scheduler_t));
+null_scheduler(void **state)
+{
+    pcmk__assert_asserts(pcmk__set_scheduler_defaults(NULL));
+}
+
+static void
+check_defaults(void **state)
+{
+    uint32_t flags = 0U;
+    pcmk_scheduler_t *scheduler = NULL;
 
+    scheduler = pcmk__assert_alloc(1, sizeof(pcmk_scheduler_t));
     scheduler->priv = pcmk__assert_alloc(1, sizeof(pcmk__scheduler_private_t));
-    set_working_set_defaults(scheduler);
+    pcmk__set_scheduler_defaults(scheduler);
 
     flags = pcmk__sched_symmetric_cluster
 #if PCMK__CONCURRENT_FENCING_DEFAULT_TRUE
@@ -39,12 +46,10 @@ check_defaults(void **state) {
     assert_int_equal(scheduler->no_quorum_policy, pcmk_no_quorum_stop);
     assert_int_equal(scheduler->flags, flags);
 
-    /* Avoid calling pe_free_working_set here so we don't artificially
-     * inflate the coverage numbers.
-     */
     free(scheduler->priv);
     free(scheduler);
 }
 
 PCMK__UNIT_TEST(NULL, NULL,
+                cmocka_unit_test(null_scheduler),
                 cmocka_unit_test(check_defaults))

lib/pengine/status.c

@@ -43,7 +43,7 @@ pe_new_working_set(void)
         free(scheduler);
         return NULL;
     }
-    set_working_set_defaults(scheduler);
+    pcmk__set_scheduler_defaults(scheduler);
     return scheduler;
 }
 
@@ -432,21 +432,7 @@ set_working_set_defaults(pcmk_scheduler_t *scheduler)
     scheduler->priv->local_node_name = local_node_name;
 
     // Set defaults for everything else
-    scheduler->priv->next_ordering_id = 1;
-    scheduler->priv->next_action_id = 1;
-    scheduler->no_quorum_policy = pcmk_no_quorum_stop;
-#if PCMK__CONCURRENT_FENCING_DEFAULT_TRUE
-    pcmk__set_scheduler_flags(scheduler,
-                              pcmk__sched_symmetric_cluster
-                              |pcmk__sched_concurrent_fencing
-                              |pcmk__sched_stop_removed_resources
-                              |pcmk__sched_cancel_removed_actions);
-#else
-    pcmk__set_scheduler_flags(scheduler,
-                              pcmk__sched_symmetric_cluster
-                              |pcmk__sched_stop_removed_resources
-                              |pcmk__sched_cancel_removed_actions);
-#endif
+    pcmk__set_scheduler_defaults(scheduler);
 }
 
 pcmk_resource_t *

lib/pengine/tests/status/Makefile.am

@@ -16,7 +16,6 @@ LDADD += $(top_builddir)/lib/pengine/libpe_status_test.la
 # Add "_test" to the end of all test program names to simplify .gitignore.
 check_PROGRAMS = pe_find_node_any_test 		\
 		 pe_find_node_id_test 		\
-		 pe_new_working_set_test 	\
-		 set_working_set_defaults_test
+		 pe_new_working_set_test
 
 TESTS = $(check_PROGRAMS)

lib/pengine/tests/status/pe_new_working_set_test.c

@@ -26,12 +26,13 @@ calloc_fails(void **state) {
 }
 
 static void
-calloc_succeeds(void **state) {
+calloc_succeeds(void **state)
+{
     pcmk_scheduler_t *scheduler = pe_new_working_set();
 
-    /* Nothing else to test about this function, as all it does is call
-     * set_working_set_defaults which is also a public function and should
-     * get its own unit test.
+    /* We only need to test that the return value is non-NULL, as all the
+     * function does is call pcmk__set_scheduler_defaults(), which should have
+     * its own unit test.
      */
     assert_non_null(scheduler);