Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XML refactors: crm_xml_init() through crm_copy_xml_element() #3450

Closed
wants to merge 103 commits into from

Conversation

nrwahl2
Copy link
Contributor

@nrwahl2 nrwahl2 commented May 3, 2024

No description provided.

@nrwahl2 nrwahl2 force-pushed the nrwahl2-xml_attrs2 branch 9 times, most recently from 56551bb to 072875c Compare May 6, 2024 12:48
@nrwahl2 nrwahl2 changed the title XML deprecations: crm_xml_init() through crm_xml_set_id() XML refactors: crm_xml_init() through crm_copy_xml_element() May 7, 2024
nrwahl2 added 20 commits May 7, 2024 14:57
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>
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>
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>
Previously, if attr->set_id were the empty string, attrd_set_id() would
pass a copy to crm_xml_sanitize_id(). In that case, there's nothing to
sanitize and thus no way for crm_xml_sanitize_id() to turn its argument
into a valid XML ID (other than performing a realloc() and storing
arbitrary charactrs).

It's better to treat an empty attr->set_id the same as a NULL one, and
use the default set ID in that case.

(It is possible for attr->set_id to be the empty string because it may
be arbitrary user input from CLI tools.)

As of this commit, all callers of crm_xml_sanitize_id() are guaranteed
to pass a non-empty string as the argument.

This is technically a behavioral change, but it's quite an edge case,
and the existing behavior is problematic.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Not used yet

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This runs surprisingly fast considering that there are over 1 million
test cases.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Not used yet

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This runs surprisingly fast considering that there are over 1 million
test cases.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2 added 28 commits May 7, 2024 14:57
Preserve the body of xml_patch_versions() because the new function
returns EINVAL in the edge case where we fail to parse version numbers
as integers.

The order of arguments changes. I think it's clearer to use "source" and
then "target". In v1 patchsets, we used "add"/"remove", but we can start
moving away from that.

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>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Preserve body of patchset_process_digest() because the new function
returns without adding a digest if the target is dirty.

Drop with_digest argument, and instead check in the callers. There are
only two callers, so it's not terribly clunky. It felt awkward to take
an argument for "should we add a digest?" in a function whose purpose is
to add a digest.

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>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
And limit line length of SUMMARY

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This is highly unfortunate, but changing it would break backward
compatibility.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It's simpler to use multiple strings than to have a boolean represent
whether a file name should be treated as a raw string.

When we can make the input precedence sane at a compatibility break, an
enum for input source will likely make the most sense. Then we can bring
back callbacks to set the enum value..

Also rename "apply" to "patch" for clarity, and use G_OPTION_FLAG_NONE.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It was for logging only. It's inaccurate if we're using a string or
stdin as our input source, and it's not particularly helpful.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...in crm_diff.c.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It doesn't copy an element, so I'd like to deprecate it. It's useful,
but only slightly, and we only call it four times.

If we decide to re-add this functionality internally, we could consider
adding a NULL-terminated array argument to pcmk__xe_copy_attrs(). If
NULL, copy all attrs.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Not much to do here. Mainly doxygen, dropping a redundant call to
pcmk__xml_track_changes(), and an extra comment.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Improve doxygen and variable names. Clarify/simplify logic.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We don't have to operate on a copy of input. We're not using it for
anything else after calling this function.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
I fail to see how this is useful.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
pcmk__log_xml_patchset() already logs these, almost identically, at
notice level.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Not really necessary but easy and this is the last piece

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant