* [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing
@ 2022-12-02 12:59 Fiona Ebner
2022-12-06 17:11 ` Thomas Lamprecht
2022-12-12 10:45 ` [pve-devel] applied: " Thomas Lamprecht
0 siblings, 2 replies; 4+ messages in thread
From: Fiona Ebner @ 2022-12-02 12:59 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing
2022-12-02 12:59 [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing Fiona Ebner
@ 2022-12-06 17:11 ` Thomas Lamprecht
2022-12-07 9:02 ` Fiona Ebner
2022-12-12 10:45 ` [pve-devel] applied: " Thomas Lamprecht
1 sibling, 1 reply; 4+ messages in thread
From: Thomas Lamprecht @ 2022-12-06 17:11 UTC (permalink / raw)
To: Proxmox VE development discussion, Fiona Ebner
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 {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing
2022-12-06 17:11 ` Thomas Lamprecht
@ 2022-12-07 9:02 ` Fiona Ebner
0 siblings, 0 replies; 4+ messages in thread
From: Fiona Ebner @ 2022-12-07 9:02 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
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 :)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [pve-devel] applied: [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing
2022-12-02 12:59 [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing Fiona Ebner
2022-12-06 17:11 ` Thomas Lamprecht
@ 2022-12-12 10:45 ` Thomas Lamprecht
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2022-12-12 10:45 UTC (permalink / raw)
To: Proxmox VE development discussion, Fiona Ebner
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!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-12 10:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02 12:59 [pve-devel] [PATCH qemu-server] cfg2cmd: factor out ovmf drives printing Fiona Ebner
2022-12-06 17:11 ` Thomas Lamprecht
2022-12-07 9:02 ` Fiona Ebner
2022-12-12 10:45 ` [pve-devel] applied: " Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox