Standardize IPC code in schedulerd #3996
Conversation
|
The main upstream tasks I am thinking of for this project are:
...but basically any that come up if you search for "IPC" could be related here. |
daemons/schedulerd/schedulerd_ipc.c
Outdated
| #include <pacemaker-internal.h> | ||
|
|
||
| #include <stdbool.h> | ||
| #include <sys/stat.h> |
There was a problem hiding this comment.
What are sys/stat.h, sys/types.h, and unistd.h being used for? Comments wouldn't hurt. I see that uid_t, gid_t, and size_t are in sys/types.h.
Also I don't see stdbool.h being used for anything (grepped for bool, true, and false).
There was a problem hiding this comment.
I've been avoiding working on header file blocks because I don't know what strategy I want to apply everywhere. Should a C file include every header it needs? What about when it includes a header that includes a header for something it needs?
For instance, include/crm_internal.h includes libxml/tree.h which is needed for xmlNode. Everything includes crm_internal.h. Should a C file still include libxml/tree.h anyway to be more explicit about what its requirements? I feel like the pacemaker includes especially make this a mess.
There was a problem hiding this comment.
Our developer documentation isn't helpful on this subject. I'd like to change that to include concrete advice on what should be included and how includes should be arranged - crm_internal.h, system includes, other libraries, pacemaker libraries, local includes is the convention I think we've followed.
I guess I lean towards more explicit and more verbose inclusion, perhaps except for all our internal header files. Here's what include-what-you-use suggests:
The full include-list for schedulerd_ipc.c:
#include <crm/crm.h> // for CRM_SYSTEM_PENGINE
#include <crm_internal.h> // for crm_trace, CRM_CHECK
#include <errno.h> // for ENOMEM
#include <glib.h> // for g_byte_array_free, TRUE
#include <libxml/parser.h> // for xmlNode
#include <qb/qbipcs.h> // for qb_ipcs_connection_t
#include <stddef.h> // for NULL, size_t
#include <stdint.h> // for int32_t, uint32_t
#include <sys/types.h> // for gid_t, uid_t
#include "crm/common/ipc_internal.h" // for pcmk__client_s, pcmk__c...
#include "crm/common/messages_internal.h" // for pcmk__request_t
#include "crm/common/results.h" // for crm_exit_e, pcmk_rc_e
#include "crm/common/results_internal.h" // for PCMK__UNKNOWN_RESULT
#include "crm/common/strings_internal.h" // for pcmk__str_eq, pcmk__s
#include "crm/common/xml_element_internal.h" // for pcmk__xe_get, pcmk__xe_...
#include "crm/common/xml_internal.h" // for pcmk__xml_free
#include "crm/common/xml_names_internal.h" // for PCMK__XE_ACK, PCMK__XA_...
#include "pacemaker-schedulerd.h" // for schedulerd_handle_request
I think what I would do there is put them in the right order, and then remove all the crm/common/*_internal.h headers in favor of just crm_internal.h. My one complaint with doing that is we lose a lot of information about where things are included from - now crm_internal.h just has this mile long list of symbols that are used from it. On the other hand, maybe we can just have the assumption that if a symbol isn't listed in a comment on an include, it comes from crm_internal.h.
Doing that gives me this (ignore the git-related symbols from vim in the first column):
#include <crm_internal.h>
⚠ #include <errno.h> // ENOMEM
~ #include <stddef.h> // NULL, size_t
~ #include <stdint.h> // int32_t, uint32_t
+ #include <sys/types.h> // gid_t, uid_t
~ #include <glib.h> // g_byte_array_free, TRUE
~ #include <libxml/tree.h> // xmlNode
~_#include <qb/qbipcs.h> // qb_ipcs_connection_t
~ #include <crm/crm.h> // CRM_SYSTEM_PENGINE
+ #include <crm/common/results.h> // crm_exit_e, pcmk_rc_*
+
+ #include "pacemaker-schedulerd.h" // schedulerd_handle_request
There was a problem hiding this comment.
I'd like to change that to include concrete advice on what should be included and how includes should be arranged
That would be nice. I think Ken always wanted to standardize and document this.
See also https://projects.clusterlabs.org/T777.
crm_internal.h, system includes, other libraries, pacemaker libraries, local includes is the convention I think we've followed.
That's the one I've been following and that I got Ken on board with near the end. This basically mirrors the Python import order recommendation.
I guess I lean towards more explicit and more verbose inclusion, perhaps except for all our internal header files.
I think I agree. I've been omitting *internal.h includes from .c files, and adding them to crm_internal.h if they're not already present. I have been including *internal.h files explicitly in .h files and skipping crm_internal.h there, to avoid circular includes.
Here's what include-what-you-use suggests:
This might give duplicates sometimes, I'm not sure. For example, size_t is also provided by sys/types.h. IWYU lists it for stddef.h. I'm not sure whether that would happen or not, if we weren't already including stddef.h for NULL.
Ken has been including stdio.h for NULL -- which is fine, although I was surprised that it's provided there.
maybe we can just have the assumption that if a symbol isn't listed in a comment on an include, it comes from
crm_internal.h.
Yeah I think that's fine -- unless we want to scrap the entire crm_internal.h and go back to including all the internal stuff explicitly, I think it's fine to assume that a non-listed symbol comes from there. Also, it's not feasible to list every included symbol in the comments for a lot of files anyway. For example, we might use 20 different GLib symbols or five different stddint.h/inttypes.h symbols in one source file.
As an aside, we have a few header files that collect other header files. The ones that come to mind are xml.h and xml_internal.h. The idea was not to include the other xml*.h files directly, but rather always to include the "top-level" one. We can keep that approach or scrap it. I think it was Ken's idea and I don't care either way. It does provide a tiny bit of abstraction though.
There was a problem hiding this comment.
This might give duplicates sometimes, I'm not sure. For example,
size_tis also provided bysys/types.h. IWYU lists it forstddef.h. I'm not sure whether that would happen or not, if we weren't already includingstddef.hforNULL.Ken has been including
stdio.hforNULL-- which is fine, although I was surprised that it's provided there.
The only C standard I have laying around is C17, but I can't imagine this kind of stuff would have changed in a very long time. Anyway...
size_tis defined at least bystddef.h(7.19),stdlib.h(7.22), andstring.h(7.24.1) and then I gave up searching for it.NULLis defined at least bystddef.h(7.19),stdio.h(7.21.1) andstring.h(7.24.1) and then I also gave up looking for it too.
So I guess we have some flexibility when it comes to what file to include for these. I'm sure there's plenty of overlap elsewhere too.
There was a problem hiding this comment.
As an aside, we have a few header files that collect other header files. The ones that come to mind are
xml.handxml_internal.h. The idea was not to include the otherxml*.hfiles directly, but rather always to include the "top-level" one. We can keep that approach or scrap it. I think it was Ken's idea and I don't care either way. It does provide a tiny bit of abstraction though.
If we want to enforce this, we could add some preprocessor checking to prevent these headers from being directly included. For example, see bits/errno.h:
#ifndef _BITS_ERRNO_H
#define _BITS_ERRNO_H 1
#if !defined _ERRNO_H
# error "Never include <bits/errno.h> directly; use <errno.h> instead."
#endif
...
where _ERRNO_H is defined by errno.h, of course.
...which is already mentioned in T777. Well whatever, I'll leave this comment.
There was a problem hiding this comment.
May have to tweak the check a bit depending on the behavior we want. If I read correctly, that would allow direct includes AFTER the top-level file is included. Which would be a harmless style violation. But yeah, good idea.
And the PCMK__DIRECT_INCLUDE from T777 is one approach.
The idea behind this commit (and all the ones that will follow it) is to make all the daemons look as similar as possible when it comes to the IPC code. This will make it easier in the future to find code that's common across daemons and condense into a single set of code that they can all share. I think there's probably quite a bit of this code, but it's a little difficult to see at the moment. This commit is a little pointless given how simple the IPC in the scheduler is, but that's just how this is going to go. The idea is that daemon_ipc.c holds the code specific to IPC messages (as opposed to CPG messages), and daemon_messages.c will hold the code common to both types of messages. There's still quite a bit wrong with the code organization right now, but I'm going to fix that up over several commits.
* Add doxygen comments to explain what each function does. * Remove unnecessary whitespace in function headers. * Standardize log messages with what's happening in the same functions in attrd. * Standardize variable names (c becomes client, qbc becomes c). * Make sure return values and comments match what libqb documents.
This is more refactoring to bring this code in line with how the same things are happening in attrd in order to find common code. * schedulerd_ipc_dispatch shouldn't register message handlers, call pcmk__process_request, or deal with populating the result object. All that should be done in a new schedulerd_handle_request function. * schedulerd_handlers and the registration function can now be static since they're not needed elsewhere. If this seems a little overdesigned or pointless here, remember that other daemons support both IPC and CPG messages (schedulerd only does IPC messages) and this is how they are organized.
Nothing uses this yet.
* pcmk__serve_schedulerd_ipc now behaves like the other similarly named functions. * Add schedulerd_ipc_init to register callback functions and add them to the mainloop. This means the callback struct can now be static. * Add schedulerd_ipc_cleanup to clean up. This includes client dropping and cleanup tasks that were previously not happening. I'm not sure if this was a bug or if it doesn't really matter, but it's still good to do this in all daemons. NOTE: Once all daemons have been standardized with an _ipc_cleanup function, the mainloop_del_ipc_server function can be deprecated. That's just a pass through to qb_ipcs_destroy now, and the _ipc_cleanup functions call that.
In both of these cases, attrd_handle_request is always called first. There's no way out of attrd_handle_request without calling pcmk__reset_request, so the second call doesn't do anything.
…tch. This just brings it more in line with what's happening in attrd.
If c is NULL, pcmk__find_client will return NULL so client will be NULL. Thus, the explicit check on c is redundant.
attrd_init_ipc is now attrd_ipc_init for consistency with other daemons, and attrd_ipc_fini is now attrd_ipc_cleanup for a more straightforward name.
Going back and redoing the header files one commit at a time is more work than it's worth, as far as I'm concerned. Thus the previous commits will just be left alone with their incorrect (but compilable) includes. This started out as just correcting the headers in the files I touched as part of the IPC cleanup, but schedulerd is small enough that I might as well correct the other files while I'm in the area.
clumens
force-pushed
the
schedulerd-ipc-cleanup
branch
from
449e7ea to
fb28861
Compare
|
Updated to address review comments, and added a new commit on top to deal with the header file mess. I'd prefer to not go back and try to rework the header file changes into each commit that changes included symbols. That seems like more work that necessary. |
| #include <stddef.h> // NULL | ||
|
|
||
| #include <glib.h> // g_set_error, FALSE, G_OPTION_* | ||
| #include <qb/qblog.h> // LOG_INFO, LOG_TRACE |
There was a problem hiding this comment.
LOG_INFO is a transitive include -- it comes from syslog.h, which is included by qb/qblog.h. That's fine.
2 participants
This is the first of several PRs to clean up and standardize the IPC code in schedulerd. As I mention in at least one of these commits, the goal here is to make IPC/CPG code standard across daemons so we can find common code and pull it out into its own thing. There's likely a lot of common code to be found. There are a couple upstream tasks along these lines, but I'm not referencing any of them here because this is all just prep work.
I am basing what I did here (and what I'll do on other daemons) on the attrd code, but there's likely changes to be made to that daemon in the future too.