[pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing

[pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing

[Fiona Ebner]

No functional change is intended.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Popped out while trying the other approach mentioned in:
https://lists.proxmox.com/pipermail/pve-devel/2022-December/055091.html

Better viewed with
--color-moved=zebra --color-moved-ws=ignore-all-space
or similar.

 PVE/QemuServer.pm | 114 +++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 48 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a52a883e..619908de 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3514,6 +3514,68 @@ my sub should_disable_smm {
 	$vga->{type} && $vga->{type} =~ m/^(serial\d+|none)$/;
 }
 
+my sub print_ovmf_drive_commandlines {
+    my ($conf, $storecfg, $vmid, $arch, $q35, $version_guard) = @_;
+
+    my $d;
+    if (my $efidisk = $conf->{efidisk0}) {
+	$d = parse_drive('efidisk0', $efidisk);
+    }
+
+    my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35);
+    die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
+
+    my ($path, $format);
+    my $read_only_str = '';
+    if ($d) {
+	my ($storeid, $volname) = PVE::Storage::parse_volume_id($d->{file}, 1);
+	$format = $d->{format};
+	if ($storeid) {
+	    $path = PVE::Storage::path($storecfg, $d->{file});
+	    if (!defined($format)) {
+		my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+		$format = qemu_img_format($scfg, $volname);
+	    }
+	} else {
+	    $path = $d->{file};
+	    die "efidisk format must be specified\n"
+		if !defined($format);
+	}
+
+	$read_only_str = ',readonly=on' if drive_is_read_only($conf, $d);
+    } else {
+	log_warn("no efidisk configured! Using temporary efivars disk.");
+	$path = "/tmp/$vmid-ovmf.fd";
+	PVE::Tools::file_copy($ovmf_vars, $path, -s $ovmf_vars);
+	$format = 'raw';
+    }
+
+    my $size_str = "";
+
+    if ($format eq 'raw' && $version_guard->(4, 1, 2)) {
+	$size_str = ",size=" . (-s $ovmf_vars);
+    }
+
+    # SPI flash does lots of read-modify-write OPs, without writeback this gets really slow #3329
+    my $cache = "";
+    if ($path =~ m/^rbd:/) {
+	$cache = ',cache=writeback';
+	$path .= ':rbd_cache_policy=writeback'; # avoid write-around, we *need* to cache writes too
+    }
+
+    my $code_drive_str = "if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code";
+    my $var_drive_str = "if=pflash"
+	. ",unit=1"
+	. "$cache"
+	. ",format=$format"
+	. ",id=drive-efidisk0"
+	. "$size_str"
+	. ",file=$path"
+	. "$read_only_str";
+
+    return ($code_drive_str, $var_drive_str);
+}
+
 sub config_to_command {
     my ($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu,
         $pbs_backing) = @_;
@@ -3633,54 +3695,10 @@ sub config_to_command {
     }
 
     if ($conf->{bios} && $conf->{bios} eq 'ovmf') {
-	my $d;
-	if (my $efidisk = $conf->{efidisk0}) {
-	    $d = parse_drive('efidisk0', $efidisk);
-	}
-
-	my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35);
-	die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
-
-	my ($path, $format);
-	my $read_only_str = '';
-	if ($d) {
-	    my ($storeid, $volname) = PVE::Storage::parse_volume_id($d->{file}, 1);
-	    $format = $d->{format};
-	    if ($storeid) {
-		$path = PVE::Storage::path($storecfg, $d->{file});
-		if (!defined($format)) {
-		    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-		    $format = qemu_img_format($scfg, $volname);
-		}
-	    } else {
-		$path = $d->{file};
-		die "efidisk format must be specified\n"
-		    if !defined($format);
-	    }
-
-	    $read_only_str = ',readonly=on' if drive_is_read_only($conf, $d);
-	} else {
-	    log_warn("no efidisk configured! Using temporary efivars disk.");
-	    $path = "/tmp/$vmid-ovmf.fd";
-	    PVE::Tools::file_copy($ovmf_vars, $path, -s $ovmf_vars);
-	    $format = 'raw';
-	}
-
-	my $size_str = "";
-
-	if ($format eq 'raw' && $version_guard->(4, 1, 2)) {
-	    $size_str = ",size=" . (-s $ovmf_vars);
-	}
-
-	# SPI flash does lots of read-modify-write OPs, without writeback this gets really slow #3329
-	my $cache = "";
-	if ($path =~ m/^rbd:/) {
-		$cache = ',cache=writeback';
-		$path .= ':rbd_cache_policy=writeback'; # avoid write-around, we *need* to cache writes too
-	}
-
-	push @$cmd, '-drive', "if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code";
-	push @$cmd, '-drive', "if=pflash,unit=1$cache,format=$format,id=drive-efidisk0$size_str,file=${path}${read_only_str}";
+	my ($code_drive_str, $var_drive_str) =
+	    print_ovmf_drive_commandlines($conf, $storecfg, $vmid, $arch, $q35, $version_guard);
+	push $cmd->@*, '-drive', $code_drive_str;
+	push $cmd->@*, '-drive', $var_drive_str;
     }
 
     if ($q35) { # tell QEMU to load q35 config early
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing

[Thomas Lamprecht]

Am 02/12/2022 um 13:59 schrieb Fiona Ebner:
> No functional change is intended.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Popped out while trying the other approach mentioned in:
> https://lists.proxmox.com/pipermail/pve-devel/2022-December/055091.html

looks ok, having it in a separate method opens up a few more "line reduction without
making it harder to understand" possibilities, e.g., the following diff would drop
21 lines, but reorder the var string a bit too - what do you think?


----8<----
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 619908d..dd6ea3e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3517,63 +3517,42 @@ my sub should_disable_smm {
 my sub print_ovmf_drive_commandlines {
     my ($conf, $storecfg, $vmid, $arch, $q35, $version_guard) = @_;
 
-    my $d;
-    if (my $efidisk = $conf->{efidisk0}) {
-       $d = parse_drive('efidisk0', $efidisk);
-    }
+    my $d = $conf->{efidisk0} ? parse_drive('efidisk0', $conf->{efidisk0}) : undef;
 
     my ($ovmf_code, $ovmf_vars) = get_ovmf_files($arch, $d, $q35);
     die "uefi base image '$ovmf_code' not found\n" if ! -f $ovmf_code;
 
-    my ($path, $format);
-    my $read_only_str = '';
+    my $var_drive_str = "if=pflash,unit=1,id=drive-efidisk0";
     if ($d) {
        my ($storeid, $volname) = PVE::Storage::parse_volume_id($d->{file}, 1);
-       $format = $d->{format};
+       my ($path, $format) = $d->@{'file', 'format'};
        if ($storeid) {
            $path = PVE::Storage::path($storecfg, $d->{file});
            if (!defined($format)) {
                my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
                $format = qemu_img_format($scfg, $volname);
            }
-       } else {
-           $path = $d->{file};
-           die "efidisk format must be specified\n"
-               if !defined($format);
+       } elsif (!defined($format)) {
+           die "efidisk format must be specified\n";
        }
+       # SPI flash does lots of read-modify-write OPs, without writeback this gets really slow #3329
+       if ($path =~ m/^rbd:/) {
+           $var_drive_str .= ',cache=writeback';
+           $path .= ':rbd_cache_policy=writeback'; # avoid write-around, we *need* to cache writes too
+       }
+       $var_drive_str .= ",format=$format,file=$path";
 
-       $read_only_str = ',readonly=on' if drive_is_read_only($conf, $d);
+       $var_drive_str .= ",size=" . (-s $ovmf_vars) if $format eq 'raw' && $version_guard->(4, 1, 2);
+       $var_drive_str .= ',readonly=on' if drive_is_read_only($conf, $d);
     } else {
        log_warn("no efidisk configured! Using temporary efivars disk.");
-       $path = "/tmp/$vmid-ovmf.fd";
+       my $path = "/tmp/$vmid-ovmf.fd";
        PVE::Tools::file_copy($ovmf_vars, $path, -s $ovmf_vars);
-       $format = 'raw';
+       $var_drive_str .= ",format=raw,file=$path";
+       $var_drive_str .= ",size=" . (-s $ovmf_vars) if $version_guard->(4, 1, 2);
     }
 
-    my $size_str = "";
-
-    if ($format eq 'raw' && $version_guard->(4, 1, 2)) {
-       $size_str = ",size=" . (-s $ovmf_vars);
-    }
-
-    # SPI flash does lots of read-modify-write OPs, without writeback this gets really slow #3329
-    my $cache = "";
-    if ($path =~ m/^rbd:/) {
-       $cache = ',cache=writeback';
-       $path .= ':rbd_cache_policy=writeback'; # avoid write-around, we *need* to cache writes too
-    }
-
-    my $code_drive_str = "if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code";
-    my $var_drive_str = "if=pflash"
-       . ",unit=1"
-       . "$cache"
-       . ",format=$format"
-       . ",id=drive-efidisk0"
-       . "$size_str"
-       . ",file=$path"
-       . "$read_only_str";
-
-    return ($code_drive_str, $var_drive_str);
+    return ("if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code", $var_drive_str);
 }
 
 sub config_to_command {

Re: [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing

[Fiona Ebner]

Am 06.12.22 um 18:11 schrieb Thomas Lamprecht:
> Am 02/12/2022 um 13:59 schrieb Fiona Ebner:
>> No functional change is intended.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Popped out while trying the other approach mentioned in:
>> https://lists.proxmox.com/pipermail/pve-devel/2022-December/055091.html
> 
> looks ok, having it in a separate method opens up a few more "line reduction without
> making it harder to understand" possibilities, e.g., the following diff would drop
> 21 lines, but reorder the var string a bit too - what do you think?
> 

Seems like the tabs got converted to spaces in the diff somewhere along
the way. But other than the few lines that break the 100 char limit, it
looks ok to me :)

[pve-devel] applied: [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing

[Thomas Lamprecht]

Am 02/12/2022 um 13:59 schrieb Fiona Ebner:
> No functional change is intended.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Popped out while trying the other approach mentioned in:
> https://lists.proxmox.com/pipermail/pve-devel/2022-December/055091.html
> 
> Better viewed with
> --color-moved=zebra --color-moved-ws=ignore-all-space
> or similar.
> 
>  PVE/QemuServer.pm | 114 +++++++++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 48 deletions(-)
> 
>

applied, with the proposed followup (split into two commits), thanks!