all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Gabriel Goller <g.goller@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Lukas Wagner <l.wagner@proxmox.com>
Subject: Re: [pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version
Date: Mon, 27 Nov 2023 09:56:09 +0100	[thread overview]
Message-ID: <329f626f-e78d-413f-9220-deed3621f7a7@proxmox.com> (raw)
In-Reply-To: <59e58a82-a881-4b95-8f8d-45669bbb12e1@proxmox.com>

On 11/27/23 09:41, Gabriel Goller wrote:

> Thanks for the review!
>
> On 11/24/23 15:45, Thomas Lamprecht wrote:
>> Thanks for patch and review to both, but IMO this is still differing 
>> to much
>> from Proxmox VE's endpoint without any real justification?
>>
>> There the "boot-info" object with the required key "mode" and the 
>> optional
>> "secureboot" entry, that explicitly de-couples the general mode from 
>> some
>> mode-specific detail.
>>
>> The fitting rust struct (at least in sys) would be
>>
>> pub enum BootModeInformation {
>>      /// The BootMode is EFI/UEFI, the boolean specifies if secure 
>> boot is enabled
>>      Efi(bool),
>>      /// The BootMode is Legacy BIOS
>>      Bios,
>> }
>>
>>
>> or if one wants to be overly specific then something like:
>>
>> pub enum SecureBoot {
>>      Enabled,
>>      Disabled,
>> }
>>
>>
>> pub enum BootModeInformation {
>>      /// The BootMode is EFI/UEFI
>>      Efi(SecureBoot),
>>      /// The BootMode is Legacy BIOS
>>      Bios,
>> }
>>
>> (but could be overkill)
>>
>> It's not a hard must to keep this the same for pve-manager and pbs, 
>> but IMO
>> one should have very good reason for changing the format for relaying 
>> the
>> exact same information between two products, such inconsistencies 
>> make it
>> harder to interact with our API for any users, or external devs, and 
>> also
>> won't make it easier to reuse widgets for the (current or future) UIs..
> Ok, I will implement this in v2.
> I think I'll choose the second one with the specific enum for 
> `SecureBoot`. Will
> be more clear what is means (without looking at comments) + won't use 
> more memory
> than the bool version.
>
On a second note, I don't like this.
We don't support enum variants (fields) with the `api` macro, so I would 
have to create
a struct containing `mode` and `secureboot`. That again implies that 
bios + secureboot is
possible, which is not.
But I get your argument about consistency between products, so if we'd 
go that
way, I'd send another patch with the requested changes...




  reply	other threads:[~2023-11-27  8:56 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-24 12:02 Gabriel Goller
2023-11-24 12:02 ` [pbs-devel] [PATCH v2 proxmox 1/3] sys: add function to get boot_mode Gabriel Goller
2023-11-24 14:19   ` Lukas Wagner
2023-11-24 12:02 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] node: status: added bootmode Gabriel Goller
2023-11-24 14:20   ` Lukas Wagner
2023-11-27  8:26     ` Gabriel Goller
2023-11-27  8:32       ` Lukas Wagner
2023-11-24 12:02 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] node: status: declutter kernel-version Gabriel Goller
2023-11-24 14:19   ` Lukas Wagner
2023-11-27  8:35     ` Gabriel Goller
2023-11-24 14:18 ` [pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version Lukas Wagner
2023-11-24 14:45   ` Thomas Lamprecht
2023-11-27  8:41     ` Gabriel Goller
2023-11-27  8:56       ` Gabriel Goller [this message]
2023-11-27  9:13         ` Thomas Lamprecht

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=329f626f-e78d-413f-9220-deed3621f7a7@proxmox.com \
    --to=g.goller@proxmox.com \
    --cc=l.wagner@proxmox.com \
    --cc=pbs-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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal