From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 93066942E8 for ; Wed, 10 Apr 2024 13:19:02 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7BD04DED2 for ; Wed, 10 Apr 2024 13:19:02 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 10 Apr 2024 13:19:01 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8B6E843C9A for ; Wed, 10 Apr 2024 13:19:01 +0200 (CEST) Message-ID: <06efe557-72f3-4ccd-8967-bc9233307bf6@proxmox.com> Date: Wed, 10 Apr 2024 13:19:00 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox VE development discussion References: <20240409144713.282453-1-h.duerr@proxmox.com> <49cf5773-78e3-4582-a35c-620f113fdedc@proxmox.com> Content-Language: en-US From: =?UTF-8?Q?Hannes_D=C3=BCrr?= In-Reply-To: <49cf5773-78e3-4582-a35c-620f113fdedc@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.035 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Apr 2024 11:19:02 -0000 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 >> --- >> 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|^/|) {