XML refactors/deprecations: crm_foreach_xpath_result() to pcmk__xml_tracking_changes() #3851
Conversation
This file contains 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
PCMK_XA_FILE was misused in two places to refer to an XML element. Signed-off-by: Reid Wahl <[email protected]>
To replace crm_foreach_xpath_result(). We can add a reverse argument later if needed. Note that all internal callers are passing XPath expressions that can only match elements. So we don't have to worry about the "return an element for an attribute, etc." logic that we were using in the past via crm_foreach_xpath_result() -> pcmk__xpath_match_element() (or getXpathResult() farther in the past). Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Now all internal callers call get_xpath_object() with XPath expressions that can only match elements. This avoids the need to rely on the strange pcmk__xpath_match_element() logic when we replace get_xpath_object() with an internal function. Signed-off-by: Reid Wahl <[email protected]>
To replace get_xpath_object(). get_xpath_object() dereferences its xmlNode argument without NULL-checking it. So the fact that we now dereference it in all of the callers to get its doc member, shouldn't cause any NEW problems. Later, in per-function best practices commits, we should NULL-check the arguments where appropriate. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
I think the only unused headers we want to include are the ones that xml.h is a wrapper for: xml_*.h, minus internal headers. Update other files that break as a result, that were depending on the transitive includes. Signed-off-by: Reid Wahl <[email protected]>
It saves a little bit of space, but it obfuscates what we're doing. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
IMO this makes it clearer that we're setting flags for a particular document. It has nothing to do with any particular node within the document; we were using the node argument only to access its doc member. Also add doxygen, rename to pcmk__xml_doc_set_flags(), and take a uint32_t argument instead of an enum (since it may be a group of multiple flags). 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]>
Most of these changes are straightforward because all but one caller passed false as the lazy argument. This is equivalent to checking the the pcmk__xf_tracking flag on the node's doc. One caller passes true, which also checks the pcmk__xf_lazy flag. It's probably worth renaming pcmk__xf_lazy to something with clearer meaning later. Signed-off-by: Reid Wahl <[email protected]>
This makes no practical difference, but it classifies these two defects as "false positives" rather than as "intentional" (the default). I'm leaving the other coverity code-line annotations alone, because most or all of them are being addressed by other concurrent pull requests. https://documentation.blackduck.com/bundle/coverity-docs/page/customizing_coverity/topics/annotations/c_annotations_c_cpp_code-line.html#annotations_c_cpp_code-line_annotations Signed-off-by: Reid Wahl <[email protected]>
For clarity of purpose. Also improve doc comments for several private XML struct types. Signed-off-by: Reid Wahl <[email protected]>
This isn't necessary, but it's convenient. The force argument passes through to pcmk__xa_remove(), which pcmk__xe_remove_matching_attrs() calls. The motivation is xml_accept_changes(). Previously, we checked the document's pcmk__xf_dirty flag, cleared all the document's flags, and then accepted attr deletions throughout the tree. There was a hidden ordering dependency there. If we did not clear the document's flags before accepting the tree's attr deletions, then the document's pcmk__xf_tracking flag would still be enabled. In that case, pcmk__xa_remove() wouldn't actually remove the deleted attributes. It would just set their pcmk__xf_deleted flag... which was already set. Now, we check the doc's flag, accept attr deletions throughout the tree if set, and then clear everything in the doc that needs to be cleared. On the flip side, adding another argument is mildly inconvenient for other callers. Oh well. Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
commented
Mar 21, 2025
daemons/fenced/fenced_commands.c
Outdated
| @@ -349,7 +349,7 @@ create_async_command(xmlNode *msg) | |||
| return NULL; | |||
| } | |||
|
|
|||
| op = get_xpath_object("//@" PCMK__XA_ST_DEVICE_ACTION, msg, LOG_ERR); | |||
| op = get_xpath_object("//*[@" PCMK__XA_ST_DEVICE_ACTION "]", msg, LOG_ERR); | |||
There was a problem hiding this comment.
Look carefully through this commit please, and anything else that modifies actual XPath expressions. We're not going to get a compile time error for bad XPath syntax, or for good syntax that no longer matches what we want to.
nrwahl2
commented
Mar 21, 2025
| * \internal | ||
| * \brief Information about an XML node that is marked as deleted | ||
| * | ||
| * When change tracking is enabled and we delete an XML node, we simply mark |
There was a problem hiding this comment.
Whoops, this is wrong. We actually do delete it in free_xml_with_position().
nrwahl2
commented
Mar 21, 2025
| } pcmk__deleted_xml_t; | ||
|
|
||
| /*! | ||
| * \internal | ||
| * \brief Private data for an XML document |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
@clumens Here's the next batch of 20 commits. Enjoy.