Prevent directly including some internal subheader files #4000

clumens · 2025-11-26T22:04:45Z

No description provided.

clumens · 2025-11-26T22:09:19Z

This is an experiment to see if we can easily have a library-level main internal header file that includes all the subheaders, and then prevent anyone from directly including those subheaders. It's not foolproof, of course, but it just needs to be good enough to prevent us from making mistakes during development.

I only converted two of the private library-level header files over, and only did a little bit of reorganization of #include blocks. There's an almost endless amount of cleanups that could be done on those blocks, and I don't think it's worth doing one giant PR that fixes it all. We can deal with that stuff as we go along.

I think especially with <crm/common/internal.h>, we could have some discussion about whether or not we want one giant main header that everything includes, or if we maybe want xml_internal.h to be includeable too. We could potentially also have an IPC related includeable header file, and maybe or or two more.

include/crm/common/internal.h

include/pacemaker-internal.h

include/crm/common/mainloop_internal.h

include/crm/common/lists_internal.h

include/crm/common/internal.h

daemons/based/pacemaker-based.h

include/crm/common/acl_internal.h

tools/crm_mon.h

tools/crm_resource.h

lib/pacemaker/pcmk_sched_colocation.c

clumens · 2026-01-05T15:29:08Z

Rebased, no changes yet.

Instead of including any of the pcmki/pcmki_* headers, you should include the top-level pacemaker-internal.h instead. And then fix the one place that wasn't following this rule. Ref T777

