Prevent directly including some internal subheader files #4000

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

No description provided.

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

...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 elated 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

This needs to be defined or else testing internal headers will fail due to the #error preprocessor directives.

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.

nrwahl2 · 2025-12-01T23:52:35Z

include/crm/common/internal.h

 #include <crm/common/io_internal.h>
 #include <crm/common/iso8601_internal.h>
 #include <crm/common/mainloop_internal.h>
+#include <crm/common/memory_internal.h>


Typo in commit message: "memory elated"

pcmk__assert and pcmk__abort_as could go in a utils_internal.h I guess. I think they live in results because they're abnormal-exit functions, and crm_exit lives in results.

Edit: I see you did create such a file.

utils_internal.h also seems like a very reasonable place for PCMK__NELEM()

nrwahl2 · 2025-12-18T19:19:53Z

include/pacemaker-internal.h

 #ifndef PCMK__PACEMAKER_INTERNAL__H
 #define PCMK__PACEMAKER_INTERNAL__H

+#define PCMK__DIRECT_INCLUDE


I wonder if we should do these per-header. Like PCMK__DIRECT_INCLUDE_PCMKI or PCMK__IN_PACEMAKER_INTERNAL_H or something. (PCMK__DIRECT_INCLUDE_PACEMAKER_INTERNAL__H feels too long but it's even clearer.)

We already have PCMK__PACEMAKER_INTERNAL__H, etc., but those remain defined at the end of the header. We need one that we #undef at the end, as we're doing here with PCMK__DIRECT_INCLUDE.

My thinking is that we don't want any OTHER header that defines PCMK__DIRECT_INCLUDE to be able to include these sub-header files.

nrwahl2 · 2025-12-18T19:48:00Z

include/crm/common/mainloop_internal.h

@@ -0,0 +1,31 @@
+/*
+ * Copyright 2025 the Pacemaker project contributors


Thus far (meaning when Ken was here), we've been conservatively keeping the old copyright date from the file that we split contents out from -- so 2004-2025 in this case. The idea has been that the code is old even though the file is new. Copyright is "current year only" when all the contents of the file are new.

I don't have strong feelings about it, and I don't know to what extent the copyright notices matter.

This comment applies to other new files in this PR as well -- take it or leave it :)

nrwahl2 · 2025-12-18T20:47:57Z

include/crm/common/lists_internal.h

 */
-GList *pcmk__subtract_lists(GList *from, const GList *items,
-                            GCompareFunc cmp);
+GList *pcmk__subtract_lists(GList *from, const GList *items,GCompareFunc cmp);


space between comma and GCompareFunc

nrwahl2 · 2025-12-18T20:49:36Z

include/crm/common/internal.h

 #ifndef PCMK__CRM_COMMON_INTERNAL__H
 #define PCMK__CRM_COMMON_INTERNAL__H

-#include <stdbool.h>            // bool


extern bool pcmk__is_daemon is still present

goes away in a later commit

nrwahl2 · 2025-12-18T21:19:19Z

daemons/based/pacemaker-based.h

 #include <crm/cib.h>
 #include <crm/common/xml.h>
 #include <crm/cluster.h>
-#include <crm/common/ipc_internal.h>


Should this one be replaced by crm/common/internal.h?

nrwahl2 · 2025-12-18T21:20:36Z

include/crm/common/acl_internal.h

 */

+#ifndef PCMK__DIRECT_INCLUDE
+#error "Include <crm/common/internal.h> instead of <acl_internal.h> directly"


These messages all say "...instead of <basename.h> directly". In the pcmki commit, the messages say "...instead of <pcmki/basename.h> directly". We don't have to be consistent, but I do want to point it out.

nrwahl2 · 2025-12-18T21:26:30Z

tools/crm_mon.h


 #include <crm/common/scheduler.h>
-#include <crm/common/output_internal.h>
+#include <crm/common/internal.h>


above scheduler.h if we want alphabetical order

nrwahl2 · 2025-12-18T21:27:23Z

tools/crm_resource.h

 #include <crm/services.h>
 #include <crm/common/xml.h>
 #include <crm/common/mainloop.h>
-#include <crm/common/output_internal.h>


We've been mostly replacing these with crm/common/internal.h in header files. It's up to you -- we can leave it out and rely on the <crm_internal.h> include at the top.

nrwahl2 · 2025-12-18T21:29:28Z

lib/pacemaker/pcmk_sched_colocation.c

 #include <crm/pengine/status.h>
 #include <pacemaker-internal.h>

-#include "crm/common/util.h"


Why are we dropping the includes of util.h and xml.h here?

clumens added 20 commits November 26, 2025 17:00

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

7217f4f

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

6e65ca6

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

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

ae849f3

Ref T777

Refactor: include: Add mainloop_internal.h.

7bfa029

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.

cd4140f

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.

83c69f9

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.

3d30bb8

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

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

a92d107

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

Refactor: include: Add utils_internal.h.

e0cf63f

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.

52f413c

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.

f6904db

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.

103d4ef

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.

916bd75

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

Refactor: libcrmcommon: Fix internal includes for unit tests.

cc4736b

Ref T777

Test: test-headers.sh: Set PCMK__DIRECT_INCLUDE when testing headers.

54887e2

This needs to be defined or else testing internal headers will fail due to the #error preprocessor directives.

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

		@@ -0,0 +1,31 @@
		/*
		* Copyright 2025 the Pacemaker project contributors

Prevent directly including some internal subheader files #4000

Are you sure you want to change the base?

Prevent directly including some internal subheader files #4000

Conversation

clumens commented Nov 26, 2025

Uh oh!

clumens commented Nov 26, 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