[pve-devel] [RFC v2 qemu-server 4/4] move helper to check running QEMU version out of the 'Machine' module

[Fabian Grünbichler]

On May 28, 2024 10:50 am, Fiona Ebner wrote:
> The version of the running QEMU binary is not related to the machine
> version and so it's a bit confusing to have the helper in the
> 'Machine' module. It cannot live in the 'Helpers' module, because that
> would lead to a cyclic inclusion Helpers <-> Monitor. Thus,
> 'QMPHelpers' is chosen as the new home.
> 
> Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>

Acked-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>

but needs the first patch to be applied, or a re-order to move this
first ;)

> ---
> 
> New in v2.
> 
>  PVE/QemuMigrate.pm                    |  3 ++-
>  PVE/QemuServer/Machine.pm             | 12 ------------
>  PVE/QemuServer/QMPHelpers.pm          | 13 +++++++++++++
>  test/MigrationTest/QemuMigrateMock.pm |  4 ++++
>  test/run_config2command_tests.pl      |  4 ++--
>  5 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 34fc46ee..e71face4 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -30,6 +30,7 @@ use PVE::QemuServer::Helpers qw(min_version);
>  use PVE::QemuServer::Machine;
>  use PVE::QemuServer::Monitor qw(mon_cmd);
>  use PVE::QemuServer::Memory qw(get_current_memory);
> +use PVE::QemuServer::QMPHelpers;
>  use PVE::QemuServer;
>  
>  use PVE::AbstractMigrate;
> @@ -1140,7 +1141,7 @@ sub phase2 {
>  	    PVE::QemuServer::qemu_drive_mirror($vmid, $drive, $nbd_uri, $vmid, undef, $self->{storage_migration_jobs}, 'skip', undef, $bwlimit, $bitmap);
>  	}
>  
> -	if (PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 8, 2)) {
> +	if (PVE::QemuServer::QMPHelpers::runs_at_least_qemu_version($vmid, 8, 2)) {
>  	    $self->log('info', "switching mirror jobs to actively synced mode");
>  	    PVE::QemuServer::qemu_drive_mirror_switch_to_active_mode(
>  		$vmid,
> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
> index cc92e7e6..a3917dae 100644
> --- a/PVE/QemuServer/Machine.pm
> +++ b/PVE/QemuServer/Machine.pm
> @@ -161,18 +161,6 @@ sub can_run_pve_machine_version {
>      return 0;
>  }
>  
> -# dies if a) VM not running or not exisiting b) Version query failed
> -# So, any defined return value is valid, any invalid state can be caught by eval
> -sub runs_at_least_qemu_version {
> -    my ($vmid, $major, $minor, $extra) = @_;
> -
> -    my $v = PVE::QemuServer::Monitor::mon_cmd($vmid, 'query-version');
> -    die "could not query currently running version for VM $vmid\n" if !defined($v);
> -    $v = $v->{qemu};
> -
> -    return PVE::QemuServer::Helpers::version_cmp($v->{major}, $major, $v->{minor}, $minor, $v->{micro}, $extra) >= 0;
> -}
> -
>  sub qemu_machine_pxe {
>      my ($vmid, $conf) = @_;
>  
> diff --git a/PVE/QemuServer/QMPHelpers.pm b/PVE/QemuServer/QMPHelpers.pm
> index d3a52327..0269ea46 100644
> --- a/PVE/QemuServer/QMPHelpers.pm
> +++ b/PVE/QemuServer/QMPHelpers.pm
> @@ -3,6 +3,7 @@ package PVE::QemuServer::QMPHelpers;
>  use warnings;
>  use strict;
>  
> +use PVE::QemuServer::Helpers;
>  use PVE::QemuServer::Monitor qw(mon_cmd);
>  
>  use base 'Exporter';
> @@ -45,4 +46,16 @@ sub qemu_objectdel {
>      return 1;
>  }
>  
> +# dies if a) VM not running or not exisiting b) Version query failed
> +# So, any defined return value is valid, any invalid state can be caught by eval
> +sub runs_at_least_qemu_version {
> +    my ($vmid, $major, $minor, $extra) = @_;
> +
> +    my $v = PVE::QemuServer::Monitor::mon_cmd($vmid, 'query-version');
> +    die "could not query currently running version for VM $vmid\n" if !defined($v);
> +    $v = $v->{qemu};
> +
> +    return PVE::QemuServer::Helpers::version_cmp($v->{major}, $major, $v->{minor}, $minor, $v->{micro}, $extra) >= 0;
> +}
> +
>  1;
> diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
> index f5b44424..11c58c08 100644
> --- a/test/MigrationTest/QemuMigrateMock.pm
> +++ b/test/MigrationTest/QemuMigrateMock.pm
> @@ -188,6 +188,10 @@ $qemu_server_machine_module->mock(
>  	    if !defined($vm_status->{runningmachine});
>  	return $vm_status->{runningmachine};
>      },
> +);
> +
> +my $qemu_server_qmphelpers_module = Test::MockModule->new("PVE::QemuServer::QMPHelpers");
> +$qemu_server_qmphelpers_module->mock(
>      runs_at_least_qemu_version => sub {
>  	return 1;
>      },
> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
> index 7212acc4..d48ef562 100755
> --- a/test/run_config2command_tests.pl
> +++ b/test/run_config2command_tests.pl
> @@ -16,7 +16,7 @@ use PVE::SysFSTools;
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
>  use PVE::QemuServer::Monitor;
> -use PVE::QemuServer::Machine;
> +use PVE::QemuServer::QMPHelpers;
>  use PVE::QemuServer::CPUConfig;
>  
>  my $base_env = {
> @@ -472,7 +472,7 @@ sub do_test($) {
>      # check if QEMU version set correctly and test version_cmp
>      (my $qemu_major = get_test_qemu_version()) =~ s/\..*$//;
>      die "runs_at_least_qemu_version returned false, maybe error in version_cmp?"
> -	if !PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, $qemu_major);
> +	if !PVE::QemuServer::QMPHelpers::runs_at_least_qemu_version($vmid, $qemu_major);
>  
>      $cmdline =~ s/ -/ \\\n  -/g; # same as qm showcmd --pretty
>      $cmdline .= "\n";
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>