Fix: libcrmcommon: handle schema versions properly #1308

kgaillot · 2017-07-18T23:36:53Z

As a byproduct, this eliminates a dependency on the math library.

jnpkrn · 2017-07-19T14:39:09Z

lib/common/schemas.c

+{
+    int rc = sscanf(filename, "pacemaker-%d.%d.rng", &version[0], &version[1]);
+
+    return (rc == 2)? TRUE : FALSE;


return (rc == 2) suffices

jnpkrn · 2017-07-19T14:42:12Z

lib/common/schemas.c

@@ -134,19 +130,27 @@ get_schema_path(const char *name, const char *file)
    return crm_strdup_printf("%s/%s.rng", base, name);
 }

+static inline bool
+version_from_filename(const char *filename, int version[2])


Repeated use of int[2] might perhaps justify a typedef'd type.
(put on the line where it became obvious to me)

jnpkrn · 2017-07-19T14:53:05Z

lib/common/schemas.c

 }

 static void
-__xml_schema_add(int type, float version, const char *name,
+__xml_schema_add(int type, int version[2], const char *name,
                 const char *location, const char *transform,
                 int after_transform)


const int version[2]?

jnpkrn · 2017-07-19T14:57:38Z

Do we need to reserve negative values for anything? Otherwise I'd be in favor of unsigned char ("256 for a version part counter will be enough for everybody").

jnpkrn · 2017-07-19T16:13:00Z

lib/common/schemas.c

@@ -246,34 +255,35 @@ crm_schema_init(void)
    int lpc, max;
    const char *base = get_schema_root();
    struct dirent **namelist = NULL;
+    int zero[2] = {0, 0};


This could also be const then...

jnpkrn · 2017-07-19T16:15:14Z

Note that ceiling of 256 could also be good incentive not to bump the
versions excessively -- there are other connected costs (e.g. storage
for all the particular interim changes between point releases).

jnpkrn · 2017-07-19T17:15:20Z

lib/common/schemas.c

-        known_schemas[last].version = version;
+        sscanf(name, "%*[^-]-%d.%d",
+               &known_schemas[last].version[0],
+               &known_schemas[last].version[1]);


Also nice cleanup :) (not that I am fluent with sscanf, looked that up).

jnpkrn · 2017-07-19T17:48:26Z

lib/common/schemas.c

                    struct stat s;
                    char *xslt = NULL;

-                    transform = crm_strdup_printf("upgrade-%.1f.xsl", version);


Apparently upgrading from 2.10 was unexpected in the old world order.

jnpkrn · 2017-07-19T18:07:58Z

On the more general note, following downside for this approach to
hitting the float-trackable versions limit stated in the past:

float- and string-incompatible comparisons

which used to be noticable simplification especially in XSLT handling
(well, should it be needed). But we can live by and also XSLT should
not be impossible to handle (using substring cuts, etc.).

Looking at some other possibly affected software pieces:

pcs looks fine:
- https://github.com/ClusterLabs/pcs/blob/0.9.159/pcs/lib/cib/tools.py#L249
- https://github.com/ClusterLabs/pcs/blob/0.9.159/pcs/utils.py#L99
clufter:
- FIXME: need to provide custom sorter at
  https://pagure.io/clufter/blob/a089855abffb64c81b49781e5785d6cfcdc69a71/f/format.py#_879

TL;DR: Not the end of the world :)

for readability

kgaillot · 2017-07-19T18:11:07Z

Added commits for review items and a little more cleanup

Originally, pacemaker was treating versions of its CIB schema files as floats, simplifying comparisons in a simplistic single-digit minor version realms. As the necessity to roll out a new schema version catering the new needs is around the corner, it was decided[1] that pacemaker will not bump the major part abrubtly (against the rules[2]), but rather the overflow to a new higher digit will occur. That breaks the assumption of easy comparisons in two previously common angles: - string (e.g. when looking at /cib@validate-with XPath within CIB or at the available, respectively named schema files) - mentioned float (when parsing the numerical part out of the string): 2.11 < 2.2, .... Here, just the former is relevant; XML.etree_rng_validator_proper_specs was supposed to return ordered sequence of embedded/available schemas, which is used, for instance, to grab the lowest possible for the purpose of upgrading the schema for particular CIB document internally. Formerly, the sorting was based on lexicographical comparison, now, thanks to `namever_partition` (added with the previous commit) wired into the process, it performs combined lexicographical+version comparison so everything should keep working as expected even when 2.10+ versions of CIB schemas are common. [1] ClusterLabs/pacemaker#1308 [2] https://github.com/ClusterLabs/pacemaker/blob/master/xml/Readme.md Signed-off-by: Jan Pokorný <jpokorny@redhat.com>