Instead, go through crm/common/internal.h. I guess it's somewhat debatable if this is necessary since it's an internal header including another internal header, but the fact that it's cross-library makes me want to clean it up to preserve boundaries between libraries. While I'm here, reorganize the include blocks (though I've not gone through and verified everything with include-what-you-use or add the comments for what is being included).

...and remove a couple that are no longer needed.

This is the first part of a project to reduce crm/common/internal.h to a file that basically just includes all the other internal files. After that, we can then use the #error technique to make sure the subheaders are not included anywhere. But first, we need to move everything out of this header to avoid circular imports. Ref T777

Ref T777

This provides a place for internal mainloop related functions from crm/common/internal.h to be moved to. Ref T777

This provides a place for internal flag-related functions from crm/common/internal.h to be moved to. Ref T777

This provides a place for internal memory related functions from crm/common/internal.h to be moved to. I've also moved pcmk__mem_assert from results_internal.h to this file since it feels memory related, but I've left pcmk__assert and pcmk__abort_as. Those don't really belong in that file, but I can't think of anywhere else to put them at the moment. Ref T777

This provides a place for internal procfs-related functions from crm/common/internal.h to be moved to. Ref T777

...and into scores_internal.h where they belong. Ref T777

...and into lists_internal.h where they belong. Ref T777

This provides a place for internal miscellaneous functions from crm/common/internal.h to be moved to. I kind of hate ever having a file named utils, but this lines up with the C file in lib/common/. Ref T777

This provides a place for internal process-related functions from crm/common/internal.h to be moved to. Ref T777

...and into failcounts_internal.h where they belong. Additionally, logging.h needs to include results.h because CRM_CHECK uses crm_abort. It would be worth going through all these header files and make sure they include everything they need, but that's a project for a different day. Ref T777

This provides a place for internal cibsecrets-related functions from crm/common/internal.h to be moved to. Ref T777

It's defined in logging.c in libcrmcommon, so this is a natural place for the extern line to go. Ref T777

…aders. A couple of the subheaders need to be fixed to avoid the circular includes, but it's much better now than it was before all those previous patches to split things out of internal.h. I'm leaving PCMK__NELEM in here because I can't think of a better place for it (maybe utils_internal.h?) and it's not really doing any harm here. I've intentionally left out a couple headers: * unittest_internal.h - This is specific to unit testing and doesn't really have anything to do with public vs. private API. The _internal.h part of its name is mostly meant to make sure it's not included in the source distribution by Makefile.am * various XML headers - xml_internal.h is meant to wrap including all of those, so they don't need to be separately listed. Ref T777

Now that crm/common/internal.h includes all the private header files, crm_internal.h can include just that one. Ref T777

...headers directly. Instead of including any of the crm/common/*_internal.h headers, you should include the top-level crm/common/internal.h instead. And then fix the many, many places that were including these headers. Note that the #ifdef/#error/#endif blocks need to go above any of the multiple inclusion guards. This is because every file includes crm_internal.h first, which includes crm/common/internal.h, which includes all the internal headers, which causes the multilple inclusion guards to be set. Then when those internal headers are included directly later on, the guards will prevent the file from being included and also prevent the #error from happening. Ref T777

Ref T777

…ders. These need to be defined or else testing internal headers will fail due to the #error preprocessor directives.

nrwahl2 · 2026-01-05T19:20:04Z

tests/test-headers.sh


    cat >"$TESTFILE" <<EOF
+#define PCMK__INCLUDED_PACEMAKER_INTERNAL_H
+#define PCMK__INCLUDED_CRM_COMMON_INTERNAL_H


Ideally we wouldn't define PCMK__INCLUDED_CRM_COMMON_INTERNAL_H when testing headers in, say, include/pcmki. I'm not concerned enough to hold up the PR over it.

nrwahl2 · 2026-01-05T19:20:17Z

include/crm/common/internal.h

 #ifndef PCMK__CRM_COMMON_INTERNAL__H
 #define PCMK__CRM_COMMON_INTERNAL__H

+#define PCMK__DIRECT_INCLUDE


FWIW, I'm no longer sure what I meant by the above comment.

nrwahl2 · 2026-01-05T19:27:25Z

include/crm/common/internal.h

 #define PCMK__CRM_COMMON_INTERNAL__H

-#include <crm/common/agents_internal.h>
+#include <crm/common/action_relation_internal.h>


This one is out of order, but again, not worth holding things up

clumens changed the title ~~Prevent directly including top-level internal headers~~ Prevent directly including lower-level internal headers Nov 26, 2025

nrwahl2 self-requested a review November 26, 2025 22:08

clumens changed the title ~~Prevent directly including lower-level internal headers~~ Prevent directly including some internal subheader files Nov 26, 2025

nrwahl2 added the review: in progress PRs that are currently being reviewed label Dec 18, 2025

nrwahl2 requested changes Dec 18, 2025

View reviewed changes

nrwahl2 removed the review: in progress PRs that are currently being reviewed label Dec 18, 2025

clumens force-pushed the include-guards branch from 54887e2 to dc5bc77 Compare January 5, 2026 15:28

clumens added 18 commits January 5, 2026 12:45

Refactor: include: Don't allow including pcmki headers directly.

d0d854e

Instead of including any of the pcmki/pcmki_* headers, you should include the top-level pacemaker-internal.h instead. And then fix the one place that wasn't following this rule. Ref T777

Refactor: include: Rearrange includes in crm/common/internal.h

59bf21d

...and remove a couple that are no longer needed.

Refactor: include: Move function definitions into nvpair_internal.h.

59e4e84

Ref T777

Refactor: include: Add mainloop_internal.h.

ea6fb07

This provides a place for internal mainloop related functions from crm/common/internal.h to be moved to. Ref T777

Refactor: include: Add flags_internal.h.

c6f2e93

This provides a place for internal flag-related functions from crm/common/internal.h to be moved to. Ref T777

Refactor: include: Add procfs_internal.h.

fa7c46a

This provides a place for internal procfs-related functions from crm/common/internal.h to be moved to. Ref T777

Refactor: include: Move score-related variables out of internal.h.

ced91ba

...and into scores_internal.h where they belong. Ref T777

Refactor: include: Move list-related functions out of internal.h.

b8ee429

...and into lists_internal.h where they belong. Ref T777

Refactor: include: Add utils_internal.h.

b8d7956

This provides a place for internal miscellaneous functions from crm/common/internal.h to be moved to. I kind of hate ever having a file named utils, but this lines up with the C file in lib/common/. Ref T777

Refactor: include: Add pid_internal.h.

45ccd34

This provides a place for internal process-related functions from crm/common/internal.h to be moved to. Ref T777

Refactor: include: Add cib_secrets_internal.h.

f9caed0

This provides a place for internal cibsecrets-related functions from crm/common/internal.h to be moved to. Ref T777

Refactor: include: Move pcmk__is_daemon into logging_internal.h.

c194865

It's defined in logging.c in libcrmcommon, so this is a natural place for the extern line to go. Ref T777

Refactor: include: Simplify includes in crm_internal.h.

63f4d5a

Now that crm/common/internal.h includes all the private header files, crm_internal.h can include just that one. Ref T777

clumens force-pushed the include-guards branch from dc5bc77 to 7aefc04 Compare January 5, 2026 18:39

clumens added 2 commits January 5, 2026 13:44

Refactor: libcrmcommon: Fix internal includes for unit tests.

6222148

Ref T777

clumens added 2 commits January 5, 2026 13:44

Test: test-headers.sh: Set internal include guards before testing hea…

ce0c423

…ders. These need to be defined or else testing internal headers will fail due to the #error preprocessor directives.

Refactor: include: Move PCMK__NELEMS to utils_internal.h.

620d334

clumens force-pushed the include-guards branch from 7aefc04 to 620d334 Compare January 5, 2026 18:47

clumens added the review: in progress PRs that are currently being reviewed label Jan 5, 2026

nrwahl2 approved these changes Jan 5, 2026

View reviewed changes

nrwahl2 reviewed Jan 5, 2026

View reviewed changes

nrwahl2 removed the review: in progress PRs that are currently being reviewed label Jan 5, 2026

nrwahl2 mentioned this pull request Jan 5, 2026

Clean up based_io.c #4016

Open

nrwahl2 merged commit 81f6aed into ClusterLabs:main Jan 5, 2026
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prevent directly including some internal subheader files #4000

Prevent directly including some internal subheader files #4000

clumens commented Nov 26, 2025

Uh oh!

clumens commented Nov 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clumens commented Jan 5, 2026

Uh oh!

nrwahl2 Jan 5, 2026

Uh oh!

nrwahl2 Jan 5, 2026

Uh oh!

nrwahl2 Jan 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Prevent directly including some internal subheader files #4000

Prevent directly including some internal subheader files #4000

Conversation

clumens commented Nov 26, 2025

Uh oh!

clumens commented Nov 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clumens commented Jan 5, 2026

Uh oh!

nrwahl2 Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

nrwahl2 Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

nrwahl2 Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants