[pve-devel] [PATCH v2 qemu-server 07/11] refactor: extract QEMU machine related helpers to package

Fabian Grünbichler f.gruenbichler at proxmox.com
Wed Oct 30 11:49:22 CET 2019


On October 28, 2019 12:59 pm, Stefan Reiter wrote:
> ...PVE::QemuServer::Machine.
> 
> qemu_machine_feature_enabled is exported since it has a *lot* of users
> in PVE::QemuServer and a long enough name as it is.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> Not sure if PVE::QemuMachine wouldn't be a better package name. I'm fine with
> both (or other suggestions), if someone has preferences.

like the others, I'd suggest

PVE::Qemu::Machine

we'd probably not need the export then either, since
qemu_machine_feature_enabled
PVE::Qemu::Machine::feature_enabled
PVE::Qemu::Machine::has_feature

is not much of a difference? see below/inline as well ;)

> 
>  PVE/QemuConfig.pm         |   3 +-
>  PVE/QemuMigrate.pm        |   3 +-
>  PVE/QemuServer.pm         | 101 +++-----------------------------------
>  PVE/QemuServer/Machine.pm | 100 +++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer/Makefile   |   1 +
>  PVE/VZDump/QemuServer.pm  |   3 +-
>  6 files changed, 115 insertions(+), 96 deletions(-)
>  create mode 100644 PVE/QemuServer/Machine.pm
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 06ace83..e7af9ad 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -11,6 +11,7 @@ use PVE::INotify;
>  use PVE::ProcFSTools;
>  use PVE::QemuSchema;
>  use PVE::QemuServer;
> +use PVE::QemuServer::Machine;
>  use PVE::QMP qw(vm_mon_cmd vm_mon_cmd_nocheck);
>  use PVE::Storage;
>  use PVE::Tools;
> @@ -150,7 +151,7 @@ sub __snapshot_save_vmstate {
>      $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
>  
>      my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
> -    my $runningmachine = PVE::QemuServer::get_current_qemu_machine($vmid);
> +    my $runningmachine = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
>  
>      if ($suspend) {
>  	$conf->{vmstate} = $statefile;
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index aea7eac..9ac78f8 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -12,6 +12,7 @@ use PVE::Cluster;
>  use PVE::Storage;
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
> +use PVE::QemuServer::Machine;
>  use PVE::QMP qw(vm_mon_cmd vm_mon_cmd_nocheck);
>  use Time::HiRes qw( usleep );
>  use PVE::RPCEnvironment;
> @@ -217,7 +218,7 @@ sub prepare {
>  	die "can't migrate running VM without --online\n" if !$online;
>  	$running = $pid;
>  
> -	$self->{forcemachine} = PVE::QemuServer::qemu_machine_pxe($vmid, $conf);
> +	$self->{forcemachine} = PVE::QemuServer::Machine::qemu_machine_pxe($vmid, $conf);
>  
>      }
>      my $loc_res = PVE::QemuServer::check_local_resources($conf, 1);
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ed137fc..20a6380 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -42,6 +42,7 @@ use PVE::QMPClient;
>  use PVE::QemuConfig;
>  use PVE::QemuSchema;
>  use PVE::QemuServer::Cloudinit;
> +use PVE::QemuServer::Machine qw(qemu_machine_feature_enabled);
>  use PVE::QemuServer::Memory;
>  use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr print_pcie_root_port);
>  use PVE::QemuServer::USB qw(parse_usb_device);
> @@ -1827,20 +1828,14 @@ sub path_is_scsi {
>      return $res;
>  }
>  
> -sub machine_type_is_q35 {
> -    my ($conf) = @_;
> -
> -    return $conf->{machine} && ($conf->{machine} =~ m/q35/) ? 1 : 0;
> -}
> -
>  sub print_tabletdevice_full {
>      my ($conf, $arch) = @_;
>  
> -    my $q35 = machine_type_is_q35($conf);
> +    my $q35 = PVE::QemuServer::Machine::machine_type_is_q35($conf);
>  
>      # we use uhci for old VMs because tablet driver was buggy in older qemu
>      my $usbbus;
> -    if (machine_type_is_q35($conf) || $arch eq 'aarch64') {
> +    if (PVE::QemuServer::Machine::machine_type_is_q35($conf) || $arch eq 'aarch64') {
>  	$usbbus = 'ehci';
>      } else {
>  	$usbbus = 'uhci';
> @@ -2189,7 +2184,7 @@ sub print_vga_device {
>  	$memory = ",ram_size=67108864,vram_size=33554432";
>      }
>  
> -    my $q35 = machine_type_is_q35($conf);
> +    my $q35 = PVE::QemuServer::Machine::machine_type_is_q35($conf);
>      my $vgaid = "vga" . ($id // '');
>      my $pciaddr;
>  
> @@ -3478,7 +3473,7 @@ sub config_to_command {
>  
>      die "detected old qemu-kvm binary ($kvmver)\n" if $vernum < 15000;
>  
> -    my $q35 = machine_type_is_q35($conf);
> +    my $q35 = PVE::QemuServer::Machine::machine_type_is_q35($conf);
>      my $hotplug_features = parse_hotplug_features(defined($conf->{hotplug}) ? $conf->{hotplug} : '1');
>      my $use_old_bios_files = undef;
>      ($use_old_bios_files, $machine_type) = qemu_use_old_bios_files($machine_type);
> @@ -4112,7 +4107,7 @@ sub vm_devices_list {
>  sub vm_deviceplug {
>      my ($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type) = @_;
>  
> -    my $q35 = machine_type_is_q35($conf);
> +    my $q35 = PVE::QemuServer::Machine::machine_type_is_q35($conf);
>  
>      my $devices_list = vm_devices_list($vmid);
>      return 1 if defined($devices_list->{$deviceid});
> @@ -4188,7 +4183,7 @@ sub vm_deviceplug {
>  
>  	return undef if !qemu_netdevadd($vmid, $conf, $arch, $device, $deviceid);
>  
> -	my $machine_type = PVE::QemuServer::qemu_machine_pxe($vmid, $conf);
> +	my $machine_type = PVE::QemuServer::Machine::qemu_machine_pxe($vmid, $conf);
>  	my $use_old_bios_files = undef;
>  	($use_old_bios_files, $machine_type) = qemu_use_old_bios_files($machine_type);
>  
> @@ -4502,7 +4497,7 @@ sub qemu_usb_hotplug {
>  sub qemu_cpu_hotplug {
>      my ($vmid, $conf, $vcpus) = @_;
>  
> -    my $machine_type = PVE::QemuServer::get_current_qemu_machine($vmid);
> +    my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
>  
>      my $sockets = 1;
>      $sockets = $conf->{smp} if $conf->{smp}; # old style - no longer iused
> @@ -6986,23 +6981,6 @@ sub clone_disk {
>      return $disk;
>  }
>  
> -# this only works if VM is running
> -sub get_current_qemu_machine {
> -    my ($vmid) = @_;
> -
> -    my $cmd = { execute => 'query-machines', arguments => {} };
> -    my $res = vm_qmp_command($vmid, $cmd);
> -
> -    my ($current, $default);
> -    foreach my $e (@$res) {
> -	$default = $e->{name} if $e->{'is-default'};
> -	$current = $e->{name} if $e->{'is-current'};
> -    }
> -
> -    # fallback to the default machine if current is not supported by qemu
> -    return $current || $default || 'pc';
> -}
> -
>  sub get_running_qemu_version {
>      my ($vmid) = @_;
>      my $cmd = { execute => 'query-version', arguments => {} };
> @@ -7010,69 +6988,6 @@ sub get_running_qemu_version {
>      return "$res->{qemu}->{major}.$res->{qemu}->{minor}";
>  }
>  
> -sub qemu_machine_feature_enabled {
> -    my ($machine, $kvmver, $version_major, $version_minor) = @_;
> -
> -    my $current_major;
> -    my $current_minor;
> -
> -    if ($machine && $machine =~ m/^((?:pc(-i440fx|-q35)?|virt)-(\d+)\.(\d+))/) {
> -
> -	$current_major = $3;
> -	$current_minor = $4;
> -
> -    } elsif ($kvmver =~ m/^(\d+)\.(\d+)/) {
> -
> -	$current_major = $1;
> -	$current_minor = $2;
> -    }
> -
> -    return 1 if version_cmp($current_major, $version_major, $current_minor, $version_minor) >= 0;
> -}
> -
> -# 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;
> -    die "cannot compare odd count of versions" if $size & 1;
> -
> -    for (my $i = 0; $i < $size; $i += 2) {
> -	my ($a, $b) = splice(@versions, 0, 2);
> -	$a //= 0;
> -	$b //= 0;
> -
> -	return 1 if $a > $b;
> -	return -1 if $a < $b;
> -    }
> -    return 0;
> -}
> -
> -sub runs_at_least_qemu_version {
> -    my ($vmid, $major, $minor, $extra) = @_;
> -
> -    my $v = eval { vm_qmp_command($vmid, { execute => 'query-version' }) } // {};
> -    $v = $v->{qemu};
> -
> -    return version_cmp($v->{major}, $major, $v->{minor}, $minor, $v->{micro}, $extra) >= 0;
> -}
> -
> -sub qemu_machine_pxe {
> -    my ($vmid, $conf) = @_;
> -
> -    my $machine =  PVE::QemuServer::get_current_qemu_machine($vmid);
> -
> -    if ($conf->{machine} && $conf->{machine} =~ m/\.pxe$/) {
> -	$machine .= '.pxe';
> -    }
> -
> -    return $machine;
> -}
> -
>  sub qemu_use_old_bios_files {
>      my ($machine_type) = @_;
>  
> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
> new file mode 100644
> index 0000000..46c1101
> --- /dev/null
> +++ b/PVE/QemuServer/Machine.pm
> @@ -0,0 +1,100 @@
> +package PVE::QemuServer::Machine;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::QemuConfig;
> +use PVE::QMP;
> +
> +use base 'Exporter';
> +our @EXPORT_OK = qw(
> +qemu_machine_feature_enabled
> +);
> +
> +sub machine_type_is_q35 {
> +    my ($conf) = @_;
> +
> +    return $conf->{machine} && ($conf->{machine} =~ m/q35/) ? 1 : 0;
> +}
> +
> +# this only works if VM is running
> +sub get_current_qemu_machine {
> +    my ($vmid) = @_;
> +
> +    my $cmd = { execute => 'query-machines', arguments => {} };
> +    my $res = PVE::QMP::vm_qmp_command($vmid, $cmd);
> +
> +    my ($current, $default);
> +    foreach my $e (@$res) {
> +	$default = $e->{name} if $e->{'is-default'};
> +	$current = $e->{name} if $e->{'is-current'};
> +    }
> +
> +    # fallback to the default machine if current is not supported by qemu
> +    return $current || $default || 'pc';
> +}
> +
> +sub qemu_machine_feature_enabled {
> +    my ($machine, $kvmver, $version_major, $version_minor) = @_;
> +
> +    my $current_major;
> +    my $current_minor;
> +
> +    if ($machine && $machine =~ m/^((?:pc(-i440fx|-q35)?|virt)-(\d+)\.(\d+))/) {
> +
> +	$current_major = $3;
> +	$current_minor = $4;
> +
> +    } elsif ($kvmver =~ m/^(\d+)\.(\d+)/) {
> +
> +	$current_major = $1;
> +	$current_minor = $2;
> +    }
> +
> +    return 1 if version_cmp($current_major, $version_major, $current_minor, $version_minor) >= 0;
> +}

we could also use this opportunity and change the signature and/or name 
here. this method does not actually check if a 'feature' is enabled, it
- does a version check
- using a pinned machine type version if available
- using the kvm binary version string otherwise

most call-sites pass in both strings, some just the former, some just 
the latter. maybe you could evaluate whether that makes sense?

maybe another helper to extract $machine_ver from $machine, and a single 

PVE::Qemu::Machine::check_version

or even

PVE::Qemu::Machine::min_version('4.0.1', 3, 9)

with just version string to check, minimum major, minimum minor
would make more sense? I think it would allow to merge the $machine_type 
and $kvmver parameters for a lot of the helpers in QemuServer.pm ..

> +# 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;
> +    die "cannot compare odd count of versions" if $size & 1;
> +
> +    for (my $i = 0; $i < $size; $i += 2) {
> +	my ($a, $b) = splice(@versions, 0, 2);
> +	$a //= 0;
> +	$b //= 0;
> +
> +	return 1 if $a > $b;
> +	return -1 if $a < $b;
> +    }
> +    return 0;
> +}
> +
> +sub runs_at_least_qemu_version {
> +    my ($vmid, $major, $minor, $extra) = @_;
> +
> +    my $v = eval { PVE::QMP::vm_qmp_command($vmid, { execute => 'query-version' }) } // {};
> +    $v = $v->{qemu};
> +
> +    return version_cmp($v->{major}, $major, $v->{minor}, $minor, $v->{micro}, $extra) >= 0;
> +}
> +
> +sub qemu_machine_pxe {
> +    my ($vmid, $conf) = @_;
> +
> +    my $machine =  get_current_qemu_machine($vmid);
> +
> +    if ($conf->{machine} && $conf->{machine} =~ m/\.pxe$/) {
> +	$machine .= '.pxe';
> +    }
> +
> +    return $machine;
> +}
> +
> +1;
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index afc39a3..35380c3 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -5,6 +5,7 @@ SOURCES=PCI.pm		\
>  	OVF.pm		\
>  	Cloudinit.pm	\
>  	Agent.pm	\
> +	Machine.pm	\
>  
>  .PHONY: install
>  install: ${SOURCES}
> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
> index dc6b29a..f8a2f81 100644
> --- a/PVE/VZDump/QemuServer.pm
> +++ b/PVE/VZDump/QemuServer.pm
> @@ -19,6 +19,7 @@ use PVE::VZDump;
>  
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
> +use PVE::QemuServer::Machine;
>  use PVE::QMP qw(vm_mon_cmd);
>  
>  use base qw (PVE::VZDump::Plugin);
> @@ -79,7 +80,7 @@ sub prepare {
>  	    $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
>  	    return;
>  	} elsif ($self->{vm_was_running} && $drive->{iothread}) {
> -	    if (!PVE::QemuServer::runs_at_least_qemu_version($vmid, 4, 0, 1)) {
> +	    if (!PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4, 0, 1)) {
>  		die "disk '$ds' '$volid' (iothread=on) can't use backup feature with running QEMU " .
>  		    "version < 4.0.1! Either set backup=no for this drive or upgrade QEMU and restart VM\n";
>  	    }
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 




More information about the pve-devel mailing list