Fix systemd unit overrides #3832

nrwahl2 · 2025-03-03T00:54:27Z

These have been getting in the way of trying to set timeouts via override files in #3818.

nrwahl2 · 2025-03-03T03:54:17Z

Coverity has started complaining about some non-issues, some of which are related to these changes and some of which are not. Sigh.

lib/services/systemd.c

clumens · 2025-03-03T17:26:41Z

lib/common/strings.c

@@ -994,7 +994,7 @@ static bool
 str_any_of(const char *s, va_list args, uint32_t flags)
 {
    if (s == NULL) {
-        return pcmk_is_set(flags, pcmk__str_null_matches);
+        return false;
    }

    while (1) {


Might as well make this while (true) while you're here.

whoops forgot this; at least it's not important

clumens

Aside from my one nit picky comment, I think this PR is good. I'm okay with merging this after #3833 is merged, but without waiting on #3835 as long as there's no coverity errors specific to this code.

nrwahl2 · 2025-03-05T19:06:59Z

as long as there's no coverity errors specific to this code.

These are specific to this code, although they're false positives.

[2025-03-03T01:14:32.824Z] Error: REVERSE_INULL (CWE-476):
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:947: deref_ptr_in_call: Dereferencing pointer "filename".
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:964: check_after_deref: Null-checking "filename" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
[2025-03-03T01:14:32.824Z] 
[2025-03-03T01:14:32.824Z] Error: REVERSE_INULL (CWE-476):
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:878: deref_ptr: Directly dereferencing pointer "filename".
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:929: check_after_deref: Null-checking "filename" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

Similarly all those new FORWARD_NULL Coverity errors in crm_resource.c are false positives, but this PR is (SOMEHOW) what triggers Coverity to flag those.

Both of these sets of Coverity errors cause CI builds to fail. So we will have to either rework the code or suppress them. For the purpose of this PR, adding suppressions is fine (as long as no further false positives crop up).

clumens · 2025-03-05T19:45:06Z

These are specific to this code, although they're false positives.

[2025-03-03T01:14:32.824Z] Error: REVERSE_INULL (CWE-476):
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:947: deref_ptr_in_call: Dereferencing pointer "filename".
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:964: check_after_deref: Null-checking "filename" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
[2025-03-03T01:14:32.824Z] 
[2025-03-03T01:14:32.824Z] Error: REVERSE_INULL (CWE-476):
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:878: deref_ptr: Directly dereferencing pointer "filename".
[2025-03-03T01:14:32.824Z] /pacemaker/lib/services/systemd.c:929: check_after_deref: Null-checking "filename" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

It looks like coverity just doesn't understand that pcmk__g_strcat will call pcmk__assert to check for NULL. I seem to remember that we frequently run into problems where some static analysis tool doesn't understand our assert macros and functions. I wonder if there's some annotation we could use to deal with this once and for all.

Similarly all those new FORWARD_NULL Coverity errors in crm_resource.c are false positives, but this PR is (SOMEHOW) what triggers Coverity to flag those.

Weird. I don't know how coverity decides what to scan. I thought it was just looking at the files changed in a PR, but I guess I'm wrong.

Both of these sets of Coverity errors cause CI builds to fail. So we will have to either rework the code or suppress them. For the purpose of this PR, adding suppressions is fine (as long as no further false positives crop up).

In the interests of hurrying the systemd stuff along and making the backporting job less daunting, I would say let's just suppress them here and deal with them for real elsewhere.

Use crm_strdup_printf() and pcmk__str_copy() to assert on error. Also rename to systemd_unit_name(), since we handle other unit types. Signed-off-by: Reid Wahl <[email protected]>

This really shouldn't fail. Previously, we warned in this case. However, we ought to make sure the service is cluster-controlled, and a future commit will implement action timeouts via systemd overrides. So we'll actually need to make sure the override succeeds. Signed-off-by: Reid Wahl <[email protected]>

I think this is clearer. The glibc doc also recommends fchmod() over umask(): https://www.gnu.org/software/libc/manual/html_node/Setting-Permissions.html It shouldn't matter in our case. umask() isn't thread-safe, but Pacemaker is single-threaded. Still, setting the permissions explicitly seems clearer. Signed-off-by: Reid Wahl <[email protected]>

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

And improve the corresponding check in one of its callers. Signed-off-by: Reid Wahl <[email protected]>

This is a static function and we never pass in pcmk__str_null_matches. Signed-off-by: Reid Wahl <[email protected]>

Separate the path string creation logic from the directory creation logic. This reduces some duplication. Signed-off-by: Reid Wahl <[email protected]>

Previously, the override directory paths were valid only for systemd resources whose type attribute has no extension and thus defaults to ".service". For example, 'class="systemd" type="httpd"' works, but 'class="systemd" type="httpd.service"' results in an override directory /run/systemd/system/httpd.service.service.d. The same issue affected resources for non-service units. For example, for a resource managing unit "custom.timer", we would create an override directory /run/systemd/system/custom.timer.service.d. In these cases, the overrides would not be applied. This commit fixes that. We also ensure that the [Service] section is added only for service units. Signed-off-by: Reid Wahl <[email protected]>

Best practice and makes Coverity happy. Signed-off-by: Reid Wahl <[email protected]>

nrwahl2 · 2025-03-06T07:49:51Z

It looks like coverity just doesn't understand that pcmk__g_strcat will call pcmk__assert to check for NULL

I'm not sure it's about that. get_override_dir() returns a value that it initially gets from g_string_sized_new(). Coverity may just not be understanding that CRM_CHECK() sends us to the done label. Hard to tell... the execution path doesn't mention what happens at CRM_CHECK(). It only consists of the pcmk__build_path() call and the check before freeing in the done section.

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

Coverity seems not to understand what CRM_CHECK() is doing. Signed-off-by: Reid Wahl <[email protected]>

nrwahl2 requested a review from clumens March 3, 2025 00:54

nrwahl2 mentioned this pull request Mar 3, 2025

pacemaker-remoted load_env_var refactors, and assorted minor changes #3833

Merged

clumens added the review: in progress PRs that are currently being reviewed label Mar 3, 2025

clumens reviewed Mar 3, 2025

View reviewed changes

nrwahl2 mentioned this pull request Mar 5, 2025

Overhaul crm_resource #3835

Merged

clumens approved these changes Mar 5, 2025

View reviewed changes

nrwahl2 added 12 commits March 5, 2025 23:24

Refactor: libcrmservice: Improve error checks in systemd_service_name()

1ee15f9

Use crm_strdup_printf() and pcmk__str_copy() to assert on error. Also rename to systemd_unit_name(), since we handle other unit types. Signed-off-by: Reid Wahl <[email protected]>

Refactor: libcrmservice: Avoid gboolean/gchar in systemd_unit_exists()

b4cf2c2

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

Refactor: libcrmservice: Check for empty arg in invoke_unit_by_name()

9f35325

And improve the corresponding check in one of its callers. Signed-off-by: Reid Wahl <[email protected]>

Refactor: libcrmcommon: Drop check for unused flag from str_any_of()

23584c1

This is a static function and we never pass in pcmk__str_null_matches. Signed-off-by: Reid Wahl <[email protected]>

Refactor: libcrmservice: Simplify systemd override path getters

38a6f52

Separate the path string creation logic from the directory creation logic. This reduces some duplication. Signed-off-by: Reid Wahl <[email protected]>

Refactor: tools: NULL-check args in cli_resource_print()

98d9c8a

Best practice and makes Coverity happy. Signed-off-by: Reid Wahl <[email protected]>

Refactor: tools: NULL-check scheduler arg of cli_resource_move()

5d0bd9f

Best practice and makes Coverity happy. Signed-off-by: Reid Wahl <[email protected]>

Refactor: tools: NULL-check scheduler arg of cli_cleanup_all()

af18549

Best practice and makes Coverity happy. Signed-off-by: Reid Wahl <[email protected]>

Refactor: tools: NULL-check scheduler arg in cli_resource_fail()

e596ecc

Best practice and makes Coverity happy. Signed-off-by: Reid Wahl <[email protected]>

nrwahl2 added 2 commits March 6, 2025 00:17

Doc: tools: Suppress Coverity false positives in crm_resource.c

aca863a

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

Doc: libcrmservice: Suppress Coverity false positives

bf3ae33

Coverity seems not to understand what CRM_CHECK() is doing. Signed-off-by: Reid Wahl <[email protected]>

nrwahl2 force-pushed the nrwahl2-systemd branch from f16ea2e to bf3ae33 Compare March 6, 2025 08:17

clumens merged commit 47fa0d8 into ClusterLabs:main Mar 6, 2025
1 check passed

nrwahl2 deleted the nrwahl2-systemd branch March 6, 2025 19:38

clumens mentioned this pull request Mar 10, 2025

Fix systemd unit overrides #3840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix systemd unit overrides #3832

Fix systemd unit overrides #3832

Uh oh!

nrwahl2 commented Mar 3, 2025

Uh oh!

nrwahl2 commented Mar 3, 2025

Uh oh!

Uh oh!

clumens Mar 3, 2025

Uh oh!

nrwahl2 Mar 12, 2025 •

edited

Loading

Uh oh!

clumens left a comment •

edited

Loading

Uh oh!

nrwahl2 commented Mar 5, 2025

Uh oh!

clumens commented Mar 5, 2025

Uh oh!

nrwahl2 commented Mar 6, 2025

Uh oh!

Uh oh!

Uh oh!

Fix systemd unit overrides #3832

Fix systemd unit overrides #3832

Uh oh!

Conversation

nrwahl2 commented Mar 3, 2025

Uh oh!

nrwahl2 commented Mar 3, 2025

Uh oh!

Uh oh!

clumens Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

nrwahl2 Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clumens left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrwahl2 commented Mar 5, 2025

Uh oh!

clumens commented Mar 5, 2025

Uh oh!

nrwahl2 commented Mar 6, 2025

Uh oh!

Uh oh!

Uh oh!

nrwahl2 Mar 12, 2025 •

edited

Loading

clumens left a comment •

edited

Loading