Originally, pacemaker was treating versions of its CIB schemas (respectively named files) as floats, simplifying comparisons in a simplistic single-digit minor version realms. As the necessity to roll out a new schema version catering the new needs is around the corner, it was decided[1] that pacemaker will not bump the major part abrubtly (against the rules[2]), but rather the overflow to a new higher digit in the minor part will occur. That breaks assumption of easy comparisons in two previously common angles: - string (e.g. when looking at /cib@validate-with XPath within CIB or at the available, respectively named schema files); example: pacemaker-2.10 < pacemaker-2.2 - mentioned float (when parsing the numerical part out of the string): example: 2.11 < 2.2 Here, just the former is relevant; XML.etree_rng_validator_proper_specs was supposed to return ordered sequence of embedded/available schemas, which is used, for instance, to grab the lowest possible for the purpose of upgrading the schema for particular CIB document internally. Formerly, the sorting was based on lexicographical comparison, now, thanks to `namever_partition` (added with the previous commit) wired into the process, it performs combined lexicographical+version comparison so everything should keep working as expected even when 2.10+ versions of CIB schemas are common. [1] ClusterLabs/pacemaker#1308 [2] https://github.com/ClusterLabs/pacemaker/blob/master/xml/Readme.md Signed-off-by: Jan Pokorný <jpokorny@redhat.com>

Originally, pacemaker was treating versions of its CIB schemas (respectively named files) as floats, simplifying comparisons in a simplistic single-digit minor version realms. As the necessity to roll out a new schema version catering the new needs is around the corner, it was decided[1] that pacemaker will not bump the major part abrubtly (against the rules[2]), but rather the overflow to a new higher digit in the minor part will occur. That breaks assumption of easy comparisons in two previously common angles: - string (e.g. when looking at /cib@validate-with XPath within CIB or at the available, respectively named schema files); example: pacemaker-2.10 < pacemaker-2.2 - mentioned float (when parsing the numerical part out of the string): example: 2.11 < 2.2 Here, just the former is relevant; XML.etree_rng_validator_proper_specs was supposed to return ordered sequence of embedded/available schemas, which is used, for instance, to grab the lowest possible for the purpose of upgrading the schema for particular CIB document internally. Formerly, the sorting was based on lexicographical comparison, now, thanks to `namever_partition` (added with the previous commit) wired into the process, it performs combined lexicographical+version comparison so everything should keep working as expected even when 2.10+ versions of CIB schemas are common. Note: misc/pacemaker-borrow-schemas with it's "sort -V" should be covered, but the eye should be kept on that when pacemaker-2.10.rng actually arrives. [1] ClusterLabs/pacemaker#1308 [2] https://github.com/ClusterLabs/pacemaker/blob/master/xml/Readme.md Signed-off-by: Jan Pokorný <jpokorny@redhat.com>

kgaillot added 3 commits July 19, 2017 09:02

Fix: libcrmcommon: handle schema versions properly

2f176ae

As a byproduct, this eliminates a dependency on the math library.

Refactor: libcrmcommon: drop no-longer-needed alert key functions

7a2b74c

Test: travis: python-central no longer needed on Travis CI

1b7549a

kgaillot force-pushed the fixes branch from f4b63aa to 1b7549a Compare July 19, 2017 14:11

jnpkrn reviewed Jul 19, 2017

View reviewed changes

Refactor: libcrmcommon: define data type for schema version

1bf9dea

jnpkrn reviewed Jul 19, 2017

View reviewed changes

Doc: xml: update function name in documentation

9b543c7

jnpkrn reviewed Jul 19, 2017

View reviewed changes

Refactor: libcrmcommon: add enum for validator type

d69a8ec

for readability

kgaillot force-pushed the fixes branch from 34abcb3 to d69a8ec Compare July 19, 2017 18:09

kgaillot merged commit 989f1a8 into ClusterLabs:master Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: libcrmcommon: handle schema versions properly #1308

Fix: libcrmcommon: handle schema versions properly #1308

kgaillot commented Jul 18, 2017

jnpkrn Jul 19, 2017

jnpkrn Jul 19, 2017

jnpkrn Jul 19, 2017

jnpkrn commented Jul 19, 2017

jnpkrn Jul 19, 2017

jnpkrn commented Jul 19, 2017

jnpkrn Jul 19, 2017 •

edited

Loading

jnpkrn Jul 19, 2017

jnpkrn commented Jul 19, 2017 •

edited

Loading

kgaillot commented Jul 19, 2017

Fix: libcrmcommon: handle schema versions properly #1308

Fix: libcrmcommon: handle schema versions properly #1308

Conversation

kgaillot commented Jul 18, 2017

jnpkrn Jul 19, 2017

Choose a reason for hiding this comment

jnpkrn Jul 19, 2017

Choose a reason for hiding this comment

jnpkrn Jul 19, 2017

Choose a reason for hiding this comment

jnpkrn commented Jul 19, 2017

jnpkrn Jul 19, 2017

Choose a reason for hiding this comment

jnpkrn commented Jul 19, 2017

jnpkrn Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

jnpkrn Jul 19, 2017

Choose a reason for hiding this comment

jnpkrn commented Jul 19, 2017 • edited Loading

kgaillot commented Jul 19, 2017

jnpkrn Jul 19, 2017 •

edited

Loading

jnpkrn commented Jul 19, 2017 •

edited

Loading