[pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype

[pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype

[Hannes Duerr]

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

Re: [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype

[Thomas Lamprecht]

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|^/|) {

Re: [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype

[Hannes Dürr]

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|^/|) {