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.du...@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|^/|) {
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
