Refactor-and-deprecation preview dumping ground #3845

nrwahl2 · 2025-03-13T19:15:41Z

This is a place to view the commits that are next in line from whatever PR I have currently active, for anyone curious.

nrwahl2 · 2025-03-18T04:51:24Z

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 crm_diff.

To replace xml_accept_changes(). For now we basically mimic the logic in xml_accept_changes(), but perhaps we should be less aggressive or rename the function to reflect that it's clearing so many things. In particular, I'm not sure I like that this disables tracking, and I don't think I like that ACLs are so tightly coupled to change tracking (in many ways, not just here). Change tracking is required for ACLs but not vice-versa. Perhaps operations and flags should be kept a bit more... separate, though. Still, it's probably wise to throw away any unpacked ACLs when committing changes, as we're doing now and as we've been doing. ACLs themselves may have been added, modified, or removed as part of the changes. We want to get a current view of them before any further changes. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

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

And add doxygen. The doxygen is a bit hand-wavy for a few of them, because I don't yet understand everything they're doing. The wide spacing before the equal sign is to allow room to do a planned rename of pcmk__xf_lazy later, without having to change the other lines again. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

xml_calculate{,_significant}_changes() already takes care of this, provided the node's document doesn't have the pcmk__xf_tracking flag set yet. That is true here, since these are all new documents. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

In all of these cases, what we really care about is whether ACLs are enabled for the user in the original CIB (*current_cib). The differences come from the aliases (cib_ro and scratch) that we use for *current_cib, and whether scratch is a copy of *current_cib (in which case the return value of cib_acl_enabled() would be the same). Also add a couple of TODO comments. This is a very convoluted and confusing function and likely has some corner case bugs. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

If we simply assume that pcmk__xml_new_doc() returns non-NULL (as it currently must because it asserts on allocation error), then it's simple to make assert_comment() more self-contained. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

The only remaining uses were in cib_perform_op() and in the unit tests for pcmk__xc_create(). For cib_perform_op(), this introduces some duplication. We could functionize the duplicated code, but that entire function is a huge mess anyway, so I'm simply leaving it for now until we can revisit it as a whole. Note that pcmk__enable_acl() is equivalent to the three calls that xml_track_changes() was making. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

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