based refactors: Simplify IPC channels and cleanup#4144
Open
nrwahl2 wants to merge 23 commits into
Open
Conversation
The responsible commits were written before we replaced all gpointers with void pointers. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
clumens
reviewed
Jul 14, 2026
| pcmk__assert(cb != NULL); | ||
|
|
||
| return mainloop_add_ipc_server(CRM_SYSTEM_CRMD, QB_IPC_SHM, cb); | ||
| } |
Contributor
There was a problem hiding this comment.
Not necessarily for this PR, but I don't see why pcmk__serve_controld_ipc should have a different prototype from all the other similarly named functions.
Contributor
Author
There was a problem hiding this comment.
I started adding a commit for this. I see only one reason why the controller's "serve IPC" function should be treated differently from the others.
- If it fails in
ipc_proxy_init(), we exit fatally. - If it fails in the controller's startup (
do_started()), we shut down the controller and inhibit respawn by registering an error input.
The other pcmk__serve_*_ipc() functions exit fatally no matter what.
We can still make it more similar to the other functions, including using the same prototype.
There were four allowed values: shared-mem, socket, posix, and sysv. Libqb removed support for posix and sysv (message queues) in 2012 via commit 70a9623a. It simply returns an error if you try to set up those types of IPC. I spoke with the libqb maintainers last night, and they said that socket IPC has never worked correctly and that they are considering removing it. Sockets were mainly intended for systems where shared memory was not available. That is believed to be no longer applicable -- shared memory should be available on all the platforms that Pacemaker and libqb target for support. That leaves only shared memory as a working option. We are dropping this option rather than deprecating it. As discussed above, two of the values (posix and sysv) will fail immediately during IPC setup, and socket is very buggy. So user-facing behavior should not be affected. If the option is configured, it will simply be ignored. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
QB_IPC_NATIVE tells libqb to use shared memory (QB_IPC_SHM) unless it's
shared-memory IPC is specifically disabled at build time because it's
unavailable on the system. In that case, libqb will use sockets
(QB_IPC_SOCKET).
I spoke with the libqb maintainers last night, and they said that socket
IPC has never worked correctly and that they are considering removing
it. They told me never to use libqb socket IPC.
Libqb socket IPC was mainly intended for systems where shared memory was
not available. That is believed to be no longer applicable -- shared
memory should be available on all the platforms that Pacemaker and libqb
target for support.
The maintainers weakly suggested using QB_IPC_NATIVE (rather than
QB_IPC_SHM). I suppose this was for future-proofing ("let libqb decide
what to use"). Nonetheless, since we have a couple of use cases where we
MUST avoid blocking, and QB_IPC_SHM is non-blocking, we'll just use
QB_IPC_SHM everywhere until we have a reason not to. Besides, this makes
things explicit.
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...argument.
QB_IPC_NATIVE tells libqb to use shared memory (QB_IPC_SHM) unless
shared-memory IPC is specifically disabled at build time because it's
unavailable on the system. In that case, libqb will use sockets
(QB_IPC_SOCKET).
I spoke with the libqb maintainers last night, and they said that socket
IPC has never worked correctly and that they are considering removing
it. They told me never to use libqb socket IPC.
Libqb socket IPC was mainly intended for systems where shared memory was
not available. That is believed to be no longer applicable -- shared
memory should be available on all the platforms that Pacemaker and libqb
target for support.
The maintainers weakly suggested using QB_IPC_NATIVE (rather than
QB_IPC_SHM). I suppose this was for future-proofing ("let libqb decide
what to use"). Nonetheless, since we have a couple of use cases where we
MUST avoid blocking, and QB_IPC_SHM is non-blocking, we'll just use
QB_IPC_SHM everywhere until we have a reason not to.
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It behaves identically to cib_command. This became even more clear with the recent "Use QB_IPC_SHM in pcmk__serve_DAEMON_ipc()" commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And associated code. It behaves identically to PCMK__SERVER_BASED_RW. A key point to note is that only non-proxied cib_native clients connected to PCMK__SERVER_BASED_SHM (prior to deprecation of cib_command_nonblocking in a previous commit). cib_remote clients and proxied connections used only PCMK__SERVER_BASED_RO and PCMK__SERVER_BASED_RW. So backward compatibility should not be a concern -- this affects only local clients on a cluster node. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It has one caller and saves one line at most. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And make the mainloop variable static. No other code changes. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There is still more work to do to consolidate/reorganize the based exit/shutdown-related code. I'm keeping the pieces small to try to make it easier to reason about. based_terminate() seems too complicated. Note that based_ipc_cleanup() calls pcmk__client_cleanup(), which is why we drop the call from main. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There are two call sites. * based_cpg_destroy: This is called only via the main loop. See pcmk_cluster_set_destroy_fn() and pcmk__cpg_connect(). * based_shutdown: We can reach this only through the main loop. It's set up as a main loop signal handler via mainloop_add_signal(). The true signal handler is mainloop_signal_handler(), which sets a main loop trigger to call based_shutdown(). That trigger can't do anything unless the main loop is running. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Mostly for sanity. Between the g_main_loop_quit() call and the main loop actually stopping, the main loop runs callbacks for any sources that have already been dispatched. In case some callback actually runs, it should be able to call based_shutting_down() and get a true return value. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We were allocating an int to hold the server socket file descriptor, and we were never freeing it. (Its destroy callback, based_remote_listener_destroy(), did not free it.) Don't allocate an int, but instead call GINT_TO_POINTER() and GPOINTER_TO_INT() to cast it to and from a pointer. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We will use this soon. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Previously we were only closing the file descriptors. Instead we destroy the mainloop_io_t objects returned by mainloop_add_fd(). In order to do that, we need to start storing those objects in the first place. Previously we were ignoring the return value of mainloop_add_fd() in init_remote_listener(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Return false early where possible instead. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Also take a const argument. This function may kill a process, but it doesn't modify the mainloop_child_t. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2
force-pushed
the
nrwahl2-based_first
branch
from
July 14, 2026 22:46
552e2ce to
fce9934
Compare
Contributor
Author
|
Updated to address review. However, I'm finding at least one issue in cts-lab: The only thing that added a call to |
nrwahl2
marked this pull request as draft
July 14, 2026 23:18
nrwahl2
added
the
waiting for author
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Make it as similar as possible to the other pcmk__serve_X_ipc() functions. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Since it's the legacy equivalent of "controld". Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Contributor
Author
Yeah, it appears to work fine without that commit. Nothing depends on that commit, so I'll drop it for now. |
nrwahl2
marked this pull request as ready for review
July 15, 2026 01:17
nrwahl2
force-pushed
the
nrwahl2-based_first
branch
from
July 15, 2026 01:17
fce9934 to
0fd5c78
Compare
nrwahl2
added
needs attention
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the next batch from #4011.