From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <h.duerr@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <t.lamprecht@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
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?= <h.duerr@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <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|^/|) {