XML refactors: pcmk__xml_init() to pcmk__xml_test_cleanup_group() #3451
Conversation
| if (is_root) { | ||
| xmlFreeDoc(xml->doc); | ||
| } else { | ||
| xmlUnlinkNode(xml); |
There was a problem hiding this comment.
This preserves behavior, but it triggers a thought. I haven't traced this, but at a glance, it seems that we could:
- create a new XML node
- mark the parent dirty
- unlink/free the newly created node due to ACL denial
- the parent is still dirty even though nothing has changed anymore
Not sure if that has any downstream effects (including but not limited to unexpected log messages/diffs).
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
2 times, most recently
from
53c4b84
to
74bdd96
Compare
|
Rebased on main to resolve conflict |
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
from
74bdd96
to
35466d0
Compare
|
Rebased on main to resolve conflict |
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
2 times, most recently
from
8839c31
to
0b2959e
Compare
|
Addressed review so far and rebased on main to resolve conflicts |
|
retest this please |
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
from
0b2959e
to
5c5e497
Compare
|
Rebased on current main to resolve conflicts |
lib/common/xml.c
Outdated
| xmlNode * | ||
| expand_idref(xmlNode * input, xmlNode * top) | ||
| pcmk__xe_expand_idref(xmlNode *input, xmlNode *search) |
There was a problem hiding this comment.
I'm thinking s/expand/resolve/ and s/input/xml/
search should be unnecessary. It's only used with get_xpath_object() which if I remember correctly always searches the entire document anyway, so we could just pass input
There was a problem hiding this comment.
Agreed on names
search specifies WHICH document to search... indirectly. We pass a node and search that node's document. So my doxygen is incorrect, where it says that search is the root of the search.
This comes back again to the counterintuitive way xpath_search() and related functions are set up. A later commit in my refactor branch tries to clean all that up.
Unfortunately we're not always searching within the same document as input/xml. Consider pe__unpack_resource().
ops = pcmk__xe_first_child((*rsc)->xml, PCMK_XE_OPERATIONS, NULL, NULL);
(*rsc)->ops_xml = pcmk__xe_resolve_idref(ops, scheduler->input);
ops doesn't necessarily belong to the same doc as scheduler->input. That's true at least if we expanded from a template and end up using expanded_xml; haven't checked xml_obj.
There was a problem hiding this comment.
If we can get expanded_xml into the CIB (scheduler->input) safely, so that it's in the same document, then we might be able to drop the search argument after tracing through the other paths. Not worth it for this PR but it's on the table. That might turn out too messy with digests and output and such.
Edit: Or maybe rearrange things so that it's a pointer to an existing object in the CIB... haven't looked closely
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
from
5c5e497
to
fc3b410
Compare
|
Updated commits to address review. There must have been a small rebase on main at some point, based on the diff. |
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
from
fc3b410
to
957aef3
Compare
To replace crm_xml_init() Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace crm_xml_cleanup() Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It's only two lines and not Pacemaker-specific; this is just the standard way to free an XML node that may be part of a document. Behavior is preserved in all three callers. However, for posterity: * For free_xml_with_position(), we've already done any other checks that are necessary. * For pcmk__strip_xml_text(), a text node can't be the document root, so we don't have to worry about freeing the document. ACLs and change tracking aren't relevant for the current callers; stripping text is done only internally and is essentially an implementation detail. * For pcmk__apply_creation_acl(), we don't want to recheck ACLs (which would fail). Change tracking is irrelevant because the newly created node hasn't been accepted to the CIB yet. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
|
Two more |
To replace free_xml() Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace expand_idref() Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Call teardown function to free global XML data structures Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Should use --score. Also change formatting of --score option documentation. It appeared correctly in help output, but the man page generation yielded ugly indentation. The new formatting looks worse (but passable) for --help output and looks better (previously unpassable) for man page. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2
force-pushed
the
nrwahl2-xml_refactors
branch
from
957aef3
to
0fa5914
Compare
|
retest this please |
Seems like a nice bite-sized portion for review, even if tedious.