From: "Hannes Dürr" <h.duerr@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype
Date: Wed, 10 Apr 2024 13:19:00 +0200 [thread overview]
Message-ID: <06efe557-72f3-4ccd-8967-bc9233307bf6@proxmox.com> (raw)
In-Reply-To: <49cf5773-78e3-4582-a35c-620f113fdedc@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|^/|) {
prev parent reply other threads:[~2024-04-10 11:19 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-09 14:47 Hannes Duerr
2024-04-10 9:34 ` Thomas Lamprecht
2024-04-10 11:19 ` Hannes Dürr [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=06efe557-72f3-4ccd-8967-bc9233307bf6@proxmox.com \
--to=h.duerr@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox