* [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype @ 2024-04-09 14:47 Hannes Duerr 2024-04-10 9:34 ` Thomas Lamprecht 0 siblings, 1 reply; 3+ messages in thread From: Hannes Duerr @ 2024-04-09 14:47 UTC (permalink / raw) To: pve-devel When we obtain the devicetype, we check whether it is a CD drive. Cloudinit drives are always allocated CD drives, but if the drive has not yet been allocated, the test fails because the cd attribute has not yet been set. We therefore now explicitly check whether it is a cloudinit drive that has not yet been allocated. Signed-off-by: Hannes Duerr <h.duerr@proxmox.com> --- PVE/QemuServer/Drive.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm index 34c6e87..c829bde 100644 --- a/PVE/QemuServer/Drive.pm +++ b/PVE/QemuServer/Drive.pm @@ -853,7 +853,7 @@ sub get_scsi_devicetype { my $devicetype = 'hd'; my $path = ''; - if (drive_is_cdrom($drive)) { + if (drive_is_cdrom($drive) || drive_is_cloudinit($drive)) { $devicetype = 'cd'; } else { if ($drive->{file} =~ m|^/|) { -- 2.39.2 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype 2024-04-09 14:47 [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype Hannes Duerr @ 2024-04-10 9:34 ` Thomas Lamprecht 2024-04-10 11:19 ` Hannes Dürr 0 siblings, 1 reply; 3+ messages in thread From: Thomas Lamprecht @ 2024-04-10 9:34 UTC (permalink / raw) To: Proxmox VE development discussion, Hannes Duerr This is not bug #5365 [0] (which is about a ceph device class UX improvement) but #5363 [0]. [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5365 [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=5363 I mostly noticed because I had too loo what this is actually about, IMO the subject could be a bit more telling, targeting more our users, not our devs (and even for those it's most often not too helpful to name methods directly). How about something like: fix #5363: cloudinit: allow using scsi for the CD-ROM again That'd tell me roughly what was wrong and also that it worked previously already, i.e. fixes a regression, rather than a new enhancement. But that was just from top of my head, maybe there's ab even more telling one possible, e.g. one alternative could be: fix #5363: scsi device type: detect cloudinit also for new drives Am 09/04/2024 um 16:47 schrieb Hannes Duerr: > When we obtain the devicetype, we check whether it is a CD drive. I know this quite often comes natural, but for commit messages its IMO often a bit better to avoid first-person pronouns to avoid the reader asking "who's we?". > Cloudinit drives are always allocated CD drives, but if the drive has > not yet been allocated, the test fails because the cd attribute has not > yet been set. > We therefore now explicitly check whether it is a cloudinit > drive that has not yet been allocated. As per the bug report this seems to be a regression, in which case a reference to the commit this fixes would be nice. > Signed-off-by: Hannes Duerr <h.duerr@proxmox.com> > --- > PVE/QemuServer/Drive.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm > index 34c6e87..c829bde 100644 > --- a/PVE/QemuServer/Drive.pm > +++ b/PVE/QemuServer/Drive.pm > @@ -853,7 +853,7 @@ sub get_scsi_devicetype { unrelated, but "get_scsi_device_type" would be a slightly easier to read variant of that name. > > my $devicetype = 'hd'; > my $path = ''; > - if (drive_is_cdrom($drive)) { > + if (drive_is_cdrom($drive) || drive_is_cloudinit($drive)) { > $devicetype = 'cd'; > } else { > if ($drive->{file} =~ m|^/|) { ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype 2024-04-10 9:34 ` Thomas Lamprecht @ 2024-04-10 11:19 ` Hannes Dürr 0 siblings, 0 replies; 3+ messages in thread From: Hannes Dürr @ 2024-04-10 11:19 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion On 4/10/24 11:34, Thomas Lamprecht wrote: > This is not bug #5365 [0] (which is about a ceph device class UX improvement) > but #5363 [0]. > > [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5365 > [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=5363 Good catch, thank you ! > I mostly noticed because I had too loo what this is actually about, IMO the > subject could be a bit more telling, targeting more our users, not our > devs (and even for those it's most often not too helpful to name methods > directly). Makes sense, will keep that in mind for future patches. > > How about something like: > > fix #5363: cloudinit: allow using scsi for the CD-ROM again > > That'd tell me roughly what was wrong and also that it worked previously > already, i.e. fixes a regression, rather than a new enhancement. But that > was just from top of my head, maybe there's ab even more telling one > possible, e.g. one alternative could be: > > fix #5363: scsi device type: detect cloudinit also for new drives > > Am 09/04/2024 um 16:47 schrieb Hannes Duerr: >> When we obtain the devicetype, we check whether it is a CD drive. > I know this quite often comes natural, but for commit messages its IMO > often a bit better to avoid first-person pronouns to avoid the reader > asking "who's we?". Fair point, rephrased it. > >> Cloudinit drives are always allocated CD drives, but if the drive has >> not yet been allocated, the test fails because the cd attribute has not >> yet been set. >> We therefore now explicitly check whether it is a cloudinit >> drive that has not yet been allocated. > As per the bug report this seems to be a regression, in which case a > reference to the commit this fixes would be nice. Will do ! > >> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com> >> --- >> PVE/QemuServer/Drive.pm | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm >> index 34c6e87..c829bde 100644 >> --- a/PVE/QemuServer/Drive.pm >> +++ b/PVE/QemuServer/Drive.pm >> @@ -853,7 +853,7 @@ sub get_scsi_devicetype { > unrelated, but "get_scsi_device_type" would be a slightly easier to > read variant of that name. Agree, created a follow-up commit New Version with follow-up: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062765.html >> >> my $devicetype = 'hd'; >> my $path = ''; >> - if (drive_is_cdrom($drive)) { >> + if (drive_is_cdrom($drive) || drive_is_cloudinit($drive)) { >> $devicetype = 'cd'; >> } else { >> if ($drive->{file} =~ m|^/|) { ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-10 11:19 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-09 14:47 [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype Hannes Duerr 2024-04-10 9:34 ` Thomas Lamprecht 2024-04-10 11:19 ` Hannes Dürr
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox