WIP: XML refactors and deprecations through xml_track_changes() #3845
+5,717
−4,405
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
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
4 times, most recently
from
02c3484 to
b36e366
Compare
|
Alright, I'm finally back to where I was last spring :) It took a lot of cleanup to get things to fit into the current main. And I took different approaches and made some other improvements in places. As I write this, all but two of the public API functions from xml.h are deprecated on this branch, and we're starting on formatted output for |
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
6 times, most recently
from
5b8b280 to
b8db718
Compare
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]>
Signed-off-by: Reid Wahl <[email protected]>
The only reason we return a value here is so that pcmk__xe_set_timeval() can check it. We could have a static helper function that both of them call, so that this will be consistent, but I can't be bothered. The existing crm_xml_add_ll() prints into a static buffer. While that is undeniably more efficient, I don't think it matters. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
To replace crm_xml_add_int() Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
To replace crm_xml_add(). Also add an include of results.h to ipc.h. For some strange reason, this commit causes test-headers.sh to break due to missing symbol crm_exit_t in that header. Similar with some other include changes. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
To replace save_xml_to_file(). Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
To replace the CRM_BZ2_ string constants 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]>
pcmk__valid_stonith_watchdog_timeout() already calls crm_get_msec(value) if value is not NULL. The only thing we lose by skipping the pcmk__valid_stonith_watchdog_timeout() call for the "st_timeout == 0" case is a debug message. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
pcmk__accept_remote_connection() and pcmk__auto_stonith_watchdog_timeout() already ignore negative and zero values. The only other caller is pcmk__valid_stonith_watchdog_timeout(), in the else block. At that point, st_timeout must be positive. * If it had been negative, then it was set to a positive value in the earlier "if (st_timeout < 0)" block, via assignment from pcmk__auto_stonith_watchdog_timeout() * If it had been zero, it would have hit the "if (st_timeout == 0)" block in this if-else chain and would not have entered the else block. So if sbd_timeout had been negative there prior to this commit, then "st_timeout < sbd_timeout" still would not have been true, and we wouldn't have exited. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
The existing code casts the timeout to int before making sure it's greater than INT_MIN. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
This was an off-by-one. The number in the overflow input was LLONG_MAX. That's not an overflow and doesn't cause ERANGE. Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
from
b8db718 to
4f5d309
Compare
To replace crm_get_msec(). There are a lot of parsing improvements that would be welcome (see T841), but we'll have to be careful about making them. We're parsing user inputs with this function. So changing how we respond to invalid values, values that contain some garbage at the end, etc., affects user- facing behavior, even though the function is internal. This function basically preserves existing behavior for now, including that it doesn't yet allow negative values. One key difference is that we no longer use PCMK__PARSE_INT_DEFAULT inside it in any way. The output argument (dest) remains unchanged if there is any error besides ERANGE. Also improve unit tests for overflow, and add a test for negative input that doesn't overflow. Ref T841 Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
All callers except crm_mon already handled this. Also, crm_mon was the only caller that passed NULL for the result argument. So we now CRM_CHECK() that it's not NULL. It's easty add "NULL result" as a feature again in the future, if there's a frequent need to validate that a time+units string is parsable without actually needing its value. Signed-off-by: Reid Wahl <[email protected]>
strtoll() does this automatically. We might as well keep the original input string for logging. Signed-off-by: Reid Wahl <[email protected]>
This also avoids logging that the value will be clipped if we were going to end up returning pcmk_rc_bad_input later due to invalid units. Signed-off-by: Reid Wahl <[email protected]>
To replace crm_str_to_boolean() Signed-off-by: Reid Wahl <[email protected]>
Functionize the repeated code, test everything with both true and false as the initial value of result, test everything with NULL result argument, and ensure result is not changed upon error. Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
from
4f5d309 to
7d081c4
Compare
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.
This builds on #3843.