Re: [PATCH pve-cluster 7/9] cluster: add helpers module with version comparison functions

[David Riley <d.riley@proxmox.com>]

On 6/22/26 11:31 AM, Daniel Kral wrote:
> Nice idea for making these helpers more accessible across the packages!
>
> On Thu Jun 11, 2026 at 4:59 PM CEST, David Riley wrote:
>> Move 'version_cmp' and 'pvecfg_min_version' over from qemu-server, to
>> make them more accessible.
>>
>> Originally-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
>> Originally-by: Alexandre Derumier <aderumier@odiso.com>
>> Signed-off-by: David Riley <d.riley@proxmox.com>
>> ---
>>   debian/pve-cluster.install |  1 +
>>   src/PVE/Cluster/Helpers.pm | 51 ++++++++++++++++++++++++++++++++++++++
>>   src/PVE/Cluster/Makefile   |  2 +-
>>   3 files changed, 53 insertions(+), 1 deletion(-)
>>   create mode 100644 src/PVE/Cluster/Helpers.pm
> Hm, I think pve-common might be a more accessible location for such
> helpers and PVE::Cluster::* modules (only Setup and IPCConst at the
> moment) are only used by pve-cluster internally at the moment.

Thanks for taking a look.
My initial thought was to keep this closer to the caller from patch 8 [0]  to make it
more readable.
But you are right it would fit better into pve-common.
I'll move it in v2.

[0] https://lore.proxmox.com/pve-devel/20260611145935.147788-9-d.riley@proxmox.com/

>> diff --git a/debian/pve-cluster.install b/debian/pve-cluster.install
>> index f66cd06..46d40c4 100644
>> --- a/debian/pve-cluster.install
>> +++ b/debian/pve-cluster.install
>> @@ -5,4 +5,5 @@ usr/lib/
>>   usr/share/man/man8/pmxcfs.8
>>   usr/share/perl5/PVE/Cluster.pm
>>   usr/share/perl5/PVE/Cluster/IPCConst.pm
>> +usr/share/perl5/PVE/Cluster/Helpers.pm
>>   usr/share/perl5/PVE/IPCC.pm
>> diff --git a/src/PVE/Cluster/Helpers.pm b/src/PVE/Cluster/Helpers.pm
>> new file mode 100644
>> index 0000000..a4b41c9
>> --- /dev/null
>> +++ b/src/PVE/Cluster/Helpers.pm
>> @@ -0,0 +1,51 @@
>> +package PVE::Cluster::Helpers;
>> +
>> +use strict;
>> +use warnings;
> nit: new modules could already be introduced with 'use v5.36' so you can
>       use subroutine signatures :) [0]
>
> [0] https://perldoc.perl.org/perlsub#Signatures

Thanks for the heads up. Will adapt this in v2.

>> +
>> +use JSON;
>> +
>> +use base 'Exporter';
>> +our @EXPORT_OK = qw(pvecfg_min_version assert_min_cluster_version version_cmp);
>> +
>> +sub pvecfg_min_version {
>> +    my ($verstr, $major, $minor, $release) = @_;
>> +
>> +    return 0 if !$verstr;
>> +
>> +    if ($verstr =~ m/^(\d+)\.(\d+)(?:[.-](\d+))?/) {
>> +        return 1 if version_cmp($1, $major, $2, $minor, $3 // 0, $release) >= 0;
>> +        return 0;
>> +    }
>> +
>> +    die "internal error: cannot check version of invalid string '$verstr'";
>> +}
>> +
>> +# gets in pairs the versions you want to compares, i.e.:
>> +# ($a-major, $b-major, $a-minor, $b-minor, $a-extra, $b-extra, ...)
>> +# returns 0 if same, -1 if $a is older than $b, +1 if $a is newer than $b
>> +sub version_cmp {
>> +    my @versions = @_;
>> +
>> +    my $size = scalar(@versions);
>> +
>> +    return 0 if $size == 0;
>> +
>> +    if ($size & 1) {
>> +        my (undef, $fn, $line) = caller(0);
>> +        die "cannot compare odd count of versions, called from $fn:$line\n";
>> +    }
>> +
>> +    for (my $i = 0; $i < $size; $i += 2) {
>> +        my ($left, $right) = splice(@versions, 0, 2);
>> +        $left //= 0;
>> +        $right //= 0;
>> +
>> +        return 1 if $left > $right;
>> +        return -1 if $left < $right;
>> +    }
>> +    return 0;
>> +}
> Preexisting, but as this will be used by assert_min_cluster_version(...)
> in the next patch: the key-value pair 'version-info' contains the
> broadcasted pve-manager version of each node.
>
> For the pve-manager we sometimes use another revision number in addition
> to the usual version triple in the version string, especially around the
> time of a new major version (e.g. [1]).
>
> After the major, minor and release/patch version is compared, the part
> after should be compared lexically [2]. That means, that e.g. 9.0.0 is a
> newer version than 9.0.0~22 as 9.0.0 is lexically first.
>
> This is not needed right now, but might be worth to keep an eye on
> already so this doesn't break especially for features that require
> gatekeeping right around the release of a new major version.
>
> [1] https://git.proxmox.com/?p=pve-manager.git;a=blob;f=debian/changelog;h=a792b2c51800b4e4d8c60b8b4235bbc85386eba6;hb=HEAD#l1039
> [2] https://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-version
>
Good catch. Since qemu-server currently relies on this exact implementation, I'd prefer
not touching the underlying logic in this patch to avoid regressions.
I will however add a comment to assert_min_cluster_version(...) mentioning the current
limitation.

>> [...]