public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme
@ 2021-03-08 13:43 Dominik Csapak
  2021-04-22 20:02 ` Thomas Lamprecht
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Dominik Csapak @ 2021-03-08 13:43 UTC (permalink / raw)
  To: pve-devel

Previously, we ever only had a single boot *disk*, while possibly
having multiple cdroms/nics in the boot order

e.g. the config:

 boot: dnc
 bootdisk: scsi0
 ide0: media=cdrom,none
 scsi0: xxx
 net0: ...

would return the size of scsi0 even though it would first boot
from cdrom/network.

When editing the bootorder with such a legacy config, we
remove the 'bootdisk' property and replace the legacy notation
with an explicit order, but we only search the first disk
for the size now.

Restore that behaviour by iterating over all disks in the boot
order property string until we get one that is not a cdrom
and has a size.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
i cannot remember if that change was deliberate, but at least one
user ran into that:

https://forum.proxmox.com/threads/possible-bug-boot-disk-size-shows-as-0b.85454/

 PVE/QemuServer/Drive.pm | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index d560937..26528f9 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -522,20 +522,18 @@ sub bootdisk_size {
 
     my $bootdisks = get_bootdisks($conf);
     return if !@$bootdisks;
-    my $bootdisk = $bootdisks->[0];
-    return if !is_valid_drivename($bootdisk);
-
-    return if !$conf->{$bootdisk};
-
-    my $drive = parse_drive($bootdisk, $conf->{$bootdisk});
-    return if !defined($drive);
-
-    return if drive_is_cdrom($drive);
-
-    my $volid = $drive->{file};
-    return if !$volid;
+    for my $bootdisk (@$bootdisks) {
+	next if !is_valid_drivename($bootdisk);
+	next if !$conf->{$bootdisk};
+	my $drive = parse_drive($bootdisk, $conf->{$bootdisk});
+	next if !defined($drive);
+	next if drive_is_cdrom($drive);
+	my $volid = $drive->{file};
+	next if !$volid;
+	return $drive->{size};
+    }
 
-    return $drive->{size};
+    return;
 }
 
 sub update_disksize {
-- 
2.20.1





^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme
  2021-03-08 13:43 [pve-devel] [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme Dominik Csapak
@ 2021-04-22 20:02 ` Thomas Lamprecht
  2021-04-26  8:29 ` Stefan Reiter
  2021-04-29 14:19 ` [pve-devel] applied: " Thomas Lamprecht
  2 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2021-04-22 20:02 UTC (permalink / raw)
  To: Proxmox VE development discussion, Stefan Reiter

On 08.03.21 14:43, Dominik Csapak wrote:
> Previously, we ever only had a single boot *disk*, while possibly
> having multiple cdroms/nics in the boot order
> 
> e.g. the config:
> 
>  boot: dnc
>  bootdisk: scsi0
>  ide0: media=cdrom,none
>  scsi0: xxx
>  net0: ...
> 
> would return the size of scsi0 even though it would first boot
> from cdrom/network.
> 
> When editing the bootorder with such a legacy config, we
> remove the 'bootdisk' property and replace the legacy notation
> with an explicit order, but we only search the first disk
> for the size now.
> 
> Restore that behaviour by iterating over all disks in the boot
> order property string until we get one that is not a cdrom
> and has a size.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> i cannot remember if that change was deliberate, but at least one
> user ran into that:
> 
> https://forum.proxmox.com/threads/possible-bug-boot-disk-size-shows-as-0b.85454/
> 

@Stefan, can you, as main author behind the change in question, please give
this a review - thanks!




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme
  2021-03-08 13:43 [pve-devel] [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme Dominik Csapak
  2021-04-22 20:02 ` Thomas Lamprecht
@ 2021-04-26  8:29 ` Stefan Reiter
  2021-04-29 14:19 ` [pve-devel] applied: " Thomas Lamprecht
  2 siblings, 0 replies; 4+ messages in thread
From: Stefan Reiter @ 2021-04-26  8:29 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

On 3/8/21 2:43 PM, Dominik Csapak wrote:
> Previously, we ever only had a single boot *disk*, while possibly
> having multiple cdroms/nics in the boot order
> 
> e.g. the config:
> 
>   boot: dnc
>   bootdisk: scsi0
>   ide0: media=cdrom,none
>   scsi0: xxx
>   net0: ...
> 
> would return the size of scsi0 even though it would first boot
> from cdrom/network.
> 
> When editing the bootorder with such a legacy config, we
> remove the 'bootdisk' property and replace the legacy notation
> with an explicit order, but we only search the first disk
> for the size now.
> 
> Restore that behaviour by iterating over all disks in the boot
> order property string until we get one that is not a cdrom
> and has a size.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> i cannot remember if that change was deliberate, but at least one
> user ran into that:
> 
> https://forum.proxmox.com/threads/possible-bug-boot-disk-size-shows-as-0b.85454/
> 

Not intentional, I'm pretty sure I just went with the easy way out back 
then ;)

Reviewed-by: Stefan Reiter <s.reiter@proxmox.com>




^ permalink raw reply	[flat|nested] 4+ messages in thread

* [pve-devel] applied: [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme
  2021-03-08 13:43 [pve-devel] [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme Dominik Csapak
  2021-04-22 20:02 ` Thomas Lamprecht
  2021-04-26  8:29 ` Stefan Reiter
@ 2021-04-29 14:19 ` Thomas Lamprecht
  2 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2021-04-29 14:19 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

On 08.03.21 14:43, Dominik Csapak wrote:
> Previously, we ever only had a single boot *disk*, while possibly
> having multiple cdroms/nics in the boot order
> 
> e.g. the config:
> 
>  boot: dnc
>  bootdisk: scsi0
>  ide0: media=cdrom,none
>  scsi0: xxx
>  net0: ...
> 
> would return the size of scsi0 even though it would first boot
> from cdrom/network.
> 
> When editing the bootorder with such a legacy config, we
> remove the 'bootdisk' property and replace the legacy notation
> with an explicit order, but we only search the first disk
> for the size now.
> 
> Restore that behaviour by iterating over all disks in the boot
> order property string until we get one that is not a cdrom
> and has a size.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> i cannot remember if that change was deliberate, but at least one
> user ran into that:
> 
> https://forum.proxmox.com/threads/possible-bug-boot-disk-size-shows-as-0b.85454/
> 
>  PVE/QemuServer/Drive.pm | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
>

applied, thanks!




^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-04-29 14:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 13:43 [pve-devel] [RFC PATCH qemu-server] fix bootdisk_size for new bootorder config scheme Dominik Csapak
2021-04-22 20:02 ` Thomas Lamprecht
2021-04-26  8:29 ` Stefan Reiter
2021-04-29 14:19 ` [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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal