ACL refactors: Unindent some code and functionize unpacking permission #4035
+256
−221
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
Don't do other best practices yet. Those will be in upcoming commits. Signed-off-by: Reid Wahl <[email protected]>
Also use const for the GList iterator and the xml_acl_t variable. Signed-off-by: Reid Wahl <[email protected]>
pcmk__xml_copy() is guaranteed to return non-NULL if its second argument is non-NULL. We have already ensured that xml is not NULL, and so target is not NULL. Further, nothing in the body of the for loop can set target to NULL. Also drop a trace message that doesn't seem helpful. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
If the "else"/"docpriv->acls == NULL" block was executed, then target was set to NULL, which meant we wouldn't do anything else before returning true. With this change, target cannot be NULL at the end of the function, so we drop the NULL check there. Signed-off-by: Reid Wahl <[email protected]>
Only element nodes have attributes. Signed-off-by: Reid Wahl <[email protected]>
Call pcmk__xe_remove_matching_attrs(), which is made for this purpose. Write a simple match function to ensure we only remove non-ID attributes. Signed-off-by: Reid Wahl <[email protected]>
And rename pcmk__apply_acl() to pcmk__apply_acls(). Signed-off-by: Reid Wahl <[email protected]>
The pcmk__if_tracing() block makes sense but it feels like premature optimization and makes the code harder to read. I'm not worried about the overhead of allocating and freeing a small GString each time we apply an ACL. Signed-off-by: Reid Wahl <[email protected]>
Also rename a variable and drop a trace message that seems unhelpful. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
We drop a trace log in order to do this. To get the number of matches from pcmk__xpath_foreach_result(), we'd have to pass a struct as user data to hold both the ACL itself and the match count. That doesn't seem worth the trouble. We still have the trace message after applying an ACL to a particular match. Signed-off-by: Reid Wahl <[email protected]>
And rename to pcmk__unpack_acls(). In general, we assume that an XML node has a non-NULL doc and that its doc has a non-NULL _private field. If we ever want to start checking that (in case a node somehow originated outside of pcmk__xe_create()), we should do it more broadly. The two callers already ensure that target is not NULL. Signed-off-by: Reid Wahl <[email protected]>
It is possible that these values won't get used, but fetching and setting them is pretty lightweight. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Or at least try to clarify it a bit, by returning early if not target or group. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
For simplicity, this drops a trace message that doesn't seem helpful. Also, we drop the explicit NULL check of acls. If it's NULL, we won't enter the loop anyway, because its first child will be NULL. Signed-off-by: Reid Wahl <[email protected]>
Use the second argument of pcmk__xe_first_child() and pcmk__xe_next() to specify what type of element to get (PCMK_XE_ACL_ROLE). Also use const, unindent a block with pcmk__trace() in it, and use pcmk__xe_id(). Signed-off-by: Reid Wahl <[email protected]>
...as well as the parsing of an ACL mode. Signed-off-by: Reid Wahl <[email protected]>
An acl_permission element's ID is currently used only for logging. So we can be permissive and allow this. It seems that our usual approach to things that the schema doesn't allow is to continue processing them if it's possible (for example, if it doesn't result in a broken reference). So warn here. For missing or invalid "kind" attribute, we have to ignore the element, so set a config error. It doesn't look as if we're very consistent about when we set warnings vs. errors, so I'm just doing what feels like it makes the most sense. We could just as well call all of these warnings or call all of these errors. Also, log the parent type and ID parenthetically for errors other than missing ID. If we proceeded with unpacking an acl_permission element without an ID, we want any further log messages to have some identifying information about where or what the acl_permission element is. It doesn't hurt to log this even if the acl_permission's ID is set. Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-acls_first
branch
from
3611870 to
9d96604
Compare
clumens
approved these changes
Jan 28, 2026
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.
First batch from #4013.