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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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