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

["Hannes Dürr" <h.duerr@proxmox.com>]

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