Refactor: controller: Drop some unused symbols and start some best practices #3979

nrwahl2 · 2025-11-03T20:13:05Z

My short-term goal (not done yet) is to get rid of ha_msg_input_t and move its two members directly into fsa_data_t. This will require some more fiddling with register_fsa_error{,_adv}, register_fsa_input{,_adv}, and related functions/macros. That's the main reason that I started working on best practices for the do_X() functions at the end of this PR. I want to make it easier to reason about all this msg_data nonsense.

(For context, some of the controller's macros require a variable named msg_data to exist, even if it's a NULL dummy variable.)

nrwahl2 · 2025-11-04T09:55:24Z

At this point, having added more commits, we'll want to do a cts-lab run. We can break the PR into pieces, however.

clumens · 2025-11-04T14:33:54Z

At this point, having added more commits, we'll want to do a cts-lab run. We can break the PR into pieces, however.

Why don't you break all the "Drop..." commits out into their own PR first.

nrwahl2 · 2025-11-06T23:26:07Z

Updated to tackle all the do_X() handlers, now that #3981 is merged.

* Make function static. * Use uint8_t for log_type. * Assert pointer is non-NULL before dereferencing. * Use pcmk__is_set(). Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

No code changes aside from moving functions around. Signed-off-by: Reid Wahl <[email protected]>

...and scheduler IPC handlers. Signed-off-by: Reid Wahl <[email protected]>

This one is slightly more complex than other recent ones. * Use pcmk__is_set() instead of bitwise-and. * Drop some elses after early returns. * Drop a redundant "action & A_TE_START" check before checking cur_state. We would have returned already if that flag were not set. * Wait to register the TE UUID until after checking error conditions; register it only if our TE start is a success. * Drop the init_ok variable and use early returns instead. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

And add a FIXME comment about some confusing, likely incorrect behavior. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

For convenience Signed-off-by: Reid Wahl <[email protected]>

All callers pass NULL. Signed-off-by: Reid Wahl <[email protected]>

The user_name argument was already ignored. Signed-off-by: Reid Wahl <[email protected]>

* Give update_dc_expected() a name that is clearer, albeit longer. * Declare static variable first_join first, within the narrowest scope possible. * Use bool/true/false instead of gboolean/TRUE/FALSE. * Drop was_nack variable. * Rename tmp1 variable to more meaningful "state". * Handle NULL variables with pcmk__s(). * Create join_confirm immediately before setting its join id attribute. * Declare start_state variable within the narrowest scope possible. * Compare pointers explicitly against NULL. * !AM_I_DC instead of AM_I_DC == FALSE. * Use register_fsa_input_before(). Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

No code changes. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

Note that the "or wanting to shut down" comment was made obsolete by commit 0e4db7b in 2006. Signed-off-by: Reid Wahl <[email protected]>

Surprisingly, this seems to have never been set, even though it's been checked and cleared since at least 2004. Even as of 12c39ec, which added a new check for R_STARTING, nothing was setting it. This allows a couple of functions to be simplified. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

I think this is all we can do before functionizing pieces of do_dc_join_filter_offer(). This adds some parentheses, uses bool instaed of gboolean, avoids comparing against FALSE, moves some assignments to just before the variable is used, reorders declarations in accordance with the order of use, renames ack_nack_bool to accept, etc. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

Also touch do_cl_join_announce() since nothing needs to change within the body. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

And remove some nesting. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

Previously, we relied on a msg_data variable being declared. I found this very non-intuitive. We often created such a variable just as a dummy so that the macro would work. Signed-off-by: Reid Wahl <[email protected]>

Signed-off-by: Reid Wahl <[email protected]>

clumens · 2025-11-07T20:24:32Z

daemons/controld/controld_transition.c

