Overhaul crm_resource #3835
Merged
Overhaul crm_resource #3835
Conversation
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
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
3 times, most recently
from
64284c6 to
230d36c
Compare
nrwahl2
commented
Mar 5, 2025
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
2 times, most recently
from
ad09ed7 to
5bb7fb3
Compare
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
from
5bb7fb3 to
9dc00b7
Compare
Contributor
Author
|
rebased on main |
Contributor
Author
|
retest this please |
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
2 times, most recently
from
4ecca90 to
e5860dc
Compare
clumens
reviewed
Mar 6, 2025
|
|
||
| g_list_free(before); | ||
| g_list_free(after); | ||
| g_list_free(remaining); |
Contributor
There was a problem hiding this comment.
You could apply some best practices here... move before, after, and remaining into this block; define ele in the for loop; remove the newline from the g_set_error string.
Contributor
Author
There was a problem hiding this comment.
I agree. I didn't spend time thinking about the bodies of the handler functions or doing best practices; I just copy-pasted. I'll add a commit on top for this.
Contributor
Author
There was a problem hiding this comment.
Hmm, can't do it with before. The whole point is that I'm grabbing those before I do anything else. I think I'll leave those alone for right now, except for ele and the newline in the g_set_error() string. (I'll just update the existing commit.)
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
from
e5860dc to
1fd785f
Compare
Contributor
Author
|
Rebased on main and resolved conflicts. No other changes. |
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
from
1fd785f to
d1deb42
Compare
Contributor
Author
Wrong :) There were a few extra commits that I'd added on my local branch. I just removed them and re-pushed. |
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
from
d1deb42 to
c7d5fbf
Compare
Contributor
Author
|
Updated to address review |
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
from
c7d5fbf to
6bf2e94
Compare
Contributor
Author
|
Fixed a Coverity suppression |
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
from
6bf2e94 to
c85bd49
Compare
Contributor
Author
|
Fixed a commit history issue (no code change in the end result) |
Contributor
|
I think this looks fine. If you're happy, go ahead and rebase on main to deal with the merge conflict. |
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
And remove empty lines since each case is a one-liner plus the break. Signed-off-by: Reid Wahl <[email protected]>
It's only called once and it's clearer to put its contents in the done section of main(). Signed-off-by: Reid Wahl <[email protected]>
We can use a GString to build up a single buffer instead of building up and printing an array of gchar strings. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Allows us to avoid duplicating strings in main() Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-crm_resource
branch
from
c85bd49 to
9e934a5
Compare
bmwiedemann
pushed a commit
to bmwiedemann/openSUSE
that referenced
this pull request
Mar 15, 2025
…R 1252999 https://build.opensuse.org/request/show/1252999 by user yan_gao + dimstar_suse - tools: Fix memory leak in cli_resource_delete() (gh#ClusterLabs/pacemaker#3835) (forwarded request 1252998 from yan_gao)
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.
Coverity is throwing some new false positives in #3832. Two of them are in code that I actually modified, but a lot more are in totally unrelated code in
crm_resource. It's flagging NULL dereferences along code paths that are verifiably impossible to reach.We could add Coverity suppressions. But false positives are often a sign that the code is too hard to follow in the first place. And I think that's the case here.
The changes that I've started on also happen to be first steps toward moving more functionality into libpacemaker.
The first few commits focus on avoiding passing around
options.host_unameas much as possible. Coverity was complaining; it gets confusing to verify that it's non-NULL when it's supposed to be; and we're duplicating work with node lookups. Later commits mostly focus on breaking out the switch statement inmain()into handler functions.There's a lot more work to be done. For example, the new handler function bodies were basically copy-pasted from
main(). They take as arguments only the things that were not already global. (For example, they don't takeoutas an argument, because it's a global variable.)I haven't decided on what the end state will be. It might be a function table like we use for processing CIB operations. And there might be a set of enum flags that determine what objects (CIB connection, scheduler data, resource and node arguments, etc.) each operation needs or supports. That way we can run the necessary setup steps right before the operation, instead of calling
is_scheduler_required()(etc.) near the top ofmain().