Fix: libcib: Prevent based or cibadmin from crashing when handling an XPath query for an attribute

[gao-yan]

With 5fe98f4 in 3.0.1, if an XPath query for an attribute resulted in a
single match, for example:

$ cibadmin --query --xpath "/cib/@crm_feature_set"

, based would encounter a fatal assertion in pcmk__xml_copy():

error: pcmk__xml_copy: Triggered fatal assertion at xml.c:844 : src->type == XML_ELEMENT_NODE

The assertion wouldn't be triggered if there were multiple matches for an
attribute, despite the fact that only the last match would be displayed:

$ cibadmin --query --xpath "//@uname"
<xpath-query uname="node2"/>

But in case --node-path option was anyhow specified, cibadmin would
encounter a fatal assertion in output_xml_id():

error: output_xml_id: Triggered fatal assertion at cibadmin.c:865 : id != NULL

If --no-children option was anyhow specified, the attribute name would be
displayed as an element name and the value wouldn't be shown:

$ cibadmin --query --xpath "/cib/@crm_feature_set" --no-children
<crm_feature_set/>

This commit fixes the issues by handling non-element matches separately.

Besides, instead of being displayed with a vague element name "xpath-query",
now a matching attribute is displayed with the corresponding element name
and the ID if present, so that the output is meaningful:

$ cibadmin --query --xpath "/cib/@crm_feature_set"
<cib crm_feature_set="3.20.5"/>

Multiple matches are all displayed:

$ cibadmin --query --xpath "//@uname"
<xpath-query>
  <node id="1" uname="node1"/>
  <node id="2" uname="node2"/>
  <node_state id="1" uname="node1"/>
  <node_state id="2" uname="node2"/>
</xpath-query>

Also, although cibadmin --node-path option is deprecated, the previous
behavior is restored by handling the corresponding element of a
non-element match instead of returning nothing:

$ cibadmin --query --xpath "//@uname" --node-path
/cib/configuration/nodes/node[@id='1']
/cib/configuration/nodes/node[@id='2']
/cib/status/node_state[@id='1']
/cib/status/node_state[@id='2']

And --node-path option is respected even if --no-children is also specified.

[clumens]

@gao-yan Sorry for the inactivity on this PR. There have been a lot of XML-related refactorings lately, but I see that the bug is still present. Would you be willing to update your patches against main? I am hoping to get this into 3.0.2-final.

[gao-yan]

@clumens, thanks for paying attention. I'm revisiting this right away.

[gao-yan]

Updated the patches based on main now.

[nrwahl2]

Thanks for the patches, and thanks for your patience. Every time I've looked at this, I've gotten frustrated because I can't decide how I wanted the output to look. See review comments. I also tagged @clumens for thoughts.

[nrwahl2]

The assertion wouldn't be triggered if there were multiple matches for an
attribute, despite the fact that only the last match would be displayed:

This behavior is pretty strange to me. But it's because *answer != NULL, so pcmk__xml_copy() doesn't hit the assertion:

    if (parent == NULL) {
        xmlDoc *doc = NULL;
        
        // The copy will be the root element of a new document
        pcmk__assert(src->type == XML_ELEMENT_NODE);
    
        doc = pcmk__xml_new_doc();
        copy = xmlDocCopyNode(src, doc, 1);
        pcmk__mem_assert(copy);
    
        xmlDocSetRootElement(doc, copy);

    } else {
        copy = xmlDocCopyNode(src, parent->doc, 1);
        pcmk__mem_assert(copy);

        xmlAddChild(parent, copy);
    }

[nrwahl2]

Now a matching attribute is displayed with the corresponding element name
and the ID if present:

$ cibadmin --query --xpath "/cib/@crm_feature_set"
<cib crm_feature_set="3.20.5"/>

Multiple matches are all displayed:

$ cibadmin --query --xpath "//@uname"
<xpath-query>
  <node id="1" uname="node1"/>
  <node id="2" uname="node2"/>
  <node_state id="1" uname="node1"/>
  <node_state id="2" uname="node2"/>
</xpath-query>

I don't like that this "matched attributes" output looks the same as "matched elements" output :( But I don't have any great ideas currently. It is useful to show what element a matched attribute came from, as you have done, especially if there are multiple matches.

One possible alternative would be some other element type like <xpath-attr-match> with attributes to indicate the element name and ID, as shown below.

<xpath-query>
  <xpath-attr-match element-name="node" element-id="1" uname="node1"/>
  ...
</xpath-query>

This unfortunately assumes that the matched attribute isn't named element-name or element-id.

(I had another idea, but I forgot it before I could write it down :). I don't think it was a great idea either.)

---

I also wish that a single match were displayed in the same way as multiple matches (inside an <xpath-query> wrapper). But your proposed approach is consistent with the way we already handle single-vs-multiple element matches, so that's okay.

---

Note that your proposed approach can produce duplicate match entries if we match both an element and an attribute:

$ CIB_file=cts/cli/crm_mon.xml cibadmin --query --xpath "//@uname[. = 'cluster02']|//node[@uname = 'cluster02']"
<xpath-query>
  <node id="2" uname="cluster02"/>
  <node id="2" uname="cluster02"/>
  <node_state id="2" uname="cluster02"/>
</xpath-query>

In the above example, we matched the <node id="2" uname="cluster02"/> element. We also matched the uname="cluster02" attribute belonging to that element. But we output the attribute with the element name and ID, so we get duplicate lines. And the <node> element has no attributes besides id and uname. Again, the matched element is indistinguishable from the matched attribute.

---

@clumens Do you have any thoughts here?

[clumens]

Just a few quick thoughts...

• I've never used cibadmin --xpath before, so I have no expectations on how it's supposed to work.
• We do not have any cts regression tests anywhere that use cibadmin --xpath, which seems like an oversight.
• The crm_failcount tool processes output, but I haven't dug into the details of what it's looking for. Additionally, pcs appears to process the output as well. It stands to reason that there are other tools out there relying on the output as well, so we need to be careful not to break them.

One possible alternative would be some other element type like <xpath-attr-match> with attributes to indicate the element name and ID, as shown below.

<xpath-query>
  <xpath-attr-match element-name="node" element-id="1" uname="node1"/>
  ...
</xpath-query>

This unfortunately assumes that the matched attribute isn't named element-name or element-id.

I don't especially like this because it feels like we're processing the output and imposing additional structure on it, which seems like a business we don't want to be in.

I also wish that a single match were displayed in the same way as multiple matches (inside an <xpath-query> wrapper). But your proposed approach is consistent with the way we already handle single-vs-multiple element matches, so that's okay.

I do like this. It's consistent and it makes parsing easier for whoever is consuming this output.

Note that your proposed approach can produce duplicate match entries if we match both an element and an attribute:

$ CIB_file=cts/cli/crm_mon.xml cibadmin --query --xpath "//@uname[. = 'cluster02']|//node[@uname = 'cluster02']"
<xpath-query>
  <node id="2" uname="cluster02"/>
  <node id="2" uname="cluster02"/>
  <node_state id="2" uname="cluster02"/>
</xpath-query>

In the above example, we matched the <node id="2" uname="cluster02"/> element. We also matched the uname="cluster02" attribute belonging to that element. But we output the attribute with the element name and ID, so we get duplicate lines. And the <node> element has no attributes besides id and uname. Again, the matched element is indistinguishable from the matched attribute.

This feels like user error to me - they specified a confusing or incorrect query, so they get confusing or incorrect results.

[nrwahl2]

This patch is reasonable, but I don't think it's necessary. This behavior has been present since cib_xpath_address was added in commit dd94186, and cib_xpath_address is now deprecated. So this would be a behavioral change for a feature that's going away soon.

[nrwahl2]

IMO it would make more sense to display the matching node path of the attribute, instead of the node path of the attribute's element. So, for example,

/cib/configuration/nodes/node[@id='1']/@uname

But this matches the behavior prior to 5fe98f4. So that's fine, since this is deprecated anyway.