-    } else if (pcmk__is_set(controld_globals.fsa_input_register,
-                            R_TE_CONNECTED)) {
+    }
+    if (pcmk__is_set(controld_globals.fsa_input_register, R_TE_CONNECTED)) {


Picky, but I'd like to see spaces between all these if blocks.

I normally do too. This is kind of my one exception: related if {... early return; } if {... early return; } blocks. I feel like the braces add enough space for good readability as long as there aren't comments.

Either way, I'm not picky about it and don't mind adding the lines.

clumens · 2025-11-07T20:30:39Z

daemons/controld/controld_control.c

-    crm_debug("Registering Signal Handlers");
    mainloop_add_signal(SIGTERM, crm_shutdown);
-    mainloop_add_signal(SIGPIPE, sigpipe_ignore);
+    mainloop_add_signal(SIGPIPE, NULL); // Ignore SIGPIPE


Please also modify the doxygen on mainloop_add_signal to reference that the dispatch param can be NULL.

clumens · 2025-11-07T20:54:47Z

daemons/controld/controld_join_client.c


-    if (!pcmk__str_eq(op, CRM_OP_JOIN_ACKNAK, pcmk__str_casei)) {
-        crm_trace("Ignoring op=%s message", op);
+    if (!pcmk__str_eq(op, CRM_OP_JOIN_ACKNAK, pcmk__str_none)) {


I think when we moved from safe_str_eq(), we used pcmk__str_casei for safety (since there were so ungodly many changes, and we haven't updated them all yet.

clumens · 2025-11-07T21:02:11Z

daemons/controld/controld_join_client.c

        return;
    }
-
    if (!AM_I_DC && controld_is_local_node(welcome_from)) {


I'd prefer to see the spaces preserved on these blocks too.

clumens · 2025-11-07T21:18:10Z

daemons/controld/controld_join_client.c

+        register_fsa_error(I_ERROR);
+        return;
+    }
+    if (controld_globals.dc_name == NULL) {


Likewise with blank lines here too.

clumens · 2025-11-07T21:50:26Z

daemons/controld/controld_join_dc.c

 {
-    crm_debug("Ensuring DC, quorum and node attributes are up-to-date");
-    crm_update_quorum(pcmk__cluster_has_quorum(), TRUE);
+    crm_debug("Ensuring DC, quorum, and node attributes are up to date");


Long live the oxford comma.

clumens · 2025-11-07T21:59:16Z

daemons/controld/controld_join_dc.c

        return 1;
    }

    return 0;


Typo in the commit message - "instaed" instead of "instead". Heh.

clumens · 2025-11-07T22:02:37Z

daemons/controld/controld_join_dc.c


        } else {    // Should always be PCMK__XE_GENERATION_TUPLE
            CRM_LOG_ASSERT(false);
        }


This could also be inverted:

if (!pcmk__xe_is(generation, PCMK__XE_GENERATION_TUPLE)) { CRM_LOG_ASSERT(false); } for (int i = ...) { }

clumens · 2025-11-07T22:11:20Z

daemons/controld/controld_join_dc.c

                  pcmk__plural_s(count_welcomed));
        crmd_join_phase_log(LOG_DEBUG);
-        /* crmd_fsa_stall(FALSE); Needed? */
+        // crmd_fsa_stall(false); Needed?


This has been commented out since its introduction in 0b84e7e. Again, it apparently hasn't caused any problems that we know of, so I don't think we need to keep it around.

clumens · 2025-11-07T22:16:43Z

daemons/controld/controld_transition.c

+        crm_notice("Not invoking the TE because we are not the DC");
+        return;
+    }
+    if (pcmk__is_set(action, A_TE_INVOKE)


Likewise with blank lines in this function.

nrwahl2 requested a review from clumens November 3, 2025 20:13

nrwahl2 force-pushed the nrwahl2-controller branch from b644bc3 to e2701e0 Compare November 3, 2025 20:45

nrwahl2 force-pushed the nrwahl2-controller branch from 08958c2 to 5dd0678 Compare November 6, 2025 23:25

nrwahl2 added 24 commits November 6, 2025 17:27

Refactor: controller: Some best practices in do_log()

741704c

* Make function static. * Use uint8_t for log_type. * Assert pointer is non-NULL before dereferencing. * Use pcmk__is_set(). Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Some best practices in do_lrm_control()

5777d53

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices in do_cib_control()

d6376d5

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Drop forward declaration in controld_schedulerd.c

2cc7517

No code changes aside from moving functions around. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Some best practices in do_pe_control()

0b4c6f5

...and scheduler IPC handlers. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in do_startup()

137eb73

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in do_ha_control()

dd336cb

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices in do_readconfig()

476ee8f

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in do_timer_control()

ae0cbb4

And add a FIXME comment about some confusing, likely incorrect behavior. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices in do_msg_route()

d3ac459

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices in do_recover()

1ca1c73

Signed-off-by: Reid Wahl <[email protected]>

Refactor: libcrmcluster: Best practices for pcmk__update_peer_expected()

044c000

Signed-off-by: Reid Wahl <[email protected]>

Refactor: libcrmcluster: Macro for pcmk__update_peer_expected()

9227068

For convenience Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Drop update_attrd() user_name argument

72f3648

All callers pass NULL. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Drop update_attrd_remote_node_removed() argument

188f521

The user_name argument was already ignored. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: De-nest do_cl_join_finalize_respond() common case

4c8d5bb

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Make join_query_callback() static

75f35a6

No code changes. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in join_query_callback()

45996a0

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in do_cl_join_offer_respond()

ea91b20

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices in do_shutdown_req()

3c9489c

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in do_election_vote()

7faf12e

Note that the "or wanting to shut down" comment was made obsolete by commit 0e4db7b in 2006. Signed-off-by: Reid Wahl <[email protected]>

nrwahl2 added 22 commits November 6, 2025 17:27

Refactor: controller: Minor best practices in do_election_count_vote()

91857cf

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in do_started()

a77fe25

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices in do_cl_join_query()

7c98202

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices fo do_dc_takeover()

031942c

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_dc_release()

14ade6c

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices for do_dc_join_final()

88d06de

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices for do_election_check()

35e119c

Signed-off-by: Reid Wahl <[email protected]>

Doc: controller: Add TODO comment for A_ELECTION_START

1db56da

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_dc_join_offer_all()

6fe79d7

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices for do_dc_join_offer_one()

27abbd0

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_dc_join_ack()

ec7aeea

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Minor best practices in do_dc_join_finalize()

cff552d

Also touch do_cl_join_announce() since nothing needs to change within the body. Signed-off-by: Reid Wahl <[email protected]>

Doc: controller: Add a TODO for do_cl_join_announce()

29d8d24

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_te_invoke()

20469d2

And remove some nesting. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_pe_invoke()

bf6ec0d

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_shutdown()

dfce893

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_stop()

15a9caf

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Best practices for do_exit()

556a2fb

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Clarify register_fsa_error_adv() somewhat

e45ed18

Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Add cur_data arg to register_fsa_error()

4c986c2

Previously, we relied on a msg_data variable being declared. I found this very non-intuitive. We often created such a variable just as a dummy so that the macro would work. Signed-off-by: Reid Wahl <[email protected]>

Refactor: controller: Use register_fsa_error() in more places

73678b7

Signed-off-by: Reid Wahl <[email protected]>

nrwahl2 force-pushed the nrwahl2-controller branch from 5dd0678 to 73678b7 Compare November 6, 2025 23:27

clumens requested changes Nov 7, 2025

View reviewed changes

Refactor: controller: Drop some unused symbols and start some best practices #3979

Are you sure you want to change the base?

Refactor: controller: Drop some unused symbols and start some best practices #3979

Uh oh!

Conversation

nrwahl2 commented Nov 3, 2025

Uh oh!

nrwahl2 commented Nov 4, 2025

Uh oh!

clumens commented Nov 4, 2025

Uh oh!

nrwahl2 commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants