Fix: libpe_status, xml: Support integer as rule type attribute

[nrwahl2]

The documentation specifies three potential values for the type
attribute of rule expressions:

• string
• integer
• version

Yet the rule.rng schema and pe__eval_attr_expr() expect "number" instead
of "integer".

This pull corrects the documentation to align with the schema and code.

Thanks @tomjelinek for reporting this.

---

Edited summary:

This pull does the following:

• Creates a pcmk_parse_double() function (strtod() wrapper) and unit tests
• Creates a pcmk__char_in_any_str() function (multi-strchr() wrapper) and unit tests
• Adds "integer" to the rule schema as a valid type attribute
• Adds "integer" support to libpe_status, updates "number" to specify a floating-point comparison, and uses "integer" for integer comparisons. (Previously, "number" specified an integer comparison.)
• Updates the documentation to align with the schema and libpe_status

[nrwahl2]

Alternative options:

1. Would it make sense to parse the values with strtod() and treat them as doubles? It seems like that wouldn't be too complicated. Maybe add a pcmk_parse_double() to the strings library.

2. If Medium: cib: Fix compilation of ACL code #1 is not worthwhile, what about changing the schema to line up with the doc and deprecate "number" in favor of "integer"? Floating-point values get truncated to ints currently. IMO it's better to name the type choice accordingly and make it clear that we can only do integer comparisons.

Changing the schema seems like a tricky process due to compatibility concerns. I hit a roadblock during an initial attempt at that (mostly due to CTS's use of the rule-2.9 schema, which was last updated 3 years ago). I read over the schema readme but still not sure how to approach it if we were to go that route.

[kgaillot]

I'm thinking we should leave the documentation as-is, and modify the schema and code to accept either "integer" or "number" interchangeably. Perhaps at a future Pacemaker major-version bump we can make "number" behave as a floating point.

To modify the schema, see xml/Readme.md. This will be a compatible change.

[kgaillot]

Another possibility would be to treat "number" as a double if it contains a '.', and an integer otherwise. That would keep backward compatibility for anyone using it as an integer currently while allowing users to start using it for floating points. In that case the documentation, code and schema could be brought in line as:

• "integer", or "number" with a value without '.': integer
• "number" with a value with '.': double-precision floating point

[nrwahl2]

To modify the schema, see xml/Readme.md. This will be a compatible change.

Ack. As we discussed in chat, the source of confusion was the use of an old rule schema in the current constraints schema.

---

Another possibility would be to treat "number" as a double if it contains a '.', and an integer otherwise. That would keep backward compatibility for anyone using it as an integer currently while allowing users to start using it for floating points.

Would this actually do anything to alleviate your concern in the RHBZ? You said:

If anyone's using it, they're using "number" with integers, and they may reasonably expect values not to have any of the unusual corner cases of floating-point arithmetic, so we should keep "number" syntax and integer behavior.

If they're currently using integer strings (i.e., without a '.'), then they shouldn't be prone to any floating-point corner cases AFAIK. The floating point promotion would just add zeroes after the decimal place anyway.

Users would only hit a floating-point corner case (e.g., a comparison failing due to a precision issue in FP arithmetic) after the suggested change if they're currently using floating-point strings (i.e., with a '.') and relying on Pacemaker to truncate the value to an int. Then after the change, those values would be parsed as doubles and may compare incorrectly, in theory.

So it seems to me that parsing numbers with '.' as floating-point would be just the same as parsing ALL numbers as floating-point, when it comes to the risk of hitting unexpected behavior in a corner case.

[kgaillot]

So it seems to me that parsing numbers with '.' as floating-point would be just the same as parsing ALL numbers as floating-point, when it comes to the risk of hitting unexpected behavior in a corner case.

A bit contrived, but:

#include <stdio.h>
#include <stdlib.h>


int main(int argc, char **argv)
{
    unsigned long integer = 9223372036854775809UL;
    double number = strtod("9223372036854775809.0", NULL);

    printf("%lu %f\n", integer, number);
    return 0;
}

However that wouldn't affect our situation since we currently parse the values as 32-bit integers (which I didn't realize until checking just now), so technically you're right. :) Which makes me think we should indeed parse "number" as double, and start parsing "integer" with crm_parse_ll(). That would clean things up nicely and shouldn't affect any existing usage adversely.

[nrwahl2]

A bit contrived, but:

The best corner cases are contrived :)
And ah yes, large ints that require high precision. I forgot about losses when converting those to doubles.

That would clean things up nicely and shouldn't affect any existing usage adversely.

Ack. So I take it we're not worrying about any odd use cases where the expected result of a comparison depends on truncation to int. I support going with double for "number" as long as you're good with it, since it is more flexible.

I was working on a pcmk__scan_double() and pcmk_parse_double(). I back-burnered them to focus on commits for the main focus of this issue, after having some initial difficulty detecting overflow and underflow in a way that's reliable on both ANSI and C99. (The strtod() return value for underflow is 0 and ERANGE on ANSI, while it's "<= smallest positive normalized number and maybe ERANGE (implementation-dependent)" on C99.)

If we've settled on parsing "number" as double, I'll get back to work on that and add the result to the PR.

[kgaillot]

Ack. So I take it we're not worrying about any odd use cases where the expected result of a comparison depends on truncation to int. I support going with double for "number" as long as you're good with it, since it is more flexible.

Yep. Using a "." and expecting an integer comparison would be twisted enough that I'd consider it a bug.

I was working on a pcmk__scan_double() and pcmk_parse_double(). I back-burnered them to focus on commits for the main focus of this issue, after having some initial difficulty detecting overflow and underflow in a way that's reliable on both ANSI and C99. (The strtod() return value for underflow is 0 and ERANGE on ANSI, while it's "<= smallest positive normalized number and maybe ERANGE (implementation-dependent)" on C99.)

If we've settled on parsing "number" as double, I'll get back to work on that and add the result to the PR.

Sounds good

[nrwahl2]

This has grown pretty large. I think I'm finished, until there's feedback on things that need to be changed.

A lot of the commits are prep work (e.g., adding new utilities and their tests, copying schemas, etc.). Four of them are minor issues and opportunities I happened upon while doing the prep work. The last few commits directly address the issue.

[nrwahl2]

One other thing: Does any of this require a feature set bump?

[kgaillot]

Nice work, very thorough. I think a feature bump is worthwhile since otherwise the behavior of floating-point attribute values could change depending on which node is DC. Also, IIRC the schema version bump protects us from the user being able to upgrade the CIB before all nodes support it, but technically users can run without schema verification, so it's best not to rely solely on that (without schema verification someone could specify integer in a mixed-version cluster and get really different behavior).

[nrwahl2]

By the way, we can't rely on GCC and Clang extensions, right? GCC (and I believe Clang as well) has an extension for variadic macro arguments that would eliminate the need to pass NULL as a sentinel value to some of our newer string functions. It's not part of the standard though.

[kgaillot]

Looks good, just a couple minor comments

[nrwahl2]

I just noticed that I updated cts-scheduler instead of cts-scheduler.in for some of the added regression tests. Going to have to rewrite the list additions and re-push.

[kgaillot]

Awesome :)