[pve-devel] [PATCH 1/2] Refactored $nokvm to $kvm

[Fabian Grünbichler]

On Wed, Aug 16, 2017 at 06:24:29PM +0200, Philip Abernethy wrote:
> Where possible I reworked the code to remove the resulting inversion.
> The result should be easier to read and understand.

small nitpick: commit messages should be written in the present tense /
imperative, not past. phrases like "I implemented", "I reworked", etc
are also redundant.

e.g.:

refactor $nokvm to $kvm

for improved readability and consistency with the option name.

> I also fixed a type in the process.

s/type/typo

but see comment inline, maybe that one could be refactored (and
typo-fixed) in a separate, first commit?

> ---
>  PVE/QemuServer.pm | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 1f34101..88b1c5b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1740,8 +1740,8 @@ sub print_netdev_full {
>  sub print_cpu_device {
>      my ($conf, $id) = @_;
>  
> -    my $nokvm = defined($conf->{kvm}) && $conf->{kvm} == 0 ? 1 : 0;
> -    my $cpu = $nokvm ? "qemu64" : "kvm64";
> +    my $kvm = $conf->{kvm} // 1;
> +    my $cpu = $kvm ? "kvm64" : "qemu64";
>      if (my $cputype = $conf->{cpu}) {
>  	my $cpuconf = PVE::JSONSchema::parse_property_string($cpu_fmt, $cputype)
>  	    or die "Cannot parse cpu description: $cputype\n";
> @@ -2835,6 +2835,7 @@ sub config_to_command {
>      my $vernum = 0; # unknown
>      my $ostype = $conf->{ostype};
>      my $winversion = windows_version($ostype);
> +    my $kvm = $conf->{kvm} // 1;
>  
>      if ($kvmver =~ m/^(\d+)\.(\d+)$/) {
>  	$vernum = $1*1000000+$2*1000;
> @@ -3085,7 +3086,6 @@ sub config_to_command {
>      # time drift fix
>      my $tdf = defined($conf->{tdf}) ? $conf->{tdf} : $defaults->{tdf};
>  
> -    my $nokvm = defined($conf->{kvm}) && $conf->{kvm} == 0 ? 1 : 0;
>      my $useLocaltime = $conf->{localtime};
>  
>      if ($winversion >= 5) { # windows
> @@ -3104,7 +3104,7 @@ sub config_to_command {
>  
>      push @$rtcFlags, 'driftfix=slew' if $tdf;
>  
> -    if ($nokvm) {
> +    if (!$kvm) {
>  	push @$machineFlags, 'accel=tcg';
>      } else {
>  	die "No accelerator found!\n" if !$cpuinfo->{hvm};
> @@ -3120,7 +3120,7 @@ sub config_to_command {
>  	push @$rtcFlags, 'base=localtime';
>      }
>  
> -    my $cpu = $nokvm ? "qemu64" : "kvm64";
> +    my $cpu = $kvm ? "kvm64" : "qemu64";
>      if (my $cputype = $conf->{cpu}) {
>  	my $cpuconf = PVE::JSONSchema::parse_property_string($cpu_fmt, $cputype)
>  	    or die "Cannot parse cpu description: $cputype\n";
> @@ -3139,13 +3139,13 @@ sub config_to_command {
>  
>      if (qemu_machine_feature_enabled ($machine_type, $kvmver, 2, 3)) {
>  
> -	push @$cpuFlags , '+kvm_pv_unhalt' if !$nokvm;
> -	push @$cpuFlags , '+kvm_pv_eoi' if !$nokvm;
> +	push @$cpuFlags , '+kvm_pv_unhalt' if $kvm;
> +	push @$cpuFlags , '+kvm_pv_eoi' if $kvm;
>      }
>  
> -    add_hyperv_enlighments($cpuFlags, $winversion, $machine_type, $kvmver, $nokvm, $conf->{bios}, $gpu_passthrough);
> +    add_hyperv_enlightenments($cpuFlags, $winversion, $machine_type, $kvmver, $kvm, $conf->{bios}, $gpu_passthrough);

(I know this isn't originally your code, but since you are touching it)

maybe we could drop the $nokvm/$kvm parameter altogether, and
conditionally call it instead? return if $nokvm is the first line in the
method after all, and there is only one call site.

"if $kvm" is only 1 char longer than ", $kvm" ;)

>  
> -    push @$cpuFlags, 'enforce' if $cpu ne 'host' && !$nokvm;
> +    push @$cpuFlags, 'enforce' if $cpu ne 'host' && $kvm;
>  
>      push @$cpuFlags, 'kvm=off' if $kvm_off;
>  
> @@ -6359,10 +6359,10 @@ sub scsihw_infos {
>      return ($maxdev, $controller, $controller_prefix);
>  }
>  
> -sub add_hyperv_enlighments {
> -    my ($cpuFlags, $winversion, $machine_type, $kvmver, $nokvm, $bios, $gpu_passthrough) = @_;
> +sub add_hyperv_enlightenments {
> +    my ($cpuFlags, $winversion, $machine_type, $kvmver, $kvm, $bios, $gpu_passthrough) = @_;
>  
> -    return if $nokvm;
> +    return if !$kvm;
>      return if $winversion < 6;
>      return if $bios && $bios eq 'ovmf' && $winversion < 8;
>  
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel