Re: [PATCH v2 qemu-server 2/5] helpers: optionally get package version for kvm_user_version()

[Fiona Ebner <f.ebner@proxmox.com>]

Am 19.08.26 um 4:47 PM schrieb Christian Ebner:
> Allows to specify whether to return the binary version or the package
> version by adding a flag to kvm_user_version(). This will allow
> comparing the package version including it's revision when the binary
> version is not enough.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes:
> - new in version 2
> 
>  src/PVE/QemuServer/Helpers.pm | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm
> index dd17eef5..658400f2 100644
> --- a/src/PVE/QemuServer/Helpers.pm
> +++ b/src/PVE/QemuServer/Helpers.pm
> @@ -53,30 +53,40 @@ my $kvm_user_version = {};
>  my $kvm_mtime = {};
>  
>  sub kvm_user_version {
> -    my ($binary) = @_;
> +    my ($binary, $query_package_version) = @_;
>  
>      $binary //= get_command_for_arch(get_host_arch()); # get the native arch by default
>      my $st = stat($binary);
>  
>      my $cachedmtime = $kvm_mtime->{$binary} // -1;
> -    return $kvm_user_version->{$binary}
> -        if $kvm_user_version->{$binary}
> -        && $cachedmtime == $st->mtime;
> +    my $version_type = $query_package_version ? 'package' : 'version';
> +    if ($kvm_user_version->{$binary}->{$version_type} && $cachedmtime == $st->mtime) {

Pre-existing, but a definedness check would be slightly nicer. I'd also
only check for $kvm_user_version->{$binary} if making the package match
optional (see my comment below).

> +        return $kvm_user_version->{$binary}->{$version_type};
> +    }
>  
> -    $kvm_user_version->{$binary} = 'unknown';
> +    $kvm_user_version->{$binary} = {
> +        version => 'unknown',
> +        package => 'unknown',
> +    };

Pre-existing, but like this the values will be initialized even if the
version command later fails for some reason and then they will stay
'unknown'. I'd rather re-attempt the query instead.

>      $kvm_mtime->{$binary} = $st->mtime;
>  
> +    my $version_regex = '(\d+\.\d+(\.\d+)?)(\.\d+)?';

Nit: could compile the regex with qr already

Nit: $version_re is a bit shorter which helps in a longer regex.

Nit: could also add a second $package_re

For the package I wouldn't even match the last number, because that
cannot be defined: in debian/rules we use
--with-pkgversion="${DEB_SOURCE}_${DEB_VERSION_UPSTREAM_REVISION}
and we only use major.minor.micro-rel

>      my $code = sub {
>          my $line = shift;
> -        if ($line =~ m/^QEMU( PC)? emulator version (\d+\.\d+(\.\d+)?)(\.\d+)?[,\s]/) {
> -            $kvm_user_version->{$binary} = $2;
> +        if ($line =~
> +            m/^QEMU( PC)? emulator version $version_regex[,\s]\(pve-qemu-kvm_($version_regex-\d+)\)/

I would make the latter half of the regex an optional match, just
because we didn't require it before.

> +        ) {
> +            $kvm_user_version->{$binary} = {
> +                version => $2,
> +                package => $5,
> +            };
>          }
>      };
>  
>      eval { PVE::Tools::run_command([$binary, '--version'], outfunc => $code); };
>      warn $@ if $@;
>  
> -    return $kvm_user_version->{$binary};
> +    return $kvm_user_version->{$binary}->{$version_type};
>  }
>  
>  # Paths and